feat(engine): introduce REST based + SSZ serialized new-payload-with-witness

[Dyslex7c]

Summary

Implements the POST /new-payload-with-witness Engine API REST endpoint as specified in ethereum/execution-apis#773. This endpoint combines payload execution and witness generation into a single HTTP+SSZ call, replacing the previous two-step engine_newPayload + debug_executionWitness flow.

Also adds a engine_newPayloadWithWitness JSON-RPC method that exposes the same semantics over the standard JSON-RPC transport.

Changes

• Added NewPayloadWithWitnessSszHandler — handles POST /new-payload-with-witness, accepts the same JSON params as engine_newPayloadV5, delegates to the engine_newPayloadWithWitness JSON-RPC handler, and returns an SSZ-encoded NewPayloadWithWitnessResponseV1
• Added engine_newPayloadWithWitness JSON-RPC method on EngineRpcModule backed by NewPayloadWithWitnessHandler — same execution + witness generation logic available over JSON-RPC
• Encoding/decoding of NewPayloadWithWitnessResponseV1 is hand-rolled in SszCodec (not auto-generated) to correctly implement the spec's Union[None, T] layout with explicit selector bytes (0x00 = None, 0x01 ++ T = Some). The generator's [SszCompatibleUnion] does not support selector byte 0x00, so this cannot go through the standard path; a comment in SszWireTypes.cs explains the constraint.
• Added SszCodec.EncodeNewPayloadWithWitnessResponse and DecodeNewPayloadWithWitnessResponse — encode/decode the combined status + witness response. Non-VALID statuses encode witness as Union None (selector byte 0x00), not an empty list.
• Added a dedicated fast-path in SszMiddleware for the non-versioned /new-payload-with-witness path; the versioned /engine/v{N}/new-payload-with-witness path correctly returns 404
• Separated SszRestCapabilities from SszRestPaths; registered rest_engine_newPayloadWithWitness capability in EngineRpcCapabilitiesProvider, gated on IsEip7928Enabled

Behaviour changes affecting all SSZ-REST callers

SszEndpointHandlerBase.WriteErrorAsync now emits application/json with {"code": …, "message": "…"} for all SSZ-REST error responses — auth failures on /engine/v*/…, 404s, 413s, malformed-SSZ 400s, and 500s. Previously these were text/plain with a raw message string. Any tooling that parses the raw text body of error responses from these endpoints will need to be updated.

PayloadStatusWire.ValidationError SSZ list cap bumped from 1024 → 8192 bytes (spec correctness; affects all PayloadStatusV1 SSZ responses, not just the witness endpoint). This changes the merkleization parameters for PayloadStatusV1. Downstream tooling with hardcoded golden-byte test fixtures against the old 1024 cap will need updating.

Constructor signature change for EngineRpcModule

EngineRpcModule now requires one additional constructor parameter: INewPayloadWithWitnessHandler. It is needed unconditionally (the JSON-RPC method is registered on every network); the capability advertisement is still gated on IsEip7928Enabled.

In-tree subclasses affected: TaikoEngineRpcModule (updated in this PR). OptimismEngineRpcModule uses composition (IEngineRpcModule) and is unaffected. Downstream forks that subclass EngineRpcModule directly will need to update their constructor call sites.

Types of changes

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

Unit tests cover:

• VALIDATION_ERROR_MAX = 8192 acceptance and truncation
• UTF-8 truncation correctness (no split multi-byte code points)
• EncodeNewPayloadWithWitnessResponse for all non-VALID statuses (witness field is Union None, selector byte 0x00)
• EncodeNewPayloadWithWitnessResponse for VALID with witness (selector byte 0x01 + encoded body) and VALID with null witness (Union None)
• Middleware: 200 for VALID and SYNCING statuses, 400 for malformed JSON, 405 for non-POST, 400 + correct error code for unsupported fork
• Error responses use application/json with code and message fields
• Container layout: byte-by-byte verification that the fixed header is 13 bytes and all three offsets are correct

JSON-RPC method (EngineModuleTests.Amsterdam):

• VALID status → executionWitness populated
• VALID with null parent header → executionWitness null, status still VALID
• VALID with collector exception → executionWitness null, status still VALID
• SYNCING → executionWitness null, witness factory not invoked
• INVALID → status, LatestValidHash, and ValidationError round-trip correctly, witness factory not invoked
• engine_newPayloadV5 RPC failure → error code and message propagated correctly

[LukaszRozmej]

Deep review against the spec (execution-apis#773 / src/engine/rest.md) and the surrounding code. CI Build/Lint/Format pass; the two Hoodi sync-gate failures look unrelated.

High-priority

1. Wire format does not match the spec's Union type

Spec defines:

• latest_valid_hash: Union[None, ByteVector[32]]
• validation_error: Union[None, List[uint8, VALIDATION_ERROR_MAX]]
• witness: Union[None, ExecutionWitnessV1]

NewPayloadWithWitnessResponseV1Wire encodes all three as [SszList(1)] T[]? (a max-1 list):

public byte Status { get; set; }
[SszList(1)] public Hash256[]? LatestValidHash { get; set; }
[SszList(8192)] public byte[]? ValidationError { get; set; }
[SszList(1)] public ExecutionWitnessV1Wire[]? Witness { get; set; }

SSZ Union[None, T] is selector_byte ++ variant (1 byte for None, 0x01 ++ T for T). List[T, 1] with 0/1 elements is 0 bytes / T bytes. These differ on the wire by the selector byte. The codebase already has [SszCompatibleUnion] in Nethermind.Serialization.Ssz / the generator — this PR doesn't use it.

This is the existing convention for PayloadStatusWire / NullableBlobAndProofV2Wire, so the PR is internally consistent — but #773 is brand-new so there's no installed-base reason to perpetuate the deviation. Please verify interop against another implementation of #773 (e.g. ethrex) before merging; a CL written against the spec text will reject this body.

2. Block is executed twice on every VALID payload

engine_newPayloadV5 runs BlockProcessor.ProcessOne once. TryGenerateWitness then calls WitnessCollector.GetWitnessForExistingBlock(parent, block) which does another blockProcessor.ProcessOne(block, ProcessingOptions.ReadOnlyChain, …) (WitnessCollector.cs:22-27). That's a full re-execution — exactly the cost #773 was created to eliminate. Its benchmark target is witness collection as part of the primary execution (~700ms total); a double-execution approach blows past the 8s engine_newPayload budget on heavy blocks. Correctness is fine; the perf claim of the parent spec is undelivered. Either call this out as a follow-up perf issue or wire witness collection into the primary processing path.

3. Witness-generation failure after VALID is unrecoverable for the CL

if (status.Status == PayloadStatus.Valid)
{
    witness = TryGenerateWitness(request.ExecutionPayload);
    if (witness is null)
    {
        await WriteErrorAsync(ctx, 500, "Payload executed with VALID status but … the block has been accepted.", …);
        return;
    }
}

The block is already in the chain when we get here. If witness generation fails (parent missing, exception swallowed below), the CL sees HTTP 500 and can't distinguish "block accepted, witness failed" from "block rejected" — it may retry. Given the spec defines witness as Union[None, ExecutionWitnessV1] and explicitly allows None "when … no witness was produced", returning 200 + status=VALID + witness=None is more faithful than 500 — even though the spec prose then says VALID "MUST include the execution witness", the Union arm exists precisely for this case. At minimum, log the underlying cause.

4. The "happy-path" middleware test does not exercise the happy path

NewPayloadWithWitness_returns_200_for_valid_status:

await _middleware.InvokeAsync(ctx);
…
ctx.Response.StatusCode.Should().NotBe(StatusCodes.Status404NotFound, "…");

_witnessEnvFactory is Substitute.For<…>() with no return configured, so CreateScope() returns null → NRE → caught in TryGenerateWitness → 500. The only assertion is "not 404", so the test passes with 500. The test name claims 200 for VALID but never asserts Be(200).

Configure the factory mock so CreateScope().Env.CreateExistingBlockWitnessCollector().GetWitnessForExistingBlock(...) returns a (stub) Witness, then assert Be(200), ContentType.Should().Contain(OctetStream), and that the body decodes via NewPayloadWithWitnessResponseV1Wire.Decode with Status == 0 and non-empty Witness. This is the single most important missing test in the PR.

5. TryGenerateWitness swallows all exceptions silently

Bare catch { return null; } with no logging — any internal bug becomes a generic 500 with no breadcrumb. Inject ILogManager, log via _logger.Error("…", ex), and prefer catching specific exception types.

Medium-priority

6. WriteErrorAsync change is a quiet wire change for all SSZ-REST error responses

SszEndpointHandlerBase.WriteErrorAsync now writes application/json with {"code":…,"message":…} for every error response across the middleware (auth failures on /engine/v1/payloads, 404s, 413s, malformed-SSZ 400s, 500s). Previously: text/plain with raw message. Tooling parsing text bodies on those endpoints will break. The new shape is correct per spec — but please call it out in the PR description as a behavior change for all SSZ-REST callers, not just witness. Minor: the code value isn't escaped via JsonSerializer; safe today because it's int, but brittle.

7. Wrong Content-Type POSTs to the witness path fall through to the next middleware

if (path.Equals(WitnessPath, …))
{
    if (!POST) return true;
    string? ct = ctx.Request.ContentType;
    return ct is not null && ct.Contains("application/json", …);
}

A POST to /new-payload-with-witness with Content-Type: text/plain (or missing) makes IsSszRequest return false, the request escapes to _next, and the user gets whatever comes next — probably a 404. Spec mandates application/json request and JSON-shaped error body. Natural response is 415 or 400, not a fall-through. Symmetrical with the 405 rationale in DispatchWitnessAsync.

8. Capability-name constant is filed under "paths"

SszRestPaths.RestEngineNewPayloadWithWitness = "rest_engine_newPayloadWithWitness" lives alongside path constants like PostV4Forkchoice = "POST /engine/v4/forkchoice". The capability advertisement string isn't a path. Either rename the file or hold the capability string in EngineRpcCapabilitiesProvider directly. Also NewPayloadWithWitness = "new-payload-with-witness" is a handler Resource identifier (no leading slash) while the URL is /new-payload-with-witness — workable but worth a comment.

9. PayloadStatusWire ValidationError cap bumped 1024 → 8192

This is a spec-correctness fix for all PayloadStatusV1 SSZ responses, not just the witness endpoint. Worth calling out separately in the PR description because (a) anyone relying on the old 1024 cap (test fixtures with golden bytes, downstream tooling) sees a change, and (b) it changes merkleization parameters. The new TruncateUtf8 is correct (verified by tracing the 8190×'a' + € case).

10. Witness.Keys is silently dropped

Nethermind's Witness has four collections (State, Codes, Keys, Headers); spec ExecutionWitnessV1 has three. BuildExecutionWitnessV1Wire correctly omits Keys per spec — fine — but Keys is still built and held in Witness.Keys until disposal. Consider a collector mode that skips Keys accumulation when only spec-shaped output is needed.

Low-priority / nits

• Allocation footprint: BuildExecutionWitnessV1Wire allocates new SszWitnessItem[items.Count] and WriteSszNewPayloadWithWitnessAsync uses a fresh new ArrayBufferWriter<byte>(). For 300 MB witnesses (spec's benchmark scale) the LOH pressure is real. Existing endpoints use the same pattern, so not a regression — follow-up.
• Dead 204 branch: minimum response is 13 bytes (status + 3 offsets); length == 0 cannot happen. Harmless, remove for clarity.
• Auth 401 reports code: -32603 InternalError: spec just says "same code as JSON-RPC for Engine API errors" — there's no canonical JSON-RPC code for auth failure, but InternalError is misleading.
• Resource registered into _postRoutes even though dispatched via the special _witnessHandler fast path: worth a unit test that /engine/v1/new-payload-with-witness (versioned variant) returns 404, not a stealth dispatch.
• No symmetric test for the non-UnsupportedFork → 500 branch of the error switch (lines 57-66).

What's good

• SSZ types cleanly mirror the spec's structure modulo the Union issue.
• TruncateUtf8 is correct, properly tested for multi-byte truncation.
• 405 handling for non-POST is thoughtful and well-commented.
• EngineApiJsonContext correctly extended for ExecutionPayloadV4.
• InvalidBlockHash = 4 mapping added in both PayloadStatus and EngineStatusToSsz (forward-compatible).
• Capability registration in the right place; SszRestPathsAreAdvertised updated.
• 8192 cap is a quiet spec fix bundled in correctly.

Recommendation

Before approval:

• (Feature/jsonrpc #1) Confirm wire-format compatibility against another NDM consumer service refactoring & testing #773 implementation. If incompatible, switch to [SszCompatibleUnion].
• (Hive #4) Make the VALID middleware test actually exercise witness generation and assert 200 + content-type + body decode.
• (Netcore #3, Patricia alt 2 #5) Add logging in TryGenerateWitness; decide whether "VALID but no witness" should be 500 or 200+None.
• (Perf0602 #6, Lazynodes #9) Call out the text/plain → application/json error change and the 1024 → 8192 cap bump in the PR description.

Acceptable as follow-ups: #2 (single-execution witness), #7 (wrong-CT handling), #10/#11 (allocations / key collection).

[LukaszRozmej]

Re-review (commits c4028c5334 + 1e31a1f4f1)

Nice work on the responses. The big one (#1) is properly fixed; rounding out the rest.

Resolved

• Feature/jsonrpc #1 Wire format — NewPayloadWithWitnessResponseV1Wire is gone; EncodeNewPayloadWithWitnessResponse now hand-emits proper SSZ Union[None, T] with explicit selector bytes and offset math. The new DecodeNewPayloadWithWitnessResponse and the byte-by-byte container test (…_container_header_is_13_bytes_and_offsets_are_correct) lock the layout down. The comment block in SszWireTypes.cs explaining why this can't go through the generator (selector 0x00 unsupported by [SszCompatibleUnion]) is exactly what a future reader needs. One nit: SSZ offsets are technically uint32; you're using WriteInt32LittleEndian/ReadInt32LittleEndian. Identical bytes for values < 2³¹, which they always will be under the 16 MiB body cap — fine, but a one-line comment noting the intentional choice would help.
• Netcore #3 / Patricia alt 2 #5 — VALID-with-no-witness now returns 200 + witness=None per the Union's None arm, with a logged error including the block hash. Specific exception catches with messages. NewPayloadWithWitness_valid_status_but_witness_generation_fails_returns_200_with_null_witness covers the new behavior.
• Hive #4 — The happy-path test now actually exercises the happy path: stubs the factory chain, asserts 200 + application/octet-stream, decodes the SSZ body, checks status byte, latest_valid_hash round-trip, and witness presence. 👍
• Rlpstreams #7 — Wrong Content-Type → 415 with Accept: application/json header. Belt-and-braces (both in middleware DispatchWitnessAsync and inside the handler), which is fine.
• Vmspans #8 — SszRestCapabilities separated from SszRestPaths. Clean.
• EIP-145 #13 (auth code) — 401 now reports code: InvalidRequest instead of InternalError. Better.
• EIP-210 #14 (versioned path) — BuildRoutes now skips the witness handler, so _postRoutes/_getRoutes can't accidentally dispatch it. NewPayloadWithWitness_via_versioned_engine_path_returns_404 locks this. Also explicit test for the non-UnsupportedFork → 500 branch.
• 204 dead branch — removed.

Still open from previous review

• Feature/networking #2 (double execution) — unchanged. The REST handler and the new JSON-RPC method both call engine_newPayloadV5 then re-execute via WitnessCollector.GetWitnessForExistingBlock. Fine as a correctness-first impl, but the parent spec (NDM consumer service refactoring & testing #773) was created specifically to eliminate this. Please open a follow-up issue so it isn't lost.
• Perf0602 #6 / Lazynodes #9 — PR description still doesn't call out (a) the SSZ-REST-wide text/plain → application/json error body change and (b) the PayloadStatusWire.ValidationError cap bump 1024 → 8192. Both touch consumers outside the witness endpoint; worth flagging in the description so reviewers don't have to find them in the diff.
• committed on a wrong branch #10 (Witness.Keys) — still built and immediately dropped. Minor; follow-up.

New issues introduced by the JSON-RPC additions in 1e31a1f4f1

1. TryGenerateWitnessForBlock in EngineRpcModule.Amsterdam.cs still has the bare catch that #5 fixed in the SSZ handler.

try { … return collector.GetWitnessForExistingBlock(parent, block); }
catch { return null; }

Same anti-pattern: any failure is silently converted to witness=null with no breadcrumb. Mirror the SSZ handler's split into OperationCanceledException (warn) and Exception (error with ex argument), and log the block hash for the null-decode/null-parent early returns too.

2. The "VALID but no witness" log on the JSON-RPC path lacks the block hash.

if (witness is null && _logger.IsWarn)
    _logger.Warn("engine_newPayloadWithWitness: payload is VALID but execution witness could not be generated.");

The SSZ handler includes request.ExecutionPayload.BlockHash in its log. Match it here — without the hash this line is useless for postmortem.

3. No test for the new JSON-RPC method.
Grep shows engine_newPayloadWithWitness referenced only in SszRestPaths.cs and the capabilities test in EngineModuleTests.V1.cs (capability advertisement). Nothing actually invokes the JSON-RPC method end-to-end. Add at minimum:

• VALID → response has executionWitness populated.
• VALID with witness failure → response has executionWitness omitted (WhenWritingNull) or null, status is still VALID.
• Non-VALID statuses (SYNCING, INVALID) → executionWitness omitted, status correct.
• Error result from engine_newPayloadV5 (e.g. unsupported fork) → propagates code/message correctly.

4. EngineRpcModule constructor gained two required parameters (IBlockTree, IWitnessGeneratingBlockProcessingEnvFactory).
Breaking constructor signature. I confirmed:

• TaikoEngineRpcModule — updated in this PR ✓
• OptimismEngineRpcModule — uses composition (IEngineRpcModule engineRpcModule), unaffected ✓
• No other new EngineRpcModule(...) sites in the codebase (only EngineModuleTests.V3.cs, updated) ✓

So nothing actually breaks, but worth a note in the PR description for downstream forks. Also: IWitnessGeneratingBlockProcessingEnvFactory is now required for all networks (pre-Amsterdam too). Confirm the DI registration is unconditional, otherwise pre-Amsterdam test fixtures may fail to resolve EngineRpcModule.

5. NewPayloadWithWitnessV1Result — double blank line between LatestValidHash and ValidationError. Cosmetic.

6. IEnumerable<ISszEndpointHandler> handlers is iterated twice in the constructor (once in BuildRoutes, once to find _witnessHandler). In ASP.NET Core DI this is backed by a List so re-iteration is safe in practice, but a defensive IReadOnlyList<…> hs = handlers as IReadOnlyList<…> ?? [.. handlers] removes the implicit assumption. Or merge the two passes into one.

Recommendation

Address #1–#3 above (logging parity on the JSON-RPC path + tests for the new method) before merge. The rest of the new findings are nits / PR-description hygiene. The original concerns I raised in the first review are properly resolved.

[LukaszRozmej]

Round 3 — commits 6671f99979 + 1d382f43a4

All blocking concerns from the prior rounds are addressed. CI is fully green (541 success, 0 failures). Just a few residuals before LGTM.

Resolved this round

• JSON-RPC catch logging — TryGenerateWitnessForBlock now mirrors the SSZ handler: OperationCanceledException → warn, Exception → error with the exception captured, plus block-hash breadcrumbs on the null-decode/null-parent early returns.
• JSON-RPC log message — the "VALID but no witness" log now includes executionPayload.BlockHash and the full spec rationale.
• engine_newPayloadWithWitness JSON-RPC test coverage — EngineModuleTests.Amsterdam.cs adds six tests covering: VALID with witness, VALID with null-parent → null witness, VALID with thrown exception → null witness, SYNCING (no factory invocation), INVALID (status + LVH + ValidationError round-trip, no factory invocation), and UnsupportedFork error propagation. Coverage is exactly what I asked for; the DidNotReceive().CreateScope() assertions on non-VALID paths are a nice touch.
• Constructor double iteration — handlers materialized to IReadOnlyList<…> once; BuildRoutes signature tightened to match.
• NewPayloadWithWitnessV1Result double blank line removed.
• Taiko test fixtures — both CertainBatchLookupTests and TxPoolContentListsTests updated for the new constructor signature.

Still worth fixing before merge

1. PR description is stale on several points. It still describes the original implementation and doesn't reflect the changes that landed:

• Mentions NewPayloadWithWitnessResponseV1Wire — that struct was deleted; encoding/decoding is hand-rolled in SszCodec against the spec's Union[None, T] layout.
• Says "witness field is set to [] for any non-VALID status" — it's now Union None (selector byte 0x00), not an empty list.
• Doesn't mention the new engine_newPayloadWithWitness JSON-RPC method (separate from the REST endpoint).
• Doesn't mention that SszEndpointHandlerBase.WriteErrorAsync was changed to emit application/json with {code, message} for all SSZ-REST error responses, not just the witness endpoint. This is consumer-visible behavior change for /engine/v*/... callers.
• Doesn't mention PayloadStatusWire.ValidationError SSZ list cap bumped 1024 → 8192 (spec correctness; affects all PayloadStatusV1 responses, not just witness).
• Doesn't mention that EngineRpcModule's constructor now requires IBlockTree and IWitnessGeneratingBlockProcessingEnvFactory. Confirmed in-tree: only Taiko subclasses, all updated. Worth a note for downstream forks.

These don't block merge but the changelog/release notes will be wrong without an update.

2. Test fixture duplication in EngineModuleTests.Amsterdam.cs. StubbedEngineRpcModule and FailingNewPayloadEngineRpcModule differ only in their override of engine_newPayloadV5 — yet each repeats the same ~20-argument constructor base call. Per the project's own AGENTS.md test guidelines:

"When parts of tests are similar (shared setup, common assertions, recurring scenarios), factor those parts into helper methods or helper types …"

Suggest a single helper that takes a Func<ExecutionPayloadV4, byte[]?[], Hash256?, byte[][]?, Task<ResultWrapper<PayloadStatusV1>>> and use it for both success and failure cases. The witness factory/scope/env/collector stubbing chain is also copy-pasted across tests — a small WitnessFactoryBuilder would let each test read in 3 lines instead of 15. Strictly a refactor for maintainability; the coverage is correct.

3. engine_newPayloadV5 made virtual purely for test overriding. This widens a production type's contract to support test subclassing. Acceptable, but if the V5 handler were injectable as a delegate or strategy on the module, you'd avoid the virtual modifier and the giant constructor-arg duplication in the test stubs. Follow-up.

Carried over from round 1 (unchanged)

• Double execution — still re-executes via WitnessCollector.GetWitnessForExistingBlock after engine_newPayloadV5. The parent spec (Introduction of REST based + SSZ serialized new-payload-with-witness ethereum/execution-apis#773) was designed to eliminate this; please open a follow-up issue so the perf goal isn't forgotten.
• Witness.Keys still built and dropped on the SSZ path.

Nothing here blocks merge once the PR description is refreshed. Good iteration.

[LukaszRozmej]

Don't make things virtual just to override them in test, use decorator pattern instead, tests shouldn't shape production code API's

[Dyslex7c]

addressed all the reviews, opened a follow-up issue for double execution and updated PR description, @LukaszRozmej could you please re-review?

[LukaszRozmej]

Isn't avoiding double-execution sole purpose of this endpoint? Why not do that here? Why create issues?

[LukaszRozmej]

Overview

Implements POST /new-payload-with-witness per execution-apis#773, plus a parallel engine_newPayloadWithWitness JSON-RPC method. Core design correctly decouples the V5 execution step via a delegate and hand-rolls the Union[None, T] SSZ encoding the spec requires.

Spec compliance — ✅ matches the spec

Verified against src/engine/rest.md from execution-apis#773:

• NewPayloadWithWitnessResponseV1 container: 1 byte status + 3 × 4-byte offsets for the three Union fields → 13-byte fixed header. Implementation matches (FixedHeaderBytes = 1 + 4 + 4 + 4).
• Status byte mapping: VALID=0, INVALID=1, SYNCING=2, ACCEPTED=3, INVALID_BLOCK_HASH=4 — all present in EngineStatusToSsz, and PayloadStatus.InvalidBlockHash was added correctly.
• VALIDATION_ERROR_MAX = 8192: PayloadStatusWire.ValidationError cap bumped, and EncodeNewPayloadWithWitnessResponse uses the same constant.
• UTF-8 truncation: TruncateUtf8 correctly walks backward over continuation bytes (0x80..0xBF) and drops the partial codepoint. Covered by EncodePayloadStatus_truncation_does_not_split_multibyte_utf8_codepoint.
• Union[None, T] hand-rolled: 0x00 for None, 0x01 + variant body for Some. The [SszCompatibleUnion] generator's inability to encode the 0x00 selector is the right reason to hand-roll, and the comment in SszWireTypes.cs documents it well.
• Witness emptiness rule: hasWitness = witness is not null && ps.Status == PayloadStatus.Valid — covers both spec conditions ("None when status is not VALID or no witness was produced").
• ExecutionWitnessV1 wire: state/codes/headers (3 fields). Correctly drops Nethermind's internal Witness.Keys from the wire — spec defines only 3 fields.
• HTTP semantics: 200+octet-stream on VALID/INVALID/SYNCING, 400 for malformed JSON / UnsupportedFork (-38005), 401 auth failure, 405 non-POST, 500 for other engine errors, 415 for wrong content type. All errors use application/json.
• Capability rest_engine_newPayloadWithWitness registered in EngineRpcCapabilitiesProvider.

Spec interpretation note: spec text in §Specification point 3 says "the witness field MUST be empty ([])" for non-VALID, which is awkward given the field's type is Union[None, ExecutionWitnessV1]. The PR treats this as the None variant, which is the only sensible reading consistent with the structure definition higher in the same spec.

Issues

1. Dead code: IBlockTree constructor param on EngineRpcModule

public partial class EngineRpcModule(... IBlockTree blockTree, ILogManager logManager)
{
    protected readonly IBlockTree _blockTree = blockTree ?? throw new ArgumentNullException(nameof(blockTree));

_blockTree is declared protected readonly but never referenced anywhere in EngineRpcModule.cs or any partial (.Paris/Cancun/Prague/Osaka/Amsterdam). The witness handler already takes its own IBlockTree directly via the DI factory in MergePlugin.cs. Removing this would:

• Drop one unused public constructor parameter (still a breaking change for downstream forks subclassing EngineRpcModule, but a smaller one).
• Revert the noise added to TaikoEngineRpcModule.cs, CertainBatchLookupTests.cs, TxPoolContentListsTests.cs, and the manual-construction test in EngineModuleTests.V3.cs.

The PR description claims EngineRpcModule gains two params (IBlockTree and IWitnessGeneratingBlockProcessingEnvFactory), but only IBlockTree is added — and it isn't used. Either wire the field in for some forthcoming purpose or remove it.

2. Witness-generation logic duplicated across the two handlers

NewPayloadWithWitnessSszHandler.TryGenerateWitness and NewPayloadWithWitnessHandler.TryGenerateWitnessForBlock are byte-for-byte the same:

1. TryGetBlock() + log on failure
2. BlockTree.FindHeader(parentHash, DoNotCreateLevelIfMissing) + log on null
3. using scope; collector.GetWitnessForExistingBlock(parent, block) with identical OperationCanceledException / Exception handling and identical log wording (the only diff is a prefix string).

Cleanest fix: have NewPayloadWithWitnessSszHandler call _engineModule.engine_newPayloadWithWitness(...) instead of engine_newPayloadV5(...), then SSZ-encode the resulting NewPayloadWithWitnessV1Result. That collapses both handlers' witness paths into one, and removes the SSZ handler's dependencies on IBlockTree and IWitnessGeneratingBlockProcessingEnvFactory — simplifying SszMiddlewareConfigurer (two Bridge<> calls go away) and SszMiddlewareTests setup.

3. Duplicate / inconsistent log severities for witness-null on VALID

In NewPayloadWithWitnessHandler.HandleAsync (and mirrored in the SSZ handler): when witness generation returns null, TryGenerate* already logs the specific reason at Warn ("could not decode block", "parent header not found") or Error ("witness generation failed: …"). Then the outer code logs again at Error with a generic "could not be generated" message.

Result: a benign re-org (missing parent) emits Warn + Error for the same event, which will trip alerting on noise. Drop the outer log — the inner sites already cover all branches with appropriate severities.

4. Hand-rolled JSON for error bodies (SszEndpointHandlerBase.WriteErrorAsync)

string json = $"{{\"code\":{jsonRpcCode},\"message\":{System.Text.Json.JsonSerializer.Serialize(message)}}}";

JsonSerializer.Serialize(message) correctly escapes the message, but the outer object is string-interpolated. Negligible but inconsistent — JsonSerializer.Serialize(new { code = jsonRpcCode, message }) is one allocation, no manual brace matching, and reads better.

5. WriteErrorAsync content-type change applies to all SSZ-REST endpoints (heads-up)

The PR description acknowledges this but it's worth re-confirming with CL teams: every previously-text/plain error response (auth 401s on /engine/v*/…, 404s for unknown methods, 413s, 400 malformed-SSZ, 500s) now emits application/json. The new REST spec mandates this format for /new-payload-with-witness only; the older /engine/v{N}/… paths aren't governed by this spec.

6. Minor: ownership of Witness? in WriteSszNewPayloadWithWitnessAsync is non-obvious

private static async Task WriteSszNewPayloadWithWitnessAsync(HttpContext ctx, PayloadStatusV1 status, Witness? witness)
{
    using Witness? w = witness;
    ...

Not a bug — the witness is created in TryGenerateWitness and not owned by the outer result's using — but a one-line comment on TryGenerateWitness saying "caller takes ownership" would help the reader.

Tests

• EngineModuleTests.Amsterdam.cs: solid coverage — VALID with witness, VALID with witness-gen failure (null parent), VALID with collector throw, SYNCING/INVALID skipping witness, engine-error propagation. The WitnessHandlerBuilder is a nice testable seam.
• SszMiddlewareTests: covers 200/VALID, 200/SYNCING, 200/null-witness-on-VALID, 400 malformed JSON, 405 non-POST, 415 wrong content-type, 404 versioned path, 400 UnsupportedFork, 500 internal error. Good breadth.
• SszCodecTests: bytewise verification of the 13-byte header and offsets is exactly the right test for hand-rolled encoding — would catch any regression to the offset layout.

Risks

• Double execution on VALID payloads — well-documented (PR body + TODO + #11636). Eliminates real-time attestation benefits the spec promised, but tracked as follow-up.
• engine_newPayloadWithWitness JSON-RPC is Nethermind-only — spec defines only the REST endpoint. Adding the JSON-RPC method is fine (matches the spec author's earlier prototype phase) but is not portable across other clients.
• PayloadStatusWire ValidationError cap 1024→8192 affects all PayloadStatusV1 SSZ responses (not just witness). Spec-correct, but changes merkleization of PayloadStatusV1 if any tooling has golden-byte fixtures.

Summary

Spec compliance is correct and the SSZ encoding has the right tests. Main asks: drop the unused IBlockTree from EngineRpcModule, collapse the duplicate witness-generation logic by having the SSZ handler delegate to engine_newPayloadWithWitness, and fix the duplicate Warn+Error logging when witness generation legitimately fails.

[Dyslex7c]

Isn't avoiding double-execution sole purpose of this endpoint? Why not do that here? Why create issues?

yes actually this needs a bigger refactor so in your review it asked to create a follow-up issue. Will implement this here and close the issue

Double execution — still re-executes via WitnessCollector.GetWitnessForExistingBlock after engine_newPayloadV5. The parent spec (ethereum/execution-apis#773) was designed to eliminate this; please open a follow-up issue so the perf goal isn't forgotten.

[hudem1]

why the "asmterdam payload" ? what's the link with amsterdam ?

[Dyslex7c]

Amsterdam is the fork which enabled EIP 7928 so witness capture was activated on this. This is also stale and was implemented during the AI sweep on zkEVM tests I believe. we can use the default test fork here instead

[hudem1]

is the endpoint enabled only when eip 7928 is enabled ? that's also what the comments above the class suggests

[Dyslex7c]

it should probably be available even when EIP 7928 is disabled. It was likely introduced to fix the failing EIP 7928 BAL tests

[hudem1]

Why do we need the entire directory of amsterdam at the top? Why some tests are added as ignored by hive? What are the reason some tests are failing and marked as known failing in the ZK pyspec tests project?

The entire directory of amsterdam was actually stale from some previous tests according to @Dyslex7c.
Some tests within the eip7928_block_level_access_lists test fixture folder (that was being tested) were still failing and it was just a way to keep track of the remaining ones to fix before the PR was fully ready from what i understood.

Anyway, i removed code related to zkevm tests from this PR as it was only partially supporting them. Also then this PR now focuses on the new_payload_with_witness endpoint with ssz tests. And full support for zkevm tests is coming in a PR very soon!

[asdacap]

Probably a bad idea to expose this.

[asdacap]

Since this is a standard ethereum change, I recommend modifying the IWorldStateScopeProvider instead. In the BeginScope, add a collectWitness flag. Then expose a Witness property which should be populated right after the write batch dispose. Then wrap the state db specific logic inside the scope provider, which is already there here, just the calling code is weird. This should reduce the number of wrapped classes.

[LukaszRozmej]

Since this is a standard ethereum change, I recommend modifying the IWorldStateScopeProvider instead. In the BeginScope, add a collectWitness flag. Then expose a Witness property which should be populated right after the write batch dispose. Then wrap the state db specific logic inside the scope provider, which is already there here, just the calling code is weird. This should reduce the number of wrapped classes.

This is standard endpoint but fairly non-standard use-case. We should kind of avoid pollution of the normal path?

[asdacap]

Something along this line. #12018

[hudem1]

Continued PR in #12048 as this one was from a fork