Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HeadTracker finalization support #12082
HeadTracker finalization support #12082
Changes from 34 commits
af480ab
9da949b
9e580cc
112679e
334a4d1
83cf834
26e8d50
962464c
b86c872
00769f0
81774b4
c942663
72a2380
a01fb86
d7a9d4e
faf61d9
89a75b3
f7ab489
d9d422c
e77f529
908acf7
93b835d
2f55403
9f26066
71a0803
35c3302
bd1ea1e
6cc4fec
83ea5d1
2d5ae65
c99cea6
f77a8ab
f7c786f
dee11fc
03b07d2
9f6e5e4
924df1b
e4fe585
03489c0
4a117c4
00cbb2e
7abea66
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reaches up to finalized block. Shouldn't it be up to finalized block + history depth? Otherwise
HistoryDepth()
has no use.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've kept this logic as is. We actively try to backfill up to finality and keep blocks in cache up to history depth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certain product teams have expressed their interest in the past to have more than FinalityDepth blocks stored in Head Tracker. I figured
HistoryDepth
could play that role, and we can actually actively fetch more blocks instead of avoiding trimming, otherwise it won't be useful to users. That's why initially I proposed changing theHistoryDepth
name to something more representative.But if it requires additional changes, perhaps it might get out of scope. Is there anything else we would have to change besides this part to make it work with
FinalityDepth
+HistoryDepth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the behaviour to backfill down to
HistoryDepth
is trivial. But It would be nice to do some testing to check how long would a fresh start take for chains like Polygon and maybe do optimisation if needed.To keep this PR's size manageable I'd rather introduce this feature separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this PR, but we could do a large backfill by batched RPC calls right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, batching should improve performance for large block ranges, but it would be nice to measure actual improvement before introducing second way of backfilling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might end up spamming unnecessary logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log has the same frequency and log level as existing
Starting backfill
andFinished backfill.
It seems helpful for debugging purposes to know which block HeadTracker considers finalized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting backfill and finished backfill are also noisy and don't provide that much information. The general problem with our current logs is that we log everything and then there is a huge amount of logs to go through. In principle, we should try to log when things go wrong and only add debug messages when they change the state meaningfully. Let's keep this as it is for now, and tackle it in a different PR.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.