Skip to content

fix: reject gloas attestations/aggregates for EL-invalidated payloads#9637

Open
lodekeeper wants to merge 1 commit into
ChainSafe:unstablefrom
lodekeeper:fix/gloas-attestation-payload-failed-reject
Open

fix: reject gloas attestations/aggregates for EL-invalidated payloads#9637
lodekeeper wants to merge 1 commit into
ChainSafe:unstablefrom
lodekeeper:fix/gloas-attestation-payload-failed-reject

Conversation

@lodekeeper

Copy link
Copy Markdown
Contributor

Motivation

Part of the Gloas gossip validation work against consensus-specs PR #5294. The gloas p2p-interface spec mandates a [REJECT] for an index == 1 ("payload present for a past block") beacon_attestation / beacon_aggregate_and_proof whose referenced block's execution payload failed EL validation (attestation L525-526, aggregate L246-247). Lodestar implemented the neighbouring [IGNORE] payload not seen check but was missing this [REJECT], so such attestations/aggregates fell through to VALID and were propagated.

Description

Adds the missing [REJECT] the execution payload for block passes validation check to both validateGossipAttestation and validateAggregateAndProof, with a new EXECUTION_PAYLOAD_FAILED_VALIDATION error code.

A payload can fail EL validation in two ways, each with a distinct fork-choice representation — both are now rejected:

  1. Imported then invalidated (VALID/SYNCING at import, later invalidated via latest-valid-hash): the FULL fork-choice variant exists with executionStatus === Invalid. Detected via forkChoice.getBlockHex(root, PayloadStatus.FULL).
  2. Rejected immediately by the EL at import (newPayloadINVALID): importExecutionPayload throws before onExecutionPayload, so no FULL variant is ever created — but the envelope was already seen on gossip, so the [IGNORE] not seen check would not fire either. The payload input is now flagged via a narrow markPayloadInvalid() boolean on the existing PayloadEnvelopeInput cache entry (bounded per unfinalized block, pruned on finalization and on head-parent advance — no new state/registry on the import hot path), surfaced by chain.payloadFailedValidation().

The gloas index checks are also hoisted above the AttestationData cache lookup in attestation.ts so that a payload invalidated after its AttestationData was first cached cannot be bypassed by subsequent unaggregated attestations with the same data (mirrors validateAggregateAndProof, which already runs these checks before its cache use).

Tests

  • importExecutionPayloadInvalid.test.ts — EL INVALID at import throws EXECUTION_ENGINE_INVALID and flags the payload input invalid; a transient EL error (INVALID_BLOCK_HASH) does not flag it.
  • gloasPayloadInvalidReject.test.ts — an index == 1 gossip attestation is REJECTed for both the immediate-INVALID case (via the flag) and the FULL-invalidated case; a valid+seen payload is not rejected by this check.

Verified against the consensus-specs #5294 reference tests: the gossip_beacon_attestation__reject_payload_failed_el_validation and gossip_beacon_aggregate_and_proof__reject_payload_failed_el_validation cases pass on both minimal and mainnet.

🤖 Generated with AI assistance

Gloas p2p-interface mandates [REJECT] for an index==1 ("payload present for a
past block") attestation/aggregate whose execution payload failed EL validation.
A payload can fail two ways, each with a distinct fork-choice representation:

- imported (VALID/SYNCING) then invalidated via latest-valid-hash: the FULL
  variant exists with executionStatus === Invalid.
- rejected immediately by the EL at import (newPayload -> INVALID):
  importExecutionPayload throws before onExecutionPayload, so no FULL variant is
  created, but the envelope was already seen on gossip (so the IGNORE check would
  not fire). The payload input is now flagged via markPayloadInvalid (a narrow
  boolean on the existing, finalization-pruned PayloadEnvelopeInput cache entry),
  surfaced by chain.payloadFailedValidation.

Both validateGossipAttestation and validateAggregateAndProof REJECT with a new
EXECUTION_PAYLOAD_FAILED_VALIDATION error code covering both cases. The gloas
index checks are hoisted above the AttestationData cache so a payload invalidated
after its AttestationData was first cached cannot be bypassed by later
unaggregated attestations.

Adds unit tests: importExecutionPayload EL-INVALID sets the flag (and transient
EL errors do not); the gossip validator REJECTs an index==1 attestation for both
the immediate-INVALID and FULL-invalidated cases.

🤖 Generated with AI assistance
@lodekeeper lodekeeper requested a review from a team as a code owner July 10, 2026 08:42

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a mechanism to track execution payloads that fail Execution Layer (EL) validation immediately during import, preventing them from bypassing gossip validation. It adds a payloadInvalid flag to PayloadEnvelopeInput and updates both attestation and aggregate validation logic to reject index-1 'payload present' attestations if the referenced payload failed validation. The review feedback suggests extracting the duplicated validation logic between validateAttestationNoSignatureCheck and validateAggregateAndProof into a shared helper function to improve maintainability.

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.

Comment on lines +322 to +348
// [REJECT] If `attestation.data.index == 1` (payload present for a past block), the execution
// payload for `block` passes validation. A payload can fail EL validation with two distinct
// fork-choice representations, both of which must reject the attestation:
// - imported (VALID/SYNCING) then invalidated via latest-valid-hash: the FULL variant exists
// with `executionStatus === Invalid`.
// - rejected immediately by the EL at import (`newPayload` -> INVALID): `importExecutionPayload`
// throws before `onExecutionPayload`, so no FULL variant is created, but the envelope was
// already seen on gossip (so the IGNORE below would not fire). The payload input is flagged
// via `markPayloadInvalid`, surfaced here by `chain.payloadFailedValidation`.
const fullBlock = chain.forkChoice.getBlockHex(beaconBlockRootHex, PayloadStatus.FULL);
if (fullBlock?.executionStatus === ExecutionStatus.Invalid || chain.payloadFailedValidation(beaconBlockRootHex)) {
throw new AttestationError(GossipAction.REJECT, {
code: AttestationErrorCode.EXECUTION_PAYLOAD_FAILED_VALIDATION,
beaconBlockRoot: beaconBlockRootHex,
});
}

// [IGNORE] When `attestation.data.index == 1` (payload present for a past block), the
// corresponding execution payload for `block` has been seen (a client MAY queue
// attestations for processing once the payload is retrieved and SHOULD request the
// payload envelope via `ExecutionPayloadEnvelopesByRoot`).
if (!chain.seenPayloadEnvelope(beaconBlockRootHex)) {
throw new AttestationError(GossipAction.IGNORE, {
code: AttestationErrorCode.EXECUTION_PAYLOAD_NOT_SEEN,
beaconBlockRoot: beaconBlockRootHex,
});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is identical validation logic duplicated between validateAttestationNoSignatureCheck (in attestation.ts) and validateAggregateAndProof (in aggregateAndProof.ts) for checking if the execution payload passes validation or has been seen.

To improve maintainability and avoid duplication, consider extracting this logic into a shared helper function, for example:

export function verifyExecutionPayloadStatus(
  chain: IBeaconChain,
  beaconBlockRootHex: RootHex
): void {
  const fullBlock = chain.forkChoice.getBlockHex(beaconBlockRootHex, PayloadStatus.FULL);
  if (fullBlock?.executionStatus === ExecutionStatus.Invalid || chain.payloadFailedValidation(beaconBlockRootHex)) {
    throw new AttestationError(GossipAction.REJECT, {
      code: AttestationErrorCode.EXECUTION_PAYLOAD_FAILED_VALIDATION,
      beaconBlockRoot: beaconBlockRootHex,
    });
  }

  if (!chain.seenPayloadEnvelope(beaconBlockRootHex)) {
    throw new AttestationError(GossipAction.IGNORE, {
      code: AttestationErrorCode.EXECUTION_PAYLOAD_NOT_SEEN,
      beaconBlockRoot: beaconBlockRootHex,
    });
  }
}

This helper can then be imported and called in both files:

if (block !== null && attData.index === 1) {
  const beaconBlockRootHex = toRootHex(attData.beaconBlockRoot);
  verifyExecutionPayloadStatus(chain, beaconBlockRootHex);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — that block is byte-identical in both validators.

I kept it inline in each deliberately: these gossip validators read as a linear sequence of [REJECT]/[IGNORE] checks mapped 1:1 to the spec steps, so inlining keeps each check traceable to its spec line in place — pulling the whole check into a helper trades that spec-traceability for DRY.

The part that genuinely benefits from a single source is the subtle detection: the two fork-choice representations of a failed payload — the invalidated FULL variant (executionStatus === Invalid) vs. the payloadFailedValidation flag when newPayload rejects before any FULL variant exists. I'm happy to extract that into a shared predicate (e.g. isExecutionPayloadInvalid(chain, beaconBlockRootHex)), keeping the [REJECT]/[IGNORE] throws inline — or the full verifyExecutionPayloadStatus helper as suggested — if a maintainer prefers DRY over the inline spec-flow. Happy to do either.

* variant is ever created; this flag is the only record that the payload failed validation and
* lets gossip validation still REJECT index-1 "payload present" attestations for it.
*/
private payloadInvalid = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we keep track of this in fork choice, why do you wanna add this to our payload cache?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The immediate EL-INVALID case never reaches fork choice today. When newPayload returns INVALID, importExecutionPayload throws at step 4c before step 6's onExecutionPayload (the only thing that creates a FULL variant), and toForkChoiceExecutionStatus maps only VALID/SYNCING/ACCEPTEDINVALID hits the default throw. So there's no FULL variant at all: getBlockHex(root, FULL) is null and fork choice has no record of it.

(The LVH-invalidated case — imported VALID/SYNCING then invalidated — does live in fork choice as a FULL variant with executionStatus === Invalid, which the validators already check via getBlockHex(FULL). The cache flag only covers the immediate case.)

Your instinct is fair though: we could make fork choice the single source of truth by teaching onExecutionPayload / toForkChoiceExecutionStatus to accept Invalid and creating a FULL-Invalid variant on immediate-INVALID (before the throw) — then both cases collapse to the existing getBlockHex(FULL)?.executionStatus === Invalid check and the cache flag goes away. That's a fork-choice contract change (it rejects Invalid by design today), which is why I kept it on the payload cache — but I'm happy to move it into fork choice if you'd prefer that. Your call since it's your area.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants