fix(state-transition): preserve monotonicity of latest_justified within a block

[MegaRedHand]

Description

We recently found a bug in the spec and made a PR patching it: leanEthereum/leanSpec#781. This PR updates our code matching those changes.

What changed

• process_attestations unconditionally assigned state.latest_justified = target on every supermajority threshold crossing. When a block carried multiple supermajority attestations whose targets were not in increasing slot order, a later attestation clobbered an earlier one and the post-state ended below the highest justified target.
• The store latches latest_justified monotonically across all branches (crates/blockchain/src/store.rs:485-491). Once any branch advanced the store ahead of the canonical chain's post-state, every subsequent proposal failed JustifiedDivergenceNotClosed and the chain froze permanently.
• Fix: guard the assignment so latest_justified only advances. justified_slots continues to be updated unconditionally — multiple slots can be independently justified within a block.

Observed failure

Devnet froze at slot 27969 (finalized) / 27978 (justified) / 27984 (head). Investigation:

• Canonical head 0xef43cf66 (slot 27984) carried 3 supermajority attestations in body order targeting slots 27978, 27981, 27974 (all justifiable from source=27972).
• After process_attestations, post-state latest_justified = (27974, ...) instead of (27981, ...), because the last-applied attestation overwrote the higher justifications.
• Meanwhile, fork block 0xe88cff27 at slot 27983 had advanced the store's latest_justified to 27978.
• From then on, every proposer's build_block produced justified=27974 and failed the JustifiedDivergenceNotClosed check at crates/blockchain/src/store.rs:737. 37,787 proposal failures across 4 validators over 4 days before recovery.

Test plan

• New regression test latest_justified_does_not_regress_within_block in state_transition/src/lib.rs: 4-validator state, three supermajority attestations in body order targeting slots 4 → 9 → 6. Fails on main (ends at slot 6); passes with the fix (stays at slot 9).
• cargo test -p ethlambda-state-transition --lib — 1/1 pass
• cargo test -p ethlambda-state-transition --test stf_spectests — 51/51 pass
• cargo clippy -p ethlambda-state-transition --all-targets -- -D warnings — clean

[github-actions]

🤖 Kimi Code Review

This is a critical consensus bugfix. The change prevents latest_justified from regressing within a single block when processing attestations with non-monotonic target slots.

Approval with minor notes:

Code Correctness

• Line 310-313 (state_transition/src/lib.rs): The fix is correct. The monotonicity check target.slot > state.latest_justified.slot ensures that once a higher slot is justified within a block, later attestations cannot overwrite it with lower slots.
• Line 315-319: The set_justified call correctly remains outside the conditional, ensuring all supermajority targets are recorded in justified_slots even if they don't advance latest_justified.

Consensus Safety

• This fixes a chain-freeze vulnerability. As noted in the devnet incident (slot 27984), regression of latest_justified causes permanent JustifiedDivergenceNotClosed failures because the store's view advances monotonically while the state regresses.
• The 2/3 supermajority check (3 * vote_count >= 2 * validator_count) at line 307 correctly implements BFT threshold logic.

Testing

• Lines 560-694: Excellent regression test. The test name latest_justified_does_not_regress_within_block is descriptive, and the comment documents the exact devnet failure mode (slots 27978/27981/27974).
• Line 690-694: Verify that ShortRoot is defined in the test module's scope (not visible in diff). If not, the format string will fail to compile.

Style/Nits

• The helper functions make_validators, make_bits, and make_attestation are well-structured and improve test readability.
• Consider adding a test case where attestations are already in ascending order to ensure the fix doesn't break the normal case (though this is implicitly covered by existing tests).

Security Assessment

• Critical: Without this fix, a malicious or buggy block proposer could order attestations to cause a justification regression, leading to consensus failure.
• The fix aligns with the monotonicity invariant required by the fork choice rule (LMD GHOST / 3SF-mini).

The PR is ready to merge.

---

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

[github-actions]

🤖 Codex Code Review

No findings.

crates/blockchain/state_transition/src/lib.rs:307-318 looks correct: making latest_justified slot-monotonic matches the store-side invariant that only higher-slot justified checkpoints are promoted, and it fixes the liveness failure mode without changing vote accounting, finalization eligibility, XMSS handling, or SSZ behavior.

crates/blockchain/state_transition/src/lib.rs:626-693 is a solid regression test. It reproduces the reported non-monotonic attestation ordering within one block and asserts the post-state keeps the highest justified target, which is the right consensus invariant for this bug.

Testing gap: I could not execute cargo test -p ethlambda-state-transition latest_justified_does_not_regress_within_block in this environment because the pinned Rust 1.92.0 toolchain was not available locally and network access is blocked, so this review is based on static analysis only.

---

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

[greptile-apps]

Greptile Summary

This PR fixes a monotonicity bug in process_attestations where multiple supermajority attestations within a single block, whose target slots arrived in non-increasing order, could cause state.latest_justified to regress — ultimately freezing the devnet permanently.

• Core fix (state_transition/src/lib.rs): wraps the state.latest_justified = target assignment in a if target.slot > state.latest_justified.slot guard, so the pointer only ever advances; justified_slots and try_finalize continue to be applied unconditionally for every justified target.
• Regression test: adds latest_justified_does_not_regress_within_block — a 4-validator state with three supermajority attestations in body order targeting slots 4 → 9 → 6, asserting the post-state lands at slot 9, not 6.
• Dependency update: libssz-types is added to the crate's dev-dependencies to support the SszList construction in the new test.

Confidence Score: 5/5

Safe to merge — the change is a single-line guard that only narrows when latest_justified is written, with no risk of regression on the happy path.

The fix is minimal and precisely targeted: one conditional wrapping one assignment, leaving justified_slots and try_finalize untouched. The regression test directly reproduces the devnet failure scenario (out-of-order target slots 4→9→6) and fails on main, passes with the fix. The spec test suite (51/51) and clippy are clean.

No files require special attention.

Important Files Changed

Filename	Overview
crates/blockchain/state_transition/src/lib.rs	Single-line monotonicity guard added to process_attestations; comprehensive regression test with correct arithmetic justifiability checks across three out-of-order target slots.
crates/blockchain/state_transition/Cargo.toml	Adds libssz-types as a workspace dev-dependency to support SszList construction in the new test.
Cargo.lock	Lock file updated to reflect the new libssz-types dev-dependency; no unexpected crate additions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[process_attestations: iterate over attestations] --> B{is_valid_vote?}
    B -- No --> A
    B -- Yes --> C[Accumulate votes for target.root]
    C --> D{3 × vote_count ≥ 2 × validator_count?}
    D -- No --> A
    D -- Yes --> E{target.slot > state.latest_justified.slot?}
    E -- Yes --> F[state.latest_justified = target]
    E -- No / equal --> G[skip — guard preserves monotonicity]
    F --> H[justified_slots_ops::set_justified — always called]
    G --> H
    H --> I[try_finalize — always called]
    I --> A

Loading

_{Reviews (1): Last reviewed commit: "fix(state-transition): preserve monotoni..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

Now I have enough context to write a comprehensive review.

---

PR Review: fix(state-transition): preserve monotonicity of latest_justified within a block

Overview

This PR fixes a consensus liveness bug where process_attestations unconditionally assigned state.latest_justified = target for each supermajority attestation in a block. When a block contained multiple supermajority attestations whose target slots were non-monotonic (e.g., slots 4 → 9 → 6 in body order), the final post-state ended at the last-processed target rather than the highest justified target. This caused a permanent chain freeze when the store's monotonic latest_justified was already above the post-state's, triggering JustifiedDivergenceNotClosed on every future proposal.

---

Fix Correctness

crates/blockchain/state_transition/src/lib.rs, line 316:

if target.slot > state.latest_justified.slot {
    state.latest_justified = target;
}

The one-line guard is exactly right. Key observations:

• justified_slots_ops::set_justified remains unconditional — correct, because all independently justified slots should be recorded regardless of ordering.
• try_finalize remains unconditional — correct, because a later attestation (e.g., source=3, target=6) could trigger finalization of the source independently of whether latest_justified advanced.
• Strict > (not >=) is correct: if two attestations target the same slot with different roots, the first one wins. You wouldn't want to overwrite a justified checkpoint with a competing root at the same slot.
• Pre-state latest_justified that's already above any block attestation target is correctly handled — the guard prevents regression in that case too.

Interaction with finalization mid-loop: If try_finalize advances latest_finalized in iteration N, then in iteration N+1, set_justified uses the updated latest_finalized.slot — consistent with the shifted justified_slots window. This was pre-existing behavior and remains correct.

---

Test Coverage

The regression test latest_justified_does_not_regress_within_block is well-constructed:

• 4 validators, 3 attestations (3 of 4 validators each, so supermajority for each), target slots 4 → 9 → 6.
• Targets are verified to be justifiable from finalized_slot=0: Δ=4 ≤ 5, Δ=9=3², Δ=6=2·3 (pronic). Correct.
• Source slot 3 is marked justified in justified_slots so is_valid_vote passes.
• Asserts both latest_justified.slot == 9 and latest_justified.root == r9. Good — root is checked too.

One missing case: no test for when all attestation targets fall below a high pre-state latest_justified (pure no-op guard). This is a simple case and not critical, but would strengthen coverage.

---

Style / Project Conventions

• Comment length (lines 307–315): Eight lines is verbose by CLAUDE.md guidelines ("Only add [a comment] when the WHY is non-obvious — one short line max"). The WHY here is genuinely non-obvious, but the detailed devnet context (slots 27984, fork block, 37k failures) belongs in the PR description and commit message — not the source file. Consider trimming to the essential invariant: attestations within a block can arrive in non-monotonic slot order; take the maximum.

• Test doc comment (lines 608–633): 26 lines over multiple paragraphs. CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks." The test name and assertion message already explain the regression scenario. The docstring largely duplicates the PR body.

• make_attestation's att_slot parameter is always 3 at every call site in the test. Since AttestationData::slot is not checked anywhere in process_attestations or is_valid_vote, this has no functional impact — but it's a source of confusion about what that field represents.

---

Summary

The fix is correct, minimal, and directly addresses the observed production failure. The regression test validates the exact scenario from the devnet incident. No logic bugs or safety issues were introduced. The only actionable feedback is trimming the in-code comment and test doc comment to comply with project style guidelines.

---

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