@claude review

Claude finished @LukaszRozmej's task in 7m 35s —— View job

PR Review: feat(engine): implement Engine API v2 REST+SSZ surface per spec

• Gather context and understand the request
• Read rule files (coding-style, robustness, performance, di-patterns)
• Read key changed files
• Analyze findings
• Post inline comments for specific issues
• Post final review summary

Summary

Large, well-structured feature implementing the Engine API v2 REST+SSZ surface. Architecture is clean — the generic descriptor pattern (INewPayloadVersion<TWire>, IForkchoiceUpdatedVersion<TWire>, etc.) means adding a new fork is genuinely one struct + one DI line, and the middleware routing is straightforward. The SSZ codec is stateless and correctly separates encoding from transport.

Several issues need attention before merge.

Findings

High

1. Engine API port forced to HTTP/2 only — breaks existing HTTP/1.1 clients
Startup.cs:110

HttpProtocols.Http2 makes Kestrel refuse HTTP/1.1 on the engine port. Any CL client or tooling that connects via HTTP/1.1 (which the spec allows as a SHOULD, not MUST) will get a connection-level protocol error. This needs HttpProtocols.Http1AndHttp2 to preserve backward compatibility, or an explicit migration guide.

2. IndexOutOfRangeException caught as ssz-decode-error — too broad
SszMiddleware.cs:248

IndexOutOfRangeException can be thrown by any out-of-bounds access in handler logic (not just SSZ decoding). Catching it here converts a server bug into a 400 ssz-decode-error with a canned message, completely hiding the real problem. This should be removed from the catch clause; ordinary handler exceptions fall through to the generic 500 path below.

Medium

3. Two new JsonSerializerOptions allocations per request
SszMiddleware.cs:157 · ClientVersionSszHandler.cs:35

JsonSerializerOptions should be constructed once (static readonly). Creating a new instance per call allocates internal caches on every request. Affects two hot-path sites.

4. Empty bare catch block silently swallows exceptions
SszMiddleware.cs:161

Violates the repo robustness rule. The header-parsing catch should at minimum write a trace-level log so malformed payloads or bugs surface during debugging.

5. Hard-coded fork name list in TryRoute
SszMiddleware.cs:340

"paris" or "shanghai" or ... or "amsterdam" is a second place where the full fork list lives (the first is CapabilitiesSszHandler). Adding a new fork requires updating both sites independently. A shared static set or SszRestPaths-derived constant would prevent divergence.

6. 134217728 magic number in capabilities response
CapabilitiesSszHandler.cs:37

This is SszMiddleware.MaxBodySize. If the limit is ever changed, the advertised capability will silently lie to CLs. Reference the constant directly.

7. GetBlobsV4SszHandler missing from middleware test setup
SszMiddlewareTests.cs:103

POST /engine/v2/blobs/v4 has no route-level test coverage in SszMiddlewareTests. The handler is registered in the configurer but absent from BuildMiddleware.

Low

8. ForkActivation(long.MaxValue - 1, timestamp) should be ForkActivation.TimestampOnly(timestamp)
ForkchoiceUpdatedSszHandler.cs:37

ForkActivation.TimestampOnly is the established idiom for timestamp-only spec lookup. Using long.MaxValue - 1 as a sentinel block number is a magic value that could behave unexpectedly in some spec providers.

9. SszCodec.EncodeForkchoiceUpdatedResponse — small heap allocation avoidable
SszCodec.cs:45

byte[] idBytes = new byte[8] is used only transiently before being stored in SszPayloadId. Since Bytes.FromHexString can target a Span<byte>, this could be stackalloc byte[8] with the array allocated only after the hex decode succeeds.

10. SszCodec.GetBlobVersionedHashes — list pre-sizing
SszCodec.cs:371

List<Hash256> list = [] will resize multiple times for typical blobs-bearing payloads. Pre-sizing from transaction count would reduce allocations on this code path (called for every V3+ new payload).

What looks good

• Generic descriptor + one-DI-line extension model is solid and spec-aligned
• SszEndpointHandlerBase.WriteSszAsync correctly encodes into the response PipeWriter without an intermediate buffer, and the debug assert catches encoder length mismatches early
• ReadBodyAsync correctly handles both known-length and chunked bodies, with MaxBodySize enforced in both paths
• GetPayloadSszHandler correctly sets Cache-Control: no-store per spec
• The ForkchoiceStateWire.SafeBlockHash/FinalizedBlockHash field ordering matches the on-wire spec order
• RFC 7807 error model (type URI + optional detail, no status field) is correctly implemented
• SszBlobCell uses [StructLayout(LayoutKind.Sequential, Size = 1024)] for safe zero-copy cell representation

Deep Review — Engine API v2 REST+SSZ vs spec (execution-apis#793)

I cross-checked the implementation against the authoritative spec (ethereum/execution-apis#793 — both refactor.md and refactor-ssz.md). The implementation tracks the spec closely, but there are real deviations and a number of test gaps that mask them.

Spec-conformance findings

🟥 Blocking — /capabilities & /identity unreachable when CL sends Accept: application/json

SszMiddleware.ClassifySszRequest returns NotEngine for every GET whose Accept does not include application/octet-stream:

case "GET":
    foreach (string? v in ctx.Request.Headers.Accept)
        if (v is not null && v.Contains(MediaTypeNames.Application.Octet, ...))
            return SszRequestKind.EngineOk;
    return SszRequestKind.NotEngine;

But per spec refactor.md § Transport & framing, the diagnostic endpoints are JSON:

Diagnostic request / response (/capabilities, /identity) | Content-Type | application/json

A CL that follows the spec sends Accept: application/json for /engine/v2/capabilities and /engine/v2/identity. The middleware then drops to _next, the request never reaches CapabilitiesSszHandler / ClientVersionSszHandler, and the legacy JSON-RPC startup chain rejects the unfamiliar path. The handlers are effectively unreachable in production.

The two unit tests (Capabilities_returns_intersection_of_supported_methods, ClientVersion_returns_non_empty_response) mask the bug because MakeGetContext blindly sets Accept: application/octet-stream for every GET.

Fix: classify GETs to /engine/v2/capabilities, /engine/v2/identity as EngineOk regardless of Accept, or accept any Accept (incl. */*) — these endpoints decide their content type themselves. Then add explicit tests for Accept: application/json and Accept: */*.

🟥 Trailing-slash routing — spec violation

Spec (refactor.md § Transport & framing):

Trailing slashes are forbidden. /engine/v2/payloads is the canonical form; /engine/v2/payloads/ MUST return 404 method-not-found. No automatic redirect.

SszMiddleware.TryRoute strips the trailing / into pathSegment, then TryResolveHandler splits on the first / and discards the empty suffix:

int firstSlash = resourceStr.IndexOf('/');
if (firstSlash > 0)
{
    extraStr = resourceStr[(firstSlash + 1)..];
    resourceStr = resourceStr[..firstSlash];
}

Result: POST /engine/v2/cancun/forkchoice/ resolves to forkchoice with empty extra and is dispatched normally. No test covers this — the existing Path_with_consecutive_slashes_returns_404 test uses /engine/v1/... which fails the version check (version != 2) and so 404s for an unrelated reason.

🟥 Stray path segments after a resource silently accepted

Related to the above: TryResolveHandler looks up handlers by exact resource only and never checks AcceptsPathExtra on the candidate. So POST /engine/v2/cancun/forkchoice/whatever-i-want dispatches to ForkchoiceUpdatedSszHandler (which has AcceptsPathExtra = false) and the extra argument is silently discarded. The _postPrefixRoutes / _getPrefixRoutes arrays computed in BuildRoutes are never read — dead code. Either delete the prefix arrays or actually use them to reject extra against handlers that don't accept it.

🟥 Custody-column update runs even when forkchoice failed

Spec (refactor.md § Forkchoice update):

If the forkchoice update fails, no build is started and no custody change is applied.

EngineRpcModule.Paris.cs unconditionally applies custody after the FCU returns, regardless of result.Result.ResultType or the PayloadStatus:

ResultWrapper<ForkchoiceUpdatedV1Result> result =
    await _forkchoiceUpdatedV1Handler.Handle(forkchoiceState, payloadAttributes, version);

if (custodyColumns is not null)
{
    try { ApplyCustodyColumns(custodyColumns); }
    catch (Exception ex) { /* swallowed */ }
}
return result;

Gate the call on result.Result.ResultType == Success && result.Data?.PayloadStatus?.Status != PayloadStatus.Invalid. (As a separate concern: ApplyCustodyColumns is partial void with no implementer, so the PR description's "custody columns applied independently" is currently aspirational — worth flagging in the PR body.)

🟧 count > MAX_BODIES_REQUEST returns 400, spec says 413

GetPayloadBodiesByRangeSszHandler.cs returns 400 invalid-request for count over the cap. The spec table maps "Body exceeds an advertised limits.* value" → 413 Payload Too Large / request-too-large. Mirror the cap to MergeErrorCodes.TooLargeRequest and let ErrorCodeToHttpStatus map it to 413.

(Minor note: start <= 0 also rejects from=0. Block 0 is the genesis block; spec doesn't pin behavior but accepting from=0 is the safe interpretation.)

🟧 MaxBodySize comment references the wrong spec PR

SszMiddleware.cs cites execution-apis#764 (the predecessor PR). The relevant spec is now #793. Same comment recurs in the malformed-body catch block. Cosmetic, but the entire surface is "per #793" — the in-tree comments should match.

🟧 HTTP/1.1 still permitted on the engine port

Spec (refactor.md § Transport & framing):

HTTP/2 is required. … HTTP/1.1 is not supported.

Startup.cs configures engine API ports with HttpProtocols.Http1AndHttp2. The pragmatic justification is that legacy JSON-RPC must keep working on :8551 over HTTP/1.1, so the port stays mixed-mode. That is a defensible engineering decision but is not what the spec says. At minimum the PR description should call this out; a stricter path would enforce HTTP/2 for /engine/v2/... only and reject HTTP/1.1 requests on that path.

🟧 _supportedForksJson advertises legacy/pre-Engine forks

CapabilitiesSszHandler returns paris, shanghai, cancun, prague, osaka, amsterdam unconditionally from SszRestPaths.SupportedForksOrdered. A node configured for, say, a Cancun-only chain still advertises amsterdam as supported. The spec's example shows the same string list, but the section header (limits block) implies advertised capabilities should reflect what the EL actually serves. Consider gating the list against ISpecProvider-known forks so a CL doesn't waste a round-trip on 400 unsupported-fork.

🟧 bodies.max_count mismatch with spec example

Capabilities advertises "bodies.max_count": 32, while the spec example uses 128. The SSZ container spec pins MAX_BODIES_REQUEST = 2**5 = 32, so Nethermind is consistent with the SSZ schema. The spec example is internally inconsistent. Mention it in the PR description so reviewers don't flag it.

🟨 ForkchoiceUpdatedSszHandler uses IReleaseSpec.Name string comparison

IReleaseSpec timestampSpec = specProvider.GetSpec(ForkActivation.TimestampOnly(timestamp.Value));
if (!timestampSpec.Name.Equals(urlFork, StringComparison.OrdinalIgnoreCase)) { ... }

Fragile: any spec class that returns a Name like "Cancun NoBlobs" or "Prague-Test" will mis-match. The release-spec name is a debug surface, not a fork-routing key. Map fork URL → ForkActivation (or an enum) and compare structurally — same pattern as MapForkToVersion does for versioning.

🟨 Stale-fork URL handling not enforced by test

Spec (refactor.md § Forkchoice update):

If payload_attributes is present, the URL {fork} MUST match the fork that the new payload would belong to.

ForkchoiceUpdatedSszHandler enforces this — but only when timestamp.HasValue (i.e. attributes present). Good. However the spec also says stale-fork URL is allowed when attributes are absent. The implementation passes that through correctly. Just confirm with a test.

🟨 payloadId opacity invariant

Spec: payload_id is an opaque server-assigned token; CLs MUST NOT recompute or validate. The implementation accepts the hex token, parses it as Bytes8, and dispatches — correct. But the encoding side (SszCodec.EncodeForkchoiceUpdatedResponse) hex-decodes the existing JSON-RPC payload ID and emits it verbatim in ByteVector[8]. The comment explicitly notes this — good. (PayloadAttributes.GetPayloadId still computes BE-encoded uint64 internally via Keccak; the transport treats it as opaque bytes, which is what the spec demands.)

🟨 PayloadAttributes.GetVersion() cannot detect both-V4-fields-missing

{ TargetGasLimit: not null } => V4,
{ SlotNumber:      not null } => V4,

If both SlotNumber and TargetGasLimit are null on an Amsterdam-timestamp request, GetVersion() returns ≤V3 and the >= V4 when … validation guards never fire — but the subsequent actualVersion != timestampVersion check catches the case as UnsupportedFork/InvalidPayloadAttributes. Functionally correct, but the field-level error message ("TargetGasLimit must be provided") is more informative than the version-mismatch error. Consider treating expected version from the timestamp spec as the truth, not the inferred version from the data.

🟨 Tests use /engine/v1/… paths that can never route

Unknown_engine_path_returns_404, Post_payloads_with_unknown_extra_returns_404_not_500, Path_with_consecutive_slashes_returns_404, Authentication_failure_returns_401_and_does_not_call_engine_module all use /engine/v1/.... The middleware short-circuits at version != 2 and returns false — meaning every one of these tests passes for the wrong reason. None actually exercises the path-resolution branches they claim to cover. Replace v1 with v2 everywhere.

🟩 Correctly implemented (spot-checks)

• expected_blob_versioned_hashes removed from NewPayloadV3/V4/V5RequestWire; EL recomputes via SszCodec.GetBlobVersionedHashes.
• INVALID_BLOCK_HASH is not part of EngineStatusToSsz — matches spec drop.
• Cache-Control: no-store set on GET /payloads/{id} (GetPayloadSszHandler).
• payload_id path validated as 8-byte hex before lookup.
• HTTP/2 flow-control windows raised to 1 MiB.
• RFC 7807 body shape matches spec: only type + optional detail, no status/title, canned-detail suppression for ssz-decode-error and unauthorized.
• BlobCellsAndProofs shape: List[Optional[ByteVector[1024]], 128] matches spec.
• GetBlobsHandlerV4 returns null (→ [] on wire) for cells outside the requested IndicesBitarray, matching the per-spec rule that unrequested indices encode as empty.
• engine_exchangeTransitionConfigurationV1 retained for the legacy JSON-RPC surface but absent from the SSZ-REST capabilities tables — matches spec removal.
• BlobsBundleV2Wire.Proofs size is 0x8_0000 = 524 288 = MAX_BLOB_COMMITMENTS_PER_BLOCK * CELLS_PER_EXT_BLOB. ✓

Suggested test additions

1. GET /engine/v2/capabilities with Accept: application/json → expect 200 JSON. (Will fail today.)
2. GET /engine/v2/identity with Accept: application/json → 200 JSON. (Will fail today.)
3. POST /engine/v2/cancun/forkchoice/ (trailing slash) → 404 method-not-found.
4. POST /engine/v2/cancun/forkchoice/extra → 404 method-not-found.
5. POST /engine/v2/atlantis/payloads (unknown fork) → 400 unsupported-fork.
6. POST /engine/v2/blobs/v99 → 404 method-not-found.
7. GET /engine/v2/cancun/payloads/0xZZ... (non-hex payload_id) → 400 invalid-request.
8. GET /engine/v2/cancun/bodies?from=1&count=1000 → 413 request-too-large (once the handler is fixed).
9. RFC 7807 body shape: assert exactly {"type":"..."}/{"type":"...","detail":"..."} keys for at least one error of each class — current tests only Does.Contain("unsupported-fork").
10. Fix all /engine/v1/... tests to /engine/v2/....

Summary

The architecture is right (descriptor-per-version generic handlers, RFC 7807 error model, single source of truth in SszRestPaths), and the SSZ container shapes match the spec sketch field-for-field. The blocking items are concentrated in the routing layer:

• diagnostic endpoints unreachable for spec-compliant Accept headers
• trailing-slash / trailing-path-segment acceptance
• custody update runs on failed FCU
• a cluster of tests that don't test what they claim

These should be addressed before merge; the rest are tunable.

@claude re-review

Claude finished @LukaszRozmej's task in 5m 17s —— View job

PR Re-Review: feat(engine): implement Engine API v2 REST+SSZ surface per spec

• Gather context from prior reviews
• Read rule files (coding-style, robustness, performance)
• Read key changed files
• Analyze findings — check fixed vs. outstanding
• Post inline comments for specific issues
• Post final review summary

Summary

All blocking and high/medium issues from the June 12 LukaszRozmej deep review and the prior Claude re-review are resolved. The code is in a shippable state for non-BPO chains.

Resolved since last review

Remaining findings

Medium

ITxPool.TryGetBlobsAndProofsV1 is a breaking public-interface change — see inline comment.

Changing byte[]?[], ReadOnlyMemory<byte[]>[] → Span<byte[]?>, Span<ReadOnlyMemory<byte[]>> compiles fine in-tree (all four in-repo implementers updated) but breaks any out-of-tree ITxPool implementation at compile time. This has been raised in two prior reviews without an explicit acknowledgement. Either tick the "Breaking change" checkbox in the PR body or add a CHANGELOG note.

Low

IsDiagnosticGetPath classifies /engine/v2/capabilities/foo as diagnostic — span.StartsWith(capabilitiesPath) && span[path.Length] == '/' accepts /engine/v2/capabilities/foo (bypasses the Accept header check). TryRoute correctly 404s it afterwards, so the end-to-end behaviour is right; the classifier is just doing double duty. An exact-match would be simpler and remove the dependence on TryRoute for correctness:

return span.Equals(capabilitiesPath.AsSpan(), StringComparison.OrdinalIgnoreCase)
    || span.Equals(identityPath.AsSpan(), StringComparison.OrdinalIgnoreCase);

No test for BPO-fork FCU URL validation — GetEngineApiUrlSegment now correctly walks the parent chain so an Osaka+BPO1-timestamp payload routed to /engine/v2/osaka/forkchoice would be accepted. There's no dedicated test exercising this path yet. Low risk since BPO forks aren't live on mainnet, but worth adding when BPO1 targets appear in test configs.

What looks clean

• NamedReleaseSpec.EngineApiUrlSegment → GetEngineApiUrlSegment parent-chain walk is the right design for BPO fork routing
• GetBlobsHandlerV4: pool lifecycle (Rent → Array.Clear → return on throw), BlobsV4DirectResponse.Dispose, and cell/proof slicing (.AsSpan(0, SszBlobCell.BlobCellLength)) are all correct
• EngineRpcModule.Amsterdam.cs: custody columns logged at trace and discarded — matches flcl42's guidance ("receive it but do not react to it")
• CapabilitiesSszHandler: body built once (double-checked exchange via CompareExchange), compact JSON, for loop replacing LINQ
• SszMiddleware: TryResolveHandler uses AlternateLookup<ReadOnlySpan<char>> (no per-request string allocation), trailing-slash rejection, stray-segment rejection all verified
• EnsureBuilt memory ordering: Volatile.Write(ref _ssz) → Volatile.Write(ref _jsonRpc) publish order is correct — readers that acquire through Volatile.Read(ref _jsonRpc) are guaranteed to see _ssz (B6 is not a real race)

Third deep review — cross-checked against execution-apis#793 (refactor.md + refactor-ssz.md)

Two previous review rounds are addressed. This pass is a clean read against the spec text, focused on wire-shape and normative MUST violations. The PR is functionally close to spec but has one clear MUST violation and one likely wire-format mismatch that would break interop with any spec-conformant CL implementation.

Spec violations (MUST)

S1. PayloadAttributesAmsterdam is missing target_gas_limit (spec MUST)

Spec refactor-ssz.md §PayloadAttributes per fork:

PayloadAttributesAmsterdam {
    ...Cancun fields...
    slot_number:      Uint64
    target_gas_limit: Uint64
}

refactor.md §Forkchoice update is even more explicit:

When building a payload (Amsterdam+), the EL MUST use payload_attributes.target_gas_limit as the target value for the built block's gas_limit.

PR's PayloadAttributesWire (SszWireTypes.cs:85-94) only carries Timestamp/PrevRandao/SuggestedFeeRecipient/Withdrawals/ParentBeaconBlockRoot/SlotNumber. The field was added then removed (commit ac868b2fae remove TargetGasLimit from PayloadAttributes). Without it:

• Spec-conformant CLs sending a wire with target_gas_limit will fail to decode (extra field beyond Amsterdam schema).
• EL cannot honor the build-time MUST.
• The PR description still mentions "TargetGasLimit threaded through PayloadAttributes" — stale.

Restore the field on the Amsterdam wire and thread it through to PayloadAttributes.GetGasLimit() for the block builder.

S2. PayloadStatus.validation_error wire format diverges from Optional[String]

Spec convention: Optional[T] ≡ List[T, 1]; String ≡ List[byte, 1024]. Therefore validation_error: Optional[String] ≡ List[List[byte, 1024], 1].

PR's encoding (SszWireTypes.cs:33-38):

public partial struct PayloadStatusWire
{
    public byte Status { get; set; }
    [SszList(1)] public Hash256[]? LatestValidHash { get; set; }
    [SszList(1024)] public byte[]? ValidationError { get; set; }
}

LatestValidHash is fine — Hash256 is fixed-size so List[Hash32, 1] encodes flat. ValidationError collapses one layer: PR emits a single List[byte, 1024] directly, not List[List[byte, 1024], 1]. For a non-empty error message the PR emits 4 bytes less than spec (the inner offset is missing).

Empty case happens to match. Non-empty case is a wire-bytes mismatch.

Wrap it in a list-of-list (define an inner SszByteList wrapper containing [SszList(1024)] byte[]? Bytes, then use [SszList(1)] SszByteList[]?) to match the spec literally.

Spec ambiguity worth raising upstream

S3. BYTES_PER_CELL = 1024 (spec) vs 2048 (EIP-7594 / Ckzg)

refactor-ssz.md literally defines BYTES_PER_CELL = BYTES_PER_BLOB / CELLS_PER_EXT_BLOB = 131072 / 128 = 1024 and footnotes "(EIP-7594)" — but EIP-7594 defines BYTES_PER_CELL = FIELD_ELEMENTS_PER_CELL(64) × BYTES_PER_FIELD_ELEMENT(32) = 2048 (cells span the extended blob, which is 2× the original). I confirmed Ckzg.BytesPerCell == 2048 by running against the actual package.

My earlier fix changed SszBlobCell.BlobCellLength from 1024 to 2048 because KzgPolynomialCommitments.ComputeCells produces 2048-byte cells per EIP-7594 — encoding them into a 1024-byte slot would either throw or silently lose half the data. The PR currently ships 2048; the spec text says 1024.

The spec is marked "Status: Sketch. Constants are placeholders and need a final review before publication" so this is likely a spec error (the derivation formula is wrong). Worth flagging upstream so the spec text settles before publication; don't change the implementation back to 1024, that path is broken end-to-end with c-kzg.

Other spec gaps

S4. POST /forkchoice payload_status enum not restricted on the encoder side

refactor-ssz.md: POST /forkchoice MUST return 0/1/2 only; CLs MUST treat a 3 from /forkchoice as a protocol error.

The PR encoder (SszCodec.EngineStatusToSsz) maps all four PayloadStatus values uniformly across /payloads and /forkchoice. If the engine module's FCU path ever returns ACCEPTED, the SSZ codec will encode 3 and break a spec-conformant CL. The engine module is unlikely to return ACCEPTED for FCU in practice, but the codec shouldn't silently allow it — either reject in ForkchoiceUpdatedSszHandler post-call, or fold the value down to INVALID.

S5. /bodies doesn't filter blocks whose timestamp falls outside the URL fork's range

refactor-ssz.md §/bodies: available is false when the requested block is unavailable / pruned, or when the block's timestamp falls outside the URL fork's active range, or for range queries when the block number is past the latest known block.

PR delegates to engine_getPayloadBodiesByHashV{1,2} / *ByRangeV{1,2} which returns bodies regardless of the URL fork. A CL requesting /engine/v2/cancun/bodies?from=... may receive Shanghai-era bodies wrapped in Cancun schema. Schema-wise this works because cancun-shaped body is a superset (ExecutionPayloadBodyCancun = ExecutionPayloadBodyShanghai), but per spec those out-of-era blocks should surface as available=false. Add a per-entry timestamp check in the SSZ-REST handlers using ISpecProvider against the URL fork's segment.

S6. BlobV3EntryWire is a duplicate of BlobV2EntryWire

refactor-ssz.md §/blobs/v3:

BlobsV3Response {
    entries: List[BlobV2Entry, MAX_BLOBS_REQUEST]
}

PR defines a distinct BlobV3EntryWire (SszWireTypes.cs:398-403) that is structurally identical to BlobV2EntryWire. The bytes are wire-equivalent, but the spec specifically reuses BlobV2Entry. Minor: drop BlobV3EntryWire and use BlobV2EntryWire in GetBlobsV3ResponseWire.

S7. Spec inconsistency: bodies.max_count in capabilities example vs MAX_BODIES_REQUEST table

Not a PR issue — flagging upstream. Spec body table: MAX_BODIES_REQUEST = 2**5 = 32. Spec capabilities JSON example: "bodies.max_count": 128. PR correctly uses 32 (SszRestLimits.MaxBodiesRequest); the spec example is the inconsistent one.

S8. payload.max_bytes: 64 MiB (spec example) vs 128 MiB (PR)

Spec doesn't define a normative constant for the request body limit; the capabilities example shows 67108864 (64 MiB). PR has MaxBodySize = 0x8000000 = 128 MiB. Either is defensible — the spec doesn't pin this — but the inline comment in SszMiddleware.cs:44 saying "Matches MAX_REQUEST_BODY_SIZE in the Engine API SSZ-REST spec" is misleading since no such constant exists in the spec. Either align with the spec example (64 MiB) or drop the comment.

Already spec-compliant

• HTTP/2-required transport with INITIAL_WINDOW_SIZE = 1 MiB (SHOULD) — set on both connection and stream windows (Startup.cs:104-105). ✓
• JWT per-request auth, Authorization: Bearer <jwt> — unchanged from existing JSON-RPC path. ✓
• Content-Type: application/octet-stream enforced on hot-path POST; Accept: application/octet-stream enforced on hot-path GET; diagnostic endpoints serve JSON regardless of Accept. ✓ (ClassifySszRequest + IsDiagnosticGetPath)
• Trailing-slash rejection (/engine/v2/payloads/ → 404). ✓
• Cache-Control: no-store on all GET /payloads/{id} responses (now including error paths after the last fix round). ✓
• Path validation for payloadId (hex Bytes8, otherwise 400 invalid-request). ✓
• RFC 7807 error body shape with required type, optional detail; canned ssz-decode-error and unauthorized omit detail. ✓
• expected_blob_versioned_hashes removed from NewPayload requests; EL recomputes via GetBlobVersionedHashes. ✓
• engine_exchangeTransitionConfigurationV1 removed (deprecated since Cancun). ✓
• payload_id is opaque 8-byte token, encoded as ByteVector[8], no LE flip. ✓
• BlobCellsAndProofs shape: List[Optional[…], CELLS_PER_EXT_BLOB] for both blob_cells and proofs. ✓
• indices_bitarray: Bitvector[128] and custody_columns: Optional[Bitvector[128]] — wire types match. ✓
• Trace propagation (SHOULD) and zstd (MAY) explicitly deferred per PR description. ✓

Cleanups (non-spec; second-pass observations)

• EngineRpcModule.Amsterdam.cs:30-37 — custodyColumns is now visible in the trace log but still not applied to any custody subsystem. Leave a TODO referencing the eventual consumer or open a tracking issue, so this doesn't get lost the moment somebody wires PeerDAS custody on the EL.
• BlobV3EntryWire (see S6) — dead duplicate of BlobV2EntryWire, drop and reuse the V2 type.
• SszMiddleware.IsDiagnosticGetPath:474-484 — now that TryRoute rejects /engine/v2/{capabilities,identity}/foo, the prefix matches here are doing double duty (admit then reject). Simpler: exact-match only and let TryRoute 404 anything with an extra segment.
• SszCodec.DecodeGetBlobsRequest:113-120 allocates byte[32] per versioned hash (128 small allocations on a full request). Could write into a flat byte[hashes*32] and slice — minor, but cheap to fix.
• SszCodec.EncodeForkchoiceUpdatedResponse:46 still allocates new byte[8] for the happy-path payload_id; SszPayloadId.Bytes is byte[]? so stackalloc is structurally blocked, but the array could come from a small pool if profiling shows it matters.

Net assessment

The new SSZ-REST surface is in good shape on transport, routing, error model, and most wire types. Blocking for merge: S1 (target_gas_limit) is a clear spec MUST violation and a build-side functional gap. S2 (validation_error encoding) is a likely wire-format mismatch that affects every non-VALID payload status. S3 (BYTES_PER_CELL) needs an upstream spec clarification but the PR's current 2048 value is the only viable choice given c-kzg; just flag it on the spec PR.

S4/S5 are smaller corner cases worth fixing before the spec is finalised, since they constrain CL behaviour. S6/S7/S8 are mostly polish.

Fourth deep review — issues found in the previous fix round

While auditing the S2/S5/S8 fixes I introduced two of my own bugs and one wire-shape mismatch that mirrors the pattern in S2. All three are now fixed in f9d30816a7.

A. S5 introduced a pool-memory leak

The bodies engine handlers (GetPayloadBodiesByHashV2Handler, GetPayloadBodiesByRangeV2Handler) return a PayloadBodiesV2DirectResponse wrapping PayloadBody?[]. PayloadBody holds MemoryManager<byte>? _blockAccessList — pool-rented bytes that must be returned through PayloadBodiesV2DirectResponse.Dispose.

My S5 fix replaced the ResultWrapper:

result = ResultWrapper<IReadOnlyList<TResult?>>.Success(
    BodiesForkFilter.FilterByHash(result.Data, hashes, urlFork, blockFinder, specProvider));

This drops the original wrapper on the stack. The new wrapper's Dispose only inspects the filtered TResult?[] (a plain array, not IDisposable). Nothing calls PayloadBodiesV2DirectResponse.Dispose, so the MemoryManager rentals never return to the pool. The class has no finalizer either, so it's an outright leak per request that hits a fork-mismatched body.

Fix: chain the original disposal through wrapped.AddDisposable(result.Dispose) before reassigning. WriteSszResultAsync's using (result) now calls back into the original wrapper's Dispose after encoding completes.

B. SszValidationError (and NullableBlobCellWire / NullableKzgProofWire) still added a spurious Container header

S2 wrapped validation_error in SszValidationError to express Optional[String] = List[List[byte, 1024], 1]. But [SszContainer] without isCollectionItself: true emits a Container header — a 4-byte offset to the inner List — for every element. Net effect: 4 extra bytes per non-empty validation_error vs spec.

Same pattern in V4 blob cells. NullableBlobCellWire/NullableKzgProofWire each wrapped a [SszList(1)] T[]? in a plain Container, adding 4 bytes per cell entry × 128 cells × N blobs.

Fix: [SszContainer(isCollectionItself: true)] on all three. Verified by a regression test that pins the exact byte count:

// { Status=INVALID, LatestValidHash=[], ValidationError="bad" }:
//   1 byte status + 4+4 offsets + 0 LVH + 4 inner-list offset + 3 "bad" = 16 bytes
Assert.That(encoded, Has.Length.EqualTo(16));

This is the canonical SSZ encoding of Optional[String] for a 3-byte error.

C. Double materialization in BodiesForkFilter

if (bodies[i] is null) continue;
...
result[i] = bodies[i];

PayloadBodiesV2DirectResponse[i] calls PayloadBody.ToResult() which decodes RLP and copies the BAL bytes via .Memory.ToArray() every time. The filter was calling the indexer twice per non-null entry — for 32 bodies that doubles a non-trivial amount of work. Fixed by capturing once:

TResult? body = bodies[i];
if (body is null) continue;
...
result[i] = body;

Smaller items addressed

• Added an explicit-bytes regression test for validation_error (16 bytes exact) and an end-to-end V4 cell decode test that asserts entry shape after my container-elision change.
• Test style: my added tests now use Assert.EnterMultipleScope + Has.Length/Is.Empty to match the codebase convention.
• Dropped the defensive urlFork is null early-return inside BodiesForkFilter (callers guard now), removing one branch from the per-entry loop.

Items left from prior reviews

Still on the table from review 3:

• S1 (target_gas_limit) — deferred per author preference; will be picked up with the block-builder wiring.
• S3 (BYTES_PER_CELL spec ambiguity) — implementation stays at 2048; spec follow-up posted at ethereum/execution-apis#793 (comment).
• S4 (POST /forkchoice payload_status enum not restricted to 0/1/2 on the encoder side) — not addressed.
• S6 (BlobV3EntryWire is a wire-identical duplicate of BlobV2EntryWire) — author was unsure; raised on the spec PR for clarification.

Tests

1524 total (1522 pass, 2 skipped — unrelated). The new tests fail on the un-fixed handler (verified by git stash-ing each fix and re-running).

Fifth deep review — wire-shape limit audit

This pass swept every [SszList(N)] / [SszVector(N)] constant in the new wire types against the spec's MAX_* table and the spec's container definitions. One wire-shape divergence found and fixed in 0d43291fb2.

Fixed

SszExecutionPayloadV4.BlockAccessList limit mismatched ExecutionPayloadBodyV2Wire.BlockAccessList

Spec refactor-ssz.md:

• MAX_BAL_BYTES = MAX_BYTES_PER_TX = 2^30 = 1 GiB
• ExecutionPayloadAmsterdam.block_access_list: ByteList[MAX_BAL_BYTES]
• ExecutionPayloadBodyAmsterdam.block_access_list: ByteList[MAX_BAL_BYTES]

The PR had them inconsistent:

SszExecutionPayloadV4.BlockAccessList:        [SszList(0x0100_0000)]  // 16 MiB
ExecutionPayloadBodyV2Wire.BlockAccessList:   [SszList(0x4000_0000)]  // 1 GiB

The 16 MiB cap on the payload-submission path is 64× smaller than spec and would reject spec-conformant POST /amsterdam/payloads requests with a larger BAL while the corresponding body-by-hash response on /amsterdam/bodies/hash would happily encode them. Aligned both to 0x4000_0000.

Audit summary — all other MAX_* and container limits check out

Verified against refactor-ssz.md §MAX_*:

Containers also verified

• ExecutionPayload{V1,V2,V3,V4} field ordering matches spec's ExecutionPayloadParis → Shanghai → Cancun → Prague → Osaka → Amsterdam progression. Prague = Cancun and Osaka = Prague = Cancun shape equivalences mean GetPayloadResponseV{4,5}Wire.ExecutionPayload correctly uses SszExecutionPayloadV3; GetPayloadResponseV6Wire correctly uses SszExecutionPayloadV4.
• Optional[T] ≡ List[T, 1] convention now consistently expressed via [SszContainer(isCollectionItself: true)] wrappers (SszValidationError, NullableBlobCellWire, NullableKzgProofWire) — the spec wire layout for Optional[String] is byte-exact (regression test pins 16 bytes for "bad").
• BlobsBundleV1 vs BlobsBundleV2 shape difference matches spec (V1 one proof per blob, V2 CELLS_PER_EXT_BLOB cell proofs per blob).
• SszNumericChecks.CheckedLong converts SSZ uint64 → long with overflow → InvalidDataException → 400 ssz-decode-error. Clean.

Other checks

• OptimismEngineRpcModule composes IEngineRpcModule rather than subclassing — engine_getBlobsV4 / forkchoiceUpdatedV4 reach the SSZ-REST surface through the underlying module without needing to be re-exposed on the Optimism JSON-RPC face. ✓
• TaikoEngineRpcModule ctor and base-call propagate the new getBlobsHandlerV4 parameter. ✓
• IBlockFinder is bound to IBlockTree in BlockTreeModule so the SSZ-REST DI bridge (services.Bridge<IBlockFinder>(ctx)) resolves at runtime. ✓
• DI registrations of the bodies SSZ handlers (per descriptor version × body result type) match the handler constructor's (IEngineRpcModule, IBlockFinder, ISpecProvider) signature. ✓
• Benchmark (NewPayloadSerializationBenchmarks) drops ExpectedBlobVersionedHashes from the SSZ body to match the spec's removal of the field on NewPayload requests. ✓
• PayloadAttributes.Validate field-error reporting reorder makes the "missing field" error fire before the generic version-mismatch when attrs are below the timestamp-implied version. Sensible.

Tests

1522 pass after the BAL fix; the fix doesn't break any wire roundtrip — existing tests covered 16 MiB-sized BALs which still fit under the new (larger) 1 GiB cap.

Net

The PR's wire shapes are now (as far as I can verify) byte-equivalent to the spec for all containers I could check field-by-field against refactor-ssz.md. Issues still on the table from prior rounds:

• S1 target_gas_limit — deferred by author
• S3 BYTES_PER_CELL — spec ambiguity, flagged upstream
• S4 FCU response ACCEPTED restriction — encoder still permissive
• S6 BlobV3EntryWire dup of BlobV2EntryWire — author unsure, raised on spec PR

CI red — Pyspec stateless tests fixed by reverting BAL limit

All 16 Ethereum.Blockchain.Pyspec.Test [Sequential] shards were failing on every Amsterdam-fork test with:

Unable to determine finalized state root at block 0. Available state roots …

Traced to StatelessExecutor.InputDecoder.DecodeV1<T>:

NewPayloadRequest<TExecutionPayload>.Merkleize(input.NewPayloadRequest, out UInt256 root);
...
NewPayloadRequestRoot: new Hash256(root.ToLittleEndian())

The merkle tree depth for an SSZ variable list is fixed by its [SszList(N)] limit (ceil(log2(N/32))). My BAL-limit change in 0d43291fb2 (16 MiB → 1 GiB) altered the V4 payload's merkle root, breaking the lookup against fixtures that were generated against the 16 MiB value.

Reverted in dfa8e83ae9 — kept at [SszList(0x0100_0000)] so it matches the test fixtures. The execution-apis #793 sketch lists MAX_BAL_BYTES = MAX_BYTES_PER_TX = 2^30; that divergence is already flagged on the spec PR (comment). Re-fix once the spec value settles and fixtures align.

ExecutionPayloadBodyV2Wire.BlockAccessList stays at 0x4000_0000 — bodies aren't merkleized by the stateless executor, only decoded.

The f9d30816a7 isCollectionItself: true changes affect SszValidationError / NullableBlobCellWire / NullableKzgProofWire — none of which are inside NewPayloadRequest, so they don't touch the stateless executor's merkle path.

Tests still 1522 pass locally; CI re-running now.

@claude re-review

Claude finished @LukaszRozmej's task in 6m 15s —— View job

PR Re-Review: feat(engine): implement Engine API v2 REST+SSZ surface per spec

• Gather context from prior reviews
• Read rule files (coding-style, robustness, performance)
• Read key changed files (focusing on outstanding issues from prior reviews)
• Analyze findings — check fixed vs. outstanding
• Post inline comments for specific issues
• Post final review summary

Summary

The most recent commit (dad6790) addressed the last round of nits — removed dead fields in GetBlobsHandlerV4, cleaned up test helpers, fixed ClientVersionSszHandler to route errors through ErrorCodeToHttpStatus, and added a custody-columns TODO. The core architecture (descriptor pattern, BPO-aware fork routing via GetEngineApiUrlSegment, RFC 7807 error shape, pool lifecycle for V4 blobs) is solid and correct.

Three items from prior reviews remain open and are tracked with inline comments.

Remaining Findings

Medium

1. S1 — target_gas_limit missing from PayloadAttributesWire (Amsterdam)
→ SszWireTypes.cs:76

Spec MUST violation. A spec-conformant CL sending Amsterdam attributes with target_gas_limit will receive 400 ssz-decode-error. Explicitly deferred pending #11611, but the wire struct should accept the field now (even as unused) to avoid breaking CL interop before #11611 merges. The stale "TargetGasLimit threaded through PayloadAttributes" sentence in the PR description should also be updated.

2. S4 — ACCEPTED (status byte 3) can be emitted from /forkchoice response
→ ForkchoiceUpdatedSszHandler.cs:39

The spec forbids this for POST /forkchoice; CLs MUST treat byte 3 as a protocol error. The codec maps ACCEPTED → 3 with no guard and ForkchoiceUpdatedSszHandler passes the result through unfiltered. While the engine module is unlikely to return ACCEPTED for an FCU call in practice, the spec violation is clear and the guard is a one-liner.

3. Breaking ITxPool.TryGetBlobsAndProofsV1 still undocumented after five review rounds
→ ITxPool.cs:63

The byte[]?[] → Span<byte[]?> signature change compiles fine in-tree but breaks any out-of-tree ITxPool implementer. The "Breaking change" checkbox in the PR body is unchecked.

Low

4. S6 — BlobV3EntryWire is a wire-identical duplicate of BlobV2EntryWire
→ SszWireTypes.cs:405

The spec reuses BlobV2Entry for the V3 response. The struct is correct on the wire but semantically redundant. Flagged upstream on the spec PR for clarification.

What's been resolved since last review (dad6790 + context)