feat(p2p): add inbound BlocksByRange req/resp support#348
feat(p2p): add inbound BlocksByRange req/resp support#348dicethedev wants to merge 2 commits intolambdaclass:mainfrom
Conversation
Greptile SummaryThis PR implements inbound
Confidence Score: 3/5The serving path is mostly correct but will send a successful empty response to a peer that sends count=0, rather than rejecting the request — a one-line fix is needed before merging. The chain-walking logic, codec integration, and protocol registration are solid. The one gap is the missing count==0 guard: a peer can elicit a success response with an empty block list where the protocol expects an error, which is a real behavioural defect on the changed request-handling path. handlers.rs — specifically the validation block in handle_blocks_by_range_request and the stub response handler for inbound BlocksByRange responses.
|
| Filename | Overview |
|---|---|
| crates/net/p2p/src/req_resp/handlers.rs | Adds inbound BlocksByRange handler with chain-walking logic and a unit test; missing count==0 validation and the inbound response path is a no-op stub. |
| crates/net/p2p/src/req_resp/messages.rs | Adds BlocksByRangeRequest struct, MAX_REQUEST_BLOCKS constant, protocol ID, and BlocksByRange response payload variant — straightforward and correct. |
| crates/net/p2p/src/req_resp/codec.rs | Correctly refactors block-response decoding into a shared helper and adds BlocksByRange codec paths for both read and write directions. |
| crates/net/p2p/src/req_resp/mod.rs | Re-exports new BlocksByRange symbols; no logic changes. |
| crates/net/p2p/src/lib.rs | Registers the BlocksByRange protocol in the libp2p request-response behaviour with Full support; straightforward and correct. |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/net/p2p/src/req_resp/handlers.rs:174
Missing validation for `count == 0`. A peer sending `count=0` bypasses this guard and receives a `Response::Success` with an empty block list rather than an `INVALID_REQUEST` error. The `canonical_blocks_by_range` function short-circuits cleanly, but the caller should enforce the protocol-level constraint and reject the request explicitly, consistent with how `step == 0` is handled.
```suggestion
if request.step == 0 || request.count == 0 || request.count > MAX_REQUEST_BLOCKS {
```
### Issue 2 of 2
crates/net/p2p/src/req_resp/handlers.rs:75-82
**BlocksByRange response handler is a no-op stub**
When this node receives a `BlocksByRange` response from a peer, the blocks are logged and then silently dropped — no processing, storage, or forwarding takes place. This is fine for now since the node doesn't yet issue outbound `BlocksByRange` requests, but if a future caller sends such a request, all returned blocks will be discarded without any diagnostic. Consider adding a `warn!` that makes the "not yet implemented" intent explicit.
Reviews (1): Last reviewed commit: "feat(p2p): add inbound BlocksByRange req..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ResponsePayload::BlocksByRange(blocks) => { | ||
| info!( | ||
| kind = "blocks_by_range_response", | ||
| peer_count, | ||
| count = blocks.len(), | ||
| "P2P message received" | ||
| ); | ||
| } |
There was a problem hiding this comment.
When this node receives a BlocksByRange response from a peer, the blocks are logged and then silently dropped — no processing, storage, or forwarding takes place. This is fine for now since the node doesn't yet issue outbound BlocksByRange requests, but if a future caller sends such a request, all returned blocks will be discarded without any diagnostic. Consider adding a warn! that makes the "not yet implemented" intent explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/p2p/src/req_resp/handlers.rs
Line: 75-82
Comment:
**BlocksByRange response handler is a no-op stub**
When this node receives a `BlocksByRange` response from a peer, the blocks are logged and then silently dropped — no processing, storage, or forwarding takes place. This is fine for now since the node doesn't yet issue outbound `BlocksByRange` requests, but if a future caller sends such a request, all returned blocks will be discarded without any diagnostic. Consider adding a `warn!` that makes the "not yet implemented" intent explicit.
How can I resolve this? If you propose a fix, please make it concise.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
1 participant
🗒️ Description / Motivation
This PR adds inbound
BlocksByRangerequest-response support to the P2P req/resp protocol implementation.The change follows the recently merged spec update:
This is needed so peers can request canonical blocks by slot range, similar to the existing
BlocksByRootprotocol.The implementation:
This improves interoperability with other clients implementing the updated spec.
What Changed
Req/Resp Protocol
BlocksByRangeRequestBlocksByRangeresponse payload variant/leanconsensus/req/blocks_by_range/1/ssz_snappyCodec
Behaviour Registration
BlocksByRangein the libp2p request-response behaviourInbound Request Handling
BlocksByRangeValidation
Added validation for:
step == 0count > 1024Invalid requests return protocol error responses.
Tests
Correctness / Behavior Guarantees
Preserved Invariants
Behavior Notes
1024blocksBlocksByRoothandling patterns for consistencyTests Added / Run
Added
blocks_by_range_returns_canonical_blocks_in_requested_orderVerified With
cargo fmt --check cargo check -p ethlambda-p2p cargo test -p ethlambda-p2p blocks_by_range_returns_canonical_blocks_in_requested_order git diff --checkRelated Issues / PRs
BlocksByRangerequest support #346