Use post-block checkpoints for proposer attestation source and target

[pablodeymo]

Motivation

A bug was found during devnet3 testing in the Qlean client (C++ implementation) where finality was lagging more than 3 slots behind head. The root cause was identified through structured log analysis: the proposer's attestation was computed before the block was imported into fork choice, causing the attestation to carry stale source (justified) and target checkpoint values.

The ordering bug across clients

When a proposer builds a block containing attestations that advance justification/finalization, the state transition updates these checkpoints. The question is: does the proposer's own attestation reflect the pre-import or post-import checkpoint values?

Qlean (buggy ordering):

produceBlockWithSignatures
  ├─ produceAttestation          ← attestation BEFORE import
  │   ├─ getHead                 ← stale head
  │   └─ getLatestJustified      ← stale justified
  └─ onBlock                     ← import updates head/justified

Zeam (correct ordering):

validator.maybeDoProposal
  ├─ chain.produceBlock
  │   ├─ forkChoice.onBlock      ← import FIRST
  │   └─ forkChoice.updateHead   ← head/justified updated
  └─ chain.constructAttestationData
      ├─ forkChoice.getHead      ← fresh head
      └─ forkChoice.getLatestJustified  ← fresh justified

ethlambda (was buggy, same as Qlean):

propose_block()
  ├─ produce_block_with_signatures()   ← build block
  ├─ Attestation { source: store.latest_justified(), ... }  ← stale!
  ├─ sign attestation
  ├─ assemble SignedBlockWithAttestation
  └─ process_block() → on_block()      ← import AFTER attestation

Why we can't just reorder

In ethlambda (and Qlean), the proposer attestation is embedded inside SignedBlockWithAttestation → BlockWithAttestation. It's part of the block's hash tree root and required for signature verification during import. So unlike Zeam's architecture (where block and attestation are separate), we cannot import the block first and then create the attestation — it's a chicken-and-egg problem.

Description

The fix exploits the fact that build_block() already runs the full state transition to compute state_root. The resulting post_state contains latest_justified and latest_finalized after processing the block's attestations — but these values were previously discarded.

Changes

1. PostBlockCheckpoints struct — carries the two checkpoints (justified + finalized) from build_block back to the caller.

2. build_block / produce_block_with_signatures — return PostBlockCheckpoints alongside the block, extracted from the post-state transition result. No extra computation — just plumbing values that were already computed.

3. get_attestation_target_with_checkpoints — parameterized version of get_attestation_target that accepts justified/finalized checkpoints instead of reading from the store. The existing get_attestation_target becomes a thin wrapper that delegates with store values.

4. propose_block — uses post_checkpoints.justified as source and calls get_attestation_target_with_checkpoints with post-block checkpoints for target.

Behavior

• When block has no attestations (or attestations don't advance justification): post_state.latest_justified == head_state.latest_justified, so behavior is identical to before. No regression.
• When block attestations advance justification: proposer's attestation now correctly reflects the post-import justified checkpoint, preventing finality lag.

How to Test

make fmt    # passes
make lint   # passes
make test   # all 97 tests pass

Full devnet validation is recommended to verify finality behavior under load.

[github-actions]

🤖 Kimi Code Review

This PR correctly fixes a consensus-critical bug where the proposer's attestation used stale checkpoint data from the store rather than the post-state checkpoints resulting from processing the block's attestations.

Consensus Correctness — Approved
The fix properly handles the case where process_block (specifically process_attestations within it) advances justification/finalization. By extracting latest_justified and latest_finalized from post_state after process_block (line 1129-1132) and threading them through to the proposer attestation construction (lines 233-241 in lib.rs), the proposer now attests to the correct source/target checkpoints that reflect the state transition.

Code Quality

• Line 216, 787, 1029: Return type changes from (Block, Vec<...>) to (Block, Vec<...>, PostBlockCheckpoints) are propagated correctly throughout the call chain.
• Lines 648-654: The refactoring to extract get_attestation_target_with_checkpoints is clean and avoids duplication.
• Lines 225-228: Excellent explanatory comment clarifying why post-block checkpoints are necessary.

Minor Observation

• Line 678 in get_attestation_target_with_checkpoints: The finalized parameter is only used for its slot (finalized.slot). Consider documenting whether the root is intentionally ignored for future maintainers, or if the function should accept just finalized_slot: u64 for clarity. This is not a blocking issue.

Testing

• Line 1450: Test updated to handle the new return tuple with _post_checkpoints.

No security vulnerabilities or memory safety issues introduced. The change improves consensus fidelity and the implementation is idiomatic Rust.

---

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

[greptile-apps]

Greptile Summary

This PR fixes a finality-lagging bug where the proposer's attestation was built with stale source/target checkpoint values — read from the store before the proposed block was imported. The fix correctly extracts latest_justified and latest_finalized from the post_state already computed during build_block, and threads them through a new get_attestation_target_with_checkpoints helper so no additional state-transition work is required.

Confidence Score: 5/5

Safe to merge — the fix is minimal, correctly targeted, and all 97 tests pass.

The change threads already-computed checkpoint values from build_block's post_state to the proposer attestation. No new computation is introduced, the existing get_attestation_target is preserved as a thin wrapper (no callers broken), and the regression case (block with no attestations) is a no-op. No P0/P1 issues found.

No files require special attention.

Vulnerabilities

No security concerns identified. The change only plumbs checkpoint values that were already computed during build_block; it introduces no new trust boundaries, no external inputs, and no changes to signature verification logic.

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Adds PostBlockCheckpoints struct, extracts it from build_block's final post_state, and refactors get_attestation_target into a thin wrapper over the new get_attestation_target_with_checkpoints. Logic is sound and well-commented.
crates/blockchain/src/lib.rs	Updates propose_block to destructure post_checkpoints from produce_block_with_signatures and use post_checkpoints.justified for source and the parameterized target helper. Change is minimal and correctly scoped.

Sequence Diagram

sequenceDiagram
    participant PB as propose_block
    participant S as store (produce_block_with_signatures)
    participant BB as build_block
    participant ST as state_transition

    PB->>S: produce_block_with_signatures(slot, validator_id)
    S->>BB: build_block(head_state, slot, ...)
    BB->>ST: process_slots(&mut post_state, slot)
    BB->>ST: process_block(&mut post_state, &final_block)
    note over BB: post_state.latest_justified / .latest_finalized<br/>reflect attestations in the block
    BB-->>S: (final_block, signatures, PostBlockCheckpoints{justified, finalized})
    S-->>PB: (block, attestation_signatures, post_checkpoints)

    PB->>PB: get_attestation_target_with_checkpoints(store, post_checkpoints.justified, post_checkpoints.finalized)
    note over PB: target walk-back uses post-block finalized slot<br/>and clamps to post-block justified checkpoint

    PB->>PB: Attestation { source: post_checkpoints.justified, target: ... }
    note over PB: Fresh checkpoints — no finality lag
    PB->>PB: process_block(signed_block) [import into store]

Loading

_{Reviews (1): Last reviewed commit: "Use post-block checkpoints for proposer ..." | Re-trigger Greptile}

[github-actions]

🤖 Codex Code Review

No findings.

The patch looks correct: it now derives the proposer attestation’s source and target from the same post-block state produced during block building, instead of mixing post-state checkpoints with pre-import store checkpoints. That change in lib.rs and store.rs is consistent with the surrounding fork-choice and state-transition logic, and I don’t see a new consensus, security, or memory-safety regression in the diff.

Residual risk is test coverage, not code correctness: I could not run cargo test in this sandbox because Cargo needs writable dependency caches and tries to fetch a git dependency. A targeted regression test for the proposer path would still be useful near the existing store tests in store.rs, specifically for the case where block-local attestation processing advances justified/finalized checkpoints before the block is imported.

---

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

[github-actions]

🤖 Claude Code Review

Here is my review of PR #268:

---

Review: Use post-block checkpoints for proposer attestation source and target

Summary

The fix is correct and well-motivated. The PR resolves a real consensus bug: the proposer was embedding a stale source (pre-import latest_justified) in its attestation, which could cause other nodes to discard the vote once justification advanced during block import. The approach is minimal — it threads already-computed values through rather than adding new computation.

---

Correctness

Core logic is sound. build_block already runs the full state transition at the end to compute state_root. Extracting post_state.latest_justified / latest_finalized at that point costs nothing and gives the precise checkpoints that will be observable after import. The PostBlockCheckpoints returned from produce_block_with_signatures are then consumed exactly once in propose_block.

is_valid_vote alignment. When other nodes import the block via on_block_core, the state transition produces the same post_state.latest_justified. The proposer's attestation source now matches that checkpoint, so attestations referencing it will pass the justification check in the STF (is_valid_vote:346).

get_attestation_target_with_checkpoints is consistent. Using post_checkpoints.finalized.slot for the justifiable-slot walk-back and post_checkpoints.justified for the lower-bound clamp is correct: both guard expressions now operate on the same post-import checkpoint universe.

Justified-slot source alignment. Previously, propose_block used store.latest_justified() (store-wide maximum across all forks), while produce_attestation_data uses head_state.latest_justified. The new code derives source from post_state (a forward extension of head_state), which is more consistent with how regular validators compute their source. This is an incidental improvement.

---

Issues / Observations

1. Walk-back still starts from the parent block, not the new block (store.rs:653)

let mut target_block_root = store.head();  // = parent block, new block not imported yet

The proposer attests head = new_block_root, but the target is walked back from the parent (store.head()). The target ends up as an ancestor of the parent, which is also an ancestor of the new block, so the topology invariant target.slot ≤ head.slot holds. This was also the case before the PR and is a structural limitation (the block cannot be imported before signing the attestation containing it). No action needed unless the spec requires the walk-back to start from the head checkpoint explicitly.

2. PostBlockCheckpoints has no #[derive(Debug)] (store.rs:44)

The struct is pub and used across module boundaries. Adding #[derive(Debug)] would be conventional and costs nothing.

3. finalized.root is carried but never read

PostBlockCheckpoints::finalized is a full Checkpoint, but get_attestation_target_with_checkpoints only reads finalized.slot (store.rs:678). The root field is silently dropped. Not a bug, but a small mislead — a doc comment noting this would help future readers.

4. Potential double state transition when loop runs (store.rs:1088–1126)

When aggregated_payloads is non-empty and the loop iterates at least once, the last loop iteration already runs process_slots + process_block on the final attestation set, and then the code outside the loop runs them again to compute state_root and post_checkpoints. This redundancy pre-dates this PR (it was there for state_root computation) and is now extended to cover post_checkpoints. Functionally correct, since both invocations start from head_state.clone() and use the same final attestation set. Calling it out as context for anyone who later considers optimizing build_block.

5. Test uses _post_checkpoints (store.rs:1447)

let (block, signatures, _post_checkpoints) = build_block(...)

This is idiomatic and intentionally signals "unused." Fine as-is; no test coverage of the returned checkpoints is needed since the value is exercised by the propose_block path.

---

Verdict

Approve. The fix is correct, minimal, and well-documented. The PR description accurately describes the ordering constraint and why simple reordering isn't possible. Items 2–3 above are minor polish suggestions, not blockers.

---

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