fix(p2p): remove legacy step field from BlocksByRange request

[MegaRedHand]

Summary

• Drop the legacy phase-0 step: u64 field from BlocksByRangeRequest. The leanSpec container has only (start_slot, count); step was deprecated in Altair and explicitly omitted from the lean spec.
• Without this fix, SSZ decode requires 24 bytes while peers (and the ethereum/hive lean simulator) send the spec-compliant 16-byte encoding, so every inbound BlocksByRange request fails to decode before reaching the handler.
• Remove the now-dead step == 0 validation and simplify canonical_blocks_by_range to walk contiguous slots; update the existing unit test to the new signature.

Test plan

• cargo test -p ethlambda-p2p
• Verified against hive's reqresp/blocks_by_range suite: all 4 tests pass (previously hung at fixture timeout because the SSZ decode error prevented any response).

[greptile-apps]

Greptile Summary

This PR removes the deprecated step: u64 field from BlocksByRangeRequest, bringing the SSZ encoding down from 24 bytes to the spec-compliant 16 bytes and fixing a decode failure that caused every inbound BlocksByRange request to be silently rejected.

• messages.rs: Drops step from the SSZ struct; the derived SszDecode/SszEncode now correctly expects exactly 16 bytes (two u64 fields), matching what peers and the ethereum/hive lean simulator send.
• handlers.rs: Removes the step parameter from canonical_blocks_by_range, eliminates the dead step == 0 guard, replaces index-based slot arithmetic with a direct start_slot..=end_slot range, and updates the unit test to the new two-argument signature. Overflow safety is preserved through checked arithmetic.

Confidence Score: 5/5

Safe to merge — the change is a targeted spec-alignment fix with correct arithmetic and no regressions in logic.

The removal of step is mechanical: the struct shrinks, the function drops one parameter, and the slot iteration becomes a simple range. Overflow is still guarded by checked arithmetic. Missed slots are still handled gracefully by filter_map. The unit test covers the happy path and was updated correctly. No other call sites exist for canonical_blocks_by_range.

No files require special attention.

Important Files Changed

Filename	Overview
crates/net/p2p/src/req_resp/messages.rs	Removes the legacy step: u64 field from BlocksByRangeRequest, shrinking the SSZ-encoded size from 24 bytes to 16 bytes to match the lean spec.
crates/net/p2p/src/req_resp/handlers.rs	Removes the step parameter from canonical_blocks_by_range, drops the dead step == 0 validation, and replaces the index-based iteration with a direct slot range; logic and edge cases (checked arithmetic, missed slots) all look correct.

Sequence Diagram

sequenceDiagram
    participant Peer
    participant P2PServer
    participant Handler as handle_blocks_by_range_request
    participant Store

    Peer->>P2PServer: BlocksByRangeRequest (16 bytes: start_slot, count)
    P2PServer->>Handler: dispatch request
    Handler->>Handler: "validate count (0 or > MAX_REQUEST_BLOCKS → INVALID_REQUEST)"
    Handler->>Store: canonical_blocks_by_range(start_slot, count)
    Store-->>Handler: walk chain head → start_slot, collect roots_by_slot
    Handler->>Handler: "iterate start_slot..=end_slot, filter_map to blocks"
    Handler-->>Peer: Response::success(blocks)

Loading

_{Reviews (1): Last reviewed commit: "fix(p2p): remove legacy step field from ..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

Here is my review of PR #365:

---

PR #365 — fix(p2p): remove legacy step field from BlocksByRange request

Overview

This PR removes the deprecated step: u64 field from BlocksByRangeRequest. The field was introduced in Phase 0 of the standard Ethereum spec, deprecated in Altair, and explicitly absent from the lean spec. Its presence caused every inbound BlocksByRange request to fail SSZ decoding — the struct expected 24 bytes but spec-compliant peers (including the hive simulator) send exactly 16 bytes.

---

Correctness

SSZ size fix is correct. BlocksByRangeRequest now serializes as two u64 fields (16 bytes), matching the lean spec container of (start_slot, count).

end_slot arithmetic is correct. The simplification:

count.checked_sub(1).and_then(|last_offset| start_slot.checked_add(last_offset))

gives end_slot = start_slot + count − 1, which is identical to the old formula with step = 1 implicit. Overflow is guarded by checked_sub/checked_add, and the None arm returns an empty vec.

canonical_blocks_by_range logic is correct. The function walks the canonical chain backward from head, collects headers in [start_slot, end_slot], then iterates start_slot..=end_slot and fetches blocks — naturally skipping missed/empty slots via filter_map. This matches the spec's expectation that responses omit missing slots.

No remaining step references. A grep confirms the field was fully removed across messages, handlers, codec, and the unit test.

---

Minor Observations

• Redundant early-exit at handlers.rs:186. canonical_blocks_by_range has its own if count == 0 { return Vec::new(); } guard, but the caller at line 162 already rejects count == 0 requests before reaching this function. The check is dead code within the current call path. It's harmless as defensive programming for a private function, but worth noting.

• Variable name last_offset at line 192. This name made more sense when it represented (count − 1) * step; now it directly holds count − 1, which is also the last offset only because step is implicitly 1. Renaming to something like count_minus_one or last_index would be clearer, though the intent is still legible.

• Outbound symmetry. The codec (codec.rs:111) encodes outbound BlocksByRange requests via request.to_ssz(). With the struct change, outbound requests will also correctly be 16 bytes — both the read and write paths are fixed together.

---

Assessment

The fix is correct, necessary, and minimal. It resolves a real interoperability bug at the SSZ wire level, the arithmetic is safe, and the hive reqresp/blocks_by_range suite passing all 4 tests is solid external validation. The two minor observations above (redundant guard, variable name) are not blocking.

Approved.

---

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

[github-actions]

🤖 Kimi Code Review

Overall Assessment: This PR removes the deprecated step parameter from BlocksByRange requests, simplifying the implementation and eliminating arithmetic overflow risks. The logic changes are correct, but critical protocol compatibility concerns must be addressed.

Critical Issues

1. Protocol Breaking Change (Fork Versioning Required)

File: crates/net/p2p/src/req_resp/messages.rs (line 136-139)

Removing the step field changes the SSZ serialization schema. This is a breaking wire protocol change that will cause deserialization failures when communicating with peers using the old format (3 uint64s vs 2 uint64s).

Action Required: Gate this change behind a fork version check. The handler must support both schemas based on the negotiated protocol version or fork epoch. If this targets a specific hard fork (e.g., Capella/Deneb), the code must use the old schema for prior forks and the new schema for the target fork and later.

// Suggested pattern:
pub enum BlocksByRangeRequest {
    PreFork { start_slot: u64, count: u64, step: u64 },
    PostFork { start_slot: u64, count: u64 },
}

Code Quality & Safety

2. Arithmetic Safety Improvement (Positive)

File: crates/net/p2p/src/req_resp/handlers.rs (lines 200-203)

Removing checked_mul(step) eliminates a potential overflow vector. The new calculation:

let Some(end_slot) = count
    .checked_sub(1)
    .and_then(|last_offset| start_slot.checked_add(last_offset))

is correct for computing start_slot + count - 1 with proper underflow/overflow checks.

3. Performance: HashMap Capacity

File: crates/net/p2p/src/req_resp/handlers.rs (line 194)

roots_by_slot is bounded by count (which is capped at MAX_REQUEST_BLOCKS). Pre-allocate to avoid reallocations:

let mut roots_by_slot = HashMap::with_capacity(count as usize);

4. Logic Correctness

File: crates/net/p2p/src/req_resp/handlers.rs (lines 219-224)

The iteration change from stepped indices to contiguous range (start_slot..=end_slot) is correct and handles skipped slots properly (via filter_map lookup). This is equivalent to step=1 semantics.

Security Considerations

1. DoS Mitigation: Removing step prevents potential DoS attacks where a peer requests a sparse range with large step values, forcing expensive iteration through the canonical chain cache.

2. Request Validation: The removal of request.step == 0 validation (line 162) is safe since the field no longer exists, but ensure zero-check validation is preserved for start_slot and count (which remains).

Testing

File: crates/net/p2p/src/req_resp/handlers.rs (line 463)

The test update correctly removes the step parameter. Ensure tests cover:

• Empty ranges (count = 0)
• Ranges with skipped slots (to verify filter_map behavior)
• Overflow edge cases (start_slot + count - 1 > u64::MAX)

Summary

• Do not merge until fork-gating is implemented for the SSZ schema change.
• Consider pre-allocating the HashMap with with_capacity.
• Otherwise, the simplification is correct and improves maintainability.

---

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

[github-actions]

🤖 Codex Code Review

1. crates/net/p2p/src/req_resp/messages.rs, codec.rs, messages.rs: removing step changes the SSZ wire container for BlocksByRangeRequest from three u64s to two, but the protocol ID remains /leanconsensus/req/blocks_by_range/1/ssz_snappy. Because this request is encoded/decoded directly from the struct, old and new nodes will speak different wire formats on the same /1/ protocol. That is a breaking network change and will break rolling upgrades / mixed-version peers. This needs either backward-compatible decoding or a new protocol version.

2. crates/net/p2p/src/req_resp/handlers.rs, handlers.rs: the handler no longer implements stepped range semantics. end_slot is now computed as start_slot + count - 1, and the response iterates contiguous slots only. Any peer expecting start/count/step behavior will now either fail to interoperate or receive the wrong response shape, which is a correctness regression for range sync/backfill and can over-send blocks compared with the original request intent.

No other major issues stood out in the touched code. I couldn’t run cargo test in this environment because the sandbox blocks writes to the default Cargo/Rustup directories needed for dependency/toolchain setup.

---

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