fix(fork-choice): bound a block's slot before the state transition (leanSpec #1182)#501
Conversation
Port leanSpec #1182. on_block had no upper bound on a block's slot, so one validly-signed block on a reachable parent could drive the empty-slot walk over an unbounded range. Add two guards at the untrusted-input boundary, before the (expensive) signature verification so a crafted block is rejected cheaply: - parent-gap cap: reject block.slot - parent.slot > HISTORICAL_ROOTS_LIMIT, bounding the walk directly and independent of the clock. - clock horizon: reject block.slot > current_slot + 1, keeping far-future blocks out of the store (whole-slot margin so an intended early block still imports). ethlambda's process_slots already jumps straight to the target slot (no per-slot loop) and process_block_header already caps the historical-roots allocation, so this is defense-in-depth plus the cheap early rejection the spec places here.
🤖 Kimi Code ReviewOverall this is a well-structured defense-in-depth PR that correctly bounds DoS vectors before expensive operations. The logic is sound and the tests are comprehensive. Major concern: Constant selection
Minor issues and suggestions
Security assessment
Verdict: Approve with the caveat that the Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewFindings
No other consensus-specific regressions stood out in this diff. Testing I could not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewNow I have enough context to write the review. Review: fix(fork-choice): bound a block's slot before the state transition (PR 501)SummarySolid, well-scoped defensive change. It adds two cheap, clock/parent-relative guards in Correctness
Risk / follow-up (already flagged by the author, worth reiterating as a blocking item)
Minor / non-blocking
VerdictLogic and placement are correct and match the stated threat model; the one real gate before merge is running Automated review by Claude (Anthropic) · sonnet · custom prompt |
Greptile SummaryPorts leanSpec #1182 by adding two untrusted-input boundary checks in
Confidence Score: 4/5The new guards are correctly placed and logically sound; the only concern is a test that would silently pass even if the gap guard were removed. Both checks are correctly placed after parent-state retrieval and before the expensive signature path. The logic is consistent with the existing attestation horizon pattern. The gap-check test implicitly depends on the ordering of the two new guards — at store.time() = 0, the future-horizon check fires for the same block, so the test only catches BlockSlotGapTooLarge because the gap check runs first. If the guards were swapped, the test would still pass but no longer verify the gap guard specifically. The test section at the bottom of store.rs (around line 1574) deserves a closer look to confirm the gap test is independently exercising the gap guard.
|
| Filename | Overview |
|---|---|
| crates/blockchain/src/store.rs | Adds two early-rejection guards in on_block_core (BlockSlotGapTooLarge and BlockTooFarInFuture) before signature verification, plus two corresponding error variants and tests. Guard logic is correct; one test has an implicit dependency on check ordering that could hide a regression if the guards are reordered. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[on_block_core called] --> B{Duplicate block?}
B -- Yes --> C[return Ok early]
B -- No --> D{Parent state available?}
D -- No --> E[Err: MissingParentState]
D -- Yes --> F{slot_gap > HISTORICAL_ROOTS_LIMIT?}
F -- Yes --> G[Err: BlockSlotGapTooLarge]
F -- No --> H{slot > current_slot + 1?}
H -- Yes --> I[Err: BlockTooFarInFuture]
H -- No --> J{Attestation dedup check}
J -- Fail --> K[Err: DuplicateAttestationData / TooManyAttestationData]
J -- Pass --> L{verify=true?}
L -- Yes --> M[verify_block_signatures]
M -- Fail --> N[Err: signature error]
M -- Pass --> O[state_transition]
L -- No --> O
O --> P[Store state & update checkpoints]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[on_block_core called] --> B{Duplicate block?}
B -- Yes --> C[return Ok early]
B -- No --> D{Parent state available?}
D -- No --> E[Err: MissingParentState]
D -- Yes --> F{slot_gap > HISTORICAL_ROOTS_LIMIT?}
F -- Yes --> G[Err: BlockSlotGapTooLarge]
F -- No --> H{slot > current_slot + 1?}
H -- Yes --> I[Err: BlockTooFarInFuture]
H -- No --> J{Attestation dedup check}
J -- Fail --> K[Err: DuplicateAttestationData / TooManyAttestationData]
J -- Pass --> L{verify=true?}
L -- Yes --> M[verify_block_signatures]
M -- Fail --> N[Err: signature error]
M -- Pass --> O[state_transition]
L -- No --> O
O --> P[Store state & update checkpoints]
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/store.rs:1574-1612
**Gap test implicitly relies on check ordering**
`on_block_rejects_block_slot_gap_too_large` sets `store.time()` to `0`, so `current_slot = 0` and the horizon is slot `1`. Because `gap_slot = HISTORICAL_ROOTS_LIMIT + 1` is also beyond the horizon, *both* guards would trigger; the test receives `BlockSlotGapTooLarge` only because the gap check happens to run first in the code. If the two checks are ever reordered, the test breaks with `BlockTooFarInFuture` instead, masking the gap guard's absence. Advancing the store clock to at least `gap_slot * INTERVALS_PER_SLOT` before the call (so `current_slot >= gap_slot`, making the horizon check a no-op) would make the test solely exercise the gap guard.
Reviews (1): Last reviewed commit: "fix(fork-choice): bound a block's slot b..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…g store accessors The main merge brought in the fork-choice slot-bound guards (lambdaclass#501) that call store.time() and store.head() in their pre-Result form, but this branch changed those accessors to return Result<T, Error>. Unwrap them consistently with the surrounding code so the crate compiles.
2 participants
What
Ports leanSpec #1182: bound a block's slot in
on_blockbefore the state transition runs.Why
on_blockhad a lower slot bound but no upper bound. The transition advances the state one slot at a time from the parent up toblock.slot, and the proposer of a slot isslot % num_validators, so one key is a valid proposer for infinitely many slots. A single validly-signed block on a reachable parent (e.g. genesis) could therefore drive an unbounded empty-slot walk.ethlambda is not vulnerable to the unbounded loop (
process_slotsjumps straight to the target slot rather than iterating) andprocess_block_headeralready caps thehistorical_block_hashesallocation viaSlotGapTooLarge. This PR adds the spec's two guards at the untrusted-input boundary so a crafted block is rejected cheaply, before signature verification.Changes
crates/blockchain/src/store.rs, inon_block_corebefore signature verification:block.slot - parent.slot > HISTORICAL_ROOTS_LIMIT(BlockSlotGapTooLarge). Clock-independent; bounds the walk directly.block.slot > current_slot + 1(BlockTooFarInFuture). Whole-slot margin, so an intended early block still imports (mirrors the attestation future-slot guard, but with a whole-slot rather than one-interval margin).The existing STF-level
SlotGapTooLargeinprocess_block_headeris kept as defense-in-depth.Tests
on_block_rejects_block_too_far_in_futureon_block_rejects_block_slot_gap_too_largecargo fmt,clippy -D warnings, and the blockchain lib tests pass.