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 |
c67250f to
8fc2a3f
Compare
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 implements the REST/SSZ engine API proposed in execution-apis PR #793, adding a new sub-router mounted under
Confidence Score: 4/5The new REST/SSZ engine endpoint layer is well-structured; JWT auth is uniformly applied, body-size limits are respected, and the core forkchoice/newPayload logic is reused from the existing JSON-RPC path rather than duplicated. The three flagged items are minor: a missing RFC 7235 response header, an implicit contract in a now-optional validation parameter, and a middleware ordering quirk that allows unauthenticated requests to reset the CL activity watchdog. The changes are almost entirely additive and delegate to well-tested internal helpers. The most subtle change — making expected_blob_versioned_hashes optional — is intentional per the REST spec and does not weaken block integrity since the block hash check still applies. The timer-reset ordering issue is cosmetic in a local-only auth port context. No data-loss, auth bypass, or correctness regressions were found in the core paths. crates/networking/rpc/rpc.rs (middleware layering order), crates/networking/rpc/engine_rest/auth.rs (WWW-Authenticate header), crates/networking/rpc/engine/payload.rs (optional blob hash validation contract)
|
| Filename | Overview |
|---|---|
| crates/networking/rpc/engine_rest/auth.rs | New JWT bearer auth middleware; correctly validates token and captures X-Engine-Client-Version, but omits the RFC 7235-required WWW-Authenticate header in 401 responses. |
| crates/networking/rpc/engine_rest/mod.rs | Builds the engine REST sub-router under /engine/v2, wiring all fork-scoped and unscoped routes with JWT auth applied uniformly. |
| crates/networking/rpc/engine_rest/handlers/payloads.rs | POST /{fork}/payloads and GET /{fork}/payloads/{id}; fork-boundary and BAL structural checks look correct; REST callers bypass blob versioned-hash validation intentionally. |
| crates/networking/rpc/engine_rest/handlers/forkchoice.rs | POST /{fork}/forkchoice; maps SSZ fork-specific updates to internal types, delegates to handle_forkchoice, and builds payloads only when head is known. |
| crates/networking/rpc/engine_rest/handlers/bodies.rs | bodies-by-hash and bodies-by-range; correctly enforces BODIES_MAX_COUNT, truncates range at chain head, marks cross-fork-era entries as unavailable. |
| crates/networking/rpc/engine_rest/handlers/blobs.rs | /blobs/v1-v4 semantics (per-entry, all-or-nothing, partial, always-204) all look correct per spec. |
| crates/networking/rpc/engine_rest/types/conversions.rs | Per-fork SSZ envelope to Block + EngineCall conversion; BAL RLP decoding, requests hash, and V5 structural checks mirror the JSON-RPC path correctly. |
| crates/networking/rpc/engine/payload.rs | Refactored to accept expected_block_hash directly and made expected_blob_versioned_hashes Optional for REST callers; JSON-RPC callers still pass Some(...). |
| crates/networking/rpc/rpc.rs | Mounts /engine/v2 on the auth port and resets the CL activity watchdog for REST requests; timer-reset fires before JWT auth due to middleware layering order. |
| crates/networking/rpc/engine_rest/extractors.rs | SSZ body extractor with with_limited_body() honouring DefaultBodyLimit; LengthLimitError detection via error-chain walk is correct. |
| crates/networking/rpc/engine_rest/types/common.rs | Fork-invariant SSZ types and Optional[T] = List[T,1] helpers; constants and wire layout correct. |
| crates/blockchain/blockchain.rs | generate_bal_for_block now prefers stored BAL over re-execution, avoiding bulk re-execution on bodies range requests. |
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/networking/rpc/engine_rest/auth.rs:63-67
**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.
### Issue 2 of 3
crates/networking/rpc/engine/payload.rs:1028-1044
**Blob versioned-hash validation is silently skipped for REST callers**
`handle_new_payload_v3` now accepts `expected_blob_versioned_hashes: Option<Vec<H256>>`, and the REST path consistently passes `None`, bypassing the hash equality check that the JSON-RPC path performs. This is intentional per the REST spec (execution-apis PR #793 omits `expectedBlobVersionedHashes`); the block-hash check still provides end-to-end integrity. However, if any future caller accidentally passes `None` for a fork that should validate blobs, the validation will silently vanish. Consider adding a doc comment that `None` is only valid for transports that carry no `expectedBlobVersionedHashes`.
### Issue 3 of 3
crates/networking/rpc/rpc.rs:598-612
**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.
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
| }; | ||
|
|
||
| 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.|
|
||
| async fn handle_new_payload_v3( | ||
| payload: &ExecutionPayload, | ||
| pub(crate) async fn handle_new_payload_v3( | ||
| expected_block_hash: H256, | ||
| context: RpcApiContext, | ||
| block: Block, | ||
| expected_blob_versioned_hashes: Vec<H256>, | ||
| expected_blob_versioned_hashes: Option<Vec<H256>>, | ||
| bal: Option<BlockAccessList>, | ||
| ) -> Result<PayloadStatus, RpcErr> { | ||
| // V3 specific: validate blob hashes | ||
| let blob_versioned_hashes: Vec<H256> = block | ||
| .body | ||
| .transactions | ||
| .iter() | ||
| .flat_map(|tx| tx.blob_versioned_hashes()) | ||
| .collect(); | ||
|
|
||
| if expected_blob_versioned_hashes != blob_versioned_hashes { | ||
| return Ok(PayloadStatus::invalid_with_err( | ||
| "Invalid blob_versioned_hashes", | ||
| )); | ||
| // V3 specific: validate blob hashes (skipped when None, e.g. REST callers) | ||
| if let Some(expected) = expected_blob_versioned_hashes { | ||
| let blob_versioned_hashes: Vec<H256> = block | ||
| .body | ||
| .transactions | ||
| .iter() | ||
| .flat_map(|tx| tx.blob_versioned_hashes()) | ||
| .collect(); | ||
|
|
There was a problem hiding this comment.
handle_new_payload_v3 now accepts expected_blob_versioned_hashes: Option<Vec<H256>>, and the REST path consistently passes None, bypassing the hash equality check that the JSON-RPC path performs. This is intentional per the REST spec (execution-apis PR #793 omits expectedBlobVersionedHashes); the block-hash check still provides end-to-end integrity. However, if any future caller accidentally passes None for a fork that should validate blobs, the validation will silently vanish. Consider adding a doc comment that None is only valid for transports that carry no expectedBlobVersionedHashes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/engine/payload.rs
Line: 1028-1044
Comment:
**Blob versioned-hash validation is silently skipped for REST callers**
`handle_new_payload_v3` now accepts `expected_blob_versioned_hashes: Option<Vec<H256>>`, and the REST path consistently passes `None`, bypassing the hash equality check that the JSON-RPC path performs. This is intentional per the REST spec (execution-apis PR #793 omits `expectedBlobVersionedHashes`); the block-hash check still provides end-to-end integrity. However, if any future caller accidentally passes `None` for a fork that should validate blobs, the validation will silently vanish. Consider adding a doc comment that `None` is only valid for transports that carry no `expectedBlobVersionedHashes`.
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!
| 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.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.
| } | ||
| } | ||
| 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).
| # 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.
# Conflicts: # crates/blockchain/blockchain.rs # crates/networking/rpc/engine/payload.rs
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.
Benchmarks