fix(epbs): checkpoint restart stability + finalized state API parity - #9005
fix(epbs): checkpoint restart stability + finalized state API parity#9005lodekeeper wants to merge 4 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the stability and correctness of the EPBS beacon node, particularly concerning checkpoint synchronization and state management. It addresses critical issues that caused crashes during restarts and stalls during range synchronization, ensuring a more resilient node operation. Furthermore, it refines the serving semantics for finalized states to accurately reflect the consensus post-state, bringing the API in line with specifications and other client implementations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several important fixes to enhance the stability and correctness of EPBS checkpoint/restart behavior and state serving, aligning it with the specification. The changes include hardening against restart crashes by correctly deriving payload status from the state, fixing range sync failures by importing parent envelopes earlier, ensuring correct finalized state serving for post-Gloas forks, and improving the robustness of archival by skipping missing envelopes. The code changes are logical and directly address the issues described. I have a few minor suggestions to improve type safety by avoiding an overly aggressive type cast.
| ? { | ||
| executionPayloadBlockHash: toRootHex(state.latestExecutionPayloadHeader.blockHash), | ||
| executionPayloadNumber: state.latestExecutionPayloadHeader.blockNumber, | ||
| executionPayloadBlockHash: toRootHex((state as unknown as CachedBeaconStateGloas).latestBlockHash), |
There was a problem hiding this comment.
The cast as unknown as CachedBeaconStateGloas is a bit aggressive and bypasses all type-checking. A simpler cast as CachedBeaconStateGloas should be sufficient here, as the isForkPostGloas check ensures the state is of the correct type. Using as unknown should be reserved for cases where it's truly necessary, and ideally accompanied by a comment explaining why. A similar issue exists on line 177.
| executionPayloadBlockHash: toRootHex((state as unknown as CachedBeaconStateGloas).latestBlockHash), | |
| executionPayloadBlockHash: toRootHex((state as CachedBeaconStateGloas).latestBlockHash), |
There was a problem hiding this comment.
Good catch @gemini-code-assist — simplified these to direct as CachedBeaconStateGloas casts (removed as unknown as) in follow-up commit 1572659.
There was a problem hiding this comment.
null
| executionPayloadBlockHash: toRootHex((state as unknown as CachedBeaconStateGloas).latestBlockHash), | |
| null |
| : null, | ||
| parentBlockHash: isForkPostGloas ? toRootHex((state as CachedBeaconStateGloas).latestBlockHash) : null, | ||
| parentBlockHash: isForkPostGloas | ||
| ? toRootHex((state as unknown as CachedBeaconStateGloas).latestBlockHash) |
There was a problem hiding this comment.
Similar to the comment on line 154, the as unknown as cast is likely unnecessary here. A direct cast to CachedBeaconStateGloas should work and is safer.
| ? toRootHex((state as unknown as CachedBeaconStateGloas).latestBlockHash) | |
| ? toRootHex((state as CachedBeaconStateGloas).latestBlockHash) |
There was a problem hiding this comment.
Good catch @gemini-code-assist — simplified these to direct as CachedBeaconStateGloas casts (removed as unknown as) in follow-up commit 1572659.
There was a problem hiding this comment.
That's great to hear! I'm glad the suggestion was helpful in simplifying the code.
| ? { | ||
| executionPayloadBlockHash: toRootHex(unfinalizedState.latestExecutionPayloadHeader.blockHash), | ||
| executionPayloadNumber: unfinalizedState.latestExecutionPayloadHeader.blockNumber, | ||
| executionPayloadBlockHash: toRootHex((unfinalizedState as unknown as CachedBeaconStateGloas).latestBlockHash), |
There was a problem hiding this comment.
The cast as unknown as CachedBeaconStateGloas is a bit aggressive and bypasses all type-checking. A simpler cast as CachedBeaconStateGloas should be sufficient here, as the isForkPostGloas check ensures the state is of the correct type. Using as unknown should be reserved for cases where it's truly necessary, and ideally accompanied by a comment explaining why. A similar issue exists on line 288.
| executionPayloadBlockHash: toRootHex((unfinalizedState as unknown as CachedBeaconStateGloas).latestBlockHash), | |
| executionPayloadBlockHash: toRootHex((unfinalizedState as CachedBeaconStateGloas).latestBlockHash), |
There was a problem hiding this comment.
Good catch @gemini-code-assist — simplified these to direct as CachedBeaconStateGloas casts (removed as unknown as) in follow-up commit 1572659.
There was a problem hiding this comment.
The issue has been resolved by the author. No further action is needed from my side.
| : null, | ||
| parentBlockHash: isForkPostGloas ? toRootHex((unfinalizedState as CachedBeaconStateGloas).latestBlockHash) : null, | ||
| parentBlockHash: isForkPostGloas | ||
| ? toRootHex((unfinalizedState as unknown as CachedBeaconStateGloas).latestBlockHash) |
There was a problem hiding this comment.
Similar to the comment on line 263, the as unknown as cast is likely unnecessary here. A direct cast to CachedBeaconStateGloas should work and is safer.
| ? toRootHex((unfinalizedState as unknown as CachedBeaconStateGloas).latestBlockHash) | |
| ? toRootHex((unfinalizedState as CachedBeaconStateGloas).latestBlockHash) |
There was a problem hiding this comment.
Good catch @gemini-code-assist — simplified these to direct as CachedBeaconStateGloas casts (removed as unknown as) in follow-up commit 1572659.
There was a problem hiding this comment.
Thank you for addressing this. It's good to see the simplification of the casts.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3210d92b4f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| executionPayloadBlockHash: toRootHex(state.latestExecutionPayloadHeader.blockHash), | ||
| executionPayloadNumber: state.latestExecutionPayloadHeader.blockNumber, | ||
| executionPayloadBlockHash: toRootHex((state as unknown as CachedBeaconStateGloas).latestBlockHash), | ||
| executionPayloadNumber: 0, |
There was a problem hiding this comment.
executionPayloadNumber is hardcoded to 0 for post-Gloas anchor initialization here (and repeated in the unfinalized initializer), which drops the execution height available from the loaded state. ForkChoice.onBlock() computes child executionPayloadNumber from parentBlock.executionPayloadNumber, and getPayloadAttributesForSSE() forwards that value to payload-attributes events, so a restarted node can emit near-zero parent block numbers and propagate incorrect numbering until envelopes are replayed. This is a correctness regression for APIs/consumers that depend on parent_block_number continuity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — I took a close look here. For post-Gloas checkpoint init we currently do not have an execution block number in state (EIP-7732 removed latestExecutionPayloadHeader, and latestExecutionPayloadBid has hashes/value but no block number), so we cannot recover a reliable parent block number at this initialization point.\n\nI kept 0 as a sentinel for now to avoid fabricating potentially wrong heights, and to keep this PR focused on the restart + finalized-state correctness fixes. I agree this is worth improving separately (e.g., deriving from persisted envelope/index when available). I can open a follow-up task for that if you want.
|
squash merged via 0daf882, I fixed the build in my commit, going to close this as the change was merged manually |
2 participants
Summary
This patch fixes EPBS checkpoint/restart behavior and finalized state serving semantics to match spec intent (consensus post-state) and Prysm checkpoint behavior.
Fixes included
Restart crash (
headState does not exist) hardeningisParentBlockFull) instead of hardcodedtruepayloadStatusas FULL/EMPTY from state instead of defaulting to PENDINGgetClosestHeadState()fallback to opposite payload variant when preferred variant is missingRange sync prestate failure on first post-checkpoint block
BLOCK_ERROR_PRESTATE_MISSING / REGEN_ERROR_BLOCK_NOT_IN_FORKCHOICEloops after checkpoint syncFinalized v2 state serving semantics
finalized/justifiedstate IDs on post-Gloas forks, normalize to EMPTY checkpoint variant (consensus post-state)Finalized archival robustness
Why
On EPBS devnet-0, restarting from a synced DB state could crash at startup and checkpoint sync could stall due parent-path mismatches. Also
/eth/v2/debug/beacon/states/finalizedcould return a post-envelope variant (or 404) instead of the consensus post-state expected from checkpoint sync providers.Validation
Live-devnet validation on
epbs-devnet-0with:--checkpointSyncUrl https://checkpoint-sync.epbs-devnet-0.ethpandaops.io/--execution.engineMockconfig.epbs-devnet-0.ethpandaops.ioObserved
SyncedheadState does not existcrash)/eth/v2/debug/beacon/states/finalizedmatches checkpoint endpoint exactly (same SHA256)latest_block_hash == latest_execution_payload_bid.parent_block_hash)Notes