fix: use store's latest_justified as attestation source (leanSpec #595)

[MegaRedHand]

Summary

Applies leanSpec#595, which reverts leanSpec#506. Attestation production now uses the store's global latest_justified checkpoint again; liveness under a justified-divergence is preserved by the existing fixed-point loop in build_block, guarded by a new invariant that blocks must not lag the store's justified slot.

• produce_attestation_data returns source = store.latest_justified() (drops the head-state-derived source and the genesis ZERO-root patch; latest_justified is seeded to the anchor block root at genesis, so no correction is needed).
• produce_block_with_signatures checks post_checkpoints.justified.slot >= store.latest_justified().slot and otherwise returns a new StoreError::JustifiedDivergenceNotClosed variant. The existing call site in crates/blockchain/src/lib.rs already uses inspect_err + early return, so the node logs Failed to build block and skips proposal rather than crashing.
• Removed the unit test produce_attestation_data_uses_head_state_justified since it codified the reverted behavior. No new tests were added; the self-healing block-production path is exercised by the fork-choice spec tests against the currently pinned leanSpec commit.

Spec fixture compatibility

The pinned LEAN_SPEC_COMMIT_HASH (bc17f7ae…) is 30 commits ahead of leanSpec#595's merge commit (fcef7ef9…), with the merge base equal to that commit. No fixture regeneration is required: existing fixtures already reflect the post-595 behavior.

Test plan

• cargo fmt --all --check
• cargo clippy --workspace --all-targets -- -D warnings
• cargo test --workspace --release (313 passed, 6 ignored, 29 suites), including forkchoice_spectests (70 tests) and stf_spectests against the currently pinned leanSpec commit
• Devnet smoke test via .claude/skills/test-pr-devnet/scripts/test-branch.sh to confirm multi-client finalization still advances and the new invariant doesn't fire under normal operation

[greptile-apps]

Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance.

[github-actions]

🤖 Kimi Code Review

Item 1: Potential genesis checkpoint regression

crates/blockchain/src/store.rs, line 644 (new): The removal of the genesis substitution logic (if head_state.latest_block_header.slot == 0) may cause attestations produced at genesis to have source.root = H256::ZERO. The removed comment explicitly noted: "At genesis the checkpoint root is H256::ZERO; substitute the real genesis block root so attestation validation can look it up."

Verify that store.latest_justified() returns a checkpoint with the actual genesis block root (not H256::ZERO) at slot 0. If the store initializes its justified checkpoint from the state (which may contain H256::ZERO), attestation validation will fail because validators cannot look up a checkpoint with a zero root.

Item 2: Missing test coverage for new invariant

The removed test produce_attestation_data_uses_head_state_justified (lines 1488-1554) validated the old behavior. The new behavior—using store.latest_justified()—and the new JustifiedDivergenceNotClosed error variant lack test coverage.

Add tests to verify:

• Attestation data correctly uses the store's global justified checkpoint even when it exceeds the head state's justified checkpoint.
• produce_block_with_signatures returns Err(StoreError::JustifiedDivergenceNotClosed) when post_checkpoints.justified.slot < store.latest_justified().slot.

Item 3: Unclear function reference

crates/blockchain/src/store.rs, line 723: The comment references "the fixed-point loop in build_block". If build_block is defined in a different module or is an implementation detail of the caller, consider clarifying this reference or generalizing the comment (e.g., "the fixed-point loop in the block builder") to avoid confusion for readers of this function in isolation.

Item 4: Performance and safety improvement

Acknowledged: The removal of store.get_state(&head_root) in produce_attestation_data eliminates an expensive state lookup and removes a potential panic point (the .expect("head state exists")). This is a good optimization assuming the genesis root handling (Item 1) is verified safe.

Item 5: Invariant check correctness

crates/blockchain/src/store.rs, lines 719-728: The placement of the justified divergence check immediately after compute_post_block_checkpoints is correct. The strict inequality (<) properly allows equality (no divergence) while catching lagging justification. Ensure that compute_post_block_checkpoints accounts for attestations included in the block being produced, as the fixed-point loop expectation relies on these attestations advancing the justified checkpoint.

---

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

[github-actions]

🤖 Codex Code Review

Findings:

1. High: this change makes proposer-side liveness depend on an impossible checkpoint bootstrap when store.latest_justified advances on a non-head fork. on_block already promotes store.latest_justified from any imported block with a higher slot, even if the head stays elsewhere (store.rs). After this PR, produce_attestation_data always emits source = store.latest_justified() (store.rs), but build_block still seeds its fixed-point loop from head_state.latest_justified and only accepts payloads whose source exactly matches that current checkpoint (store.rs, store.rs). Worse, the state transition only counts votes whose source is already justified in the head state and whose checkpoints exist on that chain (lib.rs, lib.rs). So a higher justified checkpoint learned from a minority fork cannot “close the gap” through this loop; the new globally-sourced attestations are ignored by the very state transition that is supposed to advance justification. I would not merge this unless the source derivation, block-builder filter, and is_valid_vote semantics are made consistent again.

2. High: the new JustifiedDivergenceNotClosed check turns that inconsistency into a proposer DoS. If a peer feeds us a valid side-fork block that bumps store.latest_justified, produce_block_with_signatures now errors instead of proposing a valid block on the current head (store.rs). That means a recoverable fork-choice divergence becomes an availability failure for block production. At minimum this should stay as a metric/log until Item 1 is actually solvable; otherwise one higher-justified non-head import can stall local proposing.

I could not run cargo test here because rustup tries to write under a read-only home directory in this environment, so this review is from static analysis.

---

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

[github-actions]

🤖 Claude Code Review

Now I have sufficient context to write a thorough review. Here is the analysis:

---

PR Review: fix: use store's latest_justified as attestation source (leanSpec #595)

Overview

This PR reverts leanSpec #506 and re-aligns produce_attestation_data to use store.latest_justified() as the attestation source. The old head-state-derived source (with its genesis H256::ZERO patch) is removed. A new invariant in produce_block_with_signatures guards against publishing a block that lags the store's justified slot, failing closed by returning JustifiedDivergenceNotClosed instead of emitting a bad block.

---

Correctness

Genesis correction removal is sound.

init_store (storage/src/store.rs:505) seeds KEY_LATEST_JUSTIFIED to anchor_checkpoint = { root: anchor_block_root, slot: anchor_state.latest_block_header.slot }. At genesis anchor_block_root is the real block root, not H256::ZERO, so store.latest_justified().root is always a valid root from initialization onward. The patch removed from produce_attestation_data was compensating for head_state.latest_justified.root == H256::ZERO — not the store's value — and was correctly identified as unnecessary for the store path.

The analogous genesis correction in build_block (lines 1046–1053) must remain — it reads head_state.latest_justified, which is Checkpoint::default() (root = H256::ZERO) at genesis. The two code paths use different data sources and require different handling.

The invariant check is well-placed.

JustifiedDivergenceNotClosed is returned only after build_block returns Ok, so normal StoreError propagation is unaffected. The upstream call site (lib.rs:291) already wraps with inspect_err + skip-on-error, so a failing proposal is logged and the node continues normally.

Liveness under divergence: the recovery path depends on pre-existing pool attestations.

In a justified-divergence scenario (store.latest_justified().slot = N, head_state.latest_justified.slot = M, N > M), all newly produced attestations will carry source = {slot: N, ...}. build_block starts its filter at current_justified = {slot: M, ...}, so those new attestations are immediately filtered out. The fixed-point loop can only advance current_justified toward N using attestations that were produced before the divergence (i.e., attestations already in the pool with source.slot between M and N). If the pool lacks those bridging attestations, every proposal attempt returns JustifiedDivergenceNotClosed until the store's justified value is reachable via the accumulated pool. This is the intended behavior per the spec, but it means recovery is entirely pool-dependent — a hard-to-debug liveness stall could occur on a heavily pruned pool.

---

Code Quality

Stray blank line (store.rs, produce_attestation_data):

After removing let head_state = ..., a blank line was left before let head_checkpoint = .... Minor, but the surrounding function now has an odd gap that reads like removed code.

Invariant error message is slightly misleading:

#[error(
    "Produced block justified slot {block_justified_slot} \
     is behind store justified slot {store_justified_slot}; \
     fixed-point attestation loop did not converge"
)]

"Did not converge" implies the loop ran and failed to settle; the actual root cause is more often "pool had insufficient bridging attestations." A phrasing like "pool attestations insufficient to close justification gap" would be more actionable for operators reading logs.

Slot-only comparison in the invariant:

if post_checkpoints.justified.slot < store_justified_slot {

This compares slots only. Two Checkpoints with equal slots but different roots would pass the guard. In this codebase, justification is deterministic per slot (one canonical justified block per slot), so this is safe in practice, but a comment acknowledging the assumption would help future readers.

---

Test Coverage

The removed test produce_attestation_data_uses_head_state_justified codified the reverted behavior — its removal is correct. However, no new test was added for the new behavior (using store.latest_justified()). Adding a test that mirrors the removed one but asserts attestation.source == store.latest_justified() would:

• Prevent silent regression if the store-seeding logic in init_store changes
• Document the invariant that store.latest_justified().root is always the real anchor root (not H256::ZERO)

The PR acknowledges this and notes the fork-choice spec tests cover it indirectly. Acceptable for now, but worth tracking.

---

Summary

The logic is correct and the change is well-scoped. The genesis-correction removal is justified by the store-seeding path in init_store. The new invariant fails closed and is handled gracefully upstream. The main risks are:

• Operator confusion from the "did not converge" phrasing when stall is pool-driven
• The undocumented pool-dependent recovery path for divergence scenarios (correct per spec, but non-obvious)
• Missing direct test for produce_attestation_data's new contract

These are minor. The change is ready to merge pending the devnet smoke test listed in the test plan.

---

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