perf(blockchain): bound attestation ancestry walks by fork depth

[MegaRedHand]

Problem

validate_attestation_data makes three unbounded parent-chain walks per attestation, and every step of a walk is a fresh read view, a RocksDB point get, and an SSZ decode (get_block_header has no cache, unlike get_state).

A walk's length is the slot distance between its two checkpoints, so it grows without limit as finalization falls behind. On a node with finalized at slot 928 and the head near 64000:

call site	walk	steps
source ← target	7984 → ~64000	~56,000
target ← head	~0, early return	~0
finalized ← head	928 → ~64000	~63,000

That is ~119,000 header reads for a single attestation, and it runs once per gossip attestation, once per aggregate, and for the node's own votes via self-delivery. At a few hundred attestations per slot it exceeds the slot duration by orders of magnitude.

The result is a feedback loop rather than merely slow validation: validation slows, attestations are dropped, finality falls further behind, and the walks grow longer still.

Fix

BlockRoots already indexes the canonical chain by slot, written atomically with the head in update_checkpoints, so it always describes the branch ending at the stored head (it can lag a freshly imported block that fork choice has not selected, but never runs ahead).

Since the canonical chain is a single path, a walk that reaches any canonical block above the ancestor's slot can stop there: the ancestor lies on that chain exactly when the index names it at its own slot.

canonical ────●────●────●────●────●────●  head
               \
                ○────○  fork tip = descendant
                └──┬──┘
             walk only this far (fork depth)
                   ↓
      rejoin point is canonical and above ancestor.slot
                   ↓
    ancestor on chain? → one index lookup, O(1)

Cost is now bounded by how deep the descendant's branch has forked (a handful of slots) rather than by the distance to the ancestor. The finalized ← head check settles with no walk at all, since the finalized checkpoint is canonical by construction.

New storage API, the only addition:

pub fn canonical_root_at_slot(&self, slot: u64) -> Result<Option<H256>, Error>

The judgment about how far the index can be trusted stays inside the Store, so no caller can widen it.

Two ways the short circuit can be wrong

Both are load-bearing and both have a regression test.

1. An index miss must not mean "not an ancestor." A None is either a slot the canonical chain skipped or a slot below this store's anchor, and the index cannot tell those apart. A miss therefore leaves the full walk as the answer. This is also what keeps the change correct if historical backfill ever lands.

2. The short circuit must not fire at or below the ancestor's slot. I had this wrong at first, and the entire existing suite passed:

canonical:  g(0) ── a(1) ── c(2)          index = {0:g, 1:a, 2:c}
fork:       g(0) ─────────────── d(3)      d's parent is g, skipping slots 1-2

is_ancestor(a@1, d@3):
  iter1  d@3   index[3]=None        → no fire; 3 > 1 → step to parent g@0
  iter2  g@0   index[0]=Some(g) ✓   → FIRES → returns true
                                              ↑ wrong: `a` is not on d's chain

A branch that skips the ancestor's slot and rejoins canonical below it does not contain the ancestor. The slot guards have to settle the walk before the index is consulted. checkpoint_is_ancestor_rejects_ancestor_skipped_by_fork_branch covers it — verified to fail on the broken ordering and pass on the fixed one.

Check ordering left alone

I tried hoisting the four storage-free checks (source > target, head < target, slot < head.slot, and the time check) ahead of the three block lookups, so malformed and future-dated votes would cost no storage reads. It works, but it changes which reason a vote failing several checks at once is rejected for, and five leanSpec gossip-validation fixtures pin those reasons:

expected HEAD_SLOT_MISMATCH    got ATTESTATION_TOO_FAR_IN_FUTURE
expected TARGET_SLOT_MISMATCH  got ATTESTATION_SLOT_BEFORE_HEAD
expected UNKNOWN_SOURCE        got a moved check

Rejection reasons are cross-client observable, so the ordering is not ours to optimize. Reverted, with validate_attestation_reports_availability_before_cheaper_failures and a note on the function recording the constraint so it is not mistaken for an oversight later.

Testing

• The four pre-existing ancestry tests are untouched and pass.
• Six added: canonical vote end-to-end, off-chain ancestor rejected via the index, index-miss fallback, fork branch skipping the ancestor's slot, block imported before update_head ran, and the rejection-precedence fence.
• make lint clean; make test green, including all 122 forkchoice spec fixtures.

Not in scope

get_live_chain is rebuilt from a full LiveChain iteration on every head update, so at a 63k-slot finality gap it materializes a ~63,000-entry HashMap per call. Same underlying cause, separate hot path.

[github-actions]

🤖 Kimi Code Review

I'll review this PR which optimizes checkpoint_is_ancestor by using the canonical slot index to short-circuit parent walks. This is a consensus-critical change.

Overall Assessment

The optimization is sound and well-motivated, but I found several issues ranging from minor correctness concerns to a potential infinite loop bug.

---

Critical Issues

1. Infinite Loop Risk in checkpoint_is_ancestor (crates/blockchain/src/store.rs)

Lines 209-242: The loop can become infinite when current_slot == 0 and the ancestor is not found.

Consider: current_slot = 0, ancestor.slot = 0, but current_root != ancestor.root. The first check current_slot == ancestor.slot returns false (roots differ). The second check current_slot < ancestor.slot is false (equal). The canonical check requires current_slot > ancestor.slot implicitly (since ancestor_is_canonical is Some only when we haven't hit the slot guards). Then we set current_root = current_parent, look up the header, and if genesis has parent_root == H256::zero() or some self-referential value, we may loop forever or until we hit a missing block.

More directly: if we reach slot 0 with the wrong root, and the block at slot 0 has parent_root pointing to something else (or itself), we loop. Even for valid chains, if ancestor.slot = 0 and current_root at slot 0 is wrong, we fall through to:

current_root = current_parent;  // could be 0x00...00 or loop

Then get_block_header returns None and we return false. But if slot 0's parent_root is itself (genesis edge case), we loop forever.

Suggested fix: Add an explicit termination condition. After the slot checks, assert current_slot > 0 before continuing, or check that current_slot strictly decreases:

// After fetching header
let new_slot = current_header.slot;
if new_slot >= current_slot {
    // Slot did not decrease, malformed chain or loop
    return false;
}
current_slot = new_slot;

---

2. Unclear let-else with .expect() Inside Condition (crates/blockchain/src/store.rs)

Lines 232-237:

let Some(current_header) = store
    .get_block_header(&current_root)
    .expect("parent block exists")
else {
    return false;
};

This is confusing. .expect("parent block exists") panics on Err, but let-else handles None. The message "parent block exists" is misleading—it suggests the None case shouldn't happen either, but we handle it with return false.

Suggested fix: Clarify the intent. If None is truly unexpected (indicating database corruption), panic. If it's a normal case (missing block), use ? or match properly:

let current_header = store
    .get_block_header(&current_root)
    .expect("database read succeeds")
    .expect("parent block of validated chain exists"); // or handle None gracefully

Or if None is valid:

let Some(current_header) = store.get_block_header(&current_root).expect("db read") else {
    return false; // missing block, not an ancestor
};

---

Security / Consensus Issues

3. TOCTOU in Canonical Index Check (crates/blockchain/src/store.rs)

Lines 217-225: The canonical check calls store.canonical_root_at_slot(current_slot) inside the loop, opening a new read transaction each time. Between iterations, the head (and thus the canonical index) could change due to concurrent fork choice updates.

The comment at line 220 says "Reaching a canonical block strictly above the ancestor's slot settles the rest"—but if the index changes mid-walk, we might:

• Hit a block that was canonical when we checked but isn't anymore
• Miss a block that became canonical

However, Store appears to use RwLock or similar, and canonical_root_at_slot takes &self. Check if Store guarantees snapshot isolation for reads. If not, this is a race condition.

Mitigation: The function takes &Store, not &mut Store, so concurrent modifications are possible. Document this assumption or use a single read transaction for the entire walk.

---

4. ancestor_is_canonical Computed Once but Index May Change (crates/blockchain/src/store.rs)

Lines 199-204: ancestor_is_canonical is computed before the loop, but the canonical index could theoretically be updated between this call and the loop's canonical checks. This is a minor consistency issue; the loop's own canonical_root_at_slot calls are the real concern (Point 3).

---

Correctness Issues

5. Off-by-One in Canonical Short-Circuit Condition (crates/blockchain/src/store.rs)

Lines 217-225: The comment says "strictly above the ancestor's slot" but the code checks current_slot == ancestor.slot earlier and returns. So current_slot > ancestor.slot is guaranteed here. However, the logic says:

"This must stay below the slot guards, since a canonical block at or below the ancestor's slot says nothing about whether this branch passes through the ancestor."

The code correctly has the current_slot == ancestor.slot check first. But what about current_slot < ancestor.slot? That's handled second. So when we reach the canonical check, current_slot > ancestor.slot. ✓

Wait—re-reading: the comment says "strictly above" but the condition is checking if current_root at current_slot is canonical. This is correct: if we're at slot 5, ancestor at slot 3, and slot 5 is canonical, then slots 0-5 are all canonical, so we just check if ancestor is canonical.

But there's a subtle issue: what if current_slot is canonical, but ancestor.slot was skipped on the canonical chain? Then ancestor_is_canonical is None (or rather, Some(false) if the index has a different block, or None if skipped). Wait—canonical_root_at_slot(ancestor.slot) returns None for skipped slots. Then ancestor_is_canonical is None, and the if let Some(is_canonical) guard prevents entering. ✓

Actually re-reading: ancestor_is_canonical is Option<bool>. None means "index has nothing to say" (below anchor or... actually for skipped slots, canonical_root_at_slot returns None). So skipped slots correctly fall through to parent walk.

But wait—can a skipped slot ever be an ancestor? If the canonical chain skips slot 1, and we're checking if slot 1's block is an ancestor, canonical_root_at_slot(1) returns None, we fall through to walk, and we'll never hit slot 1 because it's skipped. Correct: a skipped slot has no block, so no block can be its ancestor. ✓

---

6. First Iteration Redundancy (crates/blockchain/src/store.rs)

Line 208: current_root = descendant.root and current_slot = descendant.slot. But we already checked ancestor.slot == descendant.slot at line 197. So on first iteration, current_slot == descendant.slot > ancestor.slot (assuming we passed the equality check). The canonical check then asks: is descendant.root canonical at descendant.slot?

If yes, and ancestor_is_canonical is Some(true), we return true. If ancestor_is_canonical is Some(false), we return false. This is correct: if descendant is canonical, answer is whether ancestor is canonical.

But what if descendant is canonical and ancestor is below the anchor (ancestor_is_canonical = None)? We fall through, set current_root = current_parent, and continue walking. This is correct but suboptimal—we know the chain below canonical is single-path, but we don't have index data.

---

Code Quality Issues

7. expect Messages Inconsistent with Error Handling (crates/storage/src/store.rs)

Lines 1334-1337:

let view = self.backend.begin_read().expect("read view");
Ok(view
    .get(Table::BlockRoots, &encode_block_root_key(slot))
    .expect("get block root")
    .map(|bytes| H256::from_ssz_bytes(&bytes).expect("valid block root")))

Three expects with inconsistent messages. The last one "valid block root" suggests database corruption if SSZ decode fails. Consider propagating errors instead of panicking on database corruption.

Suggested fix:

pub fn canonical_root_at_slot(&self, slot: u64) -> Result<Option<H256>, Error> {
    let view = self.backend.begin_read()?;
    let Some(bytes) = view.get(Table::BlockRoots, &encode_block_root_key(slot))? else {
        return Ok(None);
    };
    Ok(Some(H256::from_ssz_bytes(&bytes)?))
}

---

8. Test Helper header_of Panics Unnecessarily (crates/blockchain/src/store.rs)

Lines 1555-1560:

fn header_of(store: &Store, root: H256) -> BlockHeader {
    store
        .get_block_header(&root)
        .expect("get_block_header should succeed")
        .expect("test block header exists")
}

This is test code, but the double expect is unidiomatic. Use unwrap or ? in tests, or a single expect with a clear message.

---

Positive Observations

1. Excellent test coverage: The 5 new tests cover canonical short-circuit, off-chain rejection, skipped-slot rejection, missing-index fallback, and not-yet-selected block resolution. The attestation ordering test is particularly valuable for spec compliance.

2. Clear documentation: Comments explain why the canonical optimization works and why the ordering constraints exist. The canonical_root_at_slot docstring correctly warns about None semantics.

3. Transaction isolation comment: Lines 1356-1359 correctly note why canonical_blocks_in_range doesn't use canonical_root_at_slot—to maintain snapshot isolation.

---

Summary of Required Changes

Priority	Issue	Location
Critical	Add loop termination guard against infinite loop	crates/blockchain/src/store.rs:209-242
High	Clarify let-else + expect intent or panic on None	crates/blockchain/src/store.rs:232-237
Medium	Consider single read transaction for consistency	crates/blockchain/src/store.rs:217-225
Low	Propagate errors instead of expect in canonical_root_at_slot	crates/storage/src/store.rs:1334-1337
Low	Simplify test helper	crates/blockchain/src/store.rs:1555-1560

---

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

[github-actions]

🤖 Codex Code Review

Findings

1. crates/blockchain/src/store.rs:201-229, crates/storage/src/store.rs:1335-1340
checkpoint_is_ancestor() now depends on canonical_root_at_slot(), but every call to that helper opens a fresh begin_read() snapshot. That means attestation validation can observe BlockRoots from different heads within one ancestry check. The race is not theoretical: get_signed_blocks_by_slot_range() was explicitly written to avoid this exact “splice two branches into one response” hazard (crates/storage/src/store.rs:1355-1359). Here, if ancestor_is_canonical is read before a head update and current_slot is checked after it, the function can return the wrong ancestry result for a valid side-branch attestation. This is consensus-critical because it can incorrectly reject or accept votes depending on timing. The fix should be to evaluate the canonical-slot lookups against one read view for the whole check, or move the full ancestry routine into storage so it runs under a single snapshot.

No other obvious correctness problems stood out in the diff.

I couldn’t run the targeted Rust tests in this environment because cargo/rustup attempted to write under /home/runner/.rustup, which is read-only here.

---

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

[github-actions]

🤖 Claude Code Review

Review Summary

Overall this is a well-reasoned, well-tested optimization. I traced the checkpoint_is_ancestor algorithm by hand against the invariant established in block_root_index_changes (crates/storage/src/store.rs:1003-1036): the BlockRoots index always mirrors the actual parent-pointer chain from anchor to head. That invariant guarantees the key correctness property the PR relies on — if a walked block is canonical, everything below it on the descendant's chain is exactly the canonical chain, so the ancestor's own canonical-membership check is a sound proxy for "is it on this path." I couldn't find a case that breaks this (checked: siblings at the ancestor's slot, forks that skip the ancestor's slot and rejoin canonical below it, index misses from un-promoted heads, and blocks imported but not yet selected as head — all match the new regression tests and produce correct results).

Strengths:

• The ordering of the slot-equality/less-than guards before the canonical-index check (store.rs:210-232) is correct and necessary — the PR's own writeup documents exactly the bug they hit when this was ordered wrong, and the regression test (checkpoint_is_ancestor_rejects_ancestor_skipped_by_fork_branch) pins it.
• Treating an index miss as "unknown" rather than "not an ancestor" (ancestor_is_canonical: Option<bool>, the if let Some(...) = ... && short-circuit) is the right conservative default and is covered by checkpoint_is_ancestor_walks_when_slot_has_no_index_entry.
• Leaving the check ordering in validate_attestation_data untouched despite the extra storage reads is the right call — cross-client-observable rejection reasons aren't something to trade for a local perf win, and the added test pins the constraint so it doesn't get "fixed" by a future refactor.
• canonical_root_at_slot's doc correctly states the two cases a None conflates (skipped slot vs. below-anchor) and warns callers not to conflate them with "not canonical."

Finding worth a look (reported above, not necessarily blocking):

• crates/storage/src/store.rs:1349 — the new comment on get_signed_blocks_by_range asserts that reusing one view across the loop gives snapshot isolation against a concurrent head change ("a range must be served from a single snapshot so a head change partway through cannot splice two branches into one response"). That's true for the in-memory backend (the view holds an RwLockReadGuard), but for RocksDB, begin_read() just clones the Arc<DB> (crates/storage/src/backend/rocksdb.rs:67-71) and get() does a plain get_cf with default ReadOptions — no db.snapshot() is taken. So on the production backend, the stated guarantee doesn't actually hold: a reorg landing mid-loop (from a concurrent writer sharing the same Arc<dyn StorageBackend>, e.g. the RPC thread reading while the BlockChain actor calls update_checkpoints) can still produce a spliced response. This isn't new behavior from this PR (the loop itself is untouched), but the PR adds the comment as a design rationale for not switching to the new canonical_root_at_slot helper, and that rationale overstates what the current implementation provides. Worth either taking a real RocksDB snapshot in begin_read(), or softening the comment to note the guarantee is backend-dependent.

Nothing else stood out as a correctness, safety, or performance issue — the new tests are appropriately adversarial (they specifically target the two failure modes called out in the PR description), and the expect() usage on storage calls is consistent with the rest of the file's existing style (backend errors are treated as fatal/unexpected, not user-facing).

---

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