Skip to content

feat(rpc): add GET /lean/v0/config/spec#456

Open
MegaRedHand wants to merge 6 commits into
mainfrom
feat/lean-api-spec
Open

feat(rpc): add GET /lean/v0/config/spec#456
MegaRedHand wants to merge 6 commits into
mainfrom
feat/lean-api-spec

Conversation

@MegaRedHand

Copy link
Copy Markdown
Collaborator

Adds the GET /lean/v0/config/spec endpoint, which exposes chain configuration parameters (slot duration, committee sizes, finality constants, etc.) as JSON.

Allows external tools and cross-client integrations to discover runtime config without out-of-band coordination. Has unit tests and passed clippy.

Stacked on #454.

@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

File: crates/net/rpc/src/spec.rs

Critical: Hardcoded Fork Digest

  • Line 12: The FORK_DIGEST constant is hardcoded as "12345678" (dummy value). Fork digests are critical for chain identification in consensus networking—incorrect values will cause peers to reject messages or partition the node. This should be dynamically derived from genesis_validators_root and the current fork version per the consensus spec, not hardcoded. If this is intentional for early prototyping, add a TODO or compile-time warning to prevent production usage.

Safety: Unchecked Type Cast

  • Line 24: HISTORICAL_ROOTS_LIMIT as u64 assumes the source constant fits in u64. If the upstream type changes to usize (e.g., on 32-bit targets) or a larger integer, this silently truncates. Use u64::try_from() or ensure the source crate exports the constant as u64 explicitly.

Maintenance Burden

  • Line 10-11: The comment indicates FORK_DIGEST must be kept in sync with ethlambda_p2p. This violates DRY and risks drift. Move this constant to a shared configuration crate (e.g., ethlambda_types::config) that both rpc and p2p can import.

Testing Gap

  • Lines 70-72: The test asserts on timing constants but omits HISTORICAL_ROOTS_LIMIT. Add:
    assert_eq!(json["HISTORICAL_ROOTS_LIMIT"], HISTORICAL_ROOTS_LIMIT as u64);

Minor: Unnecessary Allocation

  • Line 31: FORK_DIGEST.to_string() allocates on every request. Since the response owns the data anyway, consider changing the struct field to &'static str or using Cow<'static, str> to avoid the allocation. Given this is a low-frequency config endpoint, this is merely stylistic.

Architecture Note
Consider grouping these constants into a structured ChainConfig type in the blockchain crate rather than importing them piecemeal from different modules (blockchain, types). This centralizes the protocol configuration and makes it easier to support multiple networks (mainnet, testnets, devnets).

File: crates/net/rpc/src/lib.rs

  • Line 101: Route registration looks correct. Ensure the path /lean/v0/config/spec aligns with your API versioning strategy.

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

  1. crates/net/rpc/src/spec.rs:9-10 introduces a second hard-coded FORK_DIGEST, while the networking stack already defines the real source used for gossipsub topics at crates/net/p2p/src/gossipsub/messages.rs:8. That creates a split-brain config surface: when the digest changes in p2p, /lean/v0/config/spec can silently keep returning the old value, and the new test at crates/net/rpc/src/spec.rs:56-75 would still pass because it only checks the duplicate. For a consensus client, exposing stale network identity is an interoperability bug. This should come from a shared constant/module, not a copied literal.

  2. crates/net/rpc/src/spec.rs:72-75 never asserts the new HISTORICAL_ROOTS_LIMIT field even though it is part of the added response at crates/net/rpc/src/spec.rs:20-21,31. That leaves one of the endpoint’s exported consensus constants untested and makes regressions easy to miss. Add an assertion for json["HISTORICAL_ROOTS_LIMIT"].

No consensus-path code was changed here, so I did not find issues in fork choice, attestation validation, STF, XMSS, or SSZ logic from this diff itself. I could not run the targeted Rust test in this sandbox because cargo needs write access to the default Cargo/Rustup locations and git-fetched dependencies.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@greptile-apps

greptile-apps Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds the GET /lean/v0/config/spec endpoint, which returns key protocol constants (MILLISECONDS_PER_SLOT, INTERVALS_PER_SLOT, MILLISECONDS_PER_INTERVAL, HISTORICAL_ROOTS_LIMIT, FORK_DIGEST) as JSON so external tools can discover them without out-of-band coordination.

  • Shared constant: FORK_DIGEST is moved from ethlambda_p2p into a new ethlambda_types::constants module and re-exported in both crates, eliminating the previously flagged duplication risk.
  • New handler: crates/net/rpc/src/spec.rs wires the SpecResponse struct via Axum and registers the route; a tokio::test integration test exercises all five response fields end-to-end.
  • Docs: docs/rpc.md is updated with the route table entry and a JSON example.

Confidence Score: 5/5

Safe to merge — the change is additive, all constants come from compile-time definitions, and the unit test covers the full response shape.

The handler is read-only and stateless, introduces no new mutable state or side effects, and the previously flagged constant duplication has been properly resolved by moving FORK_DIGEST to the shared types crate.

No files require special attention.

Important Files Changed

Filename Overview
crates/common/types/src/constants.rs New shared constants module; moves FORK_DIGEST out of the p2p crate so it can be imported by both the p2p and rpc crates without duplication.
crates/common/types/src/lib.rs One-line addition registering the new constants module; no issues.
crates/net/p2p/src/gossipsub/messages.rs Replaces the local FORK_DIGEST definition with a re-export from ethlambda_types::constants, resolving the previous synchronisation concern.
crates/net/rpc/src/lib.rs Registers the new spec module and merges its routes into the API router; straightforward wiring.
crates/net/rpc/src/spec.rs New GET /lean/v0/config/spec handler; all five constants (including HISTORICAL_ROOTS_LIMIT) are asserted in the unit test. No issues found.
docs/rpc.md Adds the new endpoint to the route table and documents its response shape with example values; consistent with the implementation.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Client
    participant RPC as ethlambda-rpc
    participant Types as ethlambda-types
    participant Blockchain as ethlambda-blockchain

    Client->>RPC: GET /lean/v0/config/spec
    RPC->>Types: FORK_DIGEST (constants::FORK_DIGEST)
    RPC->>Types: HISTORICAL_ROOTS_LIMIT (state::HISTORICAL_ROOTS_LIMIT)
    RPC->>Blockchain: MILLISECONDS_PER_SLOT, INTERVALS_PER_SLOT, MILLISECONDS_PER_INTERVAL
    RPC-->>Client: 200 OK JSON response with all 5 constants
Loading
%%{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 ethlambda-rpc
    participant Types as ethlambda-types
    participant Blockchain as ethlambda-blockchain

    Client->>RPC: GET /lean/v0/config/spec
    RPC->>Types: FORK_DIGEST (constants::FORK_DIGEST)
    RPC->>Types: HISTORICAL_ROOTS_LIMIT (state::HISTORICAL_ROOTS_LIMIT)
    RPC->>Blockchain: MILLISECONDS_PER_SLOT, INTERVALS_PER_SLOT, MILLISECONDS_PER_INTERVAL
    RPC-->>Client: 200 OK JSON response with all 5 constants
Loading

Reviews (2): Last reviewed commit: "refactor(types): centralize FORK_DIGEST ..." | Re-trigger Greptile

Comment thread crates/net/rpc/src/spec.rs Outdated
Comment thread crates/net/rpc/src/spec.rs
@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Here is the review:


Review: feat(rpc): add GET /lean/v0/config/spec

This PR adds a stateless GET /lean/v0/config/spec endpoint that serializes compile-time consensus constants (slot timing, HISTORICAL_ROOTS_LIMIT, fork digest) as JSON, enabling external tools and cross-client integrations to discover runtime configuration. The implementation is clean and follows the existing handler/router pattern correctly. Two confirmed issues warrant fixes before merge.


Findings

crates/net/rpc/src/spec.rs:10FORK_DIGEST is silently duplicated from ethlambda-p2p; no compile-time check enforces equality

ethlambda_p2p::gossipsub::messages::FORK_DIGEST is the canonical pub const and already has a doc-comment tracking the TODO. spec.rs declares its own private copy with a // keep in sync comment. The rpc crate does not depend on ethlambda-p2p, so a direct import isn't available today — but ethlambda-types is already a shared dependency of both crates, making it the right home for this constant. If someone updates the p2p constant when the real fork digest is introduced and misses this copy, the API endpoint will advertise a stale FORK_DIGEST while gossipsub topics use the updated one, silently breaking cross-client topic subscriptions with no build error.

Suggested fix: move FORK_DIGEST to ethlambda-types and import it from there in both ethlambda-p2p and ethlambda-rpc.


crates/net/rpc/src/spec.rs:72 — test spec_returns_lean_constants omits an assertion for HISTORICAL_ROOTS_LIMIT

The test checks MILLISECONDS_PER_SLOT, INTERVALS_PER_SLOT, MILLISECONDS_PER_INTERVAL, and FORK_DIGEST, but never asserts json["HISTORICAL_ROOTS_LIMIT"]. The field is present in SpecResponse and populated in get_spec, but dropping or mistyping the serde(rename) attribute would leave the test green while the response silently regresses.

Suggested fix: add assert_eq!(json["HISTORICAL_ROOTS_LIMIT"], HISTORICAL_ROOTS_LIMIT as u64); at line 75.


crates/net/rpc/src/spec.rs:22fork_digest: String allocates per request for a compile-time constant (low severity)

FORK_DIGEST.to_string() allocates a new heap String on every request for a value that is a &'static str. serde serializes &str and String identically in JSON. Changing the field type to fork_digest: &'static str and assigning FORK_DIGEST directly eliminates the allocation with no behavioral change.


The HISTORICAL_ROOTS_LIMIT as u64 cast is safe — the project targets 64-bit only and the value (262,144) is far below any overflow threshold. The async fn get_spec() without .await is correct; Axum's Handler trait requires async fn even for synchronous responses.


Automated review by Claude (Anthropic) · sonnet · custom prompt

Base automatically changed from feat/lean-api-router-scaffold to main June 24, 2026 19:10
@MegaRedHand MegaRedHand marked this pull request as draft June 25, 2026 19:58
# Conflicts:
#	crates/net/rpc/src/lib.rs
#	docs/rpc.md
The dummy fork digest was duplicated as a private const in the p2p
gossipsub topic builder and again in the new config/spec RPC handler,
each with a comment warning the other must be kept in sync. Move it to
ethlambda_types::constants::FORK_DIGEST so both re-use one definition.
@MegaRedHand MegaRedHand marked this pull request as ready for review July 3, 2026 20:37
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

🤖 Kimi Code Review

Overall this is a clean refactoring and well-tested feature addition. No security vulnerabilities or consensus risks identified.

Code Quality & Correctness

  • Good: Moving FORK_DIGEST to ethlambda_types::constants eliminates duplication and follows the crate's layering (networking → types).
  • Good: The SpecResponse struct uses #[serde(rename = "...")] to match leanSpec constant names exactly, ensuring API consistency.
  • Minor: HISTORICAL_ROOTS_LIMIT as u64 (line 25 in spec.rs) assumes the underlying type fits in u64. While 262144 is safely within range, consider defining the constant as u64 directly in the state module to avoid platform-dependent usize assumptions.

Rust Idioms

  • Good: Using &'static str for fork_digest in SpecResponse avoids allocation since FORK_DIGEST is a compile-time constant.
  • Good: The async fn get_spec is correctly implemented as an immediate-returning future (no unnecessary .await points).

Testing

  • Good: The integration test in spec.rs validates the full request/response cycle and checks all fields including the fork digest.
  • Note: The test imports FORK_DIGEST via use super::FORK_DIGEST (line 47) which relies on the module-level import. This is correct but consider importing directly from ethlambda_types::constants in tests to avoid relying on module re-exports.

Documentation

  • Good: The rpc.md update correctly documents the new endpoint and clarifies that FORK_DIGEST is a 4-byte hex string without 0x prefix.

Security

  • No concerns. The endpoint is read-only, returns public configuration constants, and performs no deserialization of untrusted input.

Recommendation
Approve with optional suggestion to verify HISTORICAL_ROOTS_LIMIT is defined as u64 upstream to remove the cast.


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

No blocking findings.

This PR is low risk: it centralizes FORK_DIGEST into a shared types module and adds a read-only RPC endpoint that only serializes existing constants. It does not modify fork choice, attestation validation, justification/finalization, state transition, XMSS handling, or SSZ encoding logic. The route wiring in crates/net/rpc/src/lib.rs, the constant extraction in crates/common/types/src/constants.rs, and the endpoint implementation in crates/net/rpc/src/spec.rs all look correct and idiomatic.

Assumptions and gaps:

  • I could not run the targeted Rust tests in this environment. The default cargo path is blocked by read-only ~/.rustup, and using the toolchain binary directly still hits a read-only ~/.cargo / dependency-fetch path.
  • Because of that, this review is source-only rather than execution-verified.

Change summary:

  • FORK_DIGEST is now shared from ethlambda_types, removing duplication between P2P and RPC.
  • /lean/v0/config/spec returns static protocol constants in a simple JSON shape with a focused regression test and matching docs.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

No route path conflicts, integration is clean. This is a small, well-scoped PR. Here's my review.

Review: feat(rpc): add GET /lean/v0/config/spec

Overall this is a clean, low-risk PR. The new endpoint is a straightforward static-constants responder, the FORK_DIGEST de-duplication is a sensible refactor, and docs/tests are updated consistently with existing conventions in the crate.

Correctness

No bugs found. All constant types line up correctly:

  • MILLISECONDS_PER_SLOT, INTERVALS_PER_SLOT, MILLISECONDS_PER_INTERVAL are already u64 in ethlambda-blockchain (crates/blockchain/src/lib.rs:47-51), so no cast needed.
  • HISTORICAL_ROOTS_LIMIT is usize (crates/common/types/src/state.rs:44); the as u64 cast (crates/net/rpc/src/spec.rs:28) is a widening cast and safe on all supported targets.
  • The FORK_DIGEST centralization (crates/common/types/src/constants.rs, crates/net/p2p/src/gossipsub/messages.rs:1) correctly preserves the original doc comment and value, and the p2p crate's re-export (pub use ethlambda_types::constants::FORK_DIGEST;) means no downstream callers in p2p needed changes.
  • Route registration in build_api_router (crates/net/rpc/src/lib.rs:106-113) has no path collisions with existing routes.

Security

No concerns — all exposed values (slot timing, historical roots limit, fork digest) are already public/derivable from the gossip protocol and genesis config, so this endpoint doesn't leak anything sensitive.

Minor suggestions (non-blocking)

  1. crates/net/rpc/src/spec.rs:9-19 — the five per-field #[serde(rename = "...")] attributes could be eliminated by renaming the struct fields to match leanSpec casing (e.g. milliseconds_per_slot, historical_roots_limit) and using a single #[serde(rename_all = "SCREAMING_SNAKE_CASE")] on the struct. Slightly less repetitive, same output.
  2. Scope — only 5 constants are exposed. If the intent (per the docs blurb "Protocol constants the node was built with") is eventually to mirror leanSpec's full config/spec surface (e.g. VALIDATOR_REGISTRY_LIMIT, GOSSIP_DISPARITY_INTERVALS, etc.), it may be worth a tracking issue/TODO so consumers know more fields are coming — not a blocker for this incremental addition.
  3. crates/net/rpc/src/spec.rs:39 — the test module imports items individually (use super::FORK_DIGEST; plus separate crate imports) rather than the use super::*; pattern used in the sibling genesis.rs tests. Purely stylistic; both work fine since this test exercises the full router via build_api_router rather than the sub-router directly (arguably a slightly better test since it also validates no route conflicts).

Nice touches

  • Testing through crate::build_api_router(store) (spec.rs:57) rather than just the local routes() sub-router is a good choice — it also implicitly verifies the new route doesn't collide with others.
  • The FORK_DIGEST refactor is exactly right: single source of truth, doc comment preserved, no behavior change.

No consensus-critical logic (fork choice, attestation processing, justification/finalization, XMSS, SSZ) is touched by this PR.


Automated review by Claude (Anthropic) · sonnet · custom prompt

@greptile-apps

greptile-apps Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Want your agent to iterate on Greptile's feedback? Try greploops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant