feat(rpc): add GET /lean/v0/node/{identity,syncing}#458
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.
Exposes node identity (build version) and sync status (head_slot, sync_distance, is_syncing) derived from store.time() / INTERVALS_PER_SLOT.
🤖 Codex Code ReviewFindings
No direct fork-choice, attestation, STF, XMSS, or SSZ logic changed in this PR; the risk is mainly incorrect operational signaling via the new RPC surface. I could not run the added tests here because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewThe PR introduces RPC endpoints for node syncing status and identity. Overall the implementation is sound, but there are layout, correctness, and testing gaps to address. 1. Import placement breaks documentation associationFile: The new Fix: Move the export to the end of the re-exports block or group it with other sync-related exports. 2. Race condition in sync status calculationFile: let head_slot = store.head_slot();
let wall_slot = store.time() / INTERVALS_PER_SLOT;
let sync_distance = wall_slot.saturating_sub(head_slot);
Fix: Use a single atomic read or snapshot method that returns both values together, or document that this endpoint provides a best-effort snapshot (the current comment alludes to this but the race condition should be explicitly noted). 3. Insufficient test coverage for sync edge casesFile: The
Fix: Add tests for 4. Clock skew vulnerabilityFile: If the system clock is slow (wall_slot < head_slot), Fix: Consider logging a warning when 5. Minor: Unused test import pathFile: The import Consensus Safety NoteThe endpoint documentation correctly notes this is a "simplified" stateless check without hysteresis. Ensure no validator logic uses this RPC endpoint for attestation decisions; it should only be used for monitoring. The production path should use the VerdictCode is functionally correct for a monitoring endpoint. Address Item 1 (layout) and Item 2 (race condition documentation or fix) before merging. Items 3 and 4 are recommended follow-ups. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Greptile SummaryThis PR adds two read-only HTTP endpoints —
Confidence Score: 3/5The new endpoints are additive and read-only, so they cannot corrupt state, but is_syncing can disagree with the node's actual operational sync gate in concrete scenarios. The main concern is in get_syncing: omitting the network-stall override and hysteresis band means is_syncing diverges from what the node actually uses to gate validator duties — during a network stall the tracker returns Synced while this endpoint may return is_syncing: true. The identity response is also missing the peer_id and enr fields the PR description promises, and the syncing response omits the finalized_slot it claims to include. crates/net/rpc/src/node.rs — sync logic, identity fields, and syncing response shape all need alignment with either the spec or the PR description.
|
| Filename | Overview |
|---|---|
| crates/net/rpc/src/node.rs | New file implementing /lean/v0/node/syncing and /lean/v0/node/identity endpoints; sync logic diverges from SyncStatusTracker (no hysteresis/stall override), identity response is missing peer_id/enr, and syncing response is missing finalized_slot |
| crates/blockchain/src/sync_status.rs | SYNC_LAG_THRESHOLD made pub to allow reuse in the RPC layer; no logic changes |
| crates/blockchain/src/lib.rs | Re-exports SYNC_LAG_THRESHOLD at the crate root for downstream consumers |
| crates/net/rpc/src/lib.rs | Registers node::routes() in build_api_router; minimal, low-risk change |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant RPC as axum Router
participant NodeHandler as node.rs handlers
participant Store
Client->>RPC: GET /lean/v0/node/syncing
RPC->>NodeHandler: get_syncing(State(store))
NodeHandler->>Store: store.head_slot()
Store-->>NodeHandler: head_slot: u64
NodeHandler->>Store: store.time()
Store-->>NodeHandler: intervals: u64
NodeHandler->>NodeHandler: "wall_slot = time / INTERVALS_PER_SLOT"
NodeHandler->>NodeHandler: "sync_distance = wall_slot.saturating_sub(head_slot)"
NodeHandler->>NodeHandler: "is_syncing = sync_distance > SYNC_LAG_THRESHOLD (4)"
NodeHandler-->>RPC: "JSON { is_syncing, head_slot, sync_distance }"
RPC-->>Client: 200 OK
Client->>RPC: GET /lean/v0/node/identity
RPC->>NodeHandler: get_identity()
NodeHandler->>NodeHandler: "version = env!(CARGO_PKG_VERSION)"
NodeHandler-->>RPC: "JSON { version }"
RPC-->>Client: 200 OK
%%{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 RPC as axum Router
participant NodeHandler as node.rs handlers
participant Store
Client->>RPC: GET /lean/v0/node/syncing
RPC->>NodeHandler: get_syncing(State(store))
NodeHandler->>Store: store.head_slot()
Store-->>NodeHandler: head_slot: u64
NodeHandler->>Store: store.time()
Store-->>NodeHandler: intervals: u64
NodeHandler->>NodeHandler: "wall_slot = time / INTERVALS_PER_SLOT"
NodeHandler->>NodeHandler: "sync_distance = wall_slot.saturating_sub(head_slot)"
NodeHandler->>NodeHandler: "is_syncing = sync_distance > SYNC_LAG_THRESHOLD (4)"
NodeHandler-->>RPC: "JSON { is_syncing, head_slot, sync_distance }"
RPC-->>Client: 200 OK
Client->>RPC: GET /lean/v0/node/identity
RPC->>NodeHandler: get_identity()
NodeHandler->>NodeHandler: "version = env!(CARGO_PKG_VERSION)"
NodeHandler-->>RPC: "JSON { version }"
RPC-->>Client: 200 OK
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/node.rs:7-18
**`is_syncing` diverges from `SyncStatusTracker` in two concrete cases**
The `SyncStatusTracker` applies two overrides that this stateless endpoint ignores: (1) **network-stall override** — when `network_lag > 8`, the tracker forces `syncing = false` even if the local head is far behind; this endpoint will still return `is_syncing: true`. (2) **hysteresis** — once syncing, the tracker keeps `syncing = true` until `head_lag <= threshold - band = 2`; this endpoint flips back to `false` at `head_lag <= 4`. Dashboards or orchestrators relying on this field could therefore disagree with the actual operational sync gate (used to suppress validator duties), leading to false alerts or missed gates.
### Issue 2 of 3
crates/net/rpc/src/node.rs:15-18
**`IdentityResponse` missing `peer_id` and `enr` fields**
The PR description states this endpoint "returns peer ID and ENR," but `IdentityResponse` only exposes `version` (the cargo package version). Consumers expecting a peer identifier or ENR for node discovery will find neither. Either the response struct needs `peer_id` and `enr` fields, or the endpoint description should be corrected to reflect its current scope.
### Issue 3 of 3
crates/net/rpc/src/node.rs:8-13
**`finalized_slot` missing from `SyncingResponse`**
The PR description lists "finalized slot" as one of the three pieces of sync info this endpoint returns, but `SyncingResponse` only has `head_slot` and `sync_distance`. If the omission is intentional, the description should be updated; if it was accidentally left out, `store.latest_finalized().slot` is already accessible and the field can be added straightforwardly.
Reviews (1): Last reviewed commit: "feat(rpc): add GET /lean/v0/node/{identi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| #[derive(Serialize)] | ||
| struct SyncingResponse { | ||
| is_syncing: bool, | ||
| head_slot: u64, | ||
| sync_distance: u64, | ||
| } | ||
|
|
||
| #[derive(Serialize)] | ||
| struct IdentityResponse { | ||
| version: &'static str, | ||
| } |
There was a problem hiding this comment.
is_syncing diverges from SyncStatusTracker in two concrete cases
The SyncStatusTracker applies two overrides that this stateless endpoint ignores: (1) network-stall override — when network_lag > 8, the tracker forces syncing = false even if the local head is far behind; this endpoint will still return is_syncing: true. (2) hysteresis — once syncing, the tracker keeps syncing = true until head_lag <= threshold - band = 2; this endpoint flips back to false at head_lag <= 4. Dashboards or orchestrators relying on this field could therefore disagree with the actual operational sync gate (used to suppress validator duties), leading to false alerts or missed gates.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/rpc/src/node.rs
Line: 7-18
Comment:
**`is_syncing` diverges from `SyncStatusTracker` in two concrete cases**
The `SyncStatusTracker` applies two overrides that this stateless endpoint ignores: (1) **network-stall override** — when `network_lag > 8`, the tracker forces `syncing = false` even if the local head is far behind; this endpoint will still return `is_syncing: true`. (2) **hysteresis** — once syncing, the tracker keeps `syncing = true` until `head_lag <= threshold - band = 2`; this endpoint flips back to `false` at `head_lag <= 4`. Dashboards or orchestrators relying on this field could therefore disagree with the actual operational sync gate (used to suppress validator duties), leading to false alerts or missed gates.
How can I resolve this? If you propose a fix, please make it concise.| #[derive(Serialize)] | ||
| struct IdentityResponse { | ||
| version: &'static str, | ||
| } |
There was a problem hiding this comment.
IdentityResponse missing peer_id and enr fields
The PR description states this endpoint "returns peer ID and ENR," but IdentityResponse only exposes version (the cargo package version). Consumers expecting a peer identifier or ENR for node discovery will find neither. Either the response struct needs peer_id and enr fields, or the endpoint description should be corrected to reflect its current scope.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/rpc/src/node.rs
Line: 15-18
Comment:
**`IdentityResponse` missing `peer_id` and `enr` fields**
The PR description states this endpoint "returns peer ID and ENR," but `IdentityResponse` only exposes `version` (the cargo package version). Consumers expecting a peer identifier or ENR for node discovery will find neither. Either the response struct needs `peer_id` and `enr` fields, or the endpoint description should be corrected to reflect its current scope.
How can I resolve this? If you propose a fix, please make it concise.| #[derive(Serialize)] | ||
| struct SyncingResponse { | ||
| is_syncing: bool, | ||
| head_slot: u64, | ||
| sync_distance: u64, | ||
| } |
There was a problem hiding this comment.
finalized_slot missing from SyncingResponse
The PR description lists "finalized slot" as one of the three pieces of sync info this endpoint returns, but SyncingResponse only has head_slot and sync_distance. If the omission is intentional, the description should be updated; if it was accidentally left out, store.latest_finalized().slot is already accessible and the field can be added straightforwardly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/rpc/src/node.rs
Line: 8-13
Comment:
**`finalized_slot` missing from `SyncingResponse`**
The PR description lists "finalized slot" as one of the three pieces of sync info this endpoint returns, but `SyncingResponse` only has `head_slot` and `sync_distance`. If the omission is intentional, the description should be updated; if it was accidentally left out, `store.latest_finalized().slot` is already accessible and the field can be added straightforwardly.
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewAll three top candidates confirmed. Here is the review. Review: feat(rpc): add GET /lean/v0/node/{identity,syncing}This PR adds two new HTTP endpoints to the API server:
|
… finalized_slot) - Replace store.time()-based wall_slot with real SystemTime::now() so sync_distance is correct after an offline gap (store.time() freezes during downtime, causing false is_syncing=false on restart) - Add finalized_slot field to SyncingResponse from store.latest_finalized() - Split node syncing tests: far-behind case (genesis_time=1000) asserts is_syncing=true; up-to-date case (genesis_time year 2100) asserts is_syncing=false and sync_distance=0 - Add one-line doc comment to SYNC_LAG_THRESHOLD re-export in blockchain lib.rs
1 participant
Adds two node-info endpoints:
GET /lean/v0/node/identity— returns peer ID and ENR.GET /lean/v0/node/syncing— returns current sync status (head slot, finalized slot, sync lag).Both are useful for health dashboards and orchestration tooling. Has unit tests and passed clippy.
Stacked on #454.