Skip to content

Use original finalized slot for justifiability and finalization gap checks #212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pablodeymo wants to merge 1 commit into from
Closed

Use original finalized slot for justifiability and finalization gap checks #212

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 27 additions & 5 deletions crates/blockchain/state_transition/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,11 @@ fn process_attestations(
let _timing = metrics::time_attestations_processing();
let validator_count = state.validators.len();
let mut attestations_processed: u64 = 0;
// Capture the original finalized slot before attestation processing.
// The spec uses self.latest_finalized.slot (immutable) for justifiability
// checks (Rule 6) and finalization gap checks, while using a local mutable
// finalized_slot for is_slot_justified (Rules 1-2) and window shifts.
let original_finalized_slot = state.latest_finalized.slot;
Comment on lines +215 to +219
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directly conflicts with the recently-merged commit #203 (9c7f5f6)

This PR re-introduces original_finalized_slot and uses it for Rule 6 and the finalization gap check — but commit 9c7f5f6 (merged yesterday, March 12) explicitly removed this snapshot to align with leanSpec PR #443. That commit's message states:

"The above PR in leanSpec removed the use of the original_finalized_slot snapshot and changed it to using the updated latest_finalized.slot from state."

The two PRs are directly contradictory about what the canonical spec requires:

Commit #203 / leanSpec PR #443 This PR #212
Use live state.latest_finalized.slot for Rule 6 & gap check Use frozen original_finalized_slot for Rule 6 & gap check

If leanSpec PR #443 is still merged and canonical, then this PR reintroduces the divergence that #203 was rushed in (pre-devnet) to fix, and will cause state root mismatches against other clients that follow that spec update.

Before merging, the PR needs to explicitly address one of:

  1. leanSpec PR #443 has been reverted or superseded — link to the current leanSpec commit that confirms the original-snapshot semantics.
  2. The spec is being updated back to original-snapshot semantics — the PR should reference the leanSpec PR that makes this change.
  3. Other client implementations have been audited and agree with this reading of the spec.

Without resolving this, merging risks a consensus split in the opposite direction from the one described in the PR description.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/blockchain/state_transition/src/lib.rs
Line: 215-219

Comment:
**Directly conflicts with the recently-merged commit #203 (`9c7f5f6`)**

This PR re-introduces `original_finalized_slot` and uses it for Rule 6 and the finalization gap check — but commit `9c7f5f6` (merged yesterday, March 12) *explicitly removed this snapshot* to align with [leanSpec PR #443](https://github.com/leanEthereum/leanSpec/pull/443). That commit's message states:

> "The above PR in leanSpec removed the use of the `original_finalized_slot` snapshot and changed it to using the updated `latest_finalized.slot` from state."

The two PRs are directly contradictory about what the canonical spec requires:

| Commit #203 / leanSpec PR #443 | This PR #212 |
|---|---|
| Use live `state.latest_finalized.slot` for Rule 6 & gap check | Use frozen `original_finalized_slot` for Rule 6 & gap check |

If leanSpec PR #443 is still merged and canonical, then this PR reintroduces the divergence that #203 was rushed in (pre-devnet) to fix, and will cause state root mismatches against other clients that follow that spec update.

Before merging, the PR needs to explicitly address one of:
1. leanSpec PR #443 has been **reverted** or superseded — link to the current leanSpec commit that confirms the original-snapshot semantics.
2. The spec is being updated back to original-snapshot semantics — the PR should reference the leanSpec PR that makes this change.
3. Other client implementations have been audited and agree with this reading of the spec.

Without resolving this, merging risks a consensus split in the opposite direction from the one described in the PR description.

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

let mut justifications: HashMap<H256, Vec<bool>> = state
.justifications_roots
.iter()
Expand Down Expand Up @@ -246,7 +251,7 @@ fn process_attestations(
let source = attestation_data.source;
let target = attestation_data.target;

if !is_valid_vote(state, source, target) {
if !is_valid_vote(state, source, target, original_finalized_slot) {
continue;
}

Expand Down Expand Up @@ -288,7 +293,14 @@ fn process_attestations(

justifications.remove(&target.root);

try_finalize(state, source, target, &mut justifications, &root_to_slot);
try_finalize(
state,
source,
target,
original_finalized_slot,
&mut justifications,
&root_to_slot,
);
}
}

Expand All @@ -306,7 +318,12 @@ fn process_attestations(
/// 4. Both checkpoints exist in historical_block_hashes
/// 5. Target slot > source slot
/// 6. Target slot is justifiable after the finalized slot
fn is_valid_vote(state: &State, source: Checkpoint, target: Checkpoint) -> bool {
fn is_valid_vote(
state: &State,
source: Checkpoint,
target: Checkpoint,
original_finalized_slot: u64,
) -> bool {
// Check that the source is already justified
if !justified_slots_ops::is_slot_justified(
&state.justified_slots,
Expand Down Expand Up @@ -342,7 +359,9 @@ fn is_valid_vote(state: &State, source: Checkpoint, target: Checkpoint) -> bool
}

// Ensure the target falls on a slot that can be justified after the finalized one.
if !slot_is_justifiable_after(target.slot, state.latest_finalized.slot) {
// Uses the original finalized slot from before attestation processing, matching
// the spec's use of self.latest_finalized.slot (immutable).
if !slot_is_justifiable_after(target.slot, original_finalized_slot) {
return false;
}

Expand All @@ -358,12 +377,15 @@ fn try_finalize(
state: &mut State,
source: Checkpoint,
target: Checkpoint,
original_finalized_slot: u64,
justifications: &mut HashMap<H256, Vec<bool>>,
root_to_slot: &HashMap<H256, u64>,
) {
// Consider whether finalization can advance.
// Uses the original finalized slot from before attestation processing, matching
// the spec's use of self.latest_finalized.slot (immutable).
if ((source.slot + 1)..target.slot)
.any(|slot| slot_is_justifiable_after(slot, state.latest_finalized.slot))
.any(|slot| slot_is_justifiable_after(slot, original_finalized_slot))
{
metrics::inc_finalizations("error");
return;
Comment on lines 387 to 391
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gap check uses frozen slot across multiple try_finalize calls

try_finalize can be invoked more than once within a single process_attestations call (once per attestation that crosses the supermajority threshold). After the first successful call, state.latest_finalized.slot is advanced, but subsequent calls still use the same original_finalized_slot.

Consider a block with three attestation groups:

  • Group 1 justifies slot 5 → try_finalize is called, state.latest_finalized.slot advances from 0 → 5.
  • Group 2 justifies slot 12 → try_finalize is called with original_finalized_slot = 0. The gap (5+1)..12 = [6,7,8,9,10,11] is checked against original_finalized_slot=0. delta=6 is pronic → justifiable → finalization blocked.
  • If instead we used the updated state.latest_finalized.slot=5, delta for slot 6 would be 1 ≤ 5 → justifiable → same result, but for slot 7 delta=2 (pronic: no; square: no; ≤5: no… wait 2 ≤ 5 → yes). Different justifiability windows could yield different finalization decisions.

The PR description carefully explains the two-reference distinction in the Python spec, but it's worth explicitly verifying that the Python self.latest_finalized is never mutated mid-loop (i.e., the spec writes the final result back to self.latest_finalized only after the full loop completes, keeping self.latest_finalized.slot constant at L516 and L570). If Python does update self.latest_finalized within the loop, then subsequent iterations would see the new slot — the same behavior that state.latest_finalized.slot exhibits in the Rust code — and original_finalized_slot would be wrong here too.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/blockchain/state_transition/src/lib.rs
Line: 387-391

Comment:
**Gap check uses frozen slot across multiple `try_finalize` calls**

`try_finalize` can be invoked more than once within a single `process_attestations` call (once per attestation that crosses the supermajority threshold). After the first successful call, `state.latest_finalized.slot` is advanced, but subsequent calls still use the same `original_finalized_slot`.

Consider a block with three attestation groups:
- Group 1 justifies slot 5 → `try_finalize` is called, `state.latest_finalized.slot` advances from 0 → 5.
- Group 2 justifies slot 12 → `try_finalize` is called with `original_finalized_slot = 0`. The gap `(5+1)..12 = [6,7,8,9,10,11]` is checked against `original_finalized_slot=0`. `delta=6` is pronic → justifiable → finalization blocked.
- If instead we used the *updated* `state.latest_finalized.slot=5`, delta for slot 6 would be 1 ≤ 5 → justifiable → same result, but for slot 7 delta=2 (pronic: no; square: no; ≤5: no… wait 2 ≤ 5 → yes). Different justifiability windows could yield different finalization decisions.

The PR description carefully explains the two-reference distinction in the Python spec, but it's worth explicitly verifying that the Python `self.latest_finalized` is **never mutated** mid-loop (i.e., the spec writes the final result back to `self.latest_finalized` only after the full loop completes, keeping `self.latest_finalized.slot` constant at L516 and L570). If Python does update `self.latest_finalized` within the loop, then subsequent iterations would see the new slot — the same behavior that `state.latest_finalized.slot` exhibits in the Rust code — and `original_finalized_slot` would be wrong here too.

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

Expand Down
Loading