fix(stf): reject blocks justifying a target whose source is below finalized#402
fix(stf): reject blocks justifying a target whose source is below finalized#402MegaRedHand wants to merge 2 commits into
Conversation
…alized
leanSpec's `Slot.is_justifiable_after` asserts `self >= finalized_slot`. ethlambda's `slot_is_justifiable_after` instead returned `false` on the underflow, silently accepting blocks that justify a target while the finalization scan would inspect pre-finalization slots. The spec and other clients reject such blocks (zeam errors with InvalidJustifiableSlot), so accepting them risks a consensus split: ethlambda could follow a chain its peers reject.
Surface the missing assert as `Error::SlotBeforeFinalized`, propagated through `is_valid_vote`/`try_finalize` so the block is rejected. Also guard the proposer (`entry_passes_filters`) to drop candidate attestations whose source is below the finalized slot, so we never build a block our own state transition would reject.
Observed on a mixed ethlambda/zeam devnet: block slot 21 packed an attestation {source=2, target=9} that reached supermajority while finalized=8; justifying 9 then scanned slots 3..9 (slot 3 < finalized 8).
Adds regression tests covering the rejection path and the boundary behavior of slot_is_justifiable_after.
🤖 Kimi Code ReviewThis PR fixes a critical consensus bug where Overall Assessment: Correct and necessary fix. Good test coverage. The use of Detailed Feedback1. Critical Fix Validation (state_transition/src/lib.rs)The change from returning
2. Error Handling in Block Building (block_builder.rs)The
Minor suggestion: Consider adding debug_assert!(att_data.source.slot >= projected_finalized_slot);
!slot_is_justifiable_after(s, projected_finalized_slot)
.expect("source slot >= finalized: enforced by entry_passes_filters")3. State Store Safety (store.rs)
4. State Transition Logic (state_transition/src/lib.rs)
5. Test CoverageThe new tests comprehensively cover the bug scenario:
6. Code Style
7. Potential Concern: Partial State MutationIn SummaryApprove. The PR correctly fixes a consensus-split vulnerability by enforcing the spec's invariant that justifiable slot checks must not scan pre-finalization slots. Error handling is proper, comments justify the Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewHere is the code review for PR #402: PR #402 —
|
🤖 Codex Code Review
Otherwise, the stricter I could not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR fixes a consensus-safety bug where ethlambda silently accepted blocks that leanSpec and zeam reject: attestations whose source lies far enough below the finalized slot that the finalization scan (
Confidence Score: 4/5Safe to merge; the fix correctly tightens the STF to match leanSpec/zeam semantics and is covered by well-constructed regression tests. The core logic is sound: crates/blockchain/src/block_builder.rs — the new
|
| Filename | Overview |
|---|---|
| crates/blockchain/state_transition/src/lib.rs | Core fix: slot_is_justifiable_after returns Err on underflow; is_valid_vote and try_finalize propagate it correctly. Two new tests cover the devnet scenario and boundary values. The .expect() in is_valid_vote is safe because target_already_justified rejects target.slot ≤ finalized before the call. |
| crates/blockchain/src/block_builder.rs | Adds source_before_finalized guard to entry_passes_filters; updates .expect() at the target justifiability check. The score_entry .expect() is safe because entry_passes_filters guarantees source.slot ≥ projected_finalized_slot. Minor: the new guard does not carry the is_genesis_self_vote exemption that the other three validity checks share. |
| crates/blockchain/src/store.rs | Only adds .expect() at the fork-choice target walk-back loop; the target_header.slot > finalized_slot loop guard short-circuits before the call, making the unwrap trivially safe. |
Sequence Diagram
sequenceDiagram
participant Peer as Peer Block
participant STF as process_attestations
participant IVV as is_valid_vote
participant TF as try_finalize
participant SIJA as slot_is_justifiable_after
Peer->>STF: "block with attestation {source.slot=2, target.slot=9, finalized=8}"
STF->>IVV: is_valid_vote(state, att_data)
IVV->>IVV: source justified? (slot 2 ≤ finalized 8 → implicitly true)
IVV->>IVV: "target already justified? (slot 9 > 8 → false)"
IVV->>SIJA: "slot_is_justifiable_after(target=9, finalized=8)"
SIJA-->>IVV: "Ok(true) — delta=1 ≤ 5"
IVV-->>STF: Ok(true) — valid vote, count supermajority
STF->>TF: "try_finalize(src=2, tgt=9)"
loop scan slots 3..9
TF->>SIJA: "slot_is_justifiable_after(slot=3, finalized=8)"
SIJA-->>TF: "Err(SlotBeforeFinalized{slot:3, finalized_slot:8})"
end
TF-->>STF: Err(SlotBeforeFinalized)
STF-->>Peer: block REJECTED
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/block_builder.rs:305-313
The `source_before_finalized` guard is placed before `is_genesis_self_vote` is computed, so it applies unconditionally — including to genesis self-votes (source = target = slot 0). When `projected_finalized_slot > 0`, any genesis self-vote in the attestation cache would hit this check and be silently dropped as `"source_before_finalized"`. The three other validity checks below (`target_not_after_source`, `target_already_justified`, `target_not_justifiable`) all carry an `is_genesis_self_vote` exemption. The STF drops genesis self-votes silently anyway, so this doesn't affect block validity, but the inconsistency could be surprising.
```suggestion
let is_genesis_self_vote = is_genesis_self_vote(att_data);
// The source must not be below the finalized slot. Such a vote, once it
// justifies its target, drives the finalization scan over pre-finalization
// slots, which the STF rejects (leanSpec `is_justifiable_after`
// assert). Exclude these candidates so we never build a block our own
// state transition would reject.
if !is_genesis_self_vote && att_data.source.slot < projected_finalized_slot {
return Err("source_before_finalized");
}
if !attestation_data_matches_chain(extended_historical_block_hashes, att_data) {
```
Reviews (1): Last reviewed commit: "docs: improve comments" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // The source must not be below the finalized slot. Such a vote, once it | ||
| // justifies its target, drives the finalization scan over pre-finalization | ||
| // slots, which the STF rejects (leanSpec `is_justifiable_after` | ||
| // assert). Exclude these candidates so we never build a block our own | ||
| // state transition would reject. | ||
| if att_data.source.slot < projected_finalized_slot { | ||
| return Err("source_before_finalized"); | ||
| } | ||
| if !attestation_data_matches_chain(extended_historical_block_hashes, att_data) { |
There was a problem hiding this comment.
source_before_finalized guard is placed before is_genesis_self_vote is computed, so it applies unconditionally — including to genesis self-votes (source = target = slot 0). When projected_finalized_slot > 0, any genesis self-vote in the attestation cache would hit this check and be silently dropped as "source_before_finalized". The three other validity checks below (target_not_after_source, target_already_justified, target_not_justifiable) all carry an is_genesis_self_vote exemption. The STF drops genesis self-votes silently anyway, so this doesn't affect block validity, but the inconsistency could be surprising.
| // The source must not be below the finalized slot. Such a vote, once it | |
| // justifies its target, drives the finalization scan over pre-finalization | |
| // slots, which the STF rejects (leanSpec `is_justifiable_after` | |
| // assert). Exclude these candidates so we never build a block our own | |
| // state transition would reject. | |
| if att_data.source.slot < projected_finalized_slot { | |
| return Err("source_before_finalized"); | |
| } | |
| if !attestation_data_matches_chain(extended_historical_block_hashes, att_data) { | |
| let is_genesis_self_vote = is_genesis_self_vote(att_data); | |
| // The source must not be below the finalized slot. Such a vote, once it | |
| // justifies its target, drives the finalization scan over pre-finalization | |
| // slots, which the STF rejects (leanSpec `is_justifiable_after` | |
| // assert). Exclude these candidates so we never build a block our own | |
| // state transition would reject. | |
| if !is_genesis_self_vote && att_data.source.slot < projected_finalized_slot { | |
| return Err("source_before_finalized"); | |
| } | |
| if !attestation_data_matches_chain(extended_historical_block_hashes, att_data) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/block_builder.rs
Line: 305-313
Comment:
The `source_before_finalized` guard is placed before `is_genesis_self_vote` is computed, so it applies unconditionally — including to genesis self-votes (source = target = slot 0). When `projected_finalized_slot > 0`, any genesis self-vote in the attestation cache would hit this check and be silently dropped as `"source_before_finalized"`. The three other validity checks below (`target_not_after_source`, `target_already_justified`, `target_not_justifiable`) all carry an `is_genesis_self_vote` exemption. The STF drops genesis self-votes silently anyway, so this doesn't affect block validity, but the inconsistency could be surprising.
```suggestion
let is_genesis_self_vote = is_genesis_self_vote(att_data);
// The source must not be below the finalized slot. Such a vote, once it
// justifies its target, drives the finalization scan over pre-finalization
// slots, which the STF rejects (leanSpec `is_justifiable_after`
// assert). Exclude these candidates so we never build a block our own
// state transition would reject.
if !is_genesis_self_vote && att_data.source.slot < projected_finalized_slot {
return Err("source_before_finalized");
}
if !attestation_data_matches_chain(extended_historical_block_hashes, att_data) {
```
How can I resolve this? If you propose a fix, please make it concise.
|
Superseded by #405 |
1 participant
Problem
leanSpec's
Slot.is_justifiable_after(pinnedf12000bd) assertsself >= finalized_slot. ethlambda'sslot_is_justifiable_afterinstead returnedfalseon thechecked_subunderflow — silently lenient.As a result ethlambda silently accepts a block that justifies a target while its finalization scan would inspect slots below the finalized slot. The spec and other clients reject such a block (zeam errors with
InvalidJustifiableSlot). Accepting it is a consensus-split risk: ethlambda could follow a chain its peers reject.This was observed on a mixed ethlambda/zeam devnet, where it stalled cross-client finality:
{source.slot=2, target.slot=9}(a valid, not-yet-justified, justifiable target);finalized=8, justifying target 9;3..9, hitting slot 3< finalized 8.ethlambda swallowed it; zeam (and the spec) rejected the block, so zeam halted at slot ~17 and finality froze.
Change
slot_is_justifiable_afternow returnsResult<bool, Error>and yieldsError::SlotBeforeFinalizedwhenslot < finalized_slot— the missing assert. It propagates throughis_valid_vote→process_attestationsandtry_finalize, so the offending block is rejected (matching the spec and zeam).entry_passes_filtersnow drops candidate attestations whosesource.slot < finalized_slot, so ethlambda never builds a block its own STF would reject.>= finalized(fork-choice target walk-back instore.rs, the target check in the builder) use.expect()with a documented invariant.Tests
block_rejected_when_source_below_finalized— reproduces the block-21 scenario; assertsprocess_attestationsreturnsErr(SlotBeforeFinalized).slot_is_justifiable_after_errors_below_finalized— error onslot < finalized; boundary/window/square/pronic cases otherwise.state_transitionlib + spec suite (51) andblockchainlib (21) pass; clippy-D warningsclean. The 24forkchoice_spectestsfailures are pre-existing fixture drift (missing field proofData) onmain, unrelated to this change.