Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions crates/net/rpc/src/admin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,26 @@
//! the role (hot-standby model). See leanSpec PR #636 for the full rationale.

use axum::{
Extension, Json,
Extension, Json, Router,
http::StatusCode,
response::{IntoResponse, Response},
routing::get,
};
use ethlambda_storage::Store;
use ethlambda_types::aggregator::AggregatorController;
use serde::Serialize;
use serde_json::Value;
use tracing::info;

use crate::json_response;

pub(crate) fn routes() -> Router<Store> {
Router::new().route(
"/lean/v0/admin/aggregator",
get(get_aggregator).post(post_aggregator),
)
}

#[derive(Serialize)]
struct StatusResponse {
is_aggregator: bool,
Expand All @@ -44,7 +53,9 @@ struct ToggleResponse {
/// `Extension<T>` would cause axum to short-circuit with a 500 when the
/// extension is missing, whereas `Option` yields `None` and lets us return
/// a clean 503 with a useful message.
pub async fn get_aggregator(controller: Option<Extension<AggregatorController>>) -> Response {
pub(crate) async fn get_aggregator(
controller: Option<Extension<AggregatorController>>,
) -> Response {
match controller {
Some(Extension(controller)) => json_response(StatusResponse {
is_aggregator: controller.is_enabled(),
Expand All @@ -62,7 +73,7 @@ pub async fn get_aggregator(controller: Option<Extension<AggregatorController>>)
/// `Extension<T>` would cause axum to short-circuit with a 500 when the
/// extension is missing, whereas `Option` yields `None` and lets us return
/// a clean 503 with a useful message.
pub async fn post_aggregator(
pub(crate) async fn post_aggregator(
controller: Option<Extension<AggregatorController>>,
body: Option<Json<Value>>,
) -> Response {
Expand Down
167 changes: 164 additions & 3 deletions crates/net/rpc/src/blocks.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,31 @@
use axum::{
extract::{Path, State},
Router,
extract::rejection::QueryRejection,
extract::{Path, Query, State},
http::StatusCode,
response::IntoResponse,
routing::get,
};
use ethlambda_storage::Store;
use ethlambda_types::primitives::H256;
use serde::Deserialize;
use serde_json::json;

use crate::json_response;

const MAX_RANGE_COUNT: u64 = 1024;

pub(crate) fn routes() -> Router<Store> {
Router::new()
.route("/lean/v0/blocks", get(get_blocks_by_range))
.route("/lean/v0/blocks/{block_id}", get(get_block))
.route("/lean/v0/blocks/{block_id}/header", get(get_block_header))
}

/// `GET /lean/v0/blocks/:block_id` — returns the block as JSON.
///
/// `block_id` can be a `0x`-prefixed 32-byte hex root or a decimal slot.
pub async fn get_block(
pub(crate) async fn get_block(
Path(block_id): Path<String>,
State(store): State<Store>,
) -> impl IntoResponse {
Expand All @@ -28,7 +41,7 @@ pub async fn get_block(
}

/// `GET /lean/v0/blocks/:block_id/header` — returns the block header as JSON.
pub async fn get_block_header(
pub(crate) async fn get_block_header(
Path(block_id): Path<String>,
State(store): State<Store>,
) -> impl IntoResponse {
Expand Down Expand Up @@ -81,6 +94,50 @@ fn resolve_slot(store: &Store, slot: u64) -> Result<H256, BlockIdError> {
Ok(*root)
}

#[derive(Deserialize)]
pub(crate) struct BlockRangeParams {
start_slot: u64,
count: u64,
}
Comment on lines +97 to +101

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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!


/// `GET /lean/v0/blocks?start_slot=&count=` — returns canonical blocks in the given slot range.
///
/// Returns a JSON array of blocks. Slots with no canonical block (zero root) are silently
/// skipped. `count` is capped at [`MAX_RANGE_COUNT`].
pub(crate) async fn get_blocks_by_range(
params: Result<Query<BlockRangeParams>, QueryRejection>,
State(store): State<Store>,
) -> impl IntoResponse {
let Query(params) = match params {
Ok(p) => p,
Err(err) => {
let mut response =
json_response(json!({ "error": format!("invalid query parameters: {err}") }));
*response.status_mut() = StatusCode::BAD_REQUEST;
return response;
}
};

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) {
let Some(root) = head_state.historical_block_hashes.get(slot as usize) else {
break;
};
if root.is_zero() {
continue;
}
match store.get_block(root) {
Some(block) => blocks.push(block),
None => {
tracing::warn!(%slot, %root, "block referenced by historical_block_hashes is missing from store");
}
}
}
json_response(blocks)
}
Comment on lines +107 to +139

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

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(Debug)]
enum BlockIdError {
Invalid,
Expand All @@ -98,3 +155,107 @@ impl IntoResponse for BlockIdError {
response
}
}

#[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);
}

/// `count` larger than MAX_RANGE_COUNT is silently clamped to 1024.
#[tokio::test]
async fn blocks_range_clamps_count_to_max() {
let app = crate::build_api_router(store_with_block_at_slot_1());
// count=9999 >> MAX_RANGE_COUNT=1024; only one block exists at slot 1
let resp = app
.oneshot(
Request::builder()
.uri("/lean/v0/blocks?start_slot=0&count=9999")
.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();
// Clamped to 1024 slots starting at 0; only slot 1 has a block.
assert_eq!(json.as_array().unwrap().len(), 1);
}

/// Missing or non-numeric query params return JSON 400.
#[tokio::test]
async fn blocks_range_missing_params_returns_json_400() {
let app = crate::build_api_router(store_with_block_at_slot_1());
let resp = app
.oneshot(
Request::builder()
.uri("/lean/v0/blocks?start_slot=&count=")
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();
assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
let body = resp.into_body().collect().await.unwrap().to_bytes();
let json: serde_json::Value = serde_json::from_slice(&body).unwrap();
assert!(json["error"].is_string(), "expected JSON error field");
}

/// `start_slot` beyond the length of `historical_block_hashes` returns an empty array.
#[tokio::test]
async fn blocks_range_start_slot_beyond_history_returns_empty() {
let app = crate::build_api_router(store_with_block_at_slot_1());
// historical_block_hashes has length 2 (slots 0-1); slot 999 is out of range.
let resp = app
.oneshot(
Request::builder()
.uri("/lean/v0/blocks?start_slot=999&count=10")
.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(), 0);
}
}
Comment on lines +159 to +261

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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!

73 changes: 73 additions & 0 deletions crates/net/rpc/src/core.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
use axum::{
Json, Router,
http::{HeaderValue, header},
response::IntoResponse,
routing::get,
};
use ethlambda_storage::Store;
use ethlambda_types::primitives::H256;
use libssz::SszEncode;

pub(crate) fn routes() -> Router<Store> {
Router::new()
.route("/lean/v0/health", get(crate::metrics::get_health))
.route("/lean/v0/states/finalized", get(get_latest_finalized_state))
.route("/lean/v0/blocks/finalized", get(get_latest_finalized_block))
.route(
"/lean/v0/checkpoints/justified",
get(get_latest_justified_state),
)
}

pub(crate) async fn get_latest_finalized_state(
axum::extract::State(store): axum::extract::State<Store>,
) -> impl IntoResponse {
let finalized = store.latest_finalized();
let mut state = store
.get_state(&finalized.root)
.expect("finalized state exists");

// Zero state_root to match the canonical post-state representation.
// The spec's state_transition sets state_root to zero during process_block_header,
// and only fills it in lazily at the next slot's process_slots.
// Serving the canonical form ensures checkpoint sync interoperability.
state.latest_block_header.state_root = H256::ZERO;

ssz_response(state.to_ssz())
}

pub(crate) async fn get_latest_finalized_block(
axum::extract::State(store): axum::extract::State<Store>,
) -> impl IntoResponse {
let finalized = store.latest_finalized();
// Returns 404 for genesis since it doesn't have a valid signature
match store.get_signed_block(&finalized.root) {
Some(block) => ssz_response(block.to_ssz()),
None => axum::http::StatusCode::NOT_FOUND.into_response(),
}
}

pub(crate) async fn get_latest_justified_state(
axum::extract::State(store): axum::extract::State<Store>,
) -> impl IntoResponse {
let checkpoint = store.latest_justified();
json_response(checkpoint)
}

pub(crate) fn json_response<T: serde::Serialize>(value: T) -> axum::response::Response {
let mut response = Json(value).into_response();
response.headers_mut().insert(
header::CONTENT_TYPE,
HeaderValue::from_static(crate::JSON_CONTENT_TYPE),
);
response
}

fn ssz_response(bytes: Vec<u8>) -> axum::response::Response {
let mut response = bytes.into_response();
response.headers_mut().insert(
header::CONTENT_TYPE,
HeaderValue::from_static(crate::SSZ_CONTENT_TYPE),
);
response
}
19 changes: 11 additions & 8 deletions crates/net/rpc/src/fork_choice.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
use axum::{http::HeaderValue, http::header, response::IntoResponse};
use axum::{Router, http::HeaderValue, http::header, response::IntoResponse, routing::get};
use ethlambda_storage::Store;
use ethlambda_types::{checkpoint::Checkpoint, primitives::H256};
use serde::Serialize;

use crate::json_response;

pub(crate) fn routes() -> Router<Store> {
Router::new()
.route("/lean/v0/fork_choice", get(get_fork_choice))
.route("/lean/v0/fork_choice/ui", get(get_fork_choice_ui))
}

const HTML_CONTENT_TYPE: &str = "text/html; charset=utf-8";
const FORK_CHOICE_HTML: &str = include_str!("../static/fork_choice.html");

Expand All @@ -27,7 +33,7 @@ pub struct ForkChoiceNode {
weight: u64,
}

pub async fn get_fork_choice(
pub(crate) async fn get_fork_choice(
axum::extract::State(store): axum::extract::State<Store>,
) -> impl IntoResponse {
let blocks = store.get_live_chain();
Expand Down Expand Up @@ -75,7 +81,7 @@ pub async fn get_fork_choice(
json_response(response)
}

pub async fn get_fork_choice_ui() -> impl IntoResponse {
pub(crate) async fn get_fork_choice_ui() -> impl IntoResponse {
let mut response = FORK_CHOICE_HTML.into_response();
response.headers_mut().insert(
header::CONTENT_TYPE,
Expand All @@ -87,7 +93,7 @@ pub async fn get_fork_choice_ui() -> impl IntoResponse {
#[cfg(test)]
mod tests {
use super::*;
use axum::{Router, body::Body, http::Request, http::StatusCode, routing::get};
use axum::{Router, body::Body, http::Request, http::StatusCode};
use ethlambda_storage::{Store, backend::InMemoryBackend};
use http_body_util::BodyExt;
use std::sync::Arc;
Expand All @@ -96,10 +102,7 @@ mod tests {
use crate::test_utils::create_test_state;

fn build_test_router(store: Store) -> Router {
Router::new()
.route("/lean/v0/fork_choice", get(get_fork_choice))
.route("/lean/v0/fork_choice/ui", get(get_fork_choice_ui))
.with_state(store)
routes().with_state(store)
}

#[tokio::test]
Expand Down
Loading
Loading