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 10 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.
As an optimization we can check if FinalityDepth is 0 and if it is then we can just return the initial head, avoiding making an additional RPC call. This is especially important on fast chains with instant finality.
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.
nit: Fix comment based on the new changes
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 think I've encountered a similar problem when I tried to implement a PoC of Finality Tag. Having a fixed HistoryDepth comes with certain caveats including not getting a finalized block for a period of time, which I don't think is acceptable. At the same time I know products might be interested in having a history longer than the latest finalized tag.
My suggestion is to rename HistoryDepth to something like
AdditionalHistoryDepth
and alter theSave
method to store heads:FinalityDepth
+AdditionalHistoryDepth
. This way the number can still remain dynamic and chains will be able to have an additional history buffer.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 do not mind changing the logic to make actual history depth dynamic.
But I'm not sure that we should make NOPs update configs given that they probably won't mind core storing 2500 blocks
FinalityDepth + HistoryDepth
instead of 2000FinalityDepth + AdditionalHistoryDepth
.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 name should be the least of our concerns. I'm more worried how to change the logic to avoid having a fixed history depth. I see one problem with this on
handleNewHead
where we store a head without knowing the finalized block, in comparison to backfill.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.
Do we need to trim them on handleNewHead? Can't we do it only on MarkFinalized when we have all the info?
Yes, we might use slightly more memory, but it does not seem critical as sooner or later we'll release it
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.
Perhaps you can change it there, but you also need a threshold during
Load
. What cutoff are we going to use there?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.
One option would be to call
calculateLatestFinalized
during startup andLoad
depth + history. This is only for speeding things up, it doesn't mess up with the functionality as far as I can tell.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.
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.
nit: I think we shouldn't be specific here with the consensus algorithm and drop the PoS reference. For all we know there are chains out there with PoA or other variations that also implement finality tag support.
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.
I don't think the entire app should crash if someone uses this method by mistake. It's better if we return nil & an error message so any user can handle this gracefully.
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.
Just noting, this field is already in-use by CCIP code, and is set to enabled on some changes.
So using it here means as soon as your code hits production, the relevant chains will automatically start using this by default.
I don't think it should be a problem though. Just noting.