fix(blockchain): reject blocks too far in future before pending storage

[MegaRedHand]

Summary

Adds a parent-independent future-slot check in BlockChainServer::process_or_pend_block so blocks whose start interval is more than GOSSIP_DISPARITY_INTERVALS ahead of local time are dropped before the parent-state lookup.

Mirrors the bound used by validate_attestation_data (leanSpec PR #682): an interval-based margin (not a whole-slot one), since a slot-wide tolerance would let an adversary pre-publish next-slot blocks ahead of any honest proposer.

Motivation

Today, the only slot-based gate on incoming blocks is slot <= finalized_slot. There is no upper bound, so a peer can flood blocks at arbitrary future slots and force us to:

• Persist them to RocksDB via insert_pending_block (since the fabricated parent state will be missing).
• Issue BlocksByRoot requests for the fabricated parent roots, fanning out to peers.

The new check rejects these at the orchestration layer (BlockChainServer) before any disk write or network request. on_block_core is intentionally untouched: spec tests drive store::on_block_* directly with arbitrary on_tick advancements, and we don't want a wall-clock gate at that layer.

Behavior

process_or_pend_block:
  slot ≤ finalized                                  → discard subtree, drop  (existing)
  block_start_interval > store.time() + DISPARITY   → discard subtree, drop  (NEW)
  parent state missing                              → persist as pending     (existing)
  otherwise                                         → process_block          (existing)

block_start_interval = slot * INTERVALS_PER_SLOT is the interval at which the block's slot begins. Tolerance is GOSSIP_DISPARITY_INTERVALS = 1 (~800 ms), matching the attestation bound.

Test plan

• make fmt clean
• make lint clean (cargo clippy --workspace --all-targets -- -D warnings)
• No regressions in cargo test --workspace --release from this change (the 8 fixture failures in forkchoice_spectests pre-exist on main after fix: tighten attestation future-slot bound (leanSpec #682) #317 and are unrelated).
• Devnet smoke: bring up local devnet, confirm normal block flow is unaffected and a manually crafted future-slot block is rejected with the expected warn!.

[github-actions]

🤖 Kimi Code Review

The change adds a critical DoS protection against future-block spam and aligns block validation with the existing attestation time checks. The logic is sound and the implementation is memory-safe.

Minor suggestion:

Line 430: Consider using saturating_add for consistency with the saturating_mul on line 428, though overflow is practically impossible given store_time represents current wall time:

if block_start_interval > store_time.saturating_add(GOSSIP_DISPARITY_INTERVALS) {

Security/Logic verification:

• Overflow handling: The use of saturating_mul correctly prevents overflow on maliciously large slot numbers (Item 1). If slot is u64::MAX, the saturating multiplication yields u64::MAX, which will correctly trigger the future-block rejection.
• Disparity calculation: The comparison correctly uses intervals (not slots) on both sides, matching the comment's rationale about preventing adversaries from pre-publishing next-slot blocks (Item 2).
• Cleanup: Calling discard_pending_subtree before returning prevents memory leaks from abandoned pending block entries (Item 3).

The change looks correct and ready for merge.

---

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

[github-actions]

🤖 Claude Code Review

Now I have all the context needed. Let me write the review.

---

PR Review: fix(blockchain): reject blocks too far in future before pending storage

Summary

This is a targeted, purely-additive change (21 lines, one file) that adds an early-exit guard in process_or_pend_block to reject future-slot blocks before any disk write or network fan-out. The security motivation is clear and the implementation is correct overall. A few points worth noting:

---

Correctness

Check arithmetic is sound. block_start_interval = slot * INTERVALS_PER_SLOT (interval-space conversion) compared against store.time() + GOSSIP_DISPARITY_INTERVALS correctly mirrors the existing validate_attestation_data bound. The interval-based (not slot-based) tolerance is the right choice, as the PR description explains: a full-slot margin would allow an adversary to pre-publish next-slot blocks 4 seconds ahead.

Placement is optimal. The check is inserted after the finalized-slot gate (cheap map lookup) but before the parent-state lookup (which triggers insert_pending_block + BlocksByRoot fan-out on miss). This is the correct order for defense-in-depth.

saturating_mul on the left, plain + on the right. slot.saturating_mul(INTERVALS_PER_SLOT) correctly handles a pathologically large slot value. The right-hand side store_time + GOSSIP_DISPARITY_INTERVALS uses plain addition. Since GOSSIP_DISPARITY_INTERVALS = 1, this could technically wrap on a u64::MAX store time, but this same pattern exists in validate_attestation_data (store.rs:186–187) and the scenario is practically impossible. Worth noting for consistency:

// Existing pattern in store.rs (same asymmetry):
if attestation_start_interval > store.time() + GOSSIP_DISPARITY_INTERVALS {

Not a blocker, but if you ever tighten the right side you'd want store_time.saturating_add(GOSSIP_DISPARITY_INTERVALS) to match the left side's intent.

---

Minor Issues

discard_pending_subtree is always a no-op at this call site. At the point this new guard fires, the block has never reached insert_pending_block, so its root is guaranteed absent from self.pending_blocks. discard_pending_subtree will immediately hit the let Some(...) else { return; } early exit. It's harmless, and it mirrors the call pattern in the finalized-slot guard above (where it can be meaningful, because finalization may advance after a block was stored as pending). Consider whether leaving the call is intentionally defensive (future-proofing if the call order ever changes) or silently misleading — a brief inline note like // no-op if block was never stored as pending, but kept for consistency would clarify intent.

Log field ordering. store_time appears between %slot and proposer, which mixes temporal/identity fields with a local-clock field. Per the project's field ordering convention (temporal → identity → identifiers → context → metadata), store_time is contextual/metadata and would sit more naturally after block_root/parent_root. Not a correctness issue.

---

Comments in the new block

The multi-line comment block (lines 424–428) explains the security reasoning clearly. It correctly belongs here because the interval-vs-slot distinction is a non-obvious invariant. The comment could be trimmed slightly (the last sentence about RocksDB/fan-out is already in the commit message and PR description), but this is a style preference, not a defect.

---

No Issues Found In

• The condition logic and edge cases (slot = 0, current slot, next slot during interval 4 vs intervals 0–3).
• Logging fields and ShortRoot usage (matches project conventions).
• The decision to leave on_block_core untouched (correct: spec tests bypass wall-clock checks intentionally).
• The constant definitions and imports (GOSSIP_DISPARITY_INTERVALS, INTERVALS_PER_SLOT).

---

Verdict

The fix is correct and addresses a real DoS vector. The one substantive point worth a second look is whether the discard_pending_subtree call belongs here at all (it's a no-op), and whether that should be documented or dropped. Everything else is in good shape.

---

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

[github-actions]

🤖 Codex Code Review

• High: The new future-slot rejection can strand already-received descendants. If child C arrives before parent B, the existing code persists C and indexes it under pending_blocks[B] at crates/blockchain/src/lib.rs:445 and crates/blockchain/src/lib.rs:458. When B then arrives slightly early, this PR hits the new check at crates/blockchain/src/lib.rs:429 and calls discard_pending_subtree at crates/blockchain/src/lib.rs:440, which removes the only in-memory path back to C (crates/blockchain/src/lib.rs:571). Later, if B is re-received and imported, collect_pending_children only consults that in-memory map (crates/blockchain/src/lib.rs:539), so C is never replayed even though its bytes are still in RocksDB. That is a fork-choice/liveness regression. I would not discard the subtree for “too early”; keep it queued until the slot starts, or at minimum preserve descendants.

• Medium: The check is keyed off self.store.time() (crates/blockchain/src/lib.rs:430), but that clock only advances when the actor processes Tick (crates/blockchain/src/lib.rs:129), and NewBlock shares the same mailbox (crates/blockchain/src/lib.rs:663). So a valid block that arrives just after slot start can still be judged against stale time if the tick is delayed/backlogged. Because blocks are unique, return at crates/blockchain/src/lib.rs:441 is much riskier than the analogous attestation drop. I’d either compute this bound from wall-clock time here, or retain/retry future blocks instead of hard-dropping them.

Aside from those points, the intent of the change is reasonable: rejecting fabricated future blocks before DB writes/fetch fan-out is a good defensive goal.

I couldn’t run cargo test -p ethlambda-blockchain --tests in this environment because Cargo couldn’t create its git/cache directories under the read-only home path, and network access is restricted.

---

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

[greptile-apps]

Greptile Summary

This PR adds an interval-based future-slot guard in BlockChainServer::process_or_pend_block that rejects incoming blocks whose slot start interval exceeds store.time() + GOSSIP_DISPARITY_INTERVALS (1 interval ≈ 800 ms). The check is placed after the finalized-slot discard and before the pending-block persist path, so it stops adversarially crafted far-future blocks from being written to RocksDB or triggering BlocksByRoot fan-out. The implementation directly mirrors the existing validate_attestation_data logic.

Confidence Score: 4/5

Safe to merge; the guard is logically correct, consistently mirrors the pre-existing attestation check, and touches only the orchestration layer with no spec-test impact.

No P0 or P1 issues found. The single change is a focused 21-line guard that reuses an established pattern (same interval arithmetic and same constants used in validate_attestation_data). The devnet smoke test is still marked pending in the checklist, which is a minor gap, keeping the score at 4 rather than 5.

No files require special attention.

Important Files Changed

Filename	Overview
crates/blockchain/src/lib.rs	Adds a 21-line future-slot guard before the pending-block persist path; logic is correct and mirrors the pre-existing validate_attestation_data pattern. Minor: store_time + GOSSIP_DISPARITY_INTERVALS is a plain (non-saturating) add, consistent with the existing attestation check.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[process_or_pend_block] --> B{slot ≤ finalized_slot?}
    B -- yes --> C[discard_pending_subtree\nreturn]
    B -- no --> D{block_start_interval >\nstore_time + GOSSIP_DISPARITY?\nNEW}
    D -- yes --> E[warn! log\ndiscard_pending_subtree\nreturn]
    D -- no --> F{parent state\nmissing?}
    F -- yes --> G[insert_pending_block\nto RocksDB\ntrack in-memory]
    F -- no --> H[process_block\nfork-choice update]

Loading

_{Reviews (1): Last reviewed commit: "fix(blockchain): reject blocks too far i..." | Re-trigger Greptile}