-
Notifications
You must be signed in to change notification settings - Fork 283
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
Make checkpoint sync work when finalized state is transitioned with empty slot(s) #7981
Make checkpoint sync work when finalized state is transitioned with empty slot(s) #7981
Conversation
badc3e6
to
e338f25
Compare
29eb82c
to
ce435f9
Compare
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 like the way you've kept the check in Summary
while avoiding it in some cases in AnchorPoint
but I still have a lot of doubts on 2 major points of this change:
- Could weakening of AnchorPoint creation checks introduce a bugs when AnchorPoint is created not from checkpoint sync it has not failed, but it's a bad AnchorPoint
- How transitioned AnchorPoint could affect its consumers when we start Teku
While from quick check I don't see any issues with 1st, 2nd could be definitely an issue in many places. I've commented one case in the Store
, another one for example is initalization of the Store
:
// Track latest finalized block
this.finalizedAnchor = finalizedAnchor;
this.maybeEpochStates = maybeEpochStates;
states.cache(finalizedAnchor.getRoot(), finalizedAnchor);
This way we are putting incorrect cache in states cache which is widely used. And there are a lot of other places where finalizedAnchor spreads, we should check them all and decide how to handle it. Maybe this kind of Anchor will need extra identification so we could avoid polluting some things like this state cache with it.
protected void verifyStateAndBlockConsistency() { | ||
if (state.getSlot().isGreaterThan(blockSummary.getSlot())) { | ||
// the finalized state is transitioned with empty slot(s) | ||
checkArgument( |
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'd also add verification for state.block_roots (block slot - state slot interval) and state.state_roots (block 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.
Added those checks, made it bit more complex, since don't have access to spec
in super class.
Optional.of( | ||
StateAndBlockSummary.create( | ||
finalizedAnchor.getBlockSummary(), finalizedAnchor.getState()))); | ||
return SafeFuture.completedFuture(Optional.of(finalizedAnchor)); |
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.
If it's transitioned anchor, it will be incorrect state for requested block_root
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.
AnchorPoint.getStateRoot()
still returns the state_root
of the finalzied block (last block header), not the state
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.
Method private SafeFuture<Optional<StateAndBlockSummary>> getOrRegenerateBlockAndState( final Bytes32 blockRoot)
should return state at block with blockRoot, transitioned state is not this state. This method is used by a lot of different consumers, it could be an issue
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.
Yeah AnchorPoint.getStateRoot()
returns the state_root
of the blockSummary
, so would be the state associated with the 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.
In any case, StateAndBlockSummary.create(finalizedAnchor.getBlockSummary(), finalizedAnchor.getState())
is same as just returning finalizedAnchor
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.
On startup we definitely create the Store with the anchor coming from the checkpoint URL. So it is possible that we could return it from here...
7142708
to
e05a5d7
Compare
Closing this in favour of #8221 |
PR Description
Override the consistency check for an AnchorPoint since the finalized state can be transitioned with empty slot(s)
Fixed Issue(s)
fixes #7956
Documentation
doc-change-required
label to this PR if updates are required.Changelog