fix(rpc): refuse attestation data for an unvalidated head by MegaRedHand · Pull Request #636 · lambdaclass/ethlambda

MegaRedHand · 2026-09-30T20:39:21Z

Why

We had a report of the node importing blocks optimistically while still answering attestation_data. A validator client signs whatever that endpoint returns and never sees the execution status behind it, so the refusal has to come from the node:

consensus-specs specs/bellatrix/optimistic-sync.md, "Validator assignments": an optimistic validator MUST NOT attest, propose, or take part in sync committees.
Beacon API attestation_data and aggregate_attestation: "A 503 error must be returned if the block identified by the response beacon_block_root is optimistic".

Lighthouse, Prysm and Teku all refuse attestation_data in this case. Prysm and Teku answer 503; Lighthouse answers 500.

Changes

Endpoint	Before	After
`GET /eth/v1/validator/attestation_data`	200 on an optimistic head	503
`GET /eth/v2/validator/aggregate_attestation`	200	503 when the aggregate's `beacon_block_root` is optimistic, whether or not it is the head
Both of the above, no execution client configured	200	503. Such a node imports every block as `NotRequired`, so no block is ever optimistic and the optimistic check alone would always pass
`GET /eth/v1/validator/attestation_data`, no `committee_index`	400	200. The spec marks the parameter optional and deprecated, and tells gloas clients to omit it
`POST /eth/v1/beacon/states/{state_id}/validators`, `ids` or `statuses` set to `null`	400	no filtering on that field, as the spec says
`GET /eth/v1/node/syncing`, `is_optimistic`	`true` if any root in the store is optimistic	`true` if the head is (spec: "optimistically tracking head")
`GET /eth/v1/node/health`	206 only while syncing	206 while syncing or on an optimistic head (spec: "syncing, or its execution node is optimistic or offline")

The optimistic 503 is temporary. A VALID answer to forkchoiceUpdated, which the chain actor sends at least once a slot, clears the root.

The is_optimistic change also matters for ethlambda validator. Its per-epoch duty refresh fails while the flag is set: it keeps the previous schedule and reports the node unavailable. Before this change, an optimistic block on a side branch was enough to trigger that.

Still open

Found in an audit of every route this node serves, against beacon-APIs and consensus-specs. None is fixed here: each needs a decision or is a change of its own.

#	Deviation	Impact	Why it is not fixed here
1	Validator endpoints never return the spec's 503 "currently syncing"	A node that is behind hands out attestation data for a stale head. `ethlambda validator`'s failover assumes the node answers 503 while syncing	`SyncStatusController` reports synced during a beacon catch-up, so it cannot drive this. The fix compares the head slot with the wall clock, and the allowed lag needs choosing
2	No execution client configured: `execution_optimistic` is `false` on every block, header, state and duty response, and `el_offline` is `false`	Tools and checkpoint-sync consumers trust payloads nobody verified	Either each endpoint checks for an execution client, or blocks are marked optimistic at import. The second changes the blockchain crate
3	`POST /eth/v2/beacon/blocks` refuses JSON bodies with a plain-text 415	A validator client that publishes JSON cannot publish through this node	Needs JSON decoding of `SignedBlockContents`. Not yet checked which clients publish JSON
4	produceBlockV3 always reports `consensus_block_value` as `"0"`	A validator client comparing blocks across several beacon nodes under-rates this node's	Needs the proposer reward computed
5	produceBlockV3 ignores `skip_randao_verification`	A client that sends the infinity randao gets a 500 instead of a block	Needs a production path that skips randao verification
6	`POST /eth/v2/beacon/blocks` ignores `broadcast_validation`, and answers 400 to blocks carrying blobs	`consensus` and `consensus_and_equivocation` callers get weaker checks than they asked for. A blob block gets a 400 rather than a retryable 503	Blob publishing waits on data-column support
7	Pool POSTs (`pool/attestations`, `aggregate_and_proofs`) accept JSON only	A client that submits SSZ gets a 400	Not yet checked which clients submit SSZ
8	`attestation_data` answers 400 for a slot before the head block's slot	A late or retried request after the next block has arrived misses that attestation	Needs attestation data built for a past slot
9	No v2 of `GET /eth/v1/validator/duties/proposer/{epoch}` or `GET /eth/v1/node/version`	A client calling v2 gets a 404	Not yet checked which clients call v2
10	The checkpoint anchor reports `canonical: false`	Explorers show a finalized block as non-canonical	An existing test asserts this on purpose
11	`finalized` is slot-based	An orphan at a finalized slot reads `finalized: true`	Minor
12	No 406 for an unsupported `Accept`. Axum's default rejections return 415, 422 or a plain-text 400 rather than a `{code, message}` body. `/node/health` ignores `syncing_status`	Clients that read only the status code are unaffected	Minor

Not a deviation: block production has no explicit optimistic check, and needs none. The Engine API returns no payloadId unless the head is VALID, so an optimistic head already gets a 503.

Also in this PR

style: sort the binary's module declarations. The main merge into beacon-chain-integration left mod banner; after mod beacon; in bin/ethlambda/src/main.rs, which cargo fmt --check rejects. Without that fix, Lint fails on every PR against this branch.

Test plan

cargo test -p ethlambda-rpc -p ethlambda-storage --profile release-fast --lib
cargo clippy --locked -p ethlambda-rpc -p ethlambda-storage --all-targets -- -D warnings
cargo fmt --all -- --check
New tests:
- attestation_data answers 503 on an optimistic head, then 200 once it is validated.
- aggregate_attestation does the same for the voted block.
- Both answer 503 on a node with no execution client, even though nothing is optimistic.
- attestation_data answers without committee_index, and rejects a malformed one.
- validators reads a null filter as no filter.
- /node/syncing and /node/health ignore an optimistic side branch and react to an optimistic head.
- Over a real socket, ethlambda validator's HTTP client reads the new 503 as the retryable BeaconNodeSyncing.

A validator client signs whatever attestation_data returns and never sees the execution status behind it. optimistic-sync.md forbids an optimistic validator to attest, and the Beacon API puts the refusal on the node: "A 503 error must be returned if the block identified by the response beacon_block_root is optimistic". We answered 200 instead, so a node tracking a head its execution client had not validated still handed out votes for it. attestation_data and aggregate_attestation now answer 503 while the block they name is optimistic. A 503 is also what makes a validator client, ours included, fail over to its next beacon node. The node endpoints that report the same status now follow the spec's wording as well: /node/syncing's is_optimistic is about the head rather than any optimistic root in the store, which failed our validator client's duty refresh over a branch fork choice did not pick, and /node/health answers 206 on an optimistic head, not only while syncing.

github-actions · 2026-09-30T20:40:01Z

🤖 Kimi Code Review

I'll review this PR which implements optimistic sync safety guards for the Beacon API. Let me analyze each file.

`crates/net/rpc/src/beacon/node.rs`

Lines 42-48 (head_is_optimistic): The logic is correct per spec — only the fork-choice head matters, not arbitrary optimistic roots on side branches. Good.

Lines 62-75 (get_syncing / get_health): The change from store.has_beacon_optimistic_roots() to head_is_optimistic(&store) is the core semantic fix. The old code would report optimistic if any root was optimistic, which is wrong per spec.

One concern: Line 74 — el_offline is hardcoded false with a comment acknowledging this. The PR doesn't change this, but the get_health comment now says "The offline half is not detected." This is consistent but worth noting as a gap.

Lines 169-196 (test): Good test coverage for the head-only semantics. The report closure captures store by clone correctly for async.

Minor: Line 180 — store.clone() is cloned once in the closure capture, then line 183 clones again. This is slightly redundant but harmless:

let store = store.clone();  // line 180
// ...
store.clone(),  // line 183 - unnecessary second clone

The closure already owns the cloned Store. Remove the second .clone() on line 183.

`crates/net/rpc/src/beacon/pool.rs`

Lines 344-348 (doc comment): Good, clearly documents the 503 behavior.

Lines 361-363: The require_validated check is correctly placed after the 404 check. This ordering matters: a non-existent aggregate should still 404, not 503.

Lines 625-646 (test): Good test. Verifies both the 503 when optimistic and recovery to 200 when cleared.

One issue: Lines 636-637 — let mut store = fixture.store.clone(); gets a mutable clone, but fixture.store itself is also accessible. The test doesn't modify fixture.store, so this is fine, but the naming is slightly confusing. The comment on line 635 clarifies intent.

`crates/net/rpc/src/beacon/validator.rs`

Lines 155-175 (require_validated): Well-documented, correct error type. The 503 with ServiceUnavailable matches Beacon API spec.

Lines 398-402 (doc comment update): Correctly documents that the head is always the beacon_block_root due to earlier slot validation.

Line 419: require_validated(store, head_root)? — correct placement after slot validation, before expensive state computation.

Lines 729-753 (test): Good coverage. Tests both the optimistic refusal and recovery.

`crates/net/rpc/src/beacon/validator_client_tests.rs`

Lines 55-69 (serve_with_store): Good refactoring. The Store handle lets tests mutate state while the server runs.

Lines 256-276 (test): Excellent integration test. Verifies:

The VC interprets 503 as Error::BeaconNodeSyncing
is_retryable() is true
is_optimistic_or_syncing() reflects state
Recovery after validation

One concern: Line 261 — mut store is not actually mutated in a way that requires mut. The insert_beacon_optimistic_root and remove_beacon_optimistic_root calls use &mut self, so mut is required. Check if Store's interior mutability pattern is consistent — if Store uses Arc<RwLock<...>> internally, the &mut self on these methods is unusual. Verify this doesn't indicate a locking issue.

`crates/storage/src/store.rs`

Lines 679-687 (comment update): Documentation updated to reflect new readers. Correct.

Documentation (`docs/beacon_engine.md`, `docs/rpc.md`)

Updates are accurate and match implementation.

Summary

Severity	Item	Location
Minor	Redundant `store.clone()` in test closure	`node.rs:183`
Question	`Store` mutability pattern — `&mut self` methods on what appears to be a handle	`validator_client_tests.rs:261`

The PR is correct and well-tested. The semantic change from "any optimistic root" to "head is optimistic" is the right fix per the Beacon API specification. The 503 refusals on attestation_data and aggregate_attestation properly prevent validators from signing unvalidated blocks, with good failover behavior for validator clients.

One architectural note: The require_validated helper is clean and reusable. Consider if get_block and other validator endpoints that return block roots should also use it (future work, not this PR).

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

github-actions · 2026-09-30T20:40:18Z

🤖 Codex Code Review

Looks good overall. The change aligns with Beacon API optimistic-sync semantics, and the new tests cover the key validator-facing regressions.
No correctness bugs stood out in the consensus-sensitive paths touched here:
- crates/net/rpc/src/beacon/node.rs:46 correctly narrows node optimism to the current fork-choice head, avoiding false is_optimistic / 206 responses from unrelated side branches.
- crates/net/rpc/src/beacon/validator.rs:166 centralizes the 503 gating for validator-signable responses.
- crates/net/rpc/src/beacon/pool.rs:361 correctly refuses aggregate_attestation when the voted block is still optimistic, which is the security-critical behavior here.
Security/readiness note:
- crates/net/rpc/src/beacon/validator.rs:166 checks store.is_beacon_optimistic(root) at read time, which is the right fail-closed behavior for validator duties. That prevents handing a validator data for an EL-unvalidated block.
- crates/net/rpc/src/beacon/node.rs:81 intentionally still reports el_offline: false / no offline detection. That matches the comment, but it means health remains incomplete relative to the spec if the EL is actually unavailable. Not a bug in this PR, just a known gap.
Performance/readability:
- crates/net/rpc/src/beacon/node.rs:46 is a cheap check and strictly better than scanning “any optimistic root” for the API semantics.
- crates/net/rpc/src/beacon/pool.rs:361 adds only an O(1) map lookup after aggregation lookup; negligible overhead.
- require_validated is a nice idiomatic extraction and improves maintainability.
Minor suggestion:
- crates/net/rpc/src/beacon/node.rs:46 / crates/net/rpc/src/beacon/node.rs:81: if this “head optimism” concept keeps spreading, consider exposing it as a Store helper instead of a local RPC helper to avoid duplicating semantics across endpoints later. Not required for this PR.
I did not find issues in fork choice, justification/finalization, attestation validation, state transition, XMSS, or SSZ from the actual code touched by this diff.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

github-actions · 2026-09-30T20:40:21Z

🤖 Claude Code Review

Review of PR 636: refuse validator answers for an optimistic head

The change is correct and well scoped. I found no blocking issues.

What it does

/node/syncing is_optimistic and the /node/health 206 now follow the fork-choice head only. Before, they fired on any optimistic root in the store, including one on a side branch.
attestation_data returns 503 when the head is optimistic. The new require_validated check runs after the slot checks. That ordering matters because it makes the head the beacon_block_root actually answered, as the doc comment says.
aggregate_attestation returns 503 when the voted beacon_block_root is optimistic. This is correct, since it need not be the head. The check runs after the 404, so "nothing pooled" still wins over "optimistic".

Observations (non-blocking)

has_beacon_optimistic_roots (crates/storage/src/store.rs:3270) may have lost its last caller in node.rs. It's pub, so clippy won't flag it. If nothing else uses it, remove it, because the old "any root" semantics are the trap this PR fixes.
head_is_optimistic (node.rs) calls beacon_head() and is_beacon_optimistic() separately. Each takes its own lock, so the head can move between the two calls. The effect is one stale status response, and the next poll corrects it. That is acceptable, but it's worth knowing when you read the flag.
get_syncing and get_health both read the head, and get_syncing does so more than once. A head_is_optimistic helper that takes the already-read head root would avoid the repeat, but the cost is negligible.
aggregate_attestation only checks data.beacon_block_root. The target and source roots belong to ancestors of that block. mark_validated clears ancestors along with the block (the PR says "ancestor walk"), so the vote block being validated implies its ancestors are too. The reverse can't happen, so this is sound.
The 503 maps to BeaconNodeSyncing in the client, so validator clients fail over. The new end-to-end test in validator_client_tests.rs covers this, along with the recovery after the root is removed.

Tests

They cover each case the PR changes: side-branch root vs. head root, the 503 and then 200 after validation, and the aggregate route.
They work because Store is a shared handle (fixture.store.clone() observes the later inserts). The comment in pool.rs notes this.

Docs

rpc.md, beacon_engine.md and the storage doc comment are all updated and match the code.

Overall: LGTM. Item 1 is the only thing I'd consider before merging.

Automated review by Claude (Anthropic) · sonnet · custom prompt

The merge of main into beacon-chain-integration placed `mod banner;` after `mod beacon;`, which `cargo fmt --check` rejects, so Lint fails on every PR against this branch.

A node run without --execution-endpoint imports every block as NotRequired, so no block is ever marked optimistic and the optimistic check on attestation_data always passes. It then answers with a head whose payload nobody validated, which is what optimistic-sync.md forbids a validator to attest to. Refuse with 503 instead, as block production already does for its own reason.

An aggregator signs over the block its pooled votes name, and on a node run without an execution client nothing ever validated that block's payload. Every block imports as NotRequired there, so the optimistic check on aggregate_attestation always passes. Refuse with 503, the same way attestation_data now does, through one shared helper.

The Beacon API marks committee_index optional and deprecated, and tells gloas clients to omit it. We required it, so a client that follows the deprecation got a 400 from the query extractor and missed its attestation. The value was already ignored, since electra moved the committee out of AttestationData.

POST /eth/v1/beacon/states/{state_id}/validators allows either filter to be null to mean no filtering. serde's default covers only an absent field, so an explicit null failed the whole body with a 400, and a validator client sending one could not resolve its indices.

MegaRedHand added 2 commits September 30, 2026 17:55

style: sort the binary's module declarations

8f6069a

The merge of main into beacon-chain-integration placed `mod banner;` after `mod beacon;`, which `cargo fmt --check` rejects, so Lint fails on every PR against this branch.

MegaRedHand changed the title ~~fix(rpc): refuse attestation data for an optimistic head~~ fix(rpc): refuse attestation data for an unvalidated head Sep 30, 2026

MegaRedHand added 3 commits September 30, 2026 18:17

MegaRedHand added the beacon Ethereum Beacon Chain client label Oct 1, 2026

Navigation Menu

fix(rpc): refuse attestation data for an unvalidated head - #636

fix(rpc): refuse attestation data for an unvalidated head#636
MegaRedHand wants to merge 6 commits into
beacon-chain-integrationfrom
fix/optimistic-validator-503

MegaRedHand commented Sep 30, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 30, 2026

Uh oh!

github-actions Bot commented Sep 30, 2026

Uh oh!

github-actions Bot commented Sep 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

MegaRedHand commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Changes

Still open

Also in this PR

Test plan

Uh oh!

github-actions Bot commented Sep 30, 2026

🤖 Kimi Code Review

crates/net/rpc/src/beacon/node.rs

crates/net/rpc/src/beacon/pool.rs

crates/net/rpc/src/beacon/validator.rs

crates/net/rpc/src/beacon/validator_client_tests.rs

crates/storage/src/store.rs

Documentation (docs/beacon_engine.md, docs/rpc.md)

Summary

Uh oh!

github-actions Bot commented Sep 30, 2026

🤖 Codex Code Review

Uh oh!

github-actions Bot commented Sep 30, 2026

🤖 Claude Code Review

Review of PR 636: refuse validator answers for an optimistic head

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

MegaRedHand commented Sep 30, 2026 •

edited

Loading

`crates/net/rpc/src/beacon/node.rs`

`crates/net/rpc/src/beacon/pool.rs`

`crates/net/rpc/src/beacon/validator.rs`

`crates/net/rpc/src/beacon/validator_client_tests.rs`

`crates/storage/src/store.rs`

Documentation (`docs/beacon_engine.md`, `docs/rpc.md`)