fix(storage): prune the beacon LiveChain below the finalized block by MegaRedHand · Pull Request #627 · lambdaclass/ethlambda

MegaRedHand · 2026-09-30T16:04:52Z

Why

On a beacon directory, Table::LiveChain was never pruned. Store::update_checkpoints only ran the prune step when self.chain == Chain::Lean, because pruning to a beacon checkpoint's stored slot is unsafe. That slot is the finalized epoch's start slot. When the start slot is empty, the finalized block sits strictly below it, and get_checkpoint_block → get_ancestor walks down to that block. If its row has been pruned, the result is a hard SpecAssert("root in store.blocks").

Two problems follow from leaving the table unpruned:

It grows by one row per imported block, for as long as the follower runs.
block_index() is an uncached full scan of the table, run on every get_head and every import, so a long-running follower gets slower over time.

What

update_checkpoints now matches on the chain:
- The lean arm is unchanged.
- The beacon arm prunes to the finalized block's own slot, read from BlockHeaders (a table that is never pruned). prune_live_chain(h) keeps every row with slot >= h, so the finalized block always survives, and every ancestor walk from one of its descendants stops at or above it.
- If the finalized root has no block entry, the beacon arm logs a warn! and skips pruning instead of guessing a horizon.
Three doc comments pointed at Store::promote_beacon_anchor, a function that no longer exists on this branch; they now point at the new beacon arm. The "lean-only" wording in the LiveChain comments and in docs/data_storage.md is updated too.

Gossip verdict for forks from before finality (`fc8e78fa`)

Pruning adds a second reason the finalized-ancestry walk in beacon/gossip/mod.rs can fail. Before, a walk failed only when invalidate_subtree had deleted a row above finality, which must be IGNORE. Now a walk also fails on a fork that split off below the finalized block, because the block where the fork splits off has been pruned. The spec's beacon_block rule and Prysm REJECT that case.

finalized_ancestry now tells the two apart by where the walk stopped:

Stopped at a block at or below the finalized block's own slot (other than the finalized block itself): Conflicts, which is Reject(FinalizedNotAncestor). Every descendant of the finalized block has a higher slot, so such a block can't be on the finalized chain. Lighthouse's is_finalized_checkpoint_or_descendant treats a walk that runs out of parents in its pruned tree the same way.
Stopped above that slot, or either block has no BlockHeaders row: Unknown, which is Queue(ParentNotReady), as before. This keeps the late-invalidation case at IGNORE.

get_ancestor keeps its signature. It now wraps a new get_ancestor_or_missing, which on failure returns the root it couldn't look up.

717af0fe fixes two more comments in fork_choice.rs that still said the beacon LiveChain is never pruned.

Readers audited

Reader	Needs a row below the finalized block?
`filter_block_tree`, `get_head`	No: the walk starts at the justified checkpoint
`on_block` checkpoint-descendant check	No: the parent is at or above the horizon
`compute_weights`	No: a vote for a pruned root already weighs 0
`invalidate_subtree`	No: it refuses anything at or below finality
Attestation target checks	No: a vote below finality is rejected and cannot change the head
Gossip finalized-ancestry walk	Only for forks from before finality, which are now classified by where the walk stops (see above)
Pending-block cascade, req/resp serving, Beacon API	No: they read `BlockHeaders`/`BlockRoots`

Tests

beacon_finalization_prunes_live_chain_to_the_finalized_blocks_own_slot (storage): epoch 1's start slot (32) is empty and the finalized block is at 31. After pruning, row 31 survives, row 0 is gone and row 33 remains.
beacon_finalization_skips_pruning_when_the_finalized_root_has_no_block_entry (storage): the defensive no-op path.
get_checkpoint_block_succeeds_after_pruning_past_an_empty_epoch_boundary (fork choice): the exact SpecAssert case the old comment warned about, run through a real update_checkpoints call.
Gossip (fc8e78fa): a block on a pruned pre-finality fork is rejected; a parent above the cutoff goes from Accept to Queue(ParentNotReady) after a real invalidate_subtree; three unit tests of the classification itself; one test for get_ancestor_or_missing. The two REJECT tests fail without the fix.

Ran cargo test --profile release-fast on ethlambda-storage --lib, ethlambda-state-transition --lib beacon::fork_choice and ethlambda-blockchain --lib, plus clippy with -D warnings on all three crates. Everything passes. For the follow-up commits: beacon::gossip (40 passed), beacon::fork_choice (29 passed), and clippy on ethlambda-state-transition.

Deploying

No DB_VERSION change. On an existing follower, the first finalization advance after the upgrade deletes every accumulated row below the finalized block in one batch.

Moved

Moved from lambdaclass/ethlambda_private#46, now that beacon-chain-integration lives on this repo.
Based on beacon-chain-integration @ c79fabd5, merged into the branch.

Table::LiveChain, the slot-ordered block index fork choice reads through Store::block_index(), was never pruned on a beacon directory: update_checkpoints gated the whole prune step to Chain::Lean because pruning to the stored finalized checkpoint's slot is unsafe there. A beacon checkpoint's slot is its epoch's start slot, not a real block's own slot, and when that boundary slot was skipped the finalized block itself sits below it. Pruning to it would delete the finalized block's own row, and every fork-choice walk that asks get_checkpoint_block for the ancestor at that slot (filter_block_tree, on_block, the gossip finalized-ancestry check) would fall through the gap into a hard SpecAssert instead of finding it. Left unfixed, the table also just grows forever on a beacon follower, and block_index() is an uncached full scan run on every get_head and import, so a long-running node gets slower over time. Prune to the finalized block's own slot instead, read from BlockHeaders (never pruned) rather than LiveChain (the table being pruned): prune_live_chain(h) keeps every row with slot >= h, so the finalized block's row always survives, and any walk from one of its descendants down to a slot at or above it stops at or above the finalized block rather than falling through. Hooked into Store::update_checkpoints itself, alongside the existing lean branch: it is already the one call site both chains advance finality through, already compares old and new finalized slots to fire only on an actual advance, and its own doc comment already promised this. Skip pruning (and warn) if the finalized root has no block entry, rather than guessing a horizon. Audited every other beacon reader of LiveChain/block_index (filter_block_tree, get_head, compute_weights, invalidate_subtree, the attestation target checks, the gossip finalized-ancestry walk, the pending-block cascade, req/resp range and by-root serving, the Beacon API): none needs a row below the finalized block, and several read a different, unpruned table (BlockHeaders/BlockRoots) for exactly that reason. Also updates three stale doc comments naming a Store::promote_beacon_anchor that does not exist on this branch, and the lean-only framing of the pruning docs.

…block Since LiveChain is pruned below the finalized block's own slot on beacon, the gossip finalized-ancestry walk has a second way to fail. A block on a fork that split off below the finalized block walks into a pruned row, and finalized_ancestry read every failed walk as Unknown, so a block the spec REJECTs ("the current finalized checkpoint is an ancestor of the block") was only IGNOREd. Before the pruning the same walk found the fork's block and answered Conflicts. A failed walk is now classified by the slot of the block it could not look up, read from BlockHeaders (never pruned) and measured against the finalized block's own slot. Every descendant of the finalized block has a strictly higher slot, so a different block at or below that slot cannot be on the finalized chain, whatever removed its row: Conflicts. Lighthouse's is_finalized_checkpoint_or_descendant treats a walk off its pruned tree the same way. Above that slot pruning never reaches, so a missing row there is still the late invalidate_subtree case, which stays Unknown (IGNORE) so peers that forwarded a child before the invalidation are not penalized. So is a walk whose missing block, or the finalized root, has no BlockHeaders row. get_ancestor keeps its spec signature and now wraps get_ancestor_or_missing, which returns the root the walk could not find.

Two comments in the beacon fork choice still justified building block_index once per call by saying beacon LiveChain is never pruned. Since the beacon arm of Store::update_checkpoints prunes it below the finalized block's own slot on every finalization advance, the full scan covers only the unfinalized window. The reasoning still holds, since that window grows with every block while finality stalls, so only the description of the table changes.

Brings in #48, #19, #47 and #49. Both conflicts were tests added on both sides of gossip/mod.rs and gossip/test_support.rs: kept all of them and merged the test_support module doc to name every module it serves.

… preset The test hardcoded mainnet's epoch length (blocks at 31 and 33, epoch 1 starting at 32). Under preset-minimal an epoch is 8 slots, so the finalized block sat epochs past its own checkpoint and the walk to epoch 1's start slot ran into the pruned anchor, failing both minimal-preset CI jobs with SpecAssert("root in store.blocks"). Deriving the slots from compute_start_slot_at_epoch(1) keeps the same scenario on both presets.

…ix/beacon-prune-live-chain

github-actions · 2026-09-30T16:05:52Z

🤖 Claude Code Review

Review of PR 627: prune `LiveChain` on beacon to the finalized block's own slot

I read the diff only and did not build or run the tests. I found no blocking issues.

What it does

Store::update_checkpoints now prunes LiveChain on beacon, which was never pruned before.
The horizon is the finalized block's own slot, read from BlockHeaders. It is not the epoch start slot stored in the checkpoint.
That horizon is the right one. prune_live_chain(h) keeps rows with slot >= h, so the finalized block survives even when the epoch's start slot was skipped. get_checkpoint_block would otherwise fall off the index and raise a SpecAssert.
finalized_ancestry now tells a pruned row apart from an invalidated one. The two cases need opposite verdicts, and the diff separates them cleanly.

Correctness

missing_row_ancestry (gossip/mod.rs) returns Conflicts only when the missing block is not the finalized root and its slot is <= the finalized slot. I checked the edge cases:
- Same-slot siblings of the finalized block are correctly a fork.
- Every descendant of the finalized block has a strictly higher slot, so nothing legitimate is misclassified.
- A missing BlockHeaders row for either block falls back to Unknown, which is the conservative IGNORE.
- A pending parent above finality, with no LiveChain row yet, stays Unknown.
get_ancestor keeps the same error behavior through get_ancestor_or_missing. Only the error payload changes.
The None arm in update_checkpoints skips pruning with a warn!. Skipping is safe, since an unpruned index only costs memory. The test pins that behavior.

Minor points

Attestations targeting an epoch below finalized. Any caller that walks get_ancestor to a slot below the finalized block now hits a missing row and gets a SpecAssert, where before it succeeded.
- on_block_attestation discards errors with let _, so that path is fine.
- Gossip and other validate_on_attestation paths rely on the current/previous-epoch window to reject such targets first. Finalized is normally at least two epochs behind, but that depends on those checks running before the walk. Worth confirming.
Dangling votes. As the existing comments note, compute_weights drops votes for pruned roots. get_weight still raises on them, and pruning now makes that more reachable. The spec-matching behavior is deliberate and documented, so I'm only flagging the new reachability.
Cost of the failure path. missing_row_ancestry decodes two whole blocks through block_entry. That is acceptable on an already-failed walk, but a fork-spamming peer could trigger it repeatedly. The seen-caches and gossip rate limits probably bound it.

Tests and docs

Coverage is thorough. The tests cover the empty epoch-boundary regression, the fork-below-finality REJECT, the invalidated-above-finality queue, and the missing-header Unknown case. They use real update_checkpoints calls instead of hand-built indexes.
The stale promote_beacon_anchor comments and docs/data_storage.md are updated to match.

Verdict: approve.

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

github-actions · 2026-09-30T16:06:01Z

🤖 Codex Code Review

Looks good overall. The PR fixes a real beacon fork-choice/storage bug around pruning LiveChain at epoch boundaries where no block exists, and the added tests cover the critical ancestor-walk failure mode well.

No correctness bug stood out in the core fix: pruning beacon LiveChain to the finalized block’s actual block slot instead of the stored checkpoint’s epoch-start slot preserves get_checkpoint_block(...) and avoids a hard SpecAssert during fork-choice walks. The behavior change in crates/storage/src/store.rs:1802 is consistent with how get_ancestor(...) and gossip finalized-ancestry use the index.
The new get_ancestor_or_missing(...) helper in crates/blockchain/state_transition/src/beacon/fork_choice.rs:1068 is a reasonable refactor: it preserves existing fork-choice behavior via get_ancestor(...) while letting gossip distinguish “pruned below finality” from “missing for other reasons”. That is a good consensus-safety improvement, not just cleanup.
The gossip-side classification in crates/blockchain/state_transition/src/beacon/gossip/mod.rs:427 also looks sound: if the walk falls off below the finalized block’s slot, it returns Conflicts; otherwise Unknown. That matches the intended REJECT vs IGNORE split for finalized ancestry checks.

One small concern to consider:

crates/storage/src/store.rs:1835 silently skips beacon LiveChain pruning when finalized.root has no BlockHeaders entry. That fallback is safer than guessing a horizon, but it can leave stale pre-finality rows around indefinitely if metadata is corrupted or partially restored. I don’t think this is a consensus bug, but it may be worth tightening with a stronger invariant or metric/logging so operators notice quickly. As written, repeated skips could cause unbounded growth of the beacon fork-choice working set.

No obvious security or memory-safety issues were introduced, and performance is fine: the change replaces a potentially wrong prune horizon with one block_entry(...) lookup on finalization advance, which is negligible compared with the avoided fork-choice failure.

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

github-actions · 2026-09-30T16:06:06Z

🤖 Kimi Code Review

I'll review this PR focusing on the beacon chain fork choice pruning logic and its interaction with empty epoch boundaries. This is a consensus-critical change.

Overall Assessment

This PR fixes a consensus-critical bug where beacon chain finalization pruning could delete the finalized block's own LiveChain row when an epoch boundary slot has no block. The fix correctly distinguishes pruning horizons between lean and beacon chains, and adds proper handling for missing rows during fork-choice walks. The implementation is sound with good test coverage.

Detailed Findings

1. Critical Bug Fix: Beacon Pruning Horizon (Correct)

crates/storage/src/store.rs:1806-1843

The core fix is correct. The beacon chain stores checkpoint slots as epoch start slots (beacon_checkpoint_as_stored), which may not have actual blocks. Pruning LiveChain to that slot would delete the finalized block when it sits below the epoch start (e.g., finalized block at slot 31, epoch 1 starts at slot 32).

// Line ~1830: Correctly uses block_slot from BlockHeaders, not finalized.slot
match self.block_entry(&finalized.root) {
    Some((block_slot, _)) => {
        let pruned_chain = self.prune_live_chain(block_slot).expect("prune live chain");

Verification: prune_live_chain(h) keeps rows with slot >= h, so the finalized block at block_slot survives. This matches the spec requirement that get_checkpoint_block must find ancestors at or below the epoch start slot.

2. `get_ancestor_or_missing` API Design (Minor Concern)

crates/blockchain/state_transition/src/beacon/fork_choice.rs:1053-1072

pub fn get_ancestor_or_missing(
    index: &HashMap<Root, (Slot, Root)>,
    root: Root,
    slot: Slot,
) -> std::result::Result<Root, Root> {

Using Root as the error type is semantically weak. A dedicated enum would be clearer:

pub enum AncestorError {
    Missing(Root),  // The root that couldn't be found
}

However, given the tight integration with existing code and the immediate need, this is acceptable. The extensive documentation (lines 1056-1063) mitigates confusion.

3. `missing_row_ancestry` Logic Correctness (Verified)

crates/blockchain/state_transition/src/beacon/gossip/mod.rs:423-433

fn missing_row_ancestry(store: &Store, finalized_root: Root, missing: Root) -> FinalizedAncestry {
    let Some((missing_slot, _)) = store.block_entry(&missing) else {
        return FinalizedAncestry::Unknown;
    };
    let Some((finalized_slot, _)) = store.block_entry(&finalized_root) else {
        return FinalizedAncestry::Unknown;
    };
    if missing != finalized_root && missing_slot <= finalized_slot {
        FinalizedAncestry::Conflicts
    } else {
        FinalizedAncestry::Unknown
    }
}

Logic verification:

missing == finalized_root: Returns Unknown (pruning keeps this row, so missing it means data corruption, not a fork)
missing_slot <= finalized_slot and not finalized: Confirms fork below finality → Conflicts (REJECT)
missing_slot > finalized_slot: Row missing above finality → Unknown (IGNORE, likely invalidate_subtree)

Edge case: What if missing_slot == finalized_slot but missing != finalized_root? This correctly returns Conflicts — two different blocks at the same slot below/ at finality is impossible on the canonical chain, hence a fork.

4. Test Coverage (Good, One Gap)

Strengths:

get_checkpoint_block_succeeds_after_pruning_past_an_empty_epoch_boundary — directly tests the bug scenario
beacon_finalization_prunes_live_chain_to_the_finalized_blocks_own_slot — storage-level integration test
a_walk_off_the_pruned_tree_below_the_finalized_block_conflicts — gossip outcome verification
a_walk_off_the_tree_above_the_finalized_block_is_unknown — invalidate_subtree case

Gap: No test for the exact boundary where missing_slot == finalized_slot with missing != finalized_root. The current tests use epoch_start - 1 for finalized and epoch_start + 1 for descendant. Consider adding:

// In gossip/mod.rs tests or fork_choice tests
#[test]
fn a_fork_at_the_finalized_slot_itself_conflicts() {
    // Two blocks at same slot, one is finalized, other is pruned fork base
}

5. Comment/Documentation Accuracy

crates/blockchain/src/lib.rs:1822-1826

-                        // block this node imported and is never pruned on
-                        // beacon, so the scan is the expensive part
+                        // unfinalized block, and every attestation in the
+                        // block asks the same question of it, so building the
+                        // index once is what keeps that a single scan

This comment update is accurate. The table is now pruned on beacon, so "never pruned" was wrong.

6. Error Handling in `update_checkpoints`

crates/storage/src/store.rs:1838-1843

None => {
    warn!(
        finalized_slot = finalized.slot,
        finalized_root = %ShortRoot(&finalized.root.0),
        "Skipping beacon LiveChain prune: no block entry for the finalized root"
    );
}

This defensive handling is correct. If the finalized root has no BlockHeaders entry, we cannot determine a safe pruning horizon. Skipping is the conservative choice — LiveChain grows unpruned until the block appears, but correctness is preserved.

Question: Can this log spam occur in normal operation? If update_checkpoints is called with a checkpoint before the block is stored (race in block import vs. checkpoint update), this could happen repeatedly. Consider if this needs rate limiting or if the ordering guarantees prevent it.

7. `store_finalized_past_a_fork` Test Helper

crates/blockchain/state_transition/src/beacon/gossip/test_support.rs:93-137

The helper correctly constructs the scenario. One minor issue:

let blocks = [
    (Root::ZERO, empty_block(0, Root::ZERO)),  // genesis
    (finalized, empty_block(epoch_start - 1, Root::ZERO)),  // parent is ZERO
    // ...
];

The finalized block has parent_root = Root::ZERO, which matches genesis. This is correct for a chain where genesis is at slot 0 and finalized is its direct descendant.

8. Performance Consideration: `block_entry` in Hot Path

crates/blockchain/state_transition/src/beacon/gossip/mod.rs:423-433

missing_row_ancestry calls store.block_entry twice for failed walks. This is in the gossip validation path for blocks whose ancestor walk fails. Given that:

This only runs when get_ancestor_or_missing returns Err (already exceptional)
block_entry reads from BlockHeaders, which is backed by storage

The performance impact is acceptable — this is an error path, not the common case. However, if block_entry involves deserialization, consider whether the slot could be cached or if a lighter read path exists.

Looking at Store::block_entry, it likely reads and deserializes a full block. For just the slot, this is wasteful. Consider adding a block_slot method if this becomes a bottleneck.

9. Naming: `get_ancestor_or_missing` vs. `get_ancestor`

The naming is clear but the relationship could be more explicit. Consider:

/// Returns the ancestor, or the first missing root in the chain.
/// 
/// See [`get_ancestor`] for the spec-compliant wrapper that discards the missing root.

The existing doc comment (lines 1056-1063) serves this purpose well enough.

Summary

Aspect	Assessment
Correctness	Correct — fixes the pruning horizon bug properly
Security	No vulnerabilities introduced — conservative handling of missing data
Performance	Neutral/Minor improvement — pruning reduces `LiveChain` size
Rust idioms	Acceptable — `Result<Root, Root>` is slightly unidiomatic but documented
Test coverage	Good — covers main scenarios; minor gap at exact boundary equality
Documentation	Excellent — extensive comments explain the "why"

Recommended Changes

Line crates/blockchain/state_transition/src/beacon/gossip/mod.rs:433: Add test for missing_slot == finalized_slot && missing != finalized_root boundary case.
Line crates/storage/src/store.rs:1838: Consider whether the warn! on missing finalized block entry needs rate limiting or if invariants prevent repeated hits.
Optional: Consider block_slot lightweight accessor if block_entry deserialization proves costly in missing_row_ancestry.

The PR is ready to merge with or without these minor adjustments.

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

MegaRedHand added 7 commits September 24, 2026 17:18

Merge beacon-chain-integration into fix/beacon-prune-live-chain

a92f3f9

Merge beacon-chain-integration into fix/beacon-prune-live-chain

1059271

Brings in #48, #19, #47 and #49. Both conflicts were tests added on both sides of gossip/mod.rs and gossip/test_support.rs: kept all of them and merged the test_support module doc to name every module it serves.

Merge remote-tracking branch 'origin/beacon-chain-integration' into f…

b2404ab

…ix/beacon-prune-live-chain

MegaRedHand mentioned this pull request Sep 30, 2026

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

Open

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

Aliemeka mentioned this pull request Oct 3, 2026

chore(storage): validate pending blocks before storing them #657

Open

3 tasks done

Merge branch 'beacon-chain-integration' into fix/beacon-prune-live-chain

28a693c

MegaRedHand mentioned this pull request Oct 5, 2026

feat: add Beacon Chain support #661

Open

4 tasks

Navigation Menu

fix(storage): prune the beacon LiveChain below the finalized block - #627

fix(storage): prune the beacon LiveChain below the finalized block#627
MegaRedHand wants to merge 8 commits into
beacon-chain-integrationfrom
fix/beacon-prune-live-chain

MegaRedHand commented Sep 30, 2026

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

Why

What

Gossip verdict for forks from before finality (fc8e78fa)

Readers audited

Tests

Deploying

Moved

Uh oh!

github-actions Bot commented Sep 30, 2026

🤖 Claude Code Review

Review of PR 627: prune LiveChain on beacon to the finalized block's own slot

Uh oh!

github-actions Bot commented Sep 30, 2026

🤖 Codex Code Review

Uh oh!

github-actions Bot commented Sep 30, 2026

🤖 Kimi Code Review

Overall Assessment

Detailed Findings

1. Critical Bug Fix: Beacon Pruning Horizon (Correct)

2. get_ancestor_or_missing API Design (Minor Concern)

3. missing_row_ancestry Logic Correctness (Verified)

4. Test Coverage (Good, One Gap)

5. Comment/Documentation Accuracy

6. Error Handling in update_checkpoints

7. store_finalized_past_a_fork Test Helper

8. Performance Consideration: block_entry in Hot Path

9. Naming: get_ancestor_or_missing vs. get_ancestor

Summary

Recommended Changes

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Gossip verdict for forks from before finality (`fc8e78fa`)

Review of PR 627: prune `LiveChain` on beacon to the finalized block's own slot

2. `get_ancestor_or_missing` API Design (Minor Concern)

3. `missing_row_ancestry` Logic Correctness (Verified)

6. Error Handling in `update_checkpoints`

7. `store_finalized_past_a_fork` Test Helper

8. Performance Consideration: `block_entry` in Hot Path

9. Naming: `get_ancestor_or_missing` vs. `get_ancestor`