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
eth: support bubbling up bad blocks from sync to the engine API #25190
eth: support bubbling up bad blocks from sync to the engine API #25190
Changes from all commits
9c2f1bf
07fc743
660c2a5
001a93f
7d94ebf
9a6b480
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It's problematic to me.
If we try to re-process a block and eventually it's adopted this time, we only delete "hit marker", but leave "invalidTipSet" unchanged with all descendants still marked as BAD.
Let's imagine the scenario:
invalidBlocksHits[X]
is reset to 0invalidTipsets[X+1]
is still existent andinvalidBlocksHits[X]
is 0There 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'm kind of wondering how we expect the CL to operate here. Considering that geth deems
Bn
to be invalid, would the CLa) Try to import it over and over
Bn, Bn, Bn, Bn....
, orb) Try to import the tips after it:
Bn, Bn+1, Bn+2...
.This PR lgtm if we expect scenario a), but probably won't work great if we expect scenario b).
?
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 both of them are expected. Different CL's have different strategies regarding retrying invalid blocks/syncing etc
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.
Both scenarios are handled. When a NewPayload is received, we check the block against the set of invalid blocks. If it is present, we reject if (scenario A). If the block is not present, we try to retrieve the parent state (to process on top) and if that's missing, we check the parent's hash for bad block-ness. If it is, we reject the payload (scenario B).
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.
Good catch. We need to clean up all the future blocks referencing the same bad block.
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.
So blocks[index] is the bad block and head is the head that we currently trying to sync to, 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
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 index can be inaccurate. e.g. here https://github.com/ethereum/go-ethereum/blob/master/core/blockchain.go#L1511 the returned index is the index of PrunedBlock, but not the real bad block position.
But this can be fixed in a following 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.
Recovering the ancestor shouldn't fail though, it if was already once imported?
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.
theoretically, yes. It shouldn't fail.