fix: enforce attestation checkpoint ancestry

[MegaRedHand]

Summary

Ports leanSpec PR #833: reject attestations whose checkpoints are slot-ordered but not on the same chain.

This enforces the topology implied by source <= target <= head in two places that previously only checked slots and availability:

• Gossip validation (validate_attestation_data, store.rs) now verifies source -> target and target -> head ancestry by walking parent links through the fork-choice store's block tree.
• State transition (attestation_data_matches_chain, state_transition/src/lib.rs) now matches head.root against the canonical historical_block_hashes view, not only source and target.

Root cause

validate_attestation_data checked that source/target/head blocks were known and that their checkpoint slots matched the block slots. It did not check that target was actually an ancestor of head, or that source was an ancestor of target.

Because fork choice weights by attestation_data.head.root and _accumulate_ancestor_weights walks upward from the attested head giving weight to that branch's ancestors, accepting a known-but-sibling head lets a validator present a canonical source -> target FFG vote while injecting LMD-GHOST weight into a non-canonical branch. That is a fork-choice vote-splitting vector.

attestation_data_matches_chain is the state-transition complement. Justification is still keyed on target.root, so a sibling head does not justify a non-canonical target; checking head there keeps counted votes internally consistent (no decoupled FFG vote on the canonical chain with an LMD head on a sibling fork).

The two checks use different reference frames intentionally (matching leanSpec): gossip validation has the fork-choice store tree available, while state transition validates against the linear canonical historical_block_hashes view.

Notes on the gossip walk

checkpoint_is_ancestor walks parent links via get_block_header, capped at MAX_ANCESTRY_WALK_SLOTS (128) to bound per-attestation work on the gossip hot path. A missing block, a skipped (zero-hash) slot, or exceeding the cap is treated conservatively as non-ancestry (the vote is rejected).

Changes

File	Change
crates/blockchain/src/store.rs	New checkpoint_is_ancestor helper (parent walk, 128-slot cap); two ancestry checks in validate_attestation_data; new StoreError::SourceNotAncestorOfTarget / TargetNotAncestorOfHead variants
crates/blockchain/state_transition/src/lib.rs	attestation_data_matches_chain now also rejects zero-hash / out-of-range / mismatched head

Tests

New unit tests, all passing:

• store::validate_attestation_rejects_head_on_sibling_fork
• store::validate_attestation_rejects_source_on_sibling_fork
• store::validate_attestation_accepts_proper_ancestor_chain
• state_transition::matches_chain_rejects_off_canonical_head (off-canonical / zero-hash / out-of-range head, plus a canonical control)

make fmt and make lint clean.

Note: stf_spectests fail on main independent of this change (stale fixture schema: missing field attestationPubkey); verified identical failure with these changes stashed.

[github-actions]

🤖 Kimi Code Review

This PR implements critical consensus validation (leanSpec #833) ensuring attestation consistency: source → target → head must form a single parent chain. This prevents FFG/LMD vote splitting attacks where a validator might cast a canonical FFG vote (source/target) but pair it with a non-canonical LMD head.

Review Summary

Correctness & Security: ✅ Correct. The ancestry checks in both validate_attestation_data (gossip validation) and attestation_data_matches_chain (state transition) are essential for consensus safety. The logic properly rejects:

• Sibling forks between source/target
• Sibling forks between target/head
• Zero-hash checkpoints
• Out-of-range slots

Code Quality: Good. Well-documented, follows Rust idioms (while let), comprehensive unit tests covering edge cases.

Minor Observations

Performance consideration (crates/blockchain/src/store.rs:124-140):
The checkpoint_is_ancestor function performs a linear walk from descendant to ancestor. In pathological cases (e.g., attestation referencing a very old source checkpoint), this could traverse thousands of slots. While acceptable for gossip validation (attestations typically reference recent checkpoints), consider documenting this complexity or adding a comment that this is bounded by the weak subjectivity period in practice.

Test robustness (crates/blockchain/src/store.rs:1204+):
The test helper insert_test_block creates blocks with state_root: H256::ZERO. Ensure this doesn't trigger any SSZ validation or Merkle root checks elsewhere in the test setup that might fail. The current implementation appears safe for header-only ancestry walks.

State transition consistency (crates/blockchain/state_transition/src/lib.rs:489-522):
The additional head check in attestation_data_matches_chain correctly ensures that justification accounting (keyed by target.root) remains consistent with the LMD head vote. The boundary check at line 514 prevents panics on malformed input.

Verdict

Approve. This is a correct and necessary consensus-layer fix with good test coverage. The ancestry validation logic aligns with the Ethereum consensus specification requirements for vote consistency.

---

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

[github-actions]

🤖 Codex Code Review

1. crates/blockchain/state_transition/src/lib.rs:371 and :497-518, together with crates/blockchain/src/store.rs:534-553 and :40-49: the new STF rule rejects off-canonical head votes for justification, but block import still unconditionally promotes every block-body attestation into the known fork-choice pool. After on_block_core(), update_head() will still count those sibling-head votes in LMD GHOST, so the PR does not actually eliminate their fork-choice influence and creates a split between STF validity and post-import fork choice. These attestations need to be filtered before insert_known_aggregated_payloads_batch() or otherwise tracked as rejected.

2. crates/blockchain/src/store.rs:123-139 and :202-205: checkpoint_is_ancestor() adds an unbounded parent-chain walk on the gossip fast path, and validate_attestation_data() calls it twice before signature verification. An attacker can send old-but-known roots to force O(depth) header lookups per attestation, which turns into a chain-age-dependent DoS vector. I’d want either a bound on acceptable slot deltas here or a cheaper ancestry check backed by canonical slot/root data.

The added unit coverage is useful, and the intended chain-consistency rule itself looks reasonable. I couldn’t run the Rust tests in this environment because cargo tried to fetch the pinned toolchain and network access is blocked.

---

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

[github-actions]

🤖 Claude Code Review

Here is the review:

---

PR Review: fix: enforce attestation checkpoint ancestry (PR #419)

Overview

This PR ports leanSpec #833, closing a fork-choice vote-splitting vector: a validator could craft an attestation with a canonical source → target FFG vote but an LMD-GHOST head on a sibling branch, injecting weight into a non-canonical fork without violating any slot-ordering constraint. The fix enforces source → target → head parent-chain ancestry in two places:

• Gossip validation (store.rs) via a new checkpoint_is_ancestor helper and two ancestry checks in validate_attestation_data.
• State transition (state_transition/src/lib.rs) via an extended attestation_data_matches_chain that now also rejects a mismatched or zero-hash head.

The PR description is excellent — the root-cause explanation and the rationale for using different reference frames (fork-choice store tree vs. historical_block_hashes) are clear and accurate.

---

Correctness

checkpoint_is_ancestor (store.rs:123–140) — logic is sound:

• The ancestor.slot > descendant.slot early-return is correct.
• Walking via parent_root correctly traverses across gap slots (empty slots produce no block, so parent links naturally skip them).
• When ancestor.slot == descendant.slot, the loop finds current_header.slot == ancestor.slot immediately and returns current_root == ancestor.root — a checkpoint is only trivially its own ancestor if the roots match, which is correct.
• The conservative false return when a block is missing from the store is the right defensive posture and is clearly documented.

attestation_data_matches_chain (state_transition/src/lib.rs:497–519) — extension is correct:

• The H256::ZERO guard and head_slot >= len() bounds check mirror the existing source/target guards.
• Checking historical_block_hashes[head_slot] == data.head.root correctly requires the attested head to lie on the same canonical chain as the block being processed.
• If the chain had reorganized since the attestation was created, this check will (correctly) reject the attestation — votes from a now-detached fork should not influence justification on the new canonical chain.

---

Issues

1. Redundant first store fetch in checkpoint_is_ancestor

validate_attestation_data already holds target_header and head_header in scope (lines 158–164). When checkpoint_is_ancestor(store, &data.target, &data.head) is called, the first iteration of the while loop fetches store.get_block_header(&data.head.root) again — an extra round-trip per attestation validation. Same for the source → target call re-fetching data.target.root. This is a minor inefficiency, not a bug. For a slightly tighter API, the function could accept an already-fetched BlockHeader for the initial descendant, or the ancestry walk could start from the pre-fetched header's parent_root.

2. O(slots) walk with no depth documentation

checkpoint_is_ancestor is O(descendant.slot − ancestor.slot) store reads. For typical attestations (source is the latest justified checkpoint, target and head are within the same epoch), this is a handful of reads. But there is no explicit comment about the expected depth, and no guard against a pathological input where source.slot is 0 and head.slot is large. Given that gossip validation also runs before checkpoint_is_ancestor, and the prior slot-ordering checks bound the difference to attestation.slot, the practical bound is the slot depth of a single epoch. Still worth a note in the doc comment.

3. Slightly misleading doc comment on "skipped slot"

store.rs:122:

/// Conservative: a skipped slot or a block missing from the store yields `false`.

"A skipped slot" is misleading — the function walks parent_root links and handles slot gaps correctly (a gap slot has no block, so the parent chain simply points further back). The false is returned only when a block that should be in the store is absent. A more accurate phrasing: "Conservative: if any block along the ancestry path is absent from the store, returns false."

4. Missing test for equal-slot case

No test covers source.slot == target.slot (or target.slot == head.slot). The checkpoint_is_ancestor logic is correct for this case (requires root equality), but an explicit test would document the invariant and guard against a future refactor that breaks it.

5. Pre-existing gap: head.slot < target.slot not checked in attestation_data_matches_chain

Not introduced by this PR, but relevant after the expansion: attestation_data_matches_chain now validates all three checkpoint roots against historical_block_hashes but does not verify head.slot >= target.slot. A vote where head_slot < target_slot could pass this function if both hash lookups succeed. This is caught upstream by validate_attestation_data's HeadOlderThanTarget guard for gossip, but not within the state transition itself. Worth tracking as a hardening item.

---

Tests

The four new tests are well-constructed and cover the key cases:

• validate_attestation_rejects_head_on_sibling_fork — TargetNotAncestorOfHead on a Y-fork ✓
• validate_attestation_rejects_source_on_sibling_fork — SourceNotAncestorOfTarget when source is on an abandoned branch ✓
• validate_attestation_accepts_proper_ancestor_chain — linear canonical chain accepted ✓
• matches_chain_rejects_off_canonical_head — off-canonical / zero-hash / out-of-range head ✓

Suggested additions:

• A test where source.slot == target.slot with matching and non-matching roots (documents equal-slot behavior of checkpoint_is_ancestor).
• A test where an intermediate ancestor block is absent from the store, verifying the conservative false return.

---

Minor Nit

insert_test_block doc comment says "Empty body means get_block_header resolves it without a body row". This is slightly off — the comment implies body storage matters for the header lookup, but get_block_header only reads the BlockHeaders table. The real reason the tests work is that validate_attestation_data only calls get_block_header, never get_block_body. A cleaner comment: "validate_attestation_data only reads block headers, so body content is irrelevant for these tests."

---

Summary

The security fix is correct, well-reasoned, and addresses a genuine vote-splitting vector. The two-layer approach (store-walk for gossip, historical_block_hashes lookup for state transition) is appropriately layered. No correctness bugs found. The items above are improvements, not blockers — Items 1 and 2 (redundant fetch, depth documentation) are the most actionable.

---

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

[greptile-apps]

Greptile Summary

Ports leanSpec #833 to enforce that attestation checkpoints form a single parent chain, not just a slot-ordered sequence. Two enforcement points are updated: gossip validation walks parent links through the fork-choice store, and the state-transition chain-match function now also checks the head checkpoint against the canonical historical_block_hashes view.

• store.rs: introduces checkpoint_is_ancestor (parent-link walk) and calls it twice in validate_attestation_data — source→target and target→head — before the existing time check; adds two new error variants and three unit tests.
• state_transition/src/lib.rs: attestation_data_matches_chain now also rejects zero-hash, out-of-range, and non-canonical head roots, with a unit test covering all four cases.

Confidence Score: 3/5

The state-transition change is correct and safe, but the gossip-layer ancestry walk in store.rs is unbounded and should be capped before shipping.

The new checkpoint_is_ancestor walk is O(descendant.slot minus ancestor.slot) and is invoked before the time check. An adversarial peer can trigger a walk proportional to the full chain height by referencing genesis as source, causing significant CPU cost per gossiped attestation. The state-transition side is straightforward and correct.

crates/blockchain/src/store.rs — specifically the checkpoint_is_ancestor function and its call sites in validate_attestation_data.

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Adds checkpoint_is_ancestor helper and two ancestry assertions in validate_attestation_data; the walk is unbounded and runs before the time check, creating an O(chain-height) DoS path per gossiped attestation when source is at genesis.
crates/blockchain/state_transition/src/lib.rs	Extends attestation_data_matches_chain to reject zero-hash, out-of-range, and off-canonical head checkpoints by checking historical_block_hashes at head_slot; logic is correct and well-tested.

Comments Outside Diff (1)

1. crates/blockchain/src/store.rs, line 197-207 (link)

   Unbounded ancestry walk enables O(chain-height) DoS per attestation

   checkpoint_is_ancestor walks every parent header from descendant.slot down to ancestor.slot, making its cost O(descendant.slot − ancestor.slot). The time check only bounds data.slot, not the source/target slot gap. An adversarial peer can set source = { slot: 0, root: genesis_root } and target = { root: current_canonical_tip, slot: N } — both real blocks already in the store — causing a walk of N steps before the time check fires. On a mature chain this means O(N) header fetches per gossiped attestation.

   Consider adding a depth limit to checkpoint_is_ancestor (e.g. refuse to walk more than MAX_ANCESTOR_WALK_DEPTH steps and return false). Legitimate attestations only span a small, protocol-bounded epoch gap, so a cap of a few hundred slots would never reject a valid vote while closing the DoS path.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: crates/blockchain/src/store.rs
   Line: 197-207
   
   Comment:
   **Unbounded ancestry walk enables O(chain-height) DoS per attestation**
   
   `checkpoint_is_ancestor` walks every parent header from `descendant.slot` down to `ancestor.slot`, making its cost O(descendant.slot − ancestor.slot). The time check only bounds `data.slot`, not the source/target slot gap. An adversarial peer can set `source = { slot: 0, root: genesis_root }` and `target = { root: current_canonical_tip, slot: N }` — both real blocks already in the store — causing a walk of N steps before the time check fires. On a mature chain this means O(N) header fetches per gossiped attestation.
   
   Consider adding a depth limit to `checkpoint_is_ancestor` (e.g. refuse to walk more than `MAX_ANCESTOR_WALK_DEPTH` steps and return `false`). Legitimate attestations only span a small, protocol-bounded epoch gap, so a cap of a few hundred slots would never reject a valid vote while closing the DoS path.
   
   
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
crates/blockchain/src/store.rs:197-207
**Unbounded ancestry walk enables O(chain-height) DoS per attestation**

`checkpoint_is_ancestor` walks every parent header from `descendant.slot` down to `ancestor.slot`, making its cost O(descendant.slot − ancestor.slot). The time check only bounds `data.slot`, not the source/target slot gap. An adversarial peer can set `source = { slot: 0, root: genesis_root }` and `target = { root: current_canonical_tip, slot: N }` — both real blocks already in the store — causing a walk of N steps before the time check fires. On a mature chain this means O(N) header fetches per gossiped attestation.

Consider adding a depth limit to `checkpoint_is_ancestor` (e.g. refuse to walk more than `MAX_ANCESTOR_WALK_DEPTH` steps and return `false`). Legitimate attestations only span a small, protocol-bounded epoch gap, so a cap of a few hundred slots would never reject a valid vote while closing the DoS path.

```suggestion
/// Maximum number of parent-link hops `checkpoint_is_ancestor` will follow.
/// Honest attestations span at most a few epochs; a larger gap indicates either
/// a malformed or adversarial vote and is conservatively rejected.
const MAX_ANCESTOR_WALK_DEPTH: u64 = 1024;

fn checkpoint_is_ancestor(store: &Store, ancestor: &Checkpoint, descendant: &Checkpoint) -> bool {
    if ancestor.slot > descendant.slot {
        return false;
    }
    if descendant.slot - ancestor.slot > MAX_ANCESTOR_WALK_DEPTH {
        return false;
    }

    let mut current_root = descendant.root;
    while let Some(current_header) = store.get_block_header(&current_root) {
```

_{Reviews (1): Last reviewed commit: "fix: enforce attestation checkpoint ance..." | Re-trigger Greptile}