feat(rpc): add GET /lean/v0/node/{identity,syncing}#458
Conversation
🤖 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 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:
|
Exposes node identity (build version) and sync status (head_slot, sync_distance, is_syncing) derived from store.time() / INTERVALS_PER_SLOT.
… 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
MegaRedHand
force-pushed
the
feat/lean-api-node
branch
from
7f7084c to
6d08bcd
Compare
The node/identity endpoint returned only the crate semver (CARGO_PKG_VERSION, e.g. "0.1.0"), which is far less useful for health dashboards and cross-client debugging than the full version string the binary prints for --version: semver plus git branch/short-SHA, target triple, and rustc version. That git/rustc build metadata is emitted by the binary's build.rs (vergen-git2) and is only visible to that crate at compile time, so the net/rpc crate can't reconstruct it. Carry the binary's CLIENT_VERSION in RpcConfig and have the node/identity route report it.
MegaRedHand
force-pushed
the
feat/lean-api-node
branch
from
6d58cb2 to
e9bff5e
Compare
get_syncing recomputed is_syncing as a stateless snapshot (sync_distance > SYNC_LAG_THRESHOLD), ignoring the hysteresis and network-stall override in the actor's SyncStatusTracker. The endpoint could therefore report syncing while the node considered itself synced (and vice versa), disagreeing with both duty gating and the lean_node_sync_status metric. Read the status back from the metric instead (new metrics::node_sync_status getter) so is_syncing equals lean_node_sync_status by construction. sync_distance stays a raw store-vs-wall-clock snapshot for information; the docs now note the two can differ near the threshold or during a network stall.
The previous commit read is_syncing back from the lean_node_sync_status metric; using a metric as application state is a smell. Introduce SyncStatusController (Arc<AtomicU8>, mirroring AggregatorController): the blockchain actor writes it each tick from the same SyncStatus it feeds the metric, and the RPC /lean/v0/node/syncing endpoint reads it via an Extension. The SyncStatusTracker remains the single source of truth; is_syncing, the duty gate, and the metric all derive from it, so they cannot disagree. Drops the metrics::node_sync_status getter added previously.
Resolve crates/net/rpc/src/lib.rs: keep node::routes(version) alongside the genesis::routes()/spec::routes() added on main. Update the spec route test for the two-arg build_api_router(store, version) signature.
🤖 Kimi Code ReviewThe PR is well-structured and correct. A few minor observations: Code Quality & Safety
Testing
Documentation
Nit
Conclusion No security vulnerabilities or consensus risks identified. The shared Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
I didn’t see other substantive correctness, security, performance, or Rust-idiom issues in this diff. The PR does not touch fork choice, attestation validation, justification/finalization, STF, XMSS verification, or SSZ logic directly. Verification note: I could not run the Rust tests in this environment because Cargo/rustup wants writable home/cache paths and network-backed dependency resolution, which are blocked here. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewGood, this confirms the pattern is correctly wired. I now have a complete picture of the PR. Review:
|
The node/identity endpoint reported only the client version; add the node's libp2p peer ID so health/orchestration tooling can identify the node on the wire without scraping logs. Expose local_peer_id on BuiltSwarm (already derived from the node key in build_swarm), read it in main.rs after the swarm is built, and thread it into start_rpc_server as a String. It is threaded as a separate argument rather than via RpcConfig because it is derived after config construction and does not exist in test-driver mode (which skips the node key).
build_api_router grew to three args (store, version, peer_id), and ~15 tests that don't care about identity repeated the placeholder version and peer_id. Add test_utils::test_api_router(store) that fills those in, and route the don't-care call sites through it. Tests that assert on the identity values still call build_api_router directly.
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.