⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Known Issues

Tests intentionally excluded from CI. Source of truth for the Known
Issues section the L1 workflow appends to each ef-tests job summary
and posts as a sticky PR comment.

EF Tests — Stateless coverage narrowed to EIP-8025 optional-proofs

make -C tooling/ef_tests/blockchain test calls test-stateless-zkevm
instead of test-stateless. The zkevm@v0.3.3 fixtures are filled against
bal@v5.6.1, out of sync with current bal spec; the broad target trips ~549
fixtures. Re-broaden once the zkevm bundle is regenerated.

Lines of code report

Total lines added: 1
Total lines removed: 3
Total lines changed: 4

+----------------------------------------------------+-------+------+
| File                                               | Lines | Diff |
+----------------------------------------------------+-------+------+
| ethrex/crates/blockchain/fork_choice.rs            | 170   | +1   |
+----------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/fork_choice.rs | 562   | -3   |
+----------------------------------------------------+-------+------+

🤖 Kimi Code Review

The change correctly implements the Engine API specification update (execution-apis PR 786) for handling canonical head references that are ancestors of the finalized block.

Key observations:

crates/networking/rpc/engine/fork_choice.rs:319-332

1. Spec compliance: Returning None instead of Some(head_block) properly prevents payload building when the forkchoice head is already canonical and finalized. This satisfies the requirement to return VALID with null payloadId without initiating a build process.

2. Performance: Eliminates an unnecessary storage lookup (get_block_header_by_hash) in the hot path of forkchoice handling. This is a minor but worthwhile optimization for an RPC endpoint that may be called frequently.

3. Error handling: Removing the ? operator on the storage fetch (old line 326) eliminates a potential error source where a valid forkchoice state could fail due to transient storage issues when fetching the header.

4. Security/DoS prevention: This prevents a potential DoS vector where a validator could repeatedly send forkchoiceUpdated calls with payloadAttributes on an already-canonical head, triggering unnecessary payload builds.

Suggestion: Ensure that InvalidForkChoice::NewHeadAlreadyCanonical is only returned when the head is indeed a valid ancestor of the finalized block (or the finalized block itself). If this variant could be returned in scenarios where the head changed but happens to be canonical (e.g., reorg to an already-known canonical branch), verify that skipping the payload build is still the desired behavior per the spec.

Otherwise, the change is correct and safe.

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

Greptile Summary

This PR fixes a spec-compliance bug where engine_forkchoiceUpdatedV3/V4 (and V1/V2) would start a payload build and return a non-null payloadId even when headBlockHash pointed at a canonical ancestor of the latest finalized block. Per execution-apis PR #786, that case must return {payloadStatus: VALID, payloadId: null} and the client must not initiate payload construction.

• The NewHeadAlreadyCanonical error arm in handle_forkchoice previously fetched the head block header and returned Some(header), which let the V1–V4 dispatch guards pass and call build_payload; the fix returns None so those guards short-circuit and no payload is built.
• The VALID + latestValidHash = head response shape is preserved; only the head-header fetch and the non-null payload-id path are removed.

Confidence Score: 4/5

The change is a focused, well-motivated single-arm fix with no side-effects on other fork-choice paths; safe to merge.

The diff is minimal and the logic is sound: returning None from the NewHeadAlreadyCanonical arm reliably blocks the build_payload path for all FCU versions. The blockchain-layer guard uses head.number <= stored_finalized (inclusive of head == finalized itself), while both the new comment and the spec describe the case as a strict ancestor — the equality case is also silently blocked from payload building, which is safe but the comment is slightly misleading. No tests were added for the corrected path, which makes future regressions harder to catch.

crates/networking/rpc/engine/fork_choice.rs — the NewHeadAlreadyCanonical arm and its interaction with V1/V2 handlers (which are also affected by this change, though only V3/V4 are mentioned in the PR description)

Sequence Diagram

sequenceDiagram
    participant CL as Consensus Layer
    participant RPC as engine_forkchoiceUpdatedV3
    participant HFC as handle_forkchoice
    participant AFC as apply_fork_choice
    CL->>RPC: "FCU(head = finalized-ancestor, payloadAttrs)"
    RPC->>HFC: handle_forkchoice(state, ctx, 3)
    HFC->>AFC: apply_fork_choice(head, safe, finalized)
    Note over AFC: head.number <= stored_finalized AND head_is_canonical
    AFC-->>HFC: Err(NewHeadAlreadyCanonical)
    Note over HFC: OLD: return Some(head_header) / NEW: return None
    HFC-->>RPC: (None, VALID + latestValidHash)
    Note over RPC: if let (Some(_), Some(_)) guard FAILS
    RPC-->>CL: "{payloadStatus: VALID, payloadId: null}"

_{Reviews (1): Last reviewed commit: "fix(l1): suppress payload build on FCU w..." | Re-trigger Greptile}

🤖 Codex Code Review

1. crates/networking/rpc/engine/fork_choice.rs:328 introduces a protocol-visible regression: returning None for NewHeadAlreadyCanonical skips not just payload construction, but all payload-attribute validation in the RPC handlers (:42, :75, :124, :164). After this change, requests with non-null but invalid payloadAttributes will be accepted as VALID with payloadId: null instead of returning the existing validation errors. Examples:
   • V1/V2 at Cancun timestamps bypass UnsupportedFork (:44-47, :77-80).
   • V2 post-Shanghai can omit withdrawals and still get VALID (:81-84).
   • V3/V4 can omit parent_beacon_block_root / use invalid timestamps and still get VALID (:125, :165).
     The fix should separate “do not start payload build” from “do not return the head header”; e.g. return the header plus an explicit should_build_payload flag, or a small enum describing the FCU outcome.

Other than that, the intent of aligning the no-build path with the finalized-ancestor rule looks reasonable, and I don’t see an additional security or performance concern in this small diff.

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

🤖 Claude Code Review

Here is the full review:

PR Review: fix(l1) — suppress payload build on FCU with finalized-ancestor head

Overall: This is a correct, targeted fix with good spec alignment. The change is minimal (3 net lines) and the rationale is well-explained in both the PR description and the updated comment.

Correctness

The core fix is sound. The key property to verify is whether NewHeadAlreadyCanonical is triggered only for the intended case (finalized-ancestor heads), not for any arbitrary canonical head including the current tip.

From crates/blockchain/fork_choice.rs:81-86, the guard is:

if let Some(stored_finalized) = store.get_finalized_block_number().await?
    && head.number <= stored_finalized
    && head_is_canonical
{
    return Err(InvalidForkChoice::NewHeadAlreadyCanonical);
}

All three conditions must hold simultaneously. A head at the current canonical tip (beyond the finalized height) passes this check and proceeds normally — so returning None here does not suppress payload building for the normal re-FCU case. The fix is safe.

The if let (Some(head_block), Some(attributes)) guard in the V3/V4 handlers (lines 121–131, 161–170) is the correct short-circuit point. Returning None for the header cleanly routes through existing logic with no new branching.

Minor comment imprecision

The new comment says:

"when head references a VALID ancestor of the latest known finalized block"

The actual condition is head.number <= stored_finalized, which includes the case where head is equal to the finalized block (not just a strict ancestor). The comment should say "at or below the latest known finalized block" to match the code precisely. Not a bug, but worth a one-word fix to avoid future confusion.

Unnecessary storage read eliminated

The old code called context.storage.get_block_header_by_hash(fork_choice_state.head_block_hash)? solely to produce the header that validate_attributes_v3/v4 needed before calling build_payload. Since both of those are now skipped, the storage read was dead work. Removing it is a clean improvement.

Test coverage gap

The PR validates with cargo test -p ethrex-rpc, but no targeted test was added for the specific regression scenario: FCU to a finalized-ancestor head with non-null payloadAttributes must return VALID + payloadId: null. Without a test, a future refactor of the if let (Some(_), Some(_)) guard or the NewHeadAlreadyCanonical arm could silently regress this behavior. A single engine-API integration test or unit test covering this case would strengthen confidence.

Summary

• Spec compliance: correct per execution-apis PR LEVM: (EF Tests) stTransitionTest #786
• Logic: safe — does not affect the normal re-FCU-to-tip path
• Side effects: set_synced() is correctly retained; storage read is correctly removed
• The only actionable suggestion is adding a regression test for FCU(finalized-ancestor, payloadAttributes != null) → VALID + payloadId: null
• The comment imprecision ("ancestor" vs. "at or below") is cosmetic

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

Thanks for the review @ElFantasma. Both nits addressed:

• Comment reworded to lead with the guard shape (head.number < latest strict-ancestor check, equality falls through) — 6d6a49f
• Regression test added in test/tests/rpc/fork_choice_tests.rs driving engine_forkchoiceUpdatedV3 with head = finalized ancestor + non-null payloadAttributes, asserts {VALID, latestValidHash=head, payloadId=null} per execution-apis#786 — 36a1225