feat: add gloas execution payload envelope import pipeline#8962
feat: add gloas execution payload envelope import pipeline#8962
Conversation
ensi321
changed the title
ensi321
changed the title
Summary of ChangesHello @ensi321, 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 introduces the foundational infrastructure for handling Gloas execution payload envelopes within the beacon node. It establishes a new data flow that tracks the availability of execution payloads and their associated data columns, ensuring that blocks are only processed once all necessary components are received. This enhancement is crucial for supporting future Gloas-specific functionalities and maintaining data integrity throughout the block processing pipeline. 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 the import pipeline for Gloas execution payload envelopes, a significant feature for the upcoming Gloas fork. The changes are extensive, touching the API, core chain logic, validation, caching, and fork-choice mechanisms. The introduction of PayloadEnvelopeInput and SeenPayloadEnvelopeInput provides a robust mechanism for handling the asynchronous arrival of different components of a Gloas block. The refactoring to remove Gloas-specific details from ProtoBlock is a good architectural improvement, enhancing separation of concerns. The code is well-structured, and the changes are consistent across the codebase. I've identified one minor latent issue in some commented-out code and provided a suggestion to fix it.
| // 3. Run verification steps in parallel (like verifyBlocksInEpoch) | ||
| // Note: No data availability check needed here - importExecutionPayload is only | ||
| // called when payloadInput.isComplete() is true, so all data is already available. | ||
| const [execResult, _postPayloadState] = await Promise.all([ |
There was a problem hiding this comment.
The destructuring of the result from Promise.all appears to be incorrect. The async IIFE on lines 103-118 returns an object { postPayloadState: mutableState }. The current destructuring on line 91 assigns this entire object to _postPayloadState, not just the state value. Consequently, the commented-out usage on line 153 (this.regen.processPayloadState(_postPayloadState)) would be incorrect as it would pass the object instead of the state itself.
To prevent this latent bug, you should destructure the postPayloadState property from the returned object.
| const [execResult, _postPayloadState] = await Promise.all([ | |
| const [execResult, {postPayloadState: _postPayloadState}] = await Promise.all([ |
Performance Report✔️ no performance regression detected Full benchmark results
|
Use getBlockSlotState instead of getHeadState for builder pubkey lookup during execution payload envelope validation. This ensures signature verification uses the correct builder registry state at the block's slot, rather than potentially stale head state that could differ due to reorgs or head changes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
ensi321
changed the title
|
@lodekeeper can you do a review on this? |
![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: 7c998d3b70
ℹ️ 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".
|
|
||
| const processEnvelopeTimer = metrics?.blockPayload.executionPayloadEnvelopeProcessingTime.startTimer(); | ||
| processExecutionPayloadEnvelope(postBlockState, signedEnvelope, false); | ||
| processExecutionPayloadEnvelope(postBlockState, signedEnvelope, {verifySignature: false, verifyStateRoot: true}); |
There was a problem hiding this comment.
computeEnvelopeStateRoot() creates a signed envelope whose stateRoot is still a placeholder, but this call now enables verifyStateRoot. processExecutionPayloadEnvelope() will compare that placeholder root against the freshly-mutated state and throw, so self-built Gloas block production can fail before the function ever returns the computed root. This path should skip state-root verification during root derivation and only verify once the envelope carries the computed value.
Useful? React with 👍 / 👎.
| if (this.isComplete() && !this.importClaimed) { | ||
| this.importClaimed = true; | ||
| return true; |
There was a problem hiding this comment.
shouldImport() marks importClaimed before any import work is known to succeed, and there is no reset path if importExecutionPayload() later fails (for example ELERROR/UNAVAILABLE). In that case the payload input stays complete but permanently unclaimable, so later envelope/column arrivals cannot trigger a retry and the payload can remain unimported until pruning/finalization.
Useful? React with 👍 / 👎.
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Did a pass on the latest diff. I agree with the newly raised P1s and think they should be addressed before merge:
-
computeEnvelopeStateRoot currently calls processExecutionPayloadEnvelope with verifyStateRoot=true while deriving the state root from an envelope that still has a placeholder stateRoot. That can self-fail block production in this path. This derivation step should skip state-root verification and only verify after the computed root is populated.
-
PayloadEnvelopeInput.shouldImport sets importClaimed=true before import success and never resets on import failure. If EL import fails transiently (ELERROR / UNAVAILABLE), later envelope/column arrivals cannot retry import and we can stay stuck until pruning/finalization.
Also +1 on the cache-eviction TODO thread (clearByRootAndSlot) — clearing too aggressively can wipe useful pending data paths and likely needs narrower eviction semantics.
|
@ensi321 done — I left a review pass on the latest diff (including the two new P1s and cache-eviction concern): #8962 (review) |
|
|
||
| const signatureSet = createSingleSignatureSetFromComponents( | ||
| PublicKey.fromBytes(state.builders.getReadonly(envelope.builderIndex).pubkey), | ||
| PublicKey.fromBytes((blockState as CachedBeaconStateGloas).builders.getReadonly(envelope.builderIndex).pubkey), |
There was a problem hiding this comment.
Potential correctness edge case: self-build envelopes use BUILDER_INDEX_SELF_BUILD in state-transition logic, but validation here always reads state.builders.getReadonly(envelope.builderIndex). Should we mirror processExecutionPayloadEnvelope handling and verify with proposer pubkey when builderIndex === BUILDER_INDEX_SELF_BUILD?
twoeths
left a comment
twoeths
left a comment
There was a problem hiding this comment.
did not fully review the PR
the PayloadEnvelopeInput should support having available data if it sees >= 64 columns similar to what we have pre-gloas
I have #8974 to make it easier to transition there and deduplicate persisting DataColumnSidecar to db
| // TODO GLOAS: Handle valid envelope. Need an import flow that calls `processExecutionPayloadEnvelope` and fork choice | ||
| if (payloadInput.shouldImport()) { | ||
| await chain.importExecutionPayload(payloadInput, {validSignature: true}); | ||
| chain.persistPayloadEnvelope(payloadInput); |
There was a problem hiding this comment.
we should do the same to importBlock flow, do this persist inside importExecutionPayload
in importBlock flow, we use unfinalizedBlockWrites as a job queue because it has to wait for all columns while we can import block with >= NUMBER_OF_COLUMNS / 2
so we may end up having a queue in BeaconChain and use it in importExecutionPayload
| const blobsLen = payloadInput.getBlobKzgCommitments().length; | ||
| if (blobsLen > 0) { | ||
| const {custodyColumns} = this.custodyConfig; | ||
| const dataColumnSidecars = payloadInput.getCustodyColumns(); |
There was a problem hiding this comment.
we should not assume we have all columns at this point, we may only have half of them for super node
see https://github.com/ChainSafe/lodestar/pull/8818/changes#diff-0ee3fd00ea4e500a6d0793b6d1184a397ac52749a082d92023532c3ba414787eR51
|
|
||
| // TODO GLOAS: Handle valid envelope. Need an import flow that calls `processExecutionPayloadEnvelope` and fork choice | ||
| if (payloadInput.shouldImport()) { | ||
| await chain.importExecutionPayload(payloadInput, {validSignature: true}); |
There was a problem hiding this comment.
we should not await the import here, let network gossip this message to other peers once it passes validation
should separate the whole function to // validation and // handler parts
| (async () => { | ||
| try { | ||
| // Clone state to avoid mutating the cached state | ||
| const mutableState = blockState.clone(); |
| envelope.message.executionRequests | ||
| ), | ||
|
|
||
| opts.validSignature === true |
There was a problem hiding this comment.
do we ever need this validSignature option? seems like call sites should verify it separately before reaching this function
There was a problem hiding this comment.
I think we need it for unknown payload sync (?)
I know for unknown block sync, we don't set ImportBlockOpts.validSignatures, so state transition verifies the signature.
lodestar/packages/beacon-node/src/sync/unknownBlock.ts
Lines 420 to 426 in bb27317
| }, | ||
|
|
||
| registerExecutionPayloadEnvelope(_src, _delaySec, _envelope) { | ||
| // TODO GLOAS: implement execution payload envelope monitoring |
There was a problem hiding this comment.
why is execution payload envelope involved validator monitor?
There was a problem hiding this comment.
Maybe for the case that the validator self-build a payload? Not sure if this would be useful. Any thought @nflaig ?
There was a problem hiding this comment.
we track the publish block delay in validator monitor currently, could do the same for envelopes, but validator monitor seems the wrong place to track timing info like this, we can keep the stub for now and revisit later, just realized there is still a registerBlobSidecar which is similar and doesn't really belong in the validator monitor
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| const payloadInput = chain.seenPayloadEnvelopeInput.get(blockRootHex); | ||
| if (!payloadInput) { | ||
| throw new ApiError(404, `PayloadEnvelopeInput not found for block root ${blockRootHex}`); | ||
| } |
There was a problem hiding this comment.
this can happen if block and payload are submitted in parallel as documented here #8915 we will need a queuing mechanism on the api, we can just throw for now as we currently only submit sequentially which is fine for the devnets
| // For self-builds, the proposer signs with their own validator key | ||
| // For external builders, verify using the builder's registered pubkey | ||
| // Use verify_execution_payload_envelope_signature(state, signed_envelope) | ||
| await validateApiExecutionPayloadEnvelope(chain, signedExecutionPayloadEnvelope); |
There was a problem hiding this comment.
we should probably run this validation earlier, based on ethereum/beacon-APIs#580 (comment) we wanna implement broadcast_validation so we can mirror what we have implemented in publishBlockV2
| // TODO GLOAS: Unlike publishBlock which gossips and imports in parallel, we import before gossip here. | ||
| // The publishExecutionPayloadEnvelope spec says success = "gossip validation + broadcast", so we may | ||
| // want to gossip first. Need spec clarification on whether import failure should prevent broadcast. | ||
| if (payloadInput.shouldImport()) { |
There was a problem hiding this comment.
I don't really see a reason why this should work differently than pre-gloas publish block flow, we can gossip and import in parallel
broadcast_validation is separate from this and needs to be validated earlier, the checks are not exactly the same as import checks, depending on which validation rule is chosen
so I think here we can assume all validation checks have passed already
| }, | ||
|
|
||
| registerExecutionPayloadEnvelope(_src, _delaySec, _envelope) { | ||
| // TODO GLOAS: implement execution payload envelope monitoring |
There was a problem hiding this comment.
we track the publish block delay in validator monitor currently, could do the same for envelopes, but validator monitor seems the wrong place to track timing info like this, we can keep the stub for now and revisit later, just realized there is still a registerBlobSidecar which is similar and doesn't really belong in the validator monitor
5 participants
Summary
PayloadEnvelopeInputclass to track payload envelope + data columnsimportExecutionPayloadfunction for EL verification and execution payload state transitionSeenPayloadEnvelopeInputcache for managing payload inputsSeenExecutionPayloadEnvelopesin favour ofSeenPayloadEnvelopeInputBackground
In Gloas (ePBS), beacon blocks and execution payloads have separate validity requirements:
Key Changes
New Files
payloadEnvelopeInput/payloadEnvelopeInput.ts- Tracks bid + envelope + columns with 4-state machineimportExecutionPayload.ts- EL verification, signature check, execution payload state transitionwritePayloadEnvelopeInputToDb.ts- Persists envelope + columns to DBseenPayloadEnvelopeInput.ts- Cache with finalization pruningModified Files
importBlock.ts- CreatesPayloadEnvelopeInputduring Gloas block importgossipHandlers.ts- Handlesexecution_payloadtopic, triggers import on completionextractSlotRootFns.ts- SSZ helpers for payload envelope queueingexecutionPayloadEnvelope.ts- Validation using block post-state (not head state)fork-choice- RemovedbuilderIndex/blockHashHexfrom ProtoBlock (now in PayloadEnvelopeInput)TODO
data_column_sidecargossip handler integration (commented, needs Gloas-specific validation)🤖 Generated with Claude Code