fix: require imported payload for index==1 attestation gossip - #9611
Conversation
![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 updates the attestation validation logic to ensure that the execution payload for a past block is fully imported and verified (using chain.forkChoice.hasPayloadHexUnsafe) rather than just seen on gossip, aligning with consensus-specs #5355. Additionally, it updates documentation comments in proposerPreferencesPool.ts and attestation.ts to reflect these changes. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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: fe70c83e4f
ℹ️ 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".
| if ( | ||
| block !== null && | ||
| attData.index === 1 && | ||
| !chain.forkChoice.hasPayloadHexUnsafe(toRootHex(attData.beaconBlockRoot)) |
There was a problem hiding this comment.
Please apply the same imported-payload gate to aggregate validation as well. validateAggregateAndProof still uses chain.seenPayloadEnvelope(...), and seenPayloadEnvelope includes seenPayloadEnvelopeInputCache.hasPayload before importExecutionPayload adds the FULL fork-choice variant, so an index==1 aggregate submitted through publishAggregateAndProofsV2 can still be accepted and re-published after merely seeing a gossip envelope but before DA/EL import has completed. That leaves the #5355 rule enforced for single attestations here but not for aggregated attestations carrying the same payload-present vote.
Useful? React with 👍 / 👎.
| if ( | ||
| block !== null && | ||
| attData.index === 1 && | ||
| !chain.forkChoice.hasPayloadHexUnsafe(toRootHex(attData.beaconBlockRoot)) |
There was a problem hiding this comment.
I think this code was unreachable anyways because we would queue those attestations and not process them cc @twoeths correct me if that's wrong
There was a problem hiding this comment.
yes we do queue index=1 gossip attestation awaiting for payload
for api attestation, we did not handle that. We only call await validateGossipFnRetryUnknownRoot(validateFn, network, chain, slot, beaconBlockRoot), which is not enough for gloas.
| if (block !== null && attData.index === 1 && !chain.seenPayloadEnvelope(toRootHex(attData.beaconBlockRoot))) { | ||
| // the corresponding execution payload for `block` has been fully imported, including its | ||
| // data -- i.e. `is_payload_verified(store, beacon_block_root)` returns True (consensus-specs | ||
| // #5355). `forkChoice.hasPayloadHexUnsafe` is the equivalent of `root in store.payloads`: |
There was a problem hiding this comment.
we need to add a AGENTS.md rule or something to stop these spec reference like consensus-specs #5355, I would rather not have this or have a full URL to the spec PR
Mirror the beacon_attestation is_payload_verified gating (#9611) on the beacon_aggregate_and_proof topic per https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.12/specs/gloas/p2p-interface.md#modified-beacon_aggregate_and_proof An index==1 aggregate was accepted once the envelope was merely seen on gossip, before it was imported and verified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
ethereum/consensus-specs#5355