feat(l1): engine REST/SSZ API#6770
Conversation
Prototype implementation of the REST + SSZ engine API proposed in ethereum/execution-apis PR #793. Coexists with the existing JSON-RPC engine API on the same authrpc port (:8551) — JSON-RPC behavior is unchanged. Surface (all forks Paris → Amsterdam): GET /identity ClientVersionV1 array (JSON) GET /capabilities supported_forks + endpoint limits (JSON) POST /{fork}/payloads newPayload over SSZ GET /{fork}/payloads/{id} poll built payload (SSZ envelope) POST /{fork}/forkchoice forkchoice update + optional build trigger POST /{fork}/bodies/hash bodies by hash (per-fork shape) GET /{fork}/bodies?from=&count= bodies by range POST /blobs/v{1..4} blob + cell-proof retrieval Reuses the existing handle_new_payload_v* / handle_forkchoice helpers; SSZ handlers convert to internal Block + dispatch through the same business logic the JSON-RPC handlers use. Amsterdam BAL is regenerated via Blockchain::generate_bal_for_block (mirrors JSON-RPC V2 path). Measurement (tooling/engine_bench + crates/networking/rpc/benches): microbenches and an end-to-end harness compare JSON-RPC vs REST/SSZ encode/decode and end-to-end latency for newPayload, getPayload, blobs, bodies. SSZ wire size ~50% of JSON; SSZ encode ~250× faster. Known limitations (documented inline): - Amsterdam /blobs/v4 returns empty cell bytes (mempool lacks per-cell storage); proofs are populated where available. - blobs/v2 vs blobs/v3 all-or-nothing vs partial semantics share the same code path today (mempool lacks Osaka-aware partial state). - handle_new_payload_v3's expected_blob_versioned_hashes parameter was made Option<Vec<H256>>; JSON-RPC callers wrap in Some(_), REST callers pass None (the EL recomputes from txs per spec).
# Conflicts: # crates/common/Cargo.toml
|
Lines of code reportTotal lines added: Detailed view |
iovoid
force-pushed
the
feat/engine-rest-ssz-793
branch
from
c67250f to
8fc2a3f
Compare
iovoid
force-pushed
the
feat/engine-rest-ssz-793
branch
2 times, most recently
from
fc715ae to
30e847a
Compare
🤖 Kimi Code ReviewThis is a substantial and well-engineered PR implementing the REST/SSZ Engine API (execution-apis #793). The implementation is comprehensive, secure, and well-tested. Below are specific observations: Security & CorrectnessJWT Authentication (
Request Limits (
Fork Validation (
PerformanceBAL Retrieval Optimization (
SSZ vs JSON (
Code QualityError Handling (
Type Safety (
Refactoring (
Minor Observations
TestingThe test coverage is exemplary:
SummaryNo blocking issues. The PR correctly implements the spec with strong security boundaries (auth, size limits), efficient resource usage (stored BAL preference, blocking task offload), and comprehensive validation. The code is production-ready. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewNow I have a comprehensive picture of this PR. Let me compose the review. PR #6770 — feat(l1): engine REST/SSZ APIOverviewThis PR implements the Engine REST/SSZ API per execution-apis PR #793. It adds a new The PR is well-structured, has good test coverage, and the intent behind each design decision is clearly documented. The concerns below are mostly correctness and edge-case issues rather than architectural ones. Correctness Issues1. Blob versioned hash validation silently skipped for all REST Cancun/Prague payloads (
EngineCall::V3 { .. } => {
handle_new_payload_v3(expected_block_hash, ctx, block, None, None).await
}
EngineCall::V4 { .. } => {
handle_new_payload_v3(expected_block_hash, ctx, block, None, None).await
}The code comment acknowledges this ( 2.
let proofs_ssz = match proofs.into_iter().collect::<Vec<_>>().try_into() {
Ok(p) => p,
Err(_) => return StatusCode::NO_CONTENT.into_response(),
};
3. Error message in return Err(RpcErr::Internal(format!(
"Invalid block hash. Expected {actual_block_hash:#x}, got {expected:#x}"
)));
4. Query parameter parse errors in #[derive(Deserialize)]
pub struct BodiesRangeParams {
pub from: Option<u64>,
pub count: Option<u64>,
}
pub async fn bodies_by_range(
...
Query(params): Query<BodiesRangeParams>,
) -> Response {If the CL sends Security Notes5. const BEARER_PREFIX: &str = "Bearer ";
let token = match auth_str.get(..BEARER_PREFIX.len()) {
Some(prefix) if prefix.eq_ignore_ascii_case(BEARER_PREFIX) => {
&auth_str[BEARER_PREFIX.len()..]
}The comment says "BEARER_PREFIX is ASCII, so byte index 7 is also char index 7". This is true and the code is correct. Consider adding a 6. let body = serde_json::to_vec(&self).expect("ProblemJson serializes");
let body = serde_json::to_vec(&self).unwrap_or_else(|_| br#"{"status":500}"#.to_vec());would be more resilient to future field additions that accidentally make the type unserializable. Performance Notes7. 131 KiB blob allocation on every match blob.as_ref().to_vec().try_into() {
Ok(blob_ssz) => BlobV1Entry::available(BlobAndProofV1 { blob: blob_ssz, proof: proofs[0] }),
8. Block hash recomputed repeatedly during In the block_hash: block.hash().0,
Minor Issues9. The existing // current
for (i, byte) in bytes.iter_mut().enumerate() {
*byte = u8::from_str_radix(&hex[i * 2..i * 2 + 2], 16)
.map_err(|e| format!("invalid hex: {e}"))?;
}
// simpler
let decoded = hex::decode(hex).map_err(|e| format!("invalid hex: {e}"))?;
decoded.try_into().map_err(|_| "...".to_string()).map(Self)10. pub const MAX_BLOCK_ACCESS_LIST_BYTES: usize = 16_777_216; // 16 MiBOther constants cite their spec sources (EIP numbers, refactor-ssz.md). This one does not. Add a reference to EIP-7928 or the #793 limit table so reviewers can verify the value. 11. _ => unreachable!("ForkPath restricts to spec forks"),The invariant is maintained by 12. The deps moved from optional (gated behind Positive Highlights
Automated review by Claude (Anthropic) · sonnet · custom prompt |
Greptile SummaryThis PR adds an engine REST/SSZ API alongside the existing engine JSON-RPC path. The main changes are:
Confidence Score: 4/5This is close, but the auth/watchdog ordering should be fixed before merging.
crates/networking/rpc/rpc.rs
|
| Filename | Overview |
|---|---|
| crates/networking/rpc/rpc.rs | Mounts the REST router under the auth RPC server and adds a REST watchdog reset layer that currently runs before JWT auth. |
| crates/networking/rpc/engine_rest/auth.rs | Adds JWT middleware for REST requests and captures the engine client version header. |
| crates/networking/rpc/engine/payload.rs | Refactors newPayload helpers so REST callers can pass the expected block hash directly. |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
crates/networking/rpc/rpc.rs:617
**Watchdog resets before auth**
This middleware sends the consensus-layer activity signal before the REST JWT middleware can reject the request. The REST router already applies JWT auth internally, and this outer layer wraps that router, so a request to `/engine/v2/...` with a missing or invalid token still reaches `timer.send(())` before returning 401. A process that can reach the auth RPC port can keep the CL inactivity warning quiet while the real consensus client is disconnected. Move this reset after JWT validation succeeds, or have the auth middleware trigger it only for authenticated REST requests.
Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }; | ||
|
|
||
| if let Err(err) = validate_jwt_authentication(token, &secret) { | ||
| debug!("engine REST auth rejected: {err:?}"); | ||
| return ProblemJson::unauthorized("JWT validation failed").into_response(); |
There was a problem hiding this comment.
WWW-Authenticate header in 401 responses
RFC 7235 §3.1 states that a server MUST include at least one WWW-Authenticate challenge when responding with 401 Unauthorized. The current ProblemJson::unauthorized(...) response omits this header entirely. Some HTTP client stacks treat a 401 without WWW-Authenticate as malformed and may not surface it correctly to the CL. A minimal header like WWW-Authenticate: Bearer realm="engine" would satisfy the RFC.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/engine_rest/auth.rs
Line: 63-67
Comment:
**Missing `WWW-Authenticate` header in 401 responses**
RFC 7235 §3.1 states that a server MUST include at least one `WWW-Authenticate` challenge when responding with 401 Unauthorized. The current `ProblemJson::unauthorized(...)` response omits this header entirely. Some HTTP client stacks treat a 401 without `WWW-Authenticate` as malformed and may not surface it correctly to the CL. A minimal header like `WWW-Authenticate: Bearer realm="engine"` would satisfy the RFC.
How can I resolve this? If you propose a fix, please make it concise.| let engine_rest_router = | ||
| crate::engine_rest::router(service_context.clone()).layer(axum::middleware::from_fn( | ||
| move |req: axum::extract::Request, next: axum::middleware::Next| { | ||
| let timer = rest_timer_sender.clone(); | ||
| async move { | ||
| let _ = timer.send(()); | ||
| next.run(req).await | ||
| } | ||
| }, | ||
| )); | ||
|
|
||
| let authrpc_router = Router::new() | ||
| .route("/", post(authrpc_handler)) | ||
| .with_state(service_context.clone()) | ||
| // Engine REST/SSZ (#793) is scoped under /engine/v2/... per refactor.md; |
There was a problem hiding this comment.
The timer-reset from_fn middleware is layered outside engine_rest::router, while the JWT auth middleware lives inside it. In axum's execution model outer layers run first, so the watchdog timer fires for every request on the auth port — including those with missing, expired, or forged JWT tokens — before credentials are checked. An unauthenticated process on the local machine could therefore prevent the "No messages from the consensus layer" warning from ever triggering. Consider placing the timer-reset layer inside the router after the JWT auth middleware.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/rpc.rs
Line: 598-612
Comment:
**CL activity timer is reset before JWT auth is verified**
The timer-reset `from_fn` middleware is layered *outside* `engine_rest::router`, while the JWT auth middleware lives *inside* it. In axum's execution model outer layers run first, so the watchdog timer fires for every request on the auth port — including those with missing, expired, or forged JWT tokens — before credentials are checked. An unauthenticated process on the local machine could therefore prevent the "No messages from the consensus layer" warning from ever triggering. Consider placing the timer-reset layer inside the router after the JWT auth middleware.
How can I resolve this? If you propose a fix, please make it concise.
iovoid
force-pushed
the
feat/engine-rest-ssz-793
branch
from
30e847a to
bad5c93
Compare
| // only need the expected block_hash from the payload, so we pass it | ||
| // directly and skip the `JsonExecutionPayload::from_block` intermediate. | ||
| let result = match call { | ||
| EngineCall::V1V2 => handle_new_payload_v1_v2(expected_block_hash, block, ctx, None).await, |
There was a problem hiding this comment.
REST path drops the expectedBlobVersionedHashes cross-check that the JSON-RPC NewPayloadV3/V4/V5 path runs (engine/payload.rs:142, 265, 408 now pass Some(self.expected_blob_versioned_hashes.clone()); REST passes None here and on V3/V4/V5 dispatch below).
If #793 intentionally removed the field from the SSZ envelope on the theory that the EL can derive blob hashes from the transactions itself, that's a design choice and this is correct — but the safety net that catches a CL miscomputing blob hashes is gone on this transport. Worth confirming with the spec and adding a one-line comment here noting why we pass None (it currently reads as forgotten rather than intentional). If the spec actually still requires the check via a header or trailer, the right place to enforce it is right here.
There was a problem hiding this comment.
is the SSZ envelope explicitly without expectedBlobVersionedHashes per the spec? If so, +1 — could you add a one-liner at the dispatch site mirroring the helper's comment?
| } | ||
| } | ||
| Fork::Amsterdam => { | ||
| let mut entries: Vec<BodyEntryAmsterdam> = Vec::with_capacity(blocks.len()); |
There was a problem hiding this comment.
Amsterdam bodies_by_hash / bodies_by_range does spawn_blocking per block in sequence — for BODIES_MAX_COUNT = 32 that's 32 serialized blocking-task hops. With the new stored-BAL fast path (generate_bal_for_block reads get_block_access_list first), each one is cheap, so the cost in steady state is mostly dispatch overhead. But on snap-synced nodes where pre-Amsterdam-cutover blocks lack a stored BAL, every miss falls back to full re-execution serially — a single missing BAL in a 32-block range can multi-second the response.
Not blocking — BAL persistence is the long-term answer. Two cheap mitigations to consider: (a) batch the 32 spawn_blocking dispatches into a single one that walks the slice; (b) for the re-execution fallback path, fail fast with 503 Service Unavailable instead of stalling the response (the CL can retry against another EL or drop the body request).
There was a problem hiding this comment.
Fixed, now we check store.get_block_access_list(hash)
| # libssz-merkle provides the `HashTreeRoot` trait used by the SSZ derives | ||
| # in engine_rest/types/. The libssz-derive macros expand to impls that | ||
| # reference this trait, so it must be a direct unconditional dep. | ||
| libssz-merkle = { workspace = true } |
There was a problem hiding this comment.
These four ssz crates were previously optional = true, eip-8025-gated; now all four are unconditional rpc-crate deps. That's necessary (engine_rest is unconditional in lib.rs), but it does mean every default rpc build picks them up. The added compile time isn't dramatic, but the change widens the dependency surface for non-eip-8025 builds (think make dev, EF-tests, etc.). Worth confirming that's OK — the alternative is feature-gating pub mod engine_rest; in lib.rs behind a feature so consumers who don't need REST/SSZ don't pay for it.
There was a problem hiding this comment.
The libssz crate isn't a problem, it has a small footprint.
# Conflicts: # crates/blockchain/blockchain.rs # crates/networking/rpc/engine/payload.rs
iovoid
force-pushed
the
feat/engine-rest-ssz-793
branch
from
422b38e to
7c006d8
Compare
iovoid
force-pushed
the
feat/engine-rest-ssz-793
branch
from
f6da49b to
3b700ee
Compare
# Conflicts: # crates/networking/rpc/Cargo.toml
| move |req: axum::extract::Request, next: axum::middleware::Next| { | ||
| let timer = rest_timer_sender.clone(); | ||
| async move { | ||
| let _ = timer.send(()); |
There was a problem hiding this comment.
This middleware sends the consensus-layer activity signal before the REST JWT middleware can reject the request. The REST router already applies JWT auth internally, and this outer layer wraps that router, so a request to /engine/v2/... with a missing or invalid token still reaches timer.send(()) before returning 401. A process that can reach the auth RPC port can keep the CL inactivity warning quiet while the real consensus client is disconnected. Move this reset after JWT validation succeeds, or have the auth middleware trigger it only for authenticated REST requests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/rpc.rs
Line: 617
Comment:
**Watchdog resets before auth**
This middleware sends the consensus-layer activity signal before the REST JWT middleware can reject the request. The REST router already applies JWT auth internally, and this outer layer wraps that router, so a request to `/engine/v2/...` with a missing or invalid token still reaches `timer.send(())` before returning 401. A process that can reach the auth RPC port can keep the CL inactivity warning quiet while the real consensus client is disconnected. Move this reset after JWT validation succeeds, or have the auth middleware trigger it only for authenticated REST requests.
How can I resolve this? If you propose a fix, please make it concise.Migrate the REST/SSZ engine API to the latest execution-apis #793 head and prysm #16901: - mount /engine/v2 -> /engine/v1; drop the /{fork} URL segment - select the fork via the Eth-Execution-Version request header (fork_path.rs::ForkPath -> fork_header.rs::ExecutionVersion) - RFC 7807 errors now carry only `type` (relative /engine-api/errors/ URI) + optional `detail`; invalid-forkchoice corrected to 409 - wire Amsterdam payload_attributes.target_gas_limit through FCUv4 - update engine_bench tooling + integration tests to the new scheme fix(l1): reset the CL-activity watchdog only after engine REST JWT auth succeeds (was reset before auth, letting an unauthenticated local caller keep the "no messages from the consensus layer" warning silenced).
2 participants
Motivation
We want to reduce the overhead of JSON serialization/deserialization for the large objects handled by the engine API. One of the proposals to do so is ethereum/execution-apis#793
Description
Most of the PR consists of boilerplate: defining RPC objects, route handlers and serialization/deserialization logic.
For simplicity, we only support forks starting at Fusaka.
Benchmarks