Conversation
Adds two debug endpoints for inspecting the chain:
GET /lean/v0/blocks/{block_id} → Block as JSON
GET /lean/v0/blocks/{block_id}/header → BlockHeader as JSON
is either a 0x-prefixed 32-byte hex root or a decimal slot.
Slot lookups use the head state's historical_block_hashes, so only
canonical blocks are reachable by slot — side-fork blocks must be
addressed by their root. Invalid ids return 400; unknown/empty slots
or missing roots return 404.
Derives Serialize on Block, BlockBody, AggregatedAttestation, and
AttestationData, with custom serializers for the two SszList/SszBitlist
fields (attestations as a JSON array, aggregation_bits as a 0x-hex
string matching the beacon API convention).
Adds Store::get_block, which returns header+body without signatures and
works for the genesis block (unlike get_signed_block, which requires a
signature entry).
🤖 Kimi Code ReviewOverall Assessment: The PR implements block retrieval RPC endpoints with proper error handling and comprehensive tests. The implementation is generally sound, but there is a critical missing dependency and a few minor issues. Critical Issues1. Missing
Code Correctness & Safety2. Potential panic on database corruption in
Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
No consensus-transition, fork-choice, attestation, XMSS, or SSZ-state-machine logic changed here; the concerns are confined to the new RPC path and its storage access patterns. I could not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewNow I have enough context for a thorough review. PR #301 Review: Add JSON-RPC endpoints for blocksOverviewAdds The implementation is clean and consistent with project conventions. A few issues worth addressing before merging. Correctness IssuesThe head block is unreachable by slot number (
This is a silent correctness gap: a caller who queries the latest slot by number gets a confusing 404 even though the block exists. The simplest fix is to special-case the head slot: fn resolve_slot(store: &Store, slot: u64) -> Result<H256, BlockIdError> {
let head_state = store.head_state();
// historical_block_hashes contains entries for slots 0..head_slot-1.
// The head block itself must be retrieved from latest_block_header.
if slot == head_state.latest_block_header.slot {
return Ok(head_state.latest_block_header.hash_tree_root());
}
let root = head_state
.historical_block_hashes
.get(slot as usize)
.ok_or(BlockIdError::NotFound)?;
if root.is_zero() {
return Err(BlockIdError::NotFound);
}
Ok(*root)
}At minimum, document the limitation clearly in the function doc comment. A test for Minor Issues
On 32-bit platforms
The manual fn parse_root(hex_body: &str) -> Result<H256, BlockIdError> {
let arr: [u8; 32] = hex::decode(hex_body)
.map_err(|_| BlockIdError::Invalid)?
.try_into()
.map_err(|_| BlockIdError::Invalid)?;
Ok(H256(arr))
}
The custom serializer can be replaced with one line: fn serialize_attestations<S>(attestations: &AggregatedAttestations, serializer: S) -> Result<S::Ok, S::Error>
where S: Serializer {
serializer.collect_seq(attestations.iter())
}
Slot-based lookups clone the entire head state (validators, justified_slots, etc.) just to read one element of Test Coverage GapThe What's Done Well
Automated review by Claude (Anthropic) · sonnet · custom prompt |
Greptile SummaryThis PR adds two debug HTTP endpoints ( Confidence Score: 5/5Safe to merge — no correctness issues; all two findings are P2 style suggestions. The block endpoint logic is correct, error handling covers all edge cases (400/404), genesis block is handled properly via EMPTY_BODY_ROOT, and tests cover 7 distinct paths. Only P2 suggestions remain: full-state deserialization cost in slot lookup and a missing slot-based header test. No files require special attention.
|
| Filename | Overview |
|---|---|
| crates/net/rpc/src/blocks.rs | New file: implements get_block and get_block_header handlers with block_id resolution by hex root or decimal slot; error handling is complete and correct. |
| crates/storage/src/store.rs | Adds get_block() method that correctly handles the genesis block (no body entry) by checking EMPTY_BODY_ROOT; parallels the existing get_signed_block() pattern cleanly. |
| crates/common/types/src/block.rs | Adds Serialize derives to Block, BlockBody, BlockHeader; custom serialize_attestations iterates SszList as a JSON array since SszList doesn't implement Serialize natively. |
| crates/common/types/src/attestation.rs | Adds Serialize to AttestationData and AggregatedAttestation; custom serialize_aggregation_bits encodes SszBitlist as 0x-prefixed hex matching the beacon API convention. |
| crates/net/rpc/src/lib.rs | Registers new block routes and adds comprehensive tests (7 cases) covering root lookup, slot lookup, header variant, and all error paths. |
Sequence Diagram
sequenceDiagram
participant Client
participant API as RPC API Server
participant Blocks as blocks.rs
participant Store as Store
participant DB as RocksDB Backend
Client->>API: GET /lean/v0/blocks/{block_id}
API->>Blocks: get_block(block_id)
alt block_id starts with "0x"
Blocks->>Blocks: parse_root(hex_body)
Blocks-->>Blocks: H256 root
else block_id is all digits
Blocks->>Store: head_state()
Store->>DB: get(Metadata, "head")
DB-->>Store: head root
Store->>DB: get(States, head_root)
DB-->>Store: State bytes (incl. historical_block_hashes)
Store-->>Blocks: State
Blocks->>Blocks: historical_block_hashes[slot]
alt slot out of bounds or zero root
Blocks-->>Client: 404 Not Found
end
Blocks-->>Blocks: H256 root
else invalid
Blocks-->>Client: 400 Bad Request
end
Blocks->>Store: get_block(root)
Store->>DB: get(BlockHeaders, root)
DB-->>Store: header bytes
alt body_root == EMPTY_BODY_ROOT
Store-->>Store: BlockBody::default()
else
Store->>DB: get(BlockBodies, root)
DB-->>Store: body bytes
end
Store-->>Blocks: Option<Block>
alt block found
Blocks-->>Client: 200 OK (JSON)
else
Blocks-->>Client: 404 Not Found
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/net/rpc/src/blocks.rs
Line: 72-82
Comment:
**Full state deserialization for a single slot lookup**
`store.head_state()` deserializes the entire beacon `State` — including `historical_block_hashes`, which grows linearly with the chain's age (32 bytes × N slots). On a long-running chain this can be hundreds of MB of SSZ parsing just to index one entry. Since this is a debug endpoint the cost is tolerable for now, but a dedicated `Store::get_historical_block_hash(slot: u64) -> Option<H256>` accessor that reads only the `States` entry and pulls the specific index would eliminate the overhead without much extra complexity.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/net/rpc/src/lib.rs
Line: 356-369
Comment:
**Missing test for slot-based header lookup**
`get_block_by_slot_returns_json` covers `GET /lean/v0/blocks/{slot}` but there is no corresponding test for `GET /lean/v0/blocks/{slot}/header`. The `store_with_historical_block` fixture is already set up — a quick additional case would confirm the slot→root→header path works end-to-end.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Add JSON-RPC endpoints for blocks (close..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn resolve_slot(store: &Store, slot: u64) -> Result<H256, BlockIdError> { | ||
| let head_state = store.head_state(); | ||
| let root = head_state | ||
| .historical_block_hashes | ||
| .get(slot as usize) | ||
| .ok_or(BlockIdError::NotFound)?; | ||
| if root.is_zero() { | ||
| return Err(BlockIdError::NotFound); | ||
| } | ||
| Ok(*root) | ||
| } |
There was a problem hiding this comment.
store.head_state() deserializes the entire beacon State — including historical_block_hashes, which grows linearly with the chain's age (32 bytes × N slots). On a long-running chain this can be hundreds of MB of SSZ parsing just to index one entry. Since this is a debug endpoint the cost is tolerable for now, but a dedicated Store::get_historical_block_hash(slot: u64) -> Option<H256> accessor that reads only the States entry and pulls the specific index would eliminate the overhead without much extra complexity.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/rpc/src/blocks.rs
Line: 72-82
Comment:
**Full state deserialization for a single slot lookup**
`store.head_state()` deserializes the entire beacon `State` — including `historical_block_hashes`, which grows linearly with the chain's age (32 bytes × N slots). On a long-running chain this can be hundreds of MB of SSZ parsing just to index one entry. Since this is a debug endpoint the cost is tolerable for now, but a dedicated `Store::get_historical_block_hash(slot: u64) -> Option<H256>` accessor that reads only the `States` entry and pulls the specific index would eliminate the overhead without much extra complexity.
How can I resolve this? If you propose a fix, please make it concise.| #[tokio::test] | ||
| async fn get_block_by_slot_returns_json() { | ||
| let (store, _target_root) = store_with_historical_block(); | ||
| let app = build_api_router(store); | ||
|
|
||
| let response = send(app, "/lean/v0/blocks/1").await; | ||
|
|
||
| assert_eq!(response.status(), StatusCode::OK); | ||
| let body = response.into_body().collect().await.unwrap().to_bytes(); | ||
| let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); | ||
|
|
||
| assert_eq!(json["slot"], 1); | ||
| assert!(json["body"]["attestations"].is_array()); | ||
| } |
There was a problem hiding this comment.
get_block_by_slot_returns_json covers GET /lean/v0/blocks/{slot} but there is no corresponding test for GET /lean/v0/blocks/{slot}/header. The store_with_historical_block fixture is already set up — a quick additional case would confirm the slot→root→header path works end-to-end.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/rpc/src/lib.rs
Line: 356-369
Comment:
**Missing test for slot-based header lookup**
`get_block_by_slot_returns_json` covers `GET /lean/v0/blocks/{slot}` but there is no corresponding test for `GET /lean/v0/blocks/{slot}/header`. The `store_with_historical_block` fixture is already set up — a quick additional case would confirm the slot→root→header path works end-to-end.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Motivation
Closes #75. There's currently no way to inspect individual blocks over
HTTP — useful for debugging, integration tests, and comparing state
across clients.
Description
Adds two debug endpoints:
GET /lean/v0/blocks/{block_id}— returns the block as JSONGET /lean/v0/blocks/{block_id}/header— returns the block header as JSONblock_idis either a0x-prefixed 32-byte hex root or a decimal slot.Slot lookups use the head state's
historical_block_hashes, so onlycanonical blocks are reachable by slot — side-fork blocks must be
addressed by their root. Invalid ids return 400; unknown/empty slots or
missing roots return 404.
Supporting changes:
SerializeonBlock,BlockBody,AggregatedAttestation, andAttestationData. Custom serializers for the twoSszList/SszBitlistfields (attestations as a JSON array, aggregation_bits as a
0x-hexstring matching the beacon API convention).
Store::get_block, which returns header+body without signaturesand works for the genesis block (unlike
get_signed_block, whichrequires a signature entry).
How to Test
Unit tests in
crates/net/rpc/src/lib.rscover happy paths (by root, byslot, header variant) and error paths (400 invalid id, 404 missing root,
404 future slot, 404 empty slot).
Closes
Closes #75