feat(rpc): add GET /lean/v0/blocks range query for backfill#457
feat(rpc): add GET /lean/v0/blocks range query for backfill#457MegaRedHand wants to merge 3 commits into
Conversation
Split the monolithic lib.rs API router into focused modules: core.rs holds the finalized-state/finalized-block/justified-checkpoint handlers and shared response helpers; blocks.rs, fork_choice.rs, and admin.rs each expose pub(crate) routes() -> Router<Store>. build_api_router merges them with .with_state(store). No behavior change.
🤖 Codex Code Review
No consensus-critical fork-choice / attestation / STF / SSZ logic changed in this diff beyond the RPC surface. I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewConsensus Correctness
Security & Safety
Performance
Code Quality & Testing
Minor
Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Greptile SummaryAdds a
Confidence Score: 3/5The endpoint logic itself is sound — reads are consistent, the count cap prevents runaway loops — but the error-handling gap means any caller sending a malformed request gets a plain-text 422 instead of the JSON envelope the rest of the API returns, and the PR description documents a different query interface than was actually built. The range handler silently bypasses the JSON error wrapper used everywhere else when Axum's Query extractor rejects bad or missing params; this is a real behavioral difference that would break API consumers trying to parse error responses uniformly. The interface mismatch between the PR description and the implemented query params adds integration risk before the description is corrected. crates/net/rpc/src/blocks.rs — specifically the Query extractor error path and the missing test cases for the cap and bad-input scenarios.
|
| Filename | Overview |
|---|---|
| crates/net/rpc/src/blocks.rs | Adds GET /lean/v0/blocks?start_slot=&count= range endpoint; Query extractor bypasses the JSON error wrapper used by all other handlers, causing inconsistent error responses for malformed/missing params. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant Axum Router
participant get_blocks_by_range
participant Store
Client->>Axum Router: GET /lean/v0/blocks?start_slot=N&count=K
alt Query deserialization fails (missing/bad param)
Axum Router-->>Client: 422 Unprocessable Entity (plain text)
else Query OK
Axum Router->>get_blocks_by_range: Query(params), State(store)
get_blocks_by_range->>get_blocks_by_range: "cap count = min(count, 1024)"
get_blocks_by_range->>Store: head_state()
Store-->>get_blocks_by_range: head_state snapshot
loop for slot in start_slot..start_slot+count
get_blocks_by_range->>Store: historical_block_hashes.get(slot)
alt root exists and non-zero
get_blocks_by_range->>Store: get_block(root)
Store-->>get_blocks_by_range: Block
get_blocks_by_range->>get_blocks_by_range: push to blocks vec
else slot empty or beyond tip
get_blocks_by_range->>get_blocks_by_range: skip silently
end
end
get_blocks_by_range-->>Client: 200 OK, JSON array of blocks
end
%%{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"}}}%%
sequenceDiagram
participant Client
participant Axum Router
participant get_blocks_by_range
participant Store
Client->>Axum Router: GET /lean/v0/blocks?start_slot=N&count=K
alt Query deserialization fails (missing/bad param)
Axum Router-->>Client: 422 Unprocessable Entity (plain text)
else Query OK
Axum Router->>get_blocks_by_range: Query(params), State(store)
get_blocks_by_range->>get_blocks_by_range: "cap count = min(count, 1024)"
get_blocks_by_range->>Store: head_state()
Store-->>get_blocks_by_range: head_state snapshot
loop for slot in start_slot..start_slot+count
get_blocks_by_range->>Store: historical_block_hashes.get(slot)
alt root exists and non-zero
get_blocks_by_range->>Store: get_block(root)
Store-->>get_blocks_by_range: Block
get_blocks_by_range->>get_blocks_by_range: push to blocks vec
else slot empty or beyond tip
get_blocks_by_range->>get_blocks_by_range: skip silently
end
end
get_blocks_by_range-->>Client: 200 OK, JSON array of blocks
end
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
crates/net/rpc/src/blocks.rs:106-122
**Malformed/missing query params return non-JSON error**
When `start_slot` or `count` is absent or cannot be parsed as `u64`, Axum's `Query` extractor short-circuits and returns a `422 Unprocessable Entity` with a plain-text body like `Failed to deserialize query string: missing field 'start_slot'`. Every other error path in this file returns `{"error": "…"}` JSON, so callers of this endpoint will receive an inconsistently formatted error they cannot parse the same way. The existing `Path<String>` handlers dodge this by accepting raw strings and self-validating; the `Query<BlockRangeParams>` approach delegates validation to serde at extraction time, bypassing the JSON error wrapper entirely.
### Issue 2 of 3
crates/net/rpc/src/blocks.rs:96-100
**PR description documents different query parameters than implemented**
The PR description advertises `GET /lean/v0/blocks?from=<slot>&to=<slot>` (a `from`/`to` inclusive range), but `BlockRangeParams` uses `start_slot` + `count` (a start-point + length). The in-code doc comment on the handler is correct, but the discrepancy in the PR description may mislead integrators before they read the source. Worth updating the description — or deciding which interface is actually intended.
### Issue 3 of 3
crates/net/rpc/src/blocks.rs:142-184
**`MAX_RANGE_COUNT` cap and bad-input paths are not exercised by the test**
The single test sends `count=2`, which is far below the 1024 cap, so the capping logic on line 110 is never exercised. There are also no tests for: (a) `count` exceeding `MAX_RANGE_COUNT` (expect the response to contain ≤ 1024 entries), (b) a request missing `start_slot` or `count` entirely (to document and assert the current response shape), and (c) `start_slot` pointing entirely beyond `historical_block_hashes` (to verify the 200-empty-array behavior is intentional). Adding these would harden the endpoint against regressions when the cap or error format is changed.
Reviews (1): Last reviewed commit: "feat(rpc): add GET /lean/v0/blocks range..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub(crate) async fn get_blocks_by_range( | ||
| Query(params): Query<BlockRangeParams>, | ||
| State(store): State<Store>, | ||
| ) -> impl IntoResponse { | ||
| let count = params.count.min(MAX_RANGE_COUNT); | ||
| let head_state = store.head_state(); | ||
| let mut blocks = Vec::new(); | ||
| for slot in params.start_slot..params.start_slot.saturating_add(count) { | ||
| if let Some(root) = head_state.historical_block_hashes.get(slot as usize) | ||
| && !root.is_zero() | ||
| && let Some(block) = store.get_block(root) | ||
| { | ||
| blocks.push(block); | ||
| } | ||
| } | ||
| json_response(blocks) | ||
| } |
There was a problem hiding this comment.
When start_slot or count is absent or cannot be parsed as u64, Axum's Query extractor short-circuits and returns a 422 Unprocessable Entity with a plain-text body like Failed to deserialize query string: missing field 'start_slot'. Every other error path in this file returns {"error": "…"} JSON, so callers of this endpoint will receive an inconsistently formatted error they cannot parse the same way. The existing Path<String> handlers dodge this by accepting raw strings and self-validating; the Query<BlockRangeParams> approach delegates validation to serde at extraction time, bypassing the JSON error wrapper entirely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/rpc/src/blocks.rs
Line: 106-122
Comment:
**Malformed/missing query params return non-JSON error**
When `start_slot` or `count` is absent or cannot be parsed as `u64`, Axum's `Query` extractor short-circuits and returns a `422 Unprocessable Entity` with a plain-text body like `Failed to deserialize query string: missing field 'start_slot'`. Every other error path in this file returns `{"error": "…"}` JSON, so callers of this endpoint will receive an inconsistently formatted error they cannot parse the same way. The existing `Path<String>` handlers dodge this by accepting raw strings and self-validating; the `Query<BlockRangeParams>` approach delegates validation to serde at extraction time, bypassing the JSON error wrapper entirely.
How can I resolve this? If you propose a fix, please make it concise.| #[derive(Deserialize)] | ||
| pub(crate) struct BlockRangeParams { | ||
| start_slot: u64, | ||
| count: u64, | ||
| } |
There was a problem hiding this comment.
The PR description advertises GET /lean/v0/blocks?from=<slot>&to=<slot> (a from/to inclusive range), but BlockRangeParams uses start_slot + count (a start-point + length). The in-code doc comment on the handler is correct, but the discrepancy in the PR description may mislead integrators before they read the source. Worth updating the description — or deciding which interface is actually intended.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/rpc/src/blocks.rs
Line: 96-100
Comment:
**PR description documents different query parameters than implemented**
The PR description advertises `GET /lean/v0/blocks?from=<slot>&to=<slot>` (a `from`/`to` inclusive range), but `BlockRangeParams` uses `start_slot` + `count` (a start-point + length). The in-code doc comment on the handler is correct, but the discrepancy in the PR description may mislead integrators before they read the source. Worth updating the description — or deciding which interface is actually intended.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| #[cfg(test)] | ||
| mod range_tests { | ||
| use crate::test_utils::{create_test_state, insert_block_raw, make_block}; | ||
| use axum::{ | ||
| body::Body, | ||
| http::{Request, StatusCode}, | ||
| }; | ||
| use ethlambda_storage::{Store, backend::InMemoryBackend}; | ||
| use ethlambda_types::{primitives::H256, state::JustifiedSlots}; | ||
| use http_body_util::BodyExt; | ||
| use std::sync::Arc; | ||
| use tower::ServiceExt; | ||
|
|
||
| fn store_with_block_at_slot_1() -> Store { | ||
| let backend = Arc::new(InMemoryBackend::new()); | ||
| let target = make_block(1, H256::ZERO); | ||
| let root = insert_block_raw(backend.as_ref(), &target); | ||
| let mut anchor = create_test_state(); | ||
| anchor.slot = 2; | ||
| anchor.historical_block_hashes = vec![H256::ZERO, root].try_into().unwrap(); | ||
| anchor.justified_slots = JustifiedSlots::with_length(2).unwrap(); | ||
| Store::from_anchor_state(backend, anchor) | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn blocks_range_returns_canonical_blocks() { | ||
| let app = crate::build_api_router(store_with_block_at_slot_1()); | ||
| let resp = app | ||
| .oneshot( | ||
| Request::builder() | ||
| .uri("/lean/v0/blocks?start_slot=1&count=2") | ||
| .body(Body::empty()) | ||
| .unwrap(), | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status(), StatusCode::OK); | ||
| let body = resp.into_body().collect().await.unwrap().to_bytes(); | ||
| let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); | ||
| assert_eq!(json.as_array().unwrap().len(), 1); | ||
| assert_eq!(json[0]["slot"], 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
MAX_RANGE_COUNT cap and bad-input paths are not exercised by the test
The single test sends count=2, which is far below the 1024 cap, so the capping logic on line 110 is never exercised. There are also no tests for: (a) count exceeding MAX_RANGE_COUNT (expect the response to contain ≤ 1024 entries), (b) a request missing start_slot or count entirely (to document and assert the current response shape), and (c) start_slot pointing entirely beyond historical_block_hashes (to verify the 200-empty-array behavior is intentional). Adding these would harden the endpoint against regressions when the cap or error format is changed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/rpc/src/blocks.rs
Line: 142-184
Comment:
**`MAX_RANGE_COUNT` cap and bad-input paths are not exercised by the test**
The single test sends `count=2`, which is far below the 1024 cap, so the capping logic on line 110 is never exercised. There are also no tests for: (a) `count` exceeding `MAX_RANGE_COUNT` (expect the response to contain ≤ 1024 entries), (b) a request missing `start_slot` or `count` entirely (to document and assert the current response shape), and (c) `start_slot` pointing entirely beyond `historical_block_hashes` (to verify the 200-empty-array behavior is intentional). Adding these would harden the endpoint against regressions when the cap or error format is changed.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
🤖 Claude Code ReviewNow I have verified findings. Let me compose the final review. PR Review:
|
…on storage gap, tests) - Return JSON 400 (not Axum plain-text 422) for missing/non-numeric query params by accepting Result<Query<_>, QueryRejection> and converting manually. - Emit tracing::warn! when a non-zero root in historical_block_hashes has no corresponding block in the store, making storage gaps observable in logs. - Add three new range_tests: count clamped to MAX_RANGE_COUNT, missing params return JSON 400, and start_slot beyond history length returns empty array.
1 participant
Adds a range query variant of the blocks endpoint (
GET /lean/v0/blocks?from=<slot>&to=<slot>), allowing explorers and sync helpers to fetch a contiguous sequence of block headers without issuing one request per slot.Intended primarily for backfill and checkpoint-sync helper flows. Has unit tests and passed clippy.
Stacked on #454.