[MegaRedHand]

🗒️ Description / Motivation

A checkpoint-synced ethlambda beacon paired with an execution client that is still syncing sat at its anchor for up to ~25 minutes, logging Not importing: the execution client has not validated this block and it is not an optimistic candidate for the first post-anchor block until it was 128 slots old. Found by ethrex's multisync (lambdaclass/ethrex#7461) on Hoodi: ethlambda parked ~11 minutes and failed the run's 10-minute head-stall check, while Lighthouse in the same setup imports immediately.

It is a spec deviation. specs/bellatrix/optimistic-sync.md:

def is_optimistic_candidate_block(opt_store, current_slot, block) -> bool:
    if is_execution_block(opt_store.blocks[block.parent_root]):   # parent.body.execution_payload != empty
        return True
    if block.slot + SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY <= current_slot:
        return True
    return False

and its "Checkpoint Sync" section lets the anchor be assumed VALID. ethlambda reads is_execution_block(parent) off the execution-hash cache, which was filled only when a block was imported. The anchor is never imported, so its first child looked like the child of a pre-merge block. The same gap made forkchoiceUpdated send a zero finalized/safe hash for the anchor, and a restart emptied the cache for every stored block (embedded ethrex restarts a few dozen blocks behind and answers SYNCING, so the park could repeat after a restart).

What Changed

• SignedBeaconBlock::execution_block_hash_if_execution_block (types): the one rule for which blocks have an execution hash to cache (the payload hash, minus a pre-merge all-zero one). Import now uses it, with no behavior change.
• get_forkchoice_store: caches the anchor's payload hash. Only the hash, no verdict.
• Store::restore_beacon_scratch: on resume, rebuilds the cache for the unfinalized LiveChain window it already walks, against each block's own slot so pruning still bounds it.
• docs/beacon_engine.md and the doc comments: the cache is filled at import, for the anchor, and on resume. The section that described the anchor park as expected behavior now describes the fix.

Correctness / Behavior Guarantees

• Nothing is marked validated: the anchor and resumed blocks get their hash only, so optimistic_roots, payload verdicts and latestValidHash handling are unchanged.
• Pre-merge bellatrix blocks (zero payload hash), phase0/altair and gloas blocks still cache nothing, so the 128-slot horizon still guards the merge transition and gloas is untouched.
• Behavior change: after checkpoint sync or a restart, the first new block is an optimistic candidate at once; fcU's finalized/safe hash is the anchor's payload hash instead of zero.

Tests Added / Run

• execution_block_hash_if_execution_block_gives_post_merge_hashes_only (types)
• a_post_merge_anchor_makes_its_first_child_an_optimistic_candidate, an_anchor_with_no_execution_payload_caches_no_hash, plus a gloas-anchor assertion (fork choice)
• resume_rebuilds_execution_hashes_for_the_unfinalized_window (storage)
• cargo test --profile release-fast --lib for ethlambda-types, ethlambda-storage, ethlambda-state-transition, ethlambda-blockchain: all pass.
• Live, on ethrex's multisync (ethrex+ethlambda on Hoodi, Sepolia and Platåberget) with an image of this branch: three full runs passed every EL and CL check on all three pairs, with no not an optimistic candidate line in any Hoodi log. Hoodi synced in 24 to 29 minutes. Before the fix, Hoodi failed its first run with the head stuck at the anchor for 10 minutes.

Related Issues / PRs

• Related to feat(l1): run any EL+CL pair in multisync and check the CL ethrex#7461 (the multisync check that found it)

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test (test-consensus plus test-node, at release-fast) — only the four crates' --lib tests above were run

[github-actions]

🤖 Claude Code Review

Review of PR 710: cache the anchor's execution hash, rebuild it on resume

The approach is sound and I found no blocking issues. The anchor is the one block that never goes through on_block, so its execution hash was never cached. That made its first child look pre-merge to is_optimistic_candidate_block. The same gap zeroed forkchoiceUpdated's finalized and safe hashes. Caching only the hash, not a verdict, keeps the "anchor is assumed valid" model intact.

What looks good

• SignedBeaconBlock::execution_block_hash_if_execution_block (crates/common/types/src/beacon/containers/mod.rs) puts the "non-zero payload hash means execution block" rule in one place. on_block, the anchor path and resume all use it, so they cannot drift apart.
• get_forkchoice_store reads the hash before insert_signed_block consumes the block. That avoids a clone of a large structure, which fits the repo's ownership guidance.
• Resume (restore_beacon_scratch) caches each hash against the block's own slot, so prune_beacon_el_block_hashes still bounds the cache. The restart path now matches the live path.
• Tests cover the post-merge anchor, the zero-hash and phase0 anchors, a gloas anchor (no hash cached), and the resume window boundary including a pre-merge row.

Minor points (non-blocking)

1. Stale doc comment. The restore_beacon_scratch doc comment (crates/storage/src/store.rs, around line 3719) still says it rebuilds only verified_payloads and block_timeliness. It should mention el_block_hashes.
2. Startup cost. Resume now decodes every full block body in the unfinalized window through read_with and decode_beacon_block_value. That should be fine, since the window is bounded. It is still the first full-body decode on the startup path, and the doc comment already says this scan is linear in chain length. A header-only way to read the hash would avoid it if resume time ever matters. That would need the payload hash in the header table, so it is not worth doing here.
3. Finalized block's cache entry. The finalized block is included because slot >= floor. When floor falls back to anchor_slot, the anchor is included too. Both are intended, and the test pins the finalized case. A short comment saying so next to the floor computation would help the next reader.
4. Doc numbers. The docs/beacon_engine.md rewrite drops the measured "85 slots back, about 9 minutes" example. That is reasonable now that the wait is gone.

Overall this is a clean, well-tested fix. Only the stale doc comment in point 1 is worth touching before merge.

---

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

[github-actions]

🤖 Codex Code Review

Looks good overall. The change is narrowly scoped, matches the spec intent around is_execution_block(parent), and the new tests cover the main consensus risk: a post-merge checkpoint anchor incorrectly looking pre-merge on the first descendant import.

• No correctness bug stands out in the touched logic. Caching the anchor’s EL hash in get_forkchoice_store at crates/blockchain/state_transition/src/beacon/fork_choice.rs:1125 fixes a real fork-choice/import edge case without changing payload-validity semantics.
• The switch to execution_block_hash_if_execution_block() in on_block at crates/blockchain/state_transition/src/beacon/fork_choice.rs:4740 is an improvement: it centralizes the “zero hash means not an execution block” rule and avoids duplicating that check.
• The resume-path rebuild in storage is consistent with the in-memory cache contract and correctly bounded to the unfinalized window, which is important for both optimistic import and latestValidHash walks.

Notes

• I did not find a security issue in this patch. It does not weaken payload validation, fork-choice safety, or finalization logic; it only restores the parent execution-block signal for anchor/resume cases.
• Performance impact should be negligible: one extra payload-hash extraction for the anchor, plus resume-time reconstruction from already persisted blocks.
• Test coverage is good for the changed behavior, especially crates/blockchain/state_transition/src/beacon/fork_choice.rs:6832 and the resume test in crates/storage/src/store.rs:8192.

Minor suggestion

• Consider adding one assertion that the anchor seeding does not accidentally affect pre-Gloas/Phase0 fork behavior beyond “no cache entry”. The existing test at crates/blockchain/state_transition/src/beacon/fork_choice.rs:6858 is close, but an explicit assertion on is_optimistic_candidate_block for a Phase0 anchor child would make the regression boundary even clearer.

Validation

• I attempted to run the targeted tests, but the environment blocks rustup from creating temp files under /home/runner/.rustup, so I could not execute them here.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which fixes execution hash caching for checkpoint-sync anchors and resume scenarios. Let me analyze the changes carefully.

Summary

This PR fixes a consensus-critical bug where post-merge checkpoint-sync anchors and their first children were incorrectly subject to the safe_slots optimistic import delay. The root cause was that get_forkchoice_store never cached the anchor's execution payload hash (since anchors skip on_block), and resume didn't rebuild the el_block_hashes cache.

---

Detailed Review

1. crates/common/types/src/beacon/containers/mod.rs — New method execution_block_hash_if_execution_block

Lines 1340-1355: The new method centralizes the "is this actually an execution block?" logic. This is a good refactor that eliminates the repeated && !hash.is_zero() pattern.

One concern: the doc comment says "Testing the hash alone is enough, since it is a keccak digest in a real payload and zero in the empty one." This is mostly true but relies on the invariant that no valid execution payload can hash to all zeros. Given keccak256's collision resistance, this is acceptable for consensus code, but consider making this invariant explicit with a debug_assert! or comment referencing the Bellatrix spec.

The method correctly returns None for Phase0, Altair, and Gloas (per doc comment and tests).

Lines 2261-2278: Tests cover the three cases well. Good.

---

2. crates/blockchain/state_transition/src/beacon/fork_choice.rs — Anchor hash caching

Lines 1122-1133: The core fix. Caching the anchor's EL hash at store construction:

let anchor_el_hash = anchor_block.execution_block_hash_if_execution_block();
// ...
if let Some(hash) = anchor_el_hash {
    store.insert_beacon_el_block_hash(anchor_root, anchor_slot, hash);
}

Critical correctness issue: The comment at line 1125 says "Only the hash, not a verdict" — this is important. The anchor gets cached as having an execution payload, but not as VALIDATED. This means is_optimistic_candidate_block will see beacon_el_block_hash(parent_root).is_some() as true (condition 1), but the block itself still needs payload validation. This appears correct per the spec's distinction between is_execution_block and payload validity.

However, I want to flag: does this interact correctly with has_verified_payload? Let me trace through. In is_optimistic_candidate_block (around line 914), the check is:

store.beacon_el_block_hash(parent_root).is_some()

This is purely about whether the parent is an execution block, not whether it's validated. So the anchor's unvalidated hash cache is correct here.

Lines 4725-4772: The change in on_block from:

let signed_block_el_hash = signed_block.execution_block_hash();
// ...
if let Some(el_block_hash) = signed_block_el_hash
    && !el_block_hash.is_zero()

to:

let signed_block_el_hash = signed_block.execution_block_hash_if_execution_block();
// ...
if let Some(el_block_hash) = signed_block_el_hash

This is a clean refactor with no behavior change — the !is_zero() check is now inside execution_block_hash_if_execution_block. Verified correct.

---

3. crates/storage/src/store.rs — Resume rebuilds cache

Lines 3744-3815: This is the second critical fix. On resume, the code now rebuilds el_block_hashes for the unfinalized window by decoding stored block headers.

Potential performance issue: Lines 3767-3780 perform a full block decode for every block in the unfinalized window:

if let Some(hash) = view
    .read_with(Table::BlockHeaders, &root.to_ssz(), |bytes| {
        decode_beacon_block_value(bytes).execution_block_hash_if_execution_block()
    })

This calls decode_beacon_block_value which deserializes the entire SignedBeaconBlock. For a large unfinalized window (could be hundreds of blocks post-merge), this is expensive at startup.

Suggestion: Consider adding a dedicated column family or a more compact storage for just the execution hash, or optimize decode_beacon_block_value to lazily decode only the body root/payload section. However, this is bounded by prune_beacon_el_block_hashes as noted, and resume is already doing full block decodes elsewhere in this loop (line 3781-3786 does another get + decode_fork).

Actually, looking more carefully: the loop already does view.get(Table::BlockHeaders, &root.to_ssz()) at line 3781. You're doing two reads from BlockHeaders for blocks with execution payloads — one in the read_with at line 3769, and another at line 3781. This is inefficient.

Recommendation: Restructure to avoid double reads. Decode the block once and reuse:

// Rough sketch:
let block = view.get(Table::BlockHeaders, &root.to_ssz()).expect("get");
let decoded = decode_beacon_block_value(&block);
if let Some(hash) = decoded.execution_block_hash_if_execution_block() {
    el_hashes.push((root, slot, hash));
}
let fork = decoded.fork_name(); // or however you get the fork

Wait, looking at the existing code: line 3781-3786 does:

let fork = view
    .get(Table::BlockHeaders, &root.to_ssz())
    .expect("get")
    .map(|bytes| decode_beacon_block_value(&bytes).fork_name())
    .expect("a stored block header decodes");

So yes, two full decodes. This should be refactored, though it's pre-existing debt. The PR introduces a second decode in the new code. At minimum, consider a follow-up to consolidate.

Memory safety note: The el_hashes vec is built up and then inserted after the loop (lines 3815-3817), which is correct — avoids holding the scratch lock during iteration, preventing deadlock with view operations.

---

4. Test coverage

fulu_anchor_pair (lines 6806-6824): Good helper, but I notice it hardcodes Root::ZERO for parent_root and state_root. For an anchor this is correct per spec, but the helper name doesn't convey "this builds an anchor." Consider renaming to fulu_anchor_pair_with_el_hash or adding a doc comment about anchor-specific construction.

a_post_merge_anchor_makes_its_first_child_an_optimistic_candidate (lines 6827-6852): Excellent test — directly verifies the bug fix. The assertion at line 6848 that is_optimistic_candidate_block returns true for the first child is the key behavior.

an_anchor_with_no_execution_payload_caches_no_hash (lines 6854-6884): Good negative test. Covers both zero-hash (pre-merge Bellatrix shape) and Phase0.

resume_rebuilds_execution_hashes_for_the_unfinalized_window (lines 8167-8222): Good, but I want to verify the slot arithmetic. SLOTS_PER_EPOCH - 3 etc. with beacon_backend_finalized_at(finalized) — need to check if finalized is at epoch boundary. Looking at the helper name, finalized is the finalized root, and the test expects old (slot SLOTS_PER_EPOCH - 3) to have no hash after resume. This implies old is at or before finality, which gets pruned. Correct.

Wait: old has slot SLOTS_PER_EPOCH - 3, finalized has slot SLOTS_PER_EPOCH - 2. If finalized is the finalized checkpoint, then old is before finality and should be pruned. But the test asserts reopened.beacon_el_block_hash(old) == None. This seems correct if finality prunes old.

However, I need to check: beacon_backend_finalized_at(finalized) — does this set the finalized checkpoint to finalized's epoch? If so, old at slot SLOTS_PER_EPOCH - 3 is in the same epoch as finalized at slot SLOTS_PER_EPOCH - 2 (epoch 0). That would mean old is not pruned by finality but might be pruned by something else? Or maybe the test setup puts finalized at epoch 1?

This is test helper logic I can't fully verify without seeing beacon_backend_finalized_at, but the test passes per CI presumably. The intent is clear.

---

5. Documentation update

docs/beacon_engine.md (lines 148+): Well-written explanation of the change. The updated text correctly notes that the anchor hash is cached at store build time, and that resume rebuilds the cache. The removal of the "9 minutes rather than full 128 slots" anecdote is appropriate since the fix eliminates that delay entirely.

---

Issues Found

Medium: Double block decode on resume

File: crates/storage/src/store.rs
Lines: 3767-3786

The new code adds a second full block decode per block in the unfinalized window. Consider refactoring the loop to decode once:

// Current (inefficient):
if let Some(hash) = view.read_with(Table::BlockHeaders, &root.to_ssz(), |bytes| {
    decode_beacon_block_value(bytes).execution_block_hash_if_execution_block()
}) { ... }
// ... later ...
let fork = view.get(Table::BlockHeaders, &root.to_ssz())
    .map(|bytes| decode_beacon_block_value(&bytes).fork_name())
    .expect(...);

Suggested fix: Decode once, extract both hash and fork:

let block_bytes = view.get(Table::BlockHeaders, &root.to_ssz())
    .expect("get")
    .expect("a stored block header exists");
let block = decode_beacon_block_value(&block_bytes);
if let Some(hash) = block.execution_block_hash_if_execution_block() {
    el_hashes.push((root, slot, hash));
}
let fork = block.fork_name();

This is pre-existing to some degree, but the PR makes it worse by adding a second decode path.

Low: Test helper could be clearer

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs
Lines: 6806-6824

fulu_anchor_pair hardcodes anchor-specific values (Root::ZERO for parent, specific header construction). Consider a more explicit name like fulu_anchor_pair_with_hash or document the anchor invariants.

---

Consensus Correctness Verdict

The core logic appears correct and safe:

1. execution_block_hash_if_execution_block properly implements the spec's is_execution_block predicate
2. Anchor caching at store construction fixes the first-child delay without bypassing validation
3. Resume rebuild maintains correctness across restarts
4. The "hash without verdict" distinction is preserved throughout

No consensus bugs detected. The fix addresses a real liveness issue (unnecessary optimistic import delays) without introducing safety violations.

---

Summary

Aspect	Verdict
Consensus correctness	✅ Correct
Security	✅ No vulnerabilities introduced
Performance	⚠️ Double block decode on resume (medium)
Rust idioms	✅ Good use of Option, filter
Test coverage	✅ Comprehensive
Documentation	✅ Clear and accurate

Recommend merge after considering the double-decode optimization.

---

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