Remove VerifyFinalizedBlkDescendant #12046
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
potuz
added
Ready For Review
| @@ -107,6 +107,11 @@ func (s *Service) onBlock(ctx context.Context, signed interfaces.ReadOnlySignedB | |||
| return err | |||
| } | |||
|
|
|||
| // Verify that the parent block is in forkchoice | |||
| if !s.ForkChoicer().HasNode(b.ParentRoot()) { | |||
| return ErrNotDescendantOfFinalized | |||
There was a problem hiding this comment.
A bit torn about this error message. It makes sense but there's some implementation details that may not be obvious for someone that's not in depth
There was a problem hiding this comment.
I am not sure how to change this, this error is exported and used in several places. Changing it on one side and not others seems odd, I can change it to "parent not in forkchoice" but that's even less info for someone not in depth, the only thing that can trigger this error is if the block does not connect to the finalized checkpoint
There was a problem hiding this comment.
Agree. I dont think it's a big deal but just wanted to point it out
terencechain
approved these changes
Feb 25, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR removes the two helper functions
VerifyFinalizedBlkDescendantandVerifyFinalizedConsistency.Both these helpers served the purpose of validate if a given root is a descendant of the finalized checkpoint. In all usages of the latter, a simlple check for forkchoice if it has the parentroot, would work. The former is a little trickier
There are two paths in block processing where we use
VerifyFinalizedBlkDescendant, during regular block sync, and during init sync. Both paths currently callverifyBlockPreState, which in turn verifies that the block is a descendant of the finalized checkpoint.During regular sync, the block's parent must be in forkchoice. So we removed this check from
verfyBlockPreState(and added a extra safety check directly ononBlock).During init sync however, we allow insertion of blocks to
onBlockBatchwhose parent are not in forkchoice, but that they connect to the finalized checkpoint with blocks on db. This is because we do not start with the full forkchoice loaded from disk. However we removed the check fromverifyPreStatesince anyway this is checked infillInMissingBlocksToForkchoice.In addition we add a helper
InForkChoiceto chaininfo. This is in preparation to the locking PRs for forkchoice: blockchain packages are already locked and need to call forkchoice directly. Packages relying on chain_info have to call the helper that will lock forkchoice.