fix: payload status on fork chocie init - #8987
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 refines the initialization process for fork choice by introducing a standardized method for calculating the 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 aims to fix the payload status during fork choice initialization. However, it introduces a critical logic flaw for Gloas (ePBS) forks. The ProtoArray only creates PENDING and EMPTY variants for Gloas blocks during initialization, but the new logic can set the ForkChoiceStore status to FULL. This mismatch causes core fork choice functions to throw errors, leading to a denial of service for block processing and API requests. It is recommended to revert to using PayloadStatus.PENDING for Gloas blocks during initialization to maintain consistency with the ProtoArray state. Additionally, the modification for initializing from an unfinalized state introduces an issue by incorrectly using a function designed for checkpoint slots on a block that may not be at a checkpoint, potentially leading to an incorrect fork choice state.
| dataAvailabilityStatus: DataAvailabilityStatus.PreData, | ||
| payloadStatus: isForkPostGloas ? PayloadStatus.PENDING : PayloadStatus.FULL, // TODO GLOAS: Post-gloas how do we know if the checkpoint payload is FULL or EMPTY? | ||
| payloadStatus: getCheckpointPayloadStatus(unfinalizedState, computeEpochAtSlot(blockHeader.slot)), | ||
| builderIndex: isForkPostGloas |
There was a problem hiding this comment.
Initializing payloadStatus with the result of getCheckpointPayloadStatus for an unfinalized state can lead to a MISSING_PROTO_ARRAY_BLOCK error if the status is FULL. For Gloas blocks, the ProtoArray is initialized with PENDING and EMPTY variants only. Using FULL here will cause a mismatch that prevents the fork choice from correctly identifying the head or walking the block tree. Additionally, the function getCheckpointPayloadStatus is designed for checkpoint slots but is being used here for the headBlock, which is not necessarily at a checkpoint slot. This leads to incorrectly using the payload status of the epoch's checkpoint block for the headBlock. To fix this, you should determine the payload status for blockHeader.slot directly, by checking unfinalizedState.executionPayloadAvailability for the blockHeader.slot.
There was a problem hiding this comment.
Agreed on this risk. I reviewed it and confirmed this is actionable: using getCheckpointPayloadStatus(...) for the anchor/head init path can produce a FULL variant mismatch and is semantically wrong for non-checkpoint head slots.\n\nI left the same guidance to the author here: https://github.com/ChainSafe/lodestar/pull/8987#issuecomment-4003698056\n\nRecommended fix remains to keep anchor/head init at PENDING for post-Gloas and limit getCheckpointPayloadStatus usage to checkpoint-context status computation only.
| dataAvailabilityStatus: DataAvailabilityStatus.PreData, | ||
| payloadStatus: isForkPostGloas ? PayloadStatus.PENDING : PayloadStatus.FULL, // TODO GLOAS: Post-gloas how do we know if the checkpoint payload is FULL or EMPTY? | ||
| payloadStatus: getCheckpointPayloadStatus(state, checkpoint.epoch), | ||
| builderIndex: isForkPostGloas ? (state as CachedBeaconStateGloas).latestExecutionPayloadBid.builderIndex : null, |
There was a problem hiding this comment.
The pull request modifies the initialization of payloadStatus in initializeForkChoiceFromFinalizedState and initializeForkChoiceFromUnfinalizedState to use getCheckpointPayloadStatus(state, checkpoint.epoch). For Gloas (ePBS) forks, getCheckpointPayloadStatus can return PayloadStatus.FULL if the execution payload is marked as available in the state.
However, ProtoArray.onBlock, which is called during fork choice initialization (lines 134 and 305-308), only creates PENDING and EMPTY variants for Gloas blocks. It does not create the FULL variant. The FULL variant is only created later via onExecutionPayload when the payload actually arrives.
By initializing the ForkChoiceStore with payloadStatus = FULL for the finalized or justified checkpoints while ProtoArray lacks the corresponding FULL variant, several critical operations will fail:
getJustifiedBlock()andgetFinalizedBlock()will throw aMISSING_PROTO_ARRAY_BLOCKerror.isFinalizedRootOrDescendant()will fail to walk back to the finalized block if it attempts to match theFULLvariant (e.g., when a child block'sparentBlockHashrefers to the execution payload hash).findHead()will throwINVALID_BEST_NODEif it cannot verify the finalized root relationship.
This mismatch results in a complete denial of service for the node's fork choice processing and breaks related API endpoints.
There was a problem hiding this comment.
+1 from my side as reviewer. This should not initialize anchor/head payload status from checkpoint-epoch availability for the reasons above (can incorrectly choose FULL and mismatch proto-array variants).\n\nAuthor guidance is captured here: https://github.com/ChainSafe/lodestar/pull/8987#issuecomment-4003698056\n\nOnce this is wired to the correct head-slot semantics, the concern should be resolved.
Performance Report✔️ no performance regression detected Full benchmark results
|
|
@lodekeeper can you review this please? Also see if comments left by gemini-code-assist make sense |
ensi321
changed the title
|
@ensi321 I reviewed the new Gemini comments — they are actionable.\n\nThe key issue is that |
|
@ensi321 is this still relevant? I would think not, but please double check |
|
Issue it aims to solve is still relevant but the PR itself is super stale |
|
Agreed — and the PR can't really be rebased even if we wanted to: On "the issue it aims to solve is still relevant" — I re-read the init path on
The consequence: if the anchor block's own payload was revealed on chain, the first child extending it carries Note that the state can't trivially fix this itself: Caveat: I traced this by reading, not by reproducing — and neither init function has any test coverage today, so I can't rule out a compensating path in sync. Does that match your read? If so I'm happy to open a focused issue so it doesn't get lost now that this PR is closed. |
5 participants
No description provided.