Fix attestation source divergence (leanSpec PR #506)

[pablodeymo]

Summary

• Use head_state.latest_justified instead of store.latest_justified() as attestation source in produce_attestation_data, aligning voting with the block builder's filter
• Handle genesis edge case (zero-hash root substitution) matching existing build_block logic
• Add Rust unit test ported from leanSpec PR #506

Root Cause

produce_attestation_data used store.latest_justified (the global max across all forks) as the attestation source. build_block filters attestations against head_state.latest_justified (the head chain's view). When a non-head fork block advances the store-wide justified past the head chain's justified — e.g. a minority fork carrying a supermajority of attestations — every attestation is rejected during block building. This produces blocks with 0 attestations, stalling justification and finalization indefinitely.

Ref: leanSpec PR #506, 3sf-mini reference (Staker.vote() uses self.post_states[self.head].latest_justified_hash)

Note on spec test fixtures

The leanSpec commit that includes PR #506 also introduces a validator format change (pubkey → attestationPubkey + proposalPubkey) that requires a domain model migration. The spec test fixture update is deferred to a separate PR. The unit test covers the same invariant.

Test plan

• Unit test produce_attestation_data_uses_head_state_justified passes
• cargo fmt --all -- --check clean
• cargo clippy -p ethlambda-blockchain -- -D warnings clean
• Full make test with existing fixtures (running)

[github-actions]

🤖 Kimi Code Review

General Assessment
Correct and necessary consensus-layer fix. The change aligns attestation source selection with the validator's local view (head state) rather than the store's global maximum, preventing potential attestation validation mismatches. The genesis edge case handling is appropriate.

Specific Items

1. Consensus Correctness (store.rs:743-778)

   • The switch from store.latest_justified() to head_state.latest_justified is the correct behavior per the spec. Validators must attest based on their view of the justified checkpoint in the head state, not the globally highest justified checkpoint seen by the store. This prevents fork choice safety violations where validators vote on incompatible sources.

2. Genesis Handling (store.rs:753-761)

   • The substitution of H256::ZERO with the actual genesis block root (head_root) is required for attestation validation correctness. At genesis, latest_justified.root is initialized to zeros, but attestations must reference a retrievable block root.
   • Minor: Ensure head_state.latest_justified.slot is indeed 0 at genesis (it should be per spec initialization), otherwise the checkpoint epoch won't match the genesis epoch.

3. Panic Safety (store.rs:748, 764-767)

   • The two .expect() calls assume invariants that should hold in correct operation (head state and head block exist). Given this is attestation production code where these are genuine invariants, panicking is acceptable, but consider returning Result if this function is exposed to network input that could theoretically trigger race conditions during reorgs.

4. Performance (store.rs:748)

   • store.get_state(&head_root) clones the full beacon state. If this is called frequently (e.g., for every validator in a large set every slot), consider caching the head state in the calling context to avoid O(n) state copies. This is acceptable for the PR scope but worth monitoring.

Security Considerations

• Fixed: Previously, using store.latest_justified() could cause validators to attest to a source checkpoint newer than what the block builder considers valid for the head state's fork, leading to attestations being rejected or causing inclusion delay. This PR fixes that vector.

Readability

• Excellent documentation at lines 743-747 explaining the rationale and linking to the spec PR. The variable name source is clearer than the previous direct function call.

---

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

[github-actions]

🤖 Codex Code Review

Findings

1. crates/blockchain/src/store.rs:749 fixes source to use head_state.latest_justified, but target still comes from get_attestation_target(store), whose fallback clamp is based on store.latest_justified() at crates/blockchain/src/store.rs:726. In the reorg case this PR is addressing, that can still produce a mixed attestation: source from the head branch and target from a different branch/global checkpoint. That is especially risky because local validation only checks block existence and slot ordering, not ancestry, at crates/blockchain/src/store.rs:227 and crates/blockchain/state_transition/src/lib.rs:346. Result: the node can still emit or accept attestations that are internally inconsistent and later get ignored by STF. get_attestation_target should derive its lower bound from the same head-state view as source, or accept that checkpoint as an explicit parameter.

No other material correctness, security, memory-safety, or performance issues stood out in this diff. The Makefile spec bump looks routine.

---

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

[github-actions]

🤖 Claude Code Review

Now I have enough context to write the review.

---

Review: Fix attestation source divergence (leanSpec PR #506)

Summary

The fix correctly addresses the root cause: produce_attestation_data was using store.latest_justified() (global max across all forks) while build_block filters with head_state.latest_justified (head-chain view). This divergence caused all attestations to be rejected during block building when a minority fork carried a supermajority of attestations, stalling justification indefinitely.

---

Correctness

Genesis substitution alignment — The condition head_state.latest_block_header.slot == 0 with root: head_root is identical to the pattern in build_block (line 1053-1059). In both cases, the genesis state's Checkpoint::default() has root: H256::ZERO, which would fail validate_attestation_data's availability check (line 231-233) since there's no block stored at that root. The substitution is necessary and correct.

Post-genesis unjustified state — For slots ≥ 2 before any justification occurs, head_state.latest_justified is still {slot: 0, root: H256::ZERO} (only slot 0 is excluded by the genesis check). This would cause validate_attestation_data to return StoreError::UnknownSourceBlock. However, this is identical to the pre-existing behavior with store.latest_justified() and is a pre-existing limitation, not introduced by this PR.

---

Minor Issues

Redundant storage lookup — Lines 767-770 call store.get_block_header(&head_root).expect("head block exists").slot to populate head_checkpoint.slot. But head_state.latest_block_header.slot holds the same value and is already fetched on line 751. The second storage read is unnecessary:

// Current:
let head_checkpoint = Checkpoint {
    root: head_root,
    slot: store
        .get_block_header(&head_root)
        .expect("head block exists")
        .slot,
};

// Could be:
let head_checkpoint = Checkpoint {
    root: head_root,
    slot: head_state.latest_block_header.slot,
};

Genesis substitution duplicated — The same pattern appears verbatim in produce_attestation_data (lines 756-763) and build_block (lines 1053-1060). If the genesis semantics ever change, both sites need to stay in sync. A small helper like resolved_justified(head_state: &State, head_root: H256) -> Checkpoint would eliminate the duplication. Not blocking, but worth tracking.

get_attestation_target clamping uses global justified — The guard at line 726-733 still clamps to store.latest_justified(). In the exact fork divergence scenario this PR fixes, if the safe target walks behind the global (non-head) justified slot, the function returns the non-head fork's justified checkpoint as the target while the source is now head_state.latest_justified (head-chain). This produces an attestation pairing a head-chain source with a non-head-fork target — it will fail build_block's att_data.source != current_justified filter anyway. Pre-existing behavior, not introduced here, but the clamping guard's correctness assumption has shifted.

---

Positive Notes

• The fix precisely mirrors the reference implementation (Staker.vote() in p2p.py) and is well-motivated by the leanSpec PR.
• The inline doc comment on produce_attestation_data clearly explains the invariant and links to the spec PR.
• LEAN_SPEC_COMMIT_HASH is updated to include the new spec test, keeping the spec pinned to a version that covers the fix.

---

Overall: The core fix is correct and the approach is sound. The redundant get_block_header lookup is worth cleaning up; the genesis substitution duplication is a minor maintainability note. No blockers.

---

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

[greptile-apps]

Greptile Summary

This PR fixes a correctness bug where produce_attestation_data used the store-wide global maximum justified checkpoint (store.latest_justified()) as the attestation source, while build_block filters attestations against the head chain's per-state justified checkpoint (head_state.latest_justified). When a minority fork carried enough attestations to advance the store-wide justified past the head chain's justified, every produced attestation would be rejected during block building — resulting in blocks with zero attestations and stalled finalization.

Key changes:

• produce_attestation_data now derives source from head_state.latest_justified, aligning the validator's vote with the block builder's filter
• Genesis edge case (zero-hash checkpoint root) is handled identically to the existing build_block logic by substituting the real genesis block root
• LEAN_SPEC_COMMIT_HASH updated to pull in the new spec test (test_gossip_attestation_accepted_after_fork_advances_justified)

The one item worth tracking: get_attestation_target's store-wide clamp guard (line 726) now creates a potential inconsistency where source is head-chain scoped but a clamped target could theoretically return a root from the minority fork. This is unlikely in practice and build_block's process_block would reject such attestations anyway, but it is worth a follow-up to bring the clamping in line with the new per-chain semantics.

Confidence Score: 4/5

• Safe to merge; the core fix is correct and the only outstanding item is a P2 follow-up on the clamping guard in get_attestation_target
• The fix correctly aligns the attestation source with the block builder's filter, and the genesis edge case is handled consistently. The one remaining observation (clamping guard still using store.latest_justified()) is a pre-existing guard marked as out-of-spec, unlikely to trigger in the failure scenario, and defended by build_block's process_block validation. All findings are P2 — lowering from 5 only because the clamping inconsistency is worth a documented follow-up before it can be forgotten.
• crates/blockchain/src/store.rs — specifically the get_attestation_target clamping guard around line 726

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Fixes attestation source divergence by using head-state justified checkpoint instead of store-wide maximum; genesis edge case handled consistently with build_block; minor clamping inconsistency in get_attestation_target
Makefile	Bumps LEAN_SPEC_COMMIT_HASH to include the new spec test for gossip attestation accepted after fork advances justified

Sequence Diagram

sequenceDiagram
    participant V as Validator
    participant PA as produce_attestation_data
    participant GAT as get_attestation_target
    participant S as Store
    participant BB as build_block

    V->>PA: produce_attestation_data(slot)
    PA->>S: store.head()
    S-->>PA: head_root
    PA->>S: store.get_state(&head_root)
    S-->>PA: head_state

    note over PA: Genesis edge case?<br/>head_state.latest_block_header.slot == 0<br/>→ substitute head_root for H256::ZERO

    PA->>GAT: get_attestation_target(store)
    GAT->>S: store.head(), walk-back logic
    S-->>GAT: target_checkpoint
    GAT-->>PA: target_checkpoint

    PA-->>V: AttestationData { source: head_state.latest_justified, target, head }

    V->>BB: build_block(head_state, attestations)
    note over BB: Filter: att.source == head_state.latest_justified ✓<br/>(was store.latest_justified() before fix)
    BB-->>V: Block with matched attestations

Loading

Comments Outside Diff (1)

1. crates/blockchain/src/store.rs, line 726-733 (link)

   Target-clamp still uses store-wide justified, inconsistent with new source

   After this PR, produce_attestation_data derives source from head_state.latest_justified (the head chain's view), but get_attestation_target's clamping guard still returns store.latest_justified() — which can be a checkpoint from a different fork in the exact minority-fork scenario this PR is fixing.

   If the walk-back in get_attestation_target triggers the guard while store.latest_justified() is ahead of head_state.latest_justified and comes from a minority-fork block, the resulting AttestationData can end up with:

   • source.root = main-chain head-state justified root (correct head-chain checkpoint)
   • target.root = minority-fork checkpoint root (wrong chain!)

   Before this PR the source and clamped target were both store.latest_justified() (same, wrong-fork checkpoint but internally consistent). After the fix they diverge.

   In practice the walk-back from a reasonably long head chain is unlikely to fall below the minority-fork's justified slot, and build_block's process_block call would reject such an attestation anyway. However, for full consistency with the new per-chain semantics it is worth tracking whether the clamping guard should also switch to head_state.latest_justified (while continuing to use store.latest_justified() only for the warning log).

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: crates/blockchain/src/store.rs
   Line: 726-733
   
   Comment:
   **Target-clamp still uses store-wide justified, inconsistent with new source**
   
   After this PR, `produce_attestation_data` derives `source` from `head_state.latest_justified` (the head chain's view), but `get_attestation_target`'s clamping guard still returns `store.latest_justified()` — which can be a checkpoint from a **different fork** in the exact minority-fork scenario this PR is fixing.
   
   If the walk-back in `get_attestation_target` triggers the guard while `store.latest_justified()` is ahead of `head_state.latest_justified` and comes from a minority-fork block, the resulting `AttestationData` can end up with:
   - `source.root` = main-chain head-state justified root (correct head-chain checkpoint)
   - `target.root` = minority-fork checkpoint root (wrong chain!)
   
   Before this PR the source and clamped target were both `store.latest_justified()` (same, wrong-fork checkpoint but internally consistent). After the fix they diverge.
   
   In practice the walk-back from a reasonably long head chain is unlikely to fall below the minority-fork's justified slot, and `build_block`'s `process_block` call would reject such an attestation anyway. However, for full consistency with the new per-chain semantics it is worth tracking whether the clamping guard should also switch to `head_state.latest_justified` (while continuing to use `store.latest_justified()` only for the warning log).
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 726-733

Comment:
**Target-clamp still uses store-wide justified, inconsistent with new source**

After this PR, `produce_attestation_data` derives `source` from `head_state.latest_justified` (the head chain's view), but `get_attestation_target`'s clamping guard still returns `store.latest_justified()` — which can be a checkpoint from a **different fork** in the exact minority-fork scenario this PR is fixing.

If the walk-back in `get_attestation_target` triggers the guard while `store.latest_justified()` is ahead of `head_state.latest_justified` and comes from a minority-fork block, the resulting `AttestationData` can end up with:
- `source.root` = main-chain head-state justified root (correct head-chain checkpoint)
- `target.root` = minority-fork checkpoint root (wrong chain!)

Before this PR the source and clamped target were both `store.latest_justified()` (same, wrong-fork checkpoint but internally consistent). After the fix they diverge.

In practice the walk-back from a reasonably long head chain is unlikely to fall below the minority-fork's justified slot, and `build_block`'s `process_block` call would reject such an attestation anyway. However, for full consistency with the new per-chain semantics it is worth tracking whether the clamping guard should also switch to `head_state.latest_justified` (while continuing to use `store.latest_justified()` only for the warning log).

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "Fix attestation source divergence (leanS..." | Re-trigger Greptile}

[MegaRedHand]

LGTM