New payload with witness ssz

[hudem1]

Follow up of PR #11623 as this was from a forked repo

What types of changes does your code introduce?

• 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

Optional. Remove if not applicable.

Documentation

Requires documentation update

• Yes
• No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

• Yes
• No

If yes, fill in the details here. Remove if not applicable.

Remarks

Optional. Remove if not applicable.

[claude]

Claude finished @hudem1's task in 10m 33s —— View job

---

PR Review: New payload with witness SSZ

Tasks

• Load context and rule files
• Get PR diff and analyze changed files
• Review witness capture architecture
• Review SSZ codec and middleware
• Review DI patterns and registration
• Review tests
• Post inline comments
• Post final summary

---

Summary

This PR implements EIP-7928 engine_newPayloadWithWitness — a new Engine API method that executes a block and returns an execution witness (state trie nodes, bytecodes, headers) in a single call. The architecture is well thought out: a session-gated decorator chain (WitnessCapturingBlockProcessor → WitnessCapturingWorldStateProxy / WitnessCapturingTrieStore / WitnessCapturingHeaderFinder) with clean separation between the rendezvous (cross-thread coordination) and the recording logic. Both patricia and flat DB backends are covered.

No critical issues found. I found two High findings that block merge, four Medium findings that should be addressed, and a handful of Low items.

---

Findings

High

H1 – DI anti-pattern in BlockProcessingModule.cs:75 ([di-patterns.md])

The clean .AddScoped<IBlockAccessListManager, BlockAccessListManager>() was replaced with a lambda that manually lists all constructor arguments. This violates the repo's DI rules and will require updating the lambda whenever BlockAccessListManager gains or loses constructor parameters. Preferred fix: accept WitnessExecutionPredicate? directly in the constructor so the container wires it automatically. Fix this →

H2 – fallbackTrieStore not disposed in WitnessCapturingBlockProcessor.ProcessOne

worldStateManager.CreateReadOnlyTrieStore() returns a FlatReadOnlyTrieStore (flat backend) that holds a SnapshotBundle. WitnessCapturingTrieStore wraps it but is never disposed in the try/finally. Actual resource impact is low because BeginScope's ScopeCleanup nulls the bundle before return — but the code violates dispose discipline and will silently leak if the flat store ever acquires additional resources. Fix: fallbackTrieStore.Dispose() in the finally block.

---

Medium

M1 – LINQ on hot-path: WitnessTrieStoreRecorder.TouchedNodesRlp and GetWitness ([coding-style.md])

_rlpCollector.Select(static kvp => kvp.Value) is LINQ on a ConcurrentDictionary. The caller in GetWitness uses .Any() on the result. Both are prohibited by the no-LINQ rule. Replace with ConcurrentDictionary.IsEmpty (O(1), no alloc) for the Any() check and .Values for iteration.

M2 – Fragile cast from IWorldState to concrete type in WitnessCapturingMainProcessingModule.cs:41

(WitnessCapturingWorldStateProxy)ctx.Resolve<IWorldState>() breaks silently (throws InvalidCastException at startup) if any future plugin registers a decorator on top of IWorldState after this module. Register the proxy simultaneously as both IWorldState and WitnessCapturingWorldStateProxy in a single Autofac Register(...) call to avoid the cast entirely.

M3 – Fragile is NewPayloadWithWitnessSszHandler type-check in SszMiddleware.cs:87

The route-building loop excludes the witness handler by concrete type. Introduce an ISszEndpointHandler.RegisterInRoutingTable opt-out flag or stop registering the witness handler as ISszEndpointHandler altogether (it is already registered as a concrete singleton).

M4 – WitnessCaptureSession.TryArm docs incorrectly claim "post-CAS happens-before"

The trie and header recorders are written before the CAS, not after it. The <remarks> description inverts the ordering. More importantly, two concurrent TryArm callers can leave the session with the world-state recorder from winner A but the trie/header recorders from loser B (overwritten before A's CAS). The comment acknowledging serial use ("contention is theoretical") is correct, but the memory-model explanation is wrong and should be fixed to avoid future confusion.

---

Low

L1 – TruncateUtf8 silently drops the entire string if the first character is multi-byte (SszCodec.cs:115) — produces Some("") instead of a warning.

L2 – node.Keccak null guard missing in WitnessCapturingTrieStore.FindCachedOrUnknown:42 — dirty nodes with no hash yet can theoretically reach the recorder and throw NullReferenceException.

L3 – No timeout before CancelWitnessRequest in NewPayloadWithWitnessHandler.cs:70 — if the synchronous-inside-ProcessOne assumption ever breaks, the witness is silently dropped. Low risk given the test coverage, but worth a comment.

---

Test coverage

The new EngineModuleTests.WitnessCapture.cs is thorough: rendezvous unit tests, integration tests against a real Amsterdam chain, regression guard for single-execution, and round-trip SSZ codec tests. Assert.Multiple is not used where multiple independent assertions fire on the same result — would be worth adding in the larger integration tests.

The SszCodecTests additions provide strong golden-byte coverage of the new response format including offset arithmetic verification.

---

Verdict: Not mergeable until H1 and H2 are resolved.

[claude]

High – DI anti-pattern (violates di-patterns.md)

The previous one-liner .AddScoped<IBlockAccessListManager, BlockAccessListManager>() has been replaced with a manual lambda that explicitly lists every constructor argument. The rule file (di-patterns.md) calls this exact pattern an anti-pattern:

"Using the form .Add<IFoo>(ctx => new Foo(ctx.Resolve<Dep1>(), ctx.Resolve<Dep2>())) is an anti-pattern. It will cause changes to the wiring when Foo adds new dependencies, which increases review load."

Preferred fix: have BlockAccessListManager accept WitnessExecutionPredicate? directly as a constructor parameter, then re-register it with the simple AddScoped<IBlockAccessListManager, BlockAccessListManager>(). The container can resolve the optional WitnessExecutionPredicate? automatically if it is registered in witness-capable scopes and omitted elsewhere.

// In BlockProcessingModule, back to the clean form:
.AddScoped<IBlockAccessListManager, BlockAccessListManager>()

// In BlockAccessListManager constructor:
public partial class BlockAccessListManager(
    IWorldState stateProvider,
    ISpecProvider specProvider,
    IBlockhashProvider blockHashProvider,
    ILogManager logManager,
    IBlocksConfig blocksConfig,
    IWithdrawalProcessorFactory withdrawalProcessorFactory,
    PrewarmerEnvFactory? prewarmerEnvFactory = null,
    PreBlockCaches? preBlockCaches = null,
    IReadOnlyTxProcessingEnvFactory? readOnlyTxProcessingEnvFactory = null,
    WitnessExecutionPredicate? witnessExecutionPredicate = null)  // ← add this

Then derive isWitnessExecution from witnessExecutionPredicate?.IsActive in the class body.

[claude]

Medium – fallbackTrieStore is never disposed

CreateReadOnlyTrieStore() on the flat backend returns a FlatReadOnlyTrieStore that holds a disposable SnapshotBundle. WitnessCapturingTrieStore.Dispose() forwards to baseStore.Dispose(), but fallbackTrieStore is never disposed — it is neither wrapped in a using nor disposed in the finally block.

In practice the impact is low because FlatReadOnlyTrieStore._bundle is nulled out by the ScopeCleanup returned from BeginScope(parentHeader) (the using in GetWitness), so the actual dispose would be a no-op. However, the code violates dispose discipline: any future change to the store that adds resources beyond _bundle would silently leak.

// Fix: dispose in the finally block alongside session.Disarm()
WitnessCapturingTrieStore fallbackTrieStore = new(worldStateManager.CreateReadOnlyTrieStore(), session);
...
finally
{
    session.Disarm();
    fallbackTrieStore.Dispose();   // ← add this
}

[claude]

Medium – LINQ on hot-path property (violates coding-style rule)

_rlpCollector.Select(...) is a lazy LINQ query. Every consumer pays the delegate allocation and iterator overhead. The rule file explicitly prohibits LINQ when a simple loop works.

WitnessGeneratingWorldState.GetWitness also calls .Any() on this property — two LINQ allocations on what can be a per-block path.

// Preferred: expose counts directly to avoid the Any() call at the use site
public bool IsEmpty => _rlpCollector.IsEmpty;      // ConcurrentDictionary.IsEmpty is O(1), no alloc

// And for the iteration:
public IEnumerable<byte[]> TouchedNodesRlp => _rlpCollector.Values;  // no LINQ, no extra allocation

Then in WitnessGeneratingWorldState.GetWitness:

// Replace: if (!trieRecorder.TouchedNodesRlp.Any())
if (trieRecorder.IsEmpty)

[claude]

Medium – Fragile cast from IWorldState to concrete decorator type

builder.AddSingleton<WitnessCapturingWorldStateProxy>(ctx =>
    (WitnessCapturingWorldStateProxy)ctx.Resolve<IWorldState>());

This cast assumes IWorldState resolves to exactly WitnessCapturingWorldStateProxy — i.e., no other decorator sits on top of it in this scope. If any future plugin adds a decorator on IWorldState after this module loads, ctx.Resolve<IWorldState>() returns the outer decorator and the cast throws InvalidCastException at startup.

The same issue applies to the WitnessCapturingHeaderFinder bridge on line 47.

The underlying need — giving WitnessCapturingBlockProcessor direct access to the proxy — suggests using Autofac.IRegistrationSource or a keyed service. A simpler short-term fix is to register the proxy both ways in a single lambda:

builder.Register(ctx =>
{
    IWorldState inner = ctx.Resolve<IWorldState>();
    return new WitnessCapturingWorldStateProxy(inner, ctx.Resolve<WitnessCaptureSession>());
})
.As<IWorldState>()
.As<WitnessCapturingWorldStateProxy>()
.InstancePerLifetimeScope();

This avoids the cast entirely; both IWorldState and WitnessCapturingWorldStateProxy resolve the same instance.

[claude]

Medium – Fragile is type-check to exclude a handler from route-building

if (h is NewPayloadWithWitnessSszHandler) continue;

This couples the route-building loop to a specific concrete type. If NewPayloadWithWitnessSszHandler is subclassed, or a second "non-routable" handler is introduced, this check silently breaks.

A cleaner design: introduce an opt-out interface or property on ISszEndpointHandler:

// In ISszEndpointHandler:
/// <summary>False for handlers that manage their own dispatch path (e.g. the witness fast-path).</summary>
bool RegisterInRoutingTable => true;

Then the loop becomes:

if (!h.RegisterInRoutingTable) continue;

Alternatively, don't register NewPayloadWithWitnessSszHandler as ISszEndpointHandler at all (it's already registered as a concrete singleton in SszMiddlewareConfigurer), removing the need to exclude it here.

[claude]

Medium – TryArm memory-ordering comment is incorrect

The <remarks> says:

"The world-state recorder is the primary slot — the CAS on it gates the operation; the other two are written under the post-CAS happens-before..."

But the code writes _trieRecorder and _headerRecorder before the CAS, not after it. What the comment should say is that the writes happen-before the CAS release (i.e., sequentially consistent via Volatile.Write), so any reader who observes _worldStateRecorder != null via Volatile.Read is guaranteed to also observe the earlier writes. The current phrasing inverts the ordering and is misleading.

More critically: if two threads race on TryArm, thread B can overwrite _trieRecorder and _headerRecorder between thread A's volatile writes and thread A's CAS. Thread A then wins the CAS but holds the world-state recorder from A paired with trie/header recorders from B — an inconsistent state. The comment notes that "the main processing pipeline drives blocks serially so contention is theoretical", which is accurate, but the comment should reflect the actual invariant (serial use) rather than claiming a memory-ordering guarantee that doesn't hold under true concurrent use.

[claude]

Low – TruncateUtf8 silently returns an empty slice if the first character is multi-byte

If ValidationError begins with a multi-byte character that extends past maxBytes, the loop backs all the way to i = 0 and returns utf8[..0] — an empty byte array. This means a ValidationError that starts with a non-ASCII character can be silently dropped in the response rather than truncated at a character boundary. The encoded response would include hasError = true but errorLen = 1 + 0 = 1 (just the 0x01 selector with an empty body), which is a valid "Some("")" but may confuse a decoder expecting at least one character.

For resilience, consider returning utf8[..maxBytes] (potentially invalid UTF-8) when i reaches 0, or logging the truncation. The current behaviour is silent and may produce an unexpected empty error string.

[claude]

Low – node.Keccak can be null for dirty in-memory nodes

node.Keccak is Hash256?. When a dirty (not-yet-persisted) node is returned from cache and its Keccak hasn't been computed yet, passing it to recorder.Record(node.Keccak, node) passes null as the Hash256 key, which will throw NullReferenceException inside ConcurrentDictionary<Hash256AsKey, byte[]>.

In practice this path should not fire: FindCachedOrUnknown is keyed by hash, so a cached hit at that hash should have Keccak == hash. However, the write-through (readOnly: false) path decorates the live store, and dirty nodes can be in the cache with a non-Unknown NodeType but a null Keccak.

Add a null guard:

if (node.NodeType != NodeType.Unknown
    && node.Keccak is { } nodeHash   // ← guard against dirty nodes with no hash yet
    && session.TrieRecorder is { } recorder)
    recorder.Record(nodeHash, node);

[claude]

Low – Missing timeout guard on captureTask

The handler calls CancelWitnessRequest if the task is not completed, but this check happens only once, synchronously after engine_newPayloadV5 returns. If the block processor is processing on a background thread and engine_newPayloadV5 returns before processing finishes (e.g. it's queued but not yet executed), captureTask.IsCompleted would be false even for a VALID result, and the witness would be silently dropped.

The current comment acknowledges this with "BlockProcessor normally completes the TCS synchronously inside ProcessOne", and the test BlockProcessor_completes_rendezvous_task_synchronously_inside_newPayloadV5 asserts it. But there is no fallback timeout — if the assumption ever breaks (e.g. due to a future refactor that makes block processing async), CancelWitnessRequest fires and the witness is silently lost with only a WARN log.

Consider a short Task.WhenAny(captureTask, Task.Delay(TimeSpan.Zero)) (yield and re-check) or a configurable deadline before cancelling, to catch the common "already completed before we check" case more robustly. At minimum, the log message should be elevated to WARN (it already is) and include the processing time so operators can detect the broken-assumption case.