-
Notifications
You must be signed in to change notification settings - Fork 741
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
Simplify database anchor #6397
Simplify database anchor #6397
Conversation
@@ -905,9 +905,6 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> { | |||
|
|||
let block_root = get_block_header_root(block_header); | |||
|
|||
// Disallow blocks that conflict with the anchor (weak subjectivity checkpoint), if any. | |||
check_block_against_anchor_slot(block.message(), chain)?; |
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.
These checks were unnecessary because the anchor slot is always older than finalization, so any block older than the anchor is also older than finalization and will be caught by check_block_against_finalized_slot
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.
What if checkpoint sync to a non-finalized checkpoint though? In that case, the checkpoint slot could be ahead of finalized slot.
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.
In that case, we force-initialize the fork-choice as if that checkpoint was 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.
LGTM. Just a question.
@@ -905,9 +905,6 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> { | |||
|
|||
let block_root = get_block_header_root(block_header); | |||
|
|||
// Disallow blocks that conflict with the anchor (weak subjectivity checkpoint), if any. | |||
check_block_against_anchor_slot(block.message(), chain)?; |
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.
What if checkpoint sync to a non-finalized checkpoint though? In that case, the checkpoint slot could be ahead of finalized slot.
1890278
into
sigp:tree-states-archive
Proposed Changes
Based on a suggestion from @dapplion, simplify the database's anchor by making it non-optional.
Rippling this change through the codebase also allowed for some other simplifications including deleting the
BackFillState::NotRequired
enum variant, which was redundant. Previously there were two checks for a backfill sync having already completed: checking if the anchor wasNone
(theBackFillState::NotRequired
case) and checking if all desired blocks had been downloaded based on the anchor'soldest_block_slot
(theBackFillState::Complete
case). I see no need to handle these cases differently, so I've deleted theNotRequired
variant in the process of deleting the code that handledanchor == None
which is no longer a possible state.