Fix computed db checkpoint for weak subjectivity checks #4575
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
g11tech
added
the
prio-critical
Performance Report✔️ no performance regression detected Full benchmark results
|
twoeths
reviewed
Sep 21, 2022
| @@ -111,7 +111,7 @@ export function isWithinWeakSubjectivityPeriod( | |||
| wsState: BeaconStateAllForks, | |||
| wsCheckpoint: Checkpoint | |||
| ): boolean { | |||
| const wsStateEpoch = computeEpochAtSlot(wsState.slot); | |||
| const wsStateEpoch = Math.ceil(wsState.slot / SLOTS_PER_EPOCH); | |||
There was a problem hiding this comment.
@g11tech do we really need this for the fix? it's quite confusing here so either not to include this change, or drop some comments to clarify
twoeths
reviewed
Sep 21, 2022
| epoch: computeEpochAtSlot(state.latestBlockHeader.slot), | ||
| // the correct checkpoint is based on state's slot, its latestBlockHeader's slot's epoch can be | ||
| // behind the state | ||
| epoch: Math.ceil(state.slot / SLOTS_PER_EPOCH), |
There was a problem hiding this comment.
using state.slot makes sense, thru I think computeEpochAtSlot(state.slot) is enough
if SLOTS_PER_EPOCH = 32 and state.slot = 40 then this function return an epoch of 2 does not sounds right, otherwise please drop some comments to avoid the confusing
also note that there is another getCheckpointFromState function defined somewhere else which expect the real checkpoint state (state.slot % 32 = 0)
There was a problem hiding this comment.
my reasoning: if state slot is > some epoch boundary (ideally it should be on the boundary), and it its a checkpoint state, it should be for next epoch+1 only right? could be possible that the latestBlockHeader belongs from > epoch*32 slot, and hence the checkpoint root might belong to epoch+1
Also this: https://github.com/ChainSafe/lodestar/blob/unstable/packages/beacon-node/src/chain/initState.ts#L226
twoeths
approved these changes
Sep 21, 2022
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.
Motivation
The checkpoint computed from db state for weak subjetcivity checks is wrongly based on its latestBlockHeader's slot's epoch
which causes a mismatch while comparing epochs in
isWithinWeakSubjectivityPeriodand can cause a restarted node to be stuck if the last state written's latestBlockHeader's slot doesn't alignThis PR fixes the calculation of a state's checkpoint epoch