fix(beacon): persist unrealized justifications so get_head survives a restart

[MegaRedHand]

Stacked on #627. It reuses #627's pruning cutoff, so it should merge after #627.

Why

Beacon fork choice keeps each block's unrealized justified checkpoint (consensus-specs' store.unrealized_justifications[block_root]) only in memory, in BeaconScratch. A restart empties that map, and nothing refills it:

• the resume path doesn't re-import the unfinalized window, despite what the old doc comments said;
• re-delivered blocks are skipped before on_block by the has_state checks.

Once a block imported before the restart is a leaf from an epoch older than the store clock, get_voting_source fails with SpecAssert("block_root in store.unrealized_justifications"). filter_block_tree passes that error up into get_head.

What an operator sees: no crash, but warn "Failed to compute beacon head" on every tick, and the head frozen:

• lean_head_slot stays flat;
• the EL gets a stale forkchoiceUpdated;
• the Beacon API and our Status message report a stale head.

The head leaf from before the restart clears once a child of it is imported. A stale fork leaf keeps failing until justification moves past it.

Other clients

• Lighthouse persists the whole fork-choice tree, including each node's unrealized checkpoints. It writes it on every epoch transition, on every reorg and at shutdown, and falls back to the node's realized justified checkpoint when the value is missing.
• Prysm persists nothing. At startup it rebuilds fork choice from the DB, covering the finalized-to-justified chain, and seeds every node's unrealized checkpoint with the store's current justified checkpoint.

This PR does both: it persists the values the way Lighthouse does, and on a miss it falls back the way Prysm does (see "Fallback on a miss" below).

What

• New table BeaconUnrealizedJustifications. The key is the block root; the value is slot (8 bytes, big-endian) ‖ Checkpoint SSZ.
  • The key is the root alone, because both readers (get_voting_source, is_ffg_competitive) look up by root without a slot. The slot is in the value only for the pruner.
  • The in-memory map stays as a write-through cache in front of the table.
• Written by compute_pulled_up_tip (called from on_block) and when the anchor is set up in get_forkchoice_store.
• Pruned on each finalization, in the Chain::Beacon arm of Store::update_checkpoints, to fix(storage): prune the beacon LiveChain below the finalized block #627's cutoff: the finalized block's own slot. So the finalized block's own entry survives a resume that has imported nothing past it yet.
• DB_VERSION 4 → 5. A data directory written by version 4 has no row for any block it already imported. (The first commit's message says 4; the merge from beacon-chain-integration, which picked up lambdaclass/ethlambda_private#44's bump, makes it 5.)
• Fixes the two doc comments that claimed a resume re-imports the unfinalized window. Updates docs/data_storage.md (new table section, table diagram, pruning, what stays in memory) and CLAUDE.md (table count, DB_VERSION bullet).

Fallback on a miss (5a7d6445)

The entry is written right after the block's import, not in the same atomic batch, because insert_state hands the post-state to the background state writer. So a crash can still leave one block with a state but no entry: the writer flushes the state, then the process dies before the entry is written. On restart has_state skips re-importing that block.

For that case, get_voting_source no longer raises on a miss. It returns store.beacon_justified_checkpoint(), the value Prysm gives every block it rebuilds at startup:

• Viability only. That value always makes the leaf viable in filter_block_tree. It decides whether a leaf counts, never how much weight it casts, so the worst case is a stale fork leaf competing on weight, not a frozen or regressed head.
• Read-time only. The fallback is never written to the table or the cache, so it can't later be read back as the exact value.
• Logged at debug!, not warn!. get_head runs every tick, so a warning would repeat until the block stops being a leaf.
• is_ffg_competitive still raises on a miss. Only the spec tests reach it.
• Test helper. Store::delete_unrealized_justification is a public test helper, following the existing insert_live_chain_entry/delete_live_chain_entries convention, since another crate's tests call it.

Tests

• get_head_survives_a_restart_over_a_pre_restart_leaf and get_head_survives_a_restart_past_a_pre_restart_fork_leaf both fail on the base branch and pass here. They import through the real compute_pulled_up_tip, then reopen the store the way main.rs resumes.

• an_unrealized_justification_persists_across_a_reopened_store replaces the old test that pinned the value as in-memory only.

• A pruning test with an empty first slot of the finalized epoch: the finalized block's entry survives, and entries below it go.

• get_voting_source_falls_back_to_the_justified_checkpoint_on_a_miss and get_head_survives_a_dropped_unrealized_justification (the crash window: entry deleted from the table and the cache, get_head still returns that block).

After merging the base and adding the fallback:

Command	Result
ethlambda-storage --lib	128 passed, 1 ignored
ethlambda-state-transition --lib beacon::fork_choice	33 passed
ethlambda-state-transition --lib beacon::gossip	40 passed
ethlambda-blockchain --lib	132 passed
beacon_spec_tests -- fork_choice (mainnet fixtures)	151 passed
clippy on all three crates with -D warnings	clean

Deploying

Needs a fresh checkpoint sync on every follower, since the build refuses a version-4 data directory. lambdaclass/ethlambda_private#44's bump to 4 needs one too, so deploying both together costs one checkpoint sync.

Moved

• Moved from lambdaclass/ethlambda_private#50, now that beacon-chain-integration lives on this repo.
• Stacked on fix(storage): prune the beacon LiveChain below the finalized block #627 (was lambdaclass/ethlambda_private#46); its head, which carries beacon-chain-integration @ c79fabd5, is merged into the branch.

[github-actions]

🤖 Claude Code Review

Review of PR 628: persist beacon unrealized justifications

The approach is sound. It fixes a real get_head freeze after restart. The table design, prune horizon, and tests are careful and well documented. The findings below are mostly minor.

Findings

1. No read-through on a cache miss (store.rs, Store::unrealized_justification).

   • The docs call the map a "write-through cache", but a hit in the table never refills it.
   • After a restart, every get_voting_source call for a pre-restart prior-epoch leaf does a begin_read and point read. That happens once per leaf per tick, until finalization prunes it.
   • The cost is small, but one insert into the map on a table hit would make the "hit costs no backend round trip" claim true. You'd need to take the lock once more.
   • Alternatively, say in the docs that only post-restart blocks are cached.

2. The crash-window fallback can change fork choice (fork_choice.rs, get_voting_source).

   • The fallback returns store.beacon_justified_checkpoint(), which always passes the viability check (voting_source.epoch == store.justified.epoch). A leaf with a lost row is therefore always viable, even if its real unrealized source would have filtered it out.
   • The doc comment says this only ever makes a leaf "more likely to survive". That matters, because a block the spec would have excluded could win the head.
   • The window is narrow, and staying live is a reasonable trade against freezing. I'd still state the divergence from the spec plainly, and consider a warn! that fires once per root. debug! hides a consensus deviation in production.
   • Alternatively, recompute the value from the block's post-state. The old doc said this was possible, and compute_pulled_up_tip already does it.

3. DB_VERSION 5 invalidates lean data directories (store.rs:181).

   • The new table is beacon-only, but from_db_state refuses any version mismatch. Every existing lean directory therefore needs a wipe or resync, with no functional reason.
   • If that cost is intended, call it out in the PR description or release notes. Otherwise, gate the new requirement on Chain::Beacon.

4. Anchor slot mismatch (fork_choice.rs, get_forkchoice_store).

   • The anchor entry uses anchor_state.slot(). The doc comments elsewhere note that a checkpoint-synced state can sit past its own latest_block_header when the boundary slot was empty.
   • The table documents the value as the block's own slot, but here it can be the state's slot.
   • The effect is harmless, since it only makes the anchor row outlive pruning slightly longer. It is still inconsistent, so prefer the anchor block's slot.

5. Separate commit per import (set_unrealized_justification).

   • Each call opens its own write batch and commit, which is an extra commit on the block-import hot path. The PR explains why it can't join the block's batch (the post-state comes from the async state writer).
   • Keep that in mind if import latency shows up in profiles.

6. Error handling (prune_unrealized_justifications).

   • filter_map(|res| res.ok()) silently drops iterator errors, so a failed row is never pruned and nothing is logged. If prune_live_chain does the same, this is consistent. Otherwise, log the error.
   • decode_unrealized_justification_value panics on corrupt or short bytes through bytes[..8]. That is acceptable for storage that only this code writes, but a length check with a clear message would help.

Good

• Pruning to the finalized block's own slot keeps the finalized block's entry, and the test covers the empty-epoch-start case.
• The restart, stale-fork-leaf, and dropped-row tests reproduce the original bug.
• The docs and DB_VERSION bump are consistent across CLAUDE.md, docs/data_storage.md, and the table doc comments.

I only read the diff. I didn't build the code or run any tests.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which adds persistence for beacon unrealized justifications to fix a restart-induced get_head freeze.

Summary

The PR correctly identifies and fixes a critical consensus bug: BeaconScratch::unrealized_justifications was purely in-memory, so restarting a node with pre-restart leaves from older epochs caused get_voting_source to hit a hard SpecAssert, freezing get_head. The fix introduces Table::BeaconUnrealizedJustifications with a write-through cache pattern.

Issues Found

1. Race Condition in Pruning: Cache Inconsistency Window (Critical)

File: crates/storage/src/store.rs, lines 3526–3547

let mut batch = self.backend.begin_write().expect("write batch");
batch
    .delete_batch(
        Table::BeaconUnrealizedJustifications,
        keys_to_delete.clone(),
    )
    .expect("delete pruned unrealized justifications");
batch.commit().expect("commit");

let mut scratch = self.beacon.lock().unwrap();  // Lock acquired AFTER commit
for key in &keys_to_delete {
    scratch
        .unrealized_justifications
        .remove(&H256::from_slice(key));
}

Problem: The in-memory cache is updated after the database commit. If a concurrent reader calls unrealized_justification() between batch.commit() and acquiring the beacon lock, it gets a stale cache hit for a row that no longer exists in the database. On restart, that cache is gone and the fallback path triggers—but this means the node can observe two different values for the same root across the same logical state, violating determinism.

Fix: Acquire the beacon lock before the database commit, or use a write lock to cover both operations atomically:

let mut scratch = self.beacon.lock().unwrap();  // Lock FIRST
// ... collect keys_to_delete using scratch + backend view ...

let mut batch = self.backend.begin_write().expect("write batch");
batch.delete_batch(...).expect(...);
batch.commit().expect(...);

for key in &keys_to_delete {
    scratch.unrealized_justifications.remove(&H256::from_slice(key));
}
// Lock drops here, after both structures are consistent

2. Panic on Corrupted Value in decode_unrealized_justification_value (Medium)

File: crates/storage/src/store.rs, lines 744–748

fn decode_unrealized_justification_value(bytes: &[u8]) -> (u64, BeaconCheckpoint) {
    let slot = u64::from_be_bytes(bytes[..8].try_into().expect("valid slot bytes"));
    let checkpoint = BeaconCheckpoint::from_ssz_bytes(&bytes[8..]).expect("valid checkpoint");
    (slot, checkpoint)
}

Problem: Both .expect() calls will panic on corrupted database data. While other SSZ decodes in this file also use expect, this particular path is hit during pruning (line 3517) where the iterator scans arbitrary rows. A single corrupted row would crash the node during finalization.

Fix: Return Option or Result and skip/filter invalid entries during pruning, logging a warn! instead of crashing:

fn decode_unrealized_justification_value(bytes: &[u8]) -> Option<(u64, BeaconCheckpoint)> {
    let slot = u64::from_be_bytes(bytes.get(..8)?.try_into().ok()?);
    let checkpoint = BeaconCheckpoint::from_ssz_bytes(bytes.get(8..)?).ok()?;
    Some((slot, checkpoint))
}

And in prune_unrealized_justifications:

.filter(|(_, value)| {
    decode_unrealized_justification_value(value)
        .map(|(slot, _)| slot < finalized_block_slot)
        .unwrap_or(false)  // Don't prune what we can't parse; or log warn
})

3. Inefficient Full-Table Scan on Every Finalization (Performance)

File: crates/storage/src/store.rs, lines 3509–3520

The prefix_iterator(Table::BeaconUnrealizedJustifications, &[]) with &[] scans the entire table. The doc comment acknowledges this trade-off, but for large unfinalized windows (e.g., during sync or long periods without finality), this is O(n) where n = unfinalized blocks.

Mitigation: The comment says "cheap here" but doesn't quantify. With 32 slots/epoch and potentially hundreds of epochs without finality, this could be thousands of entries decoded per finalization. Consider:

• Adding a metric/counter for pruned_justifications duration (already logged count, but not time)
• Or a secondary index if this becomes problematic (though I accept the single-point-read trade-off)

4. Missing Atomicity: Block Import vs. Unrealized Justification Write (Design/Security)

File: crates/storage/src/store.rs, lines 3467–3485

The doc comment correctly notes the crash window between insert_signed_block/insert_state and set_unrealized_justification. The fallback to store.beacon_justified_checkpoint() is safe for get_voting_source, but:

Concern: This creates a liveness issue, not a safety issue, which the PR handles. However, the fallback in get_voting_source means a block can temporarily appear more viable than it should be (since store.beacon_justified_checkpoint() may have a higher epoch than the block's true unrealized justification). This could theoretically affect fork choice during the window before the true value is written.

Verification requested: The test get_head_survives_a_dropped_unrealized_justification confirms liveness, but does not verify that the fallback doesn't cause the block to be incorrectly preferred over a competing branch. Add a test where two leaves have different unrealized justifications, one is missing, and verify the fallback doesn't flip the head incorrectly.

5. Inconsistent Error Handling: expect in set_unrealized_justification (Medium)

File: crates/storage/src/store.rs, lines 3467–3485

pub fn set_unrealized_justification(...) {
    // ... cache update ...
    let mut batch = self.backend.begin_write().expect("write batch");
    // ...
    batch.commit().expect("commit");
}

Problem: This is a synchronous write on what appears to be a hot path (compute_pulled_up_tip for every block import). The expect will panic on any backend error (disk full, I/O error). Other store methods return Result; this one doesn't.

Question: Is this intentional? The anchor setup in get_forkchoice_store also uses .expect("insert") for block/state insertion, suggesting a pattern of panicking on persistence failures. For a consensus client, this is arguably correct (halt rather than continue with corrupted state), but should be documented as intentional.

6. Test Helper import_unchecked Skips Signature Verification (Minor)

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, lines 3226–3242

fn import_unchecked(...) -> Root {
    // ...
    // `state_transition` is skipped, since it would need properly signed blocks
    // and nothing here depends on it.
}

Concern: The comment is accurate for unit tests, but this helper is dangerously named if it ever leaks into production-adjacent code. Consider marking #[cfg(test)] explicitly (it may already be, via module structure) or renaming to import_unchecked_test_only.

7. Doc Comment Inaccuracy: "Two Writers"

File: docs/data_storage.md, line 541

Two writers: the anchor, at bootstrap (get_forkchoice_store), and compute_pulled_up_tip

Correction: There are three writers if you count delete_unrealized_justification (test-only, but mutates the table). The doc is accurate for production, but the test helper's existence suggests documenting it as "two production writers, one test-only deletion path."

Positive Acknowledgments

1. Excellent test coverage: The three restart/crash tests (get_head_survives_a_restart_over_a_pre_restart_leaf, get_head_survives_a_restart_past_a_pre_restart_fork_leaf, get_head_survives_a_dropped_unrealized_justification) directly reproduce the bug and verify the fix. The stale-fork-leaf test is particularly thorough.

2. Correct pruning horizon: Using the finalized block's own slot rather than the epoch-start slot matches LiveChain behavior and preserves the anchor-as-leaf case. Test beacon_finalization_prunes_unrealized_justifications_to_the_finalized_blocks_own_slot verifies this.

3. Safe fallback semantics: The get_voting_source fallback to store.beacon_justified_checkpoint() is well-justified in comments—it's always epoch-equal to store.justified.epoch, so it can only make a block more viable, never incorrectly viable.

4. DB version bump: Properly incremented to 5 with clear rationale, preventing silent misinterpretation of old directories.

Recommended Priority Order

Priority	Item	File
Critical	Fix cache-database ordering in prune	crates/storage/src/store.rs:3526
High	Harden decode_unrealized_justification_value against corruption	crates/storage/src/store.rs:744
Medium	Add test verifying fallback doesn't incorrectly flip head	crates/blockchain/.../fork_choice.rs
Low	Document expect panic policy	crates/storage/src/store.rs
Low	Rename/annotate import_unchecked	crates/blockchain/.../fork_choice.rs

---

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

[github-actions]

🤖 Codex Code Review

Looks good overall. The restart-freeze root cause is addressed in the right place: persisting per-block unrealized justifications and pruning them on the same horizon as LiveChain is the right consensus-layer fix.

• No correctness bug jumped out in the new persistence path. get_forkchoice_store now seeds the anchor row before returning, compute_pulled_up_tip persists each block’s pulled-up checkpoint, and beacon finalization prunes by the finalized block’s real slot rather than the epoch-start slot, which matches fork-choice expectations in skipped-slot epochs: crates/blockchain/state_transition/src/beacon/fork_choice.rs:1005, crates/blockchain/state_transition/src/beacon/fork_choice.rs:2093, crates/storage/src/store.rs:1861, crates/storage/src/store.rs:1871.
• The fallback in get_voting_source is a reasonable availability-over-strictness tradeoff for the documented crash window, and it avoids freezing get_head on restart. I also like that is_ffg_competitive stays strict, since it is not on the production path yet: crates/blockchain/state_transition/src/beacon/fork_choice.rs:1326, crates/blockchain/state_transition/src/beacon/fork_choice.rs:1631.
• Storage design is sensible: root-keyed lookup keeps the hot fork-choice read path cheap, while carrying slot in the value gives pruning enough information without decoding full blocks: crates/storage/src/store.rs:742, crates/storage/src/store.rs:3456, crates/storage/src/store.rs:3538.
• The main performance cost is prune_unrealized_justifications doing a full-table scan on each finalization, but that table should only cover the unfinalized window, so this seems acceptable for now: crates/storage/src/store.rs:3543.
• Test coverage is good on the risky cases: reopen persistence, skipped-slot finalization horizon, and the crash-window fallback: crates/storage/src/store.rs:7245, crates/storage/src/store.rs:7274, crates/blockchain/state_transition/src/beacon/fork_choice.rs:3201.

No blocking issues from me.

---

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