fix: handle pruned justified root in compute_lmd_ghost_head

[pablodeymo]

Motivation

ethlambda crashes during devnet operation when compute_lmd_ghost_head performs an unchecked dictionary access (blocks[&start_root]) and the start_root (justified checkpoint root) has already been pruned from the LiveChain table.

Crash sequence:

1. Node restarts from genesis (justified checkpoint = genesis block at slot 0)
2. Node syncs rapidly to slot ~7200
3. Finalization advances → prune_live_chain(finalized_slot) removes entries with slot < finalized_slot — genesis (slot 0) is pruned
4. update_head() calls compute_lmd_ghost_head(store.latest_justified().root, &blocks, ...)
5. blocks (from get_live_chain()) no longer contains the genesis root
6. PANIC at blocks[&start_root].0 — "no entry found for key"

Both update_head and update_safe_target in store.rs are affected since they both call compute_lmd_ghost_head.

Description

Replace the panicking index blocks[&start_root].0 with blocks.get(&start_root) and return start_root as a fallback when it is absent from the live chain.

// Before (panics if start_root not in blocks):
let start_slot = blocks[&start_root].0;

// After (return start_root if not in blocks):
let Some(&(start_slot, _)) = blocks.get(&start_root) else {
    return start_root;
};

Rationale: If the justified checkpoint root is not in the live chain, fork choice cannot traverse beyond it. Returning start_root is the safe fallback — it matches the existing early-return behavior for blocks.is_empty() (line 16–18) and ensures the head stays at the justified root until more blocks are available. Normal operation (justified root present in LiveChain) is unaffected since .get() returns Some and destructures identically.

How to test

make lint   # No warnings
make test   # All tests pass (26/26 forkchoice spec tests pass)

The fix only changes the error case — when start_root is absent from blocks, which currently panics.

[github-actions]

🤖 Kimi Code Review

Security & Correctness Issues

File: crates/blockchain/fork_choice/src/lib.rs

1. Line 26-28: The early return when start_root is not found in blocks is dangerous. This can cause the fork choice to return an invalid head block that doesn't exist in the local view, potentially leading to:

   • Syncing issues between nodes
   • Consensus split if different nodes have different views
   • Attack vector where missing blocks could influence fork choice

   Fix: This should be a hard error (panic! or return Result) since start_root must exist in blocks for correct fork choice operation.

2. Missing validation: The function doesn't validate that all attestations reference blocks that exist in the blocks map. This could allow attestations for non-existent blocks to influence weights.

3. No handling of attestations for future slots: Attestations for slots > current slot should probably be ignored but aren't filtered.

Performance & Style

4. Line 30: Consider pre-allocating the weights HashMap with HashMap::with_capacity(blocks.len()) to avoid rehashing.

5. Line 33: The function iterates over attestations.values() but only uses the attestation_data field. Consider changing the parameter type to avoid unnecessary cloning if attestations contains owned data.

Correctness Question

6. Fork choice algorithm: The LMD GHOST implementation appears incomplete - there's no visible loop to walk back from leaf blocks to find the head, only weight accumulation. This might be truncated in the diff, but the current visible code only accumulates weights without actually selecting the head block.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Findings

• crates/blockchain/fork_choice/src/lib.rs:26-27: If start_root is not in blocks, the function now returns start_root silently. This can mask an inconsistent fork-choice view and may allow returning a head that isn’t actually in the known block DAG. In consensus code, that’s risky: it can lead to an invalid head selection or liveness issues if upstream passes a stale/unknown root. Consider one of:
  • Return the minimum-slot root (same fallback used when start_root is zero), or
  • Change the signature to return a Result<H256, ForkChoiceError> and propagate an error, or
  • Keep a debug assertion/metric/log and fall back to a safe root.
• crates/blockchain/fork_choice/src/lib.rs:26-27: This change likely warrants a test to lock expected behavior. Add a unit test covering start_root not present in blocks, to ensure the chosen fallback is explicit and stable.

No other changes in this PR. Overall the edit is small and readable, but the silent fallback is a consensus-risky behavior change unless explicitly intended.

---

Automated review by OpenAI Codex · custom prompt

[greptile-apps]

Greptile Summary

Fixes a panic in compute_lmd_ghost_head when the justified checkpoint root has been pruned from LiveChain. The crash occurred during devnet operation after rapid sync when prune_live_chain removed the genesis block (justified root at slot 0), but fork choice still tried to access it via unchecked dictionary lookup.

Changes:

• Replaced panicking blocks[&start_root].0 with safe blocks.get(&start_root)
• Returns start_root as fallback when not found in live chain (consistent with existing blocks.is_empty() early-return on line 16-18)
• Affects both update_head and update_safe_target callers in store.rs

Impact:

• Prevents node crashes during normal operation after finalization advances
• Safe fallback: head stays at justified root until more blocks are available for fork choice traversal
• No behavioral change when justified root is present (normal operation)

Confidence Score: 5/5

• Safe to merge - fixes critical panic with minimal, well-reasoned defensive code
• The change is a targeted fix for a documented crash scenario. The fallback behavior (returning start_root when not in LiveChain) is logically sound and consistent with existing early-return patterns. All 26 forkchoice spec tests pass, and the caller code (store.rs) already handles the returned root safely with .unwrap_or(0) for slot lookups
• No files require special attention

Important Files Changed

Filename	Overview
crates/blockchain/fork_choice/src/lib.rs	Replaced panicking dictionary access with safe .get() that returns start_root when justified checkpoint has been pruned from LiveChain - prevents crash during devnet operation

_{Last reviewed commit: 9569a1c}

[github-actions]

🤖 Claude Code Review

Now I have everything I need to produce the review.

---

PR #137 Review: fix: handle pruned justified root in compute_lmd_ghost_head

Summary

This is a focused 3-line fix for a real crash: blocks[&start_root] panics when the justified checkpoint root has been pruned from LiveChain. The fix correctly replaces the panicking index with a let-else that returns start_root as fallback.

---

Correctness

The fix is correct. The crash scenario is valid: after rapid sync, prune_live_chain(finalized_slot) can remove blocks (including genesis at slot 0) from LiveChain while store.latest_justified().root still points to that pruned root. The fallback return start_root mirrors the existing early-return at line 16–18:

if blocks.is_empty() {
    return start_root;
}

Both cases mean "we cannot traverse forward from the justified root" — returning it as the head is the correct conservative choice.

The let Some(&(start_slot, _)) = blocks.get(&start_root) else { ... } pattern is idiomatic and correct. Destructuring (start_slot, _) and discarding the parent root is appropriate since only the slot is needed for the traversal filter on line 34.

---

Concerns

Missing diagnostic log (minor)

When the fallback path fires, there is no indication in the logs. This makes it harder for operators to distinguish a healthy restart from a node stuck in a degraded state. A warn! call would be valuable here:

let Some(&(start_slot, _)) = blocks.get(&start_root) else {
    warn!(
        start_root = %ShortRoot(&start_root.0),
        "Justified root not in live chain, returning it as head"
    );
    return start_root;
};

Downstream expect in update_safe_target

update_safe_target (store.rs:73) contains:

let head_state = store.get_state(&store.head()).expect("head state exists");

After the fix, if the new head (set in update_head) is the pruned justified root and its entry in the States table has also been removed, this expect would panic. This is a pre-existing invariant assumption (States is not pruned along with LiveChain) and is not introduced by this PR, but it is worth auditing whether States and LiveChain pruning are always kept in sync. If States entries for pre-finalized blocks are ever removed, this expect becomes the next crash site.

Root cause note

The scenario that triggers this fix implies latest_justified().root can refer to a block at a slot below latest_finalized().slot. In on_block, justified and finalized are updated together (lines 365–372 of store.rs), so under normal operation both advance atomically. However, during fast sync, blocks are processed in bulk — it's worth verifying that update_checkpoints is never called with a finalized slot that is ahead of the justified slot stored in the state. If that invariant does hold, the justified root should never be pruned. The fix is still the right safety net regardless, but understanding if/when latest_justified.slot < latest_finalized.slot can be true in post-sync state would help determine whether a deeper invariant assertion or log warning is warranted.

---

Spec test coverage

The PR mentions all 26 forkchoice spec tests pass. Worth noting: spec fixtures likely don't exercise this specific pruning edge case (pruned justified root during live sync), so the fix is not directly validated by them. A targeted unit test that constructs a blocks map that is missing start_root and asserts the function returns start_root without panicking would lock in the regression fix.

---

Verdict

The fix is correct and safe to merge. It eliminates a real crash with a minimal, well-reasoned change. The suggestions above (log, downstream expect, unit test) are improvements but not blockers.

---

Automated review by Claude (Anthropic) · custom prompt