Avoid ArrayPool.Shared-only finally blocks

[benaadams]

Changes

• Document that standard GC-backed code should not add or retain a try/finally solely for ArrayPool<T>.Shared.Return(...) after an unexpected exception aborts the operation.
• Require returns on normal paths and before expected failures are handled, retried, or rethrown to a recovery boundary.
• Keep existing finally blocks on recoverable/retried paths; do not replace them with catches solely for Return.
• Remove seven production and one test finally blocks whose only cleanup was returning a directly rented shared array on operation-aborting unexpected failures.
• Return immediately before deliberate throws while ownership remains local; CapabilityConverter and InputExtractor retain this behavior for their known failure paths.
• Exclude zkVM guest entry-point code, assemblies listed in BFLAT_REFS in Nethermind.Stateless.ZiskGuest/Makefile, SafeArrayPool<T>.Shared, using-based ownership helpers, custom pools, and other ownership/resource contracts from the guidance. Generated SSZ cleanup remains unchanged because its output is linked through a BFLAT_REFS assembly.
• Preserve finally cleanup for snapshot retries, channel decompression, malformed JSON/SSZ input, public callbacks, and other paths where failure is expected and handled.
• The large EraWriter hunk is mechanical de-indentation; its semantic change is only moving a shared-array return from finally to the successful path.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change)
• New feature (a non-breaking change)
• 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

Current head:

• Scoped dotnet format whitespace checks for the affected C# files — passed.
• dotnet format whitespace tools/StatelessInputGen/ --folder --verify-no-changes — passed.
• dotnet build src/Nethermind/Nethermind.slnx -c Release --no-restore — 0 warnings, 0 errors.
• dotnet build tools/StatelessInputGen/StatelessInputGen.csproj -c Release --no-restore — passed with five pre-existing assembly-version warnings.

Earlier affected-suite validation executed 2,490 tests: 2,486 passed and four skipped. No pool-identity test was added because ArrayPool<T>.Shared does not guarantee which instance a subsequent rent returns; such a test would be nondeterministic rather than proving ownership.

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[claude]

Claude finished @benaadams's task in 3m 33s —— View job

---

Review: Document ArrayPool.Shared exceptional-path return guidance

• Read the diff and surrounding rule file
• Verify the technical claim about ArrayPool<T>.Shared
• Check consistency with existing repo patterns (SafeArrayPool<T>, ArrayPoolList<T>, ArrayPoolDisposableReturn)
• Post review

The core claim is correct: ArrayPool<T>.Shared (TlsOverPerCoreLockedStacks) holds nothing but GC references, so a skipped Return costs a future allocation, not a leak — and that allocation is noise next to the cost of the throw. Docs-only change, no runtime impact, no tests needed. The issue is scoping: as written the rule is applied mechanically by agents, and this repo has two look-alike constructs it doesn't carve out.

---

Findings

Medium — carve-outs missing for SafeArrayPool<T>.Shared and the using-based helpers

.agents/rules/robustness.md:15 (inline comment with suggestion)

• SafeArrayPool<T>.Shared is one token away from the covered API and is the more common idiom in this codebase (EvmPooledMemory, Discv5/PacketCodec, TransactionProcessor, ArrayPoolList<T>, …). In the standard build it is ArrayPool<T>.Shared (SafeArrayPool.std.cs:14); under EnableZkEvm=true (Directory.Build.targets:5-10) it swaps to a custom pool documented as "zkVM is single-threaded with no GC. Allocations are forever" (SafeArrayPool.zkevm.cs:16-21). The "remains GC-reclaimable" rationale simply doesn't hold there — and Nethermind.Core / Nethermind.Trie, which call ArrayPool<T>.Shared.Return directly, are compiled into that build as well. The generic "do not generalize to custom pools" escape hatch doesn't help a reader who never registers SafeArrayPool as a custom pool; name it.
• ArrayPoolDisposableReturn (Nethermind.Core/Buffers/ArrayPoolDisposableReturn.cs:21, 5 call sites) and ArrayPoolList<T> are first-party using wrappers that compile to precisely "a try/finally solely to guarantee a shared-pool return". Bullet 14 mandates using for IDisposable; bullet 15 read literally discourages what these helpers do. Unwinding them into hand-rolled returns invites double-return / use-after-return, which corrupts the pool silently — strictly worse than the missed return being optimised away. The rule should say the aliasing hazard is the thing it is protecting, not just the allocation.

Fix this →

Low — say the rule is prospective

There are ~49 existing finally-based shared-pool returns in src/Nethermind. "Do not add" is prospective on a careful reading, but an agent doing a cleanup pass can read it as license to remove them — pure diff churn, and each rewrite is a chance to introduce a double return. One clause ("leave existing ones alone") closes it.

Low — the cost argument has a weak edge

"Dwarfed by the cost of throwing" is solidly true for small rentals on a genuinely exceptional path. It thins out for rentals ≥ 85 KB (LOH) on a path where the exception is caught and the operation repeats at high frequency — e.g. per-message decode failures from untrusted P2P input. There, abandoning drains the pool bucket and adds sustained LOH/Gen2 pressure that a finally avoids at zero steady-state cost. Worth a half-sentence qualifier, or explicitly scoping the rule to exceptions that propagate.

Low — bullet length

At four sentences this is the longest bullet in the file, in a rules doc that agents load on nearly every task, and AGENTS.md calls out reviewer fatigue as a first-order concern. The suggestion in the inline comment adds content; consider whether the whole thing wants to be two bullets (the rule, then the non-generalisation list) instead of one paragraph.

---

Verdict

No correctness, security, or performance risk in the change itself — it's a rules doc, and the claim it documents is accurate for the standard build. The Medium is worth resolving before merge because this file is machine-consumed guidance: the gap between ArrayPool<T>.Shared and SafeArrayPool<T>.Shared is one token wide, and the wrong side of it is a retain-forever pool in a no-GC environment. The suggested one-line replacement in the inline comment covers all four findings.
· feature/arraypool-shared-return-guidance

[Copilot]

Pull request overview

Updates the repository’s robustness guidelines to clarify resource-management expectations around ArrayPool<T>.Shared.Return(...) in exceptional control flow, aiming to reduce unnecessary try/finally usage while keeping the guidance narrowly scoped to the BCL shared pool.

Changes:

• Adds documentation stating that ArrayPool<T>.Shared.Return(array) does not require try/finally solely for exceptional-path cleanup.
• Explicitly warns not to generalize this exception to custom pools or other ownership/resource-release contracts.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[benaadams]

Bro, why are we throwing frequent exceptions

Do you hate performance?

[claude]

Medium — the exception needs two more carve-outs to be safe to apply mechanically in this repo:

1. SafeArrayPool<T>.Shared is spelled almost identically and is the dominant idiom here (EvmPooledMemory, Discv5/PacketCodec, TransactionProcessor, ArrayPoolList<T> …). In the standard build it is ArrayPool<T>.Shared (SafeArrayPool.std.cs:14), but under EnableZkEvm=true it becomes a custom retain-forever pool whose own docs say "zkVM is single-threaded with no GC. Allocations are forever" (SafeArrayPool.zkevm.cs:16-21). The "remains GC-reclaimable" rationale is false in that configuration — and Nethermind.Core / Nethermind.Trie, which call ArrayPool<T>.Shared.Return directly, are compiled into it too.
2. The repo already ships using-based helpers that are a try/finally around a shared-pool return — ArrayPoolDisposableReturn (Nethermind.Core/Buffers/ArrayPoolDisposableReturn.cs:21, 5 call sites) and ArrayPoolList<T>. Bullet 14 mandates using for IDisposable; this bullet, read literally, discourages exactly what those helpers do. Unwinding them into hand-rolled returns risks double-return / use-after-return, which corrupts the pool silently — a much worse outcome than the missed return this rule is optimising away.

Also worth stating that the rule is prospective (there are ~49 existing finally-based shared-pool returns; they shouldn't be stripped as churn).

Suggested change

- Do not add a `try`/`finally` solely to guarantee `ArrayPool<T>.Shared.Return(array)` after an exception. If this exact shared-pool return is skipped, the abandoned array remains GC-reclaimable, and any replacement allocation is dwarfed by the cost of throwing the exception. Return it on the normal path. Do not generalize this exception to custom pools or other `Return`, `Release`, or `Dispose` contracts, which may track ownership, use reference counting, or hold non-GC resources.

- Do not add a `try`/`finally` solely to guarantee `ArrayPool<T>.Shared.Return(array)` after an exception. If this exact shared-pool return is skipped, the abandoned array remains GC-reclaimable, and any replacement allocation is dwarfed by the cost of throwing the exception. Return it on the normal path, leave existing `finally`-based returns alone, and never add a return on a path where the array may already have been returned or handed off — a double return corrupts the pool silently. Do not generalize this exception to `SafeArrayPool<T>.Shared` (the zkEVM build swaps it for a retain-forever pool with no GC behind it), to the `using`-based helpers `ArrayPoolDisposableReturn` / `ArrayPoolList<T>`, or to other `Return`, `Release`, or `Dispose` contracts, which may track ownership, use reference counting, or hold non-GC resources.

[benaadams]

Partially fair, but legacy shouldn't get a pass

[benaadams]

Updated in 1f60687. I incorporated the EnableZkEvm/SafeArrayPool<T>.Shared carve-out, explicitly permit the using ownership helpers, and added the double-return/hand-off warning.

I also tightened the rule around direct ArrayPool<T>.Shared use: don't add or retain a try/finally solely for Return; return immediately before a deliberate throw while the code still owns the array; and keep the return in a finally when that finally is already required for other cleanup.

Following that rule, I removed 35 ArrayPool-only finally blocks. I retained the zkVM-compatible Core/Trie/SSZ cases, SafeArrayPool<T>.Shared cases, and the existing SocketClient cleanup finally that also disposes the stream.

[benaadams]

Addressed the remaining handled/repeated-exception edge in 761a2a2.

The guidance now distinguishes unexpected failures that abort the operation from expected failures that are handled, retried, or mapped at a recovery boundary. I audited all 34 production finally removals: six cross such a boundary and now return from targeted catches without adding a finally solely for ArrayPool<T>.Shared.Return; the remaining 28 are operation-aborting unexpected paths.

This covers snapshot download retries, malformed channel and multi-segment SSZ input, capped-array JSON failures, and per-block stateless-input extraction failures. The PR description has the audit and validation details.

@claude review

[claude]

Claude finished @benaadams's task in 7m 25s —— View job

---

Review: ArrayPool.Shared finally-block removals

• Read the full diff (19 files)
• Audit each removal for double-return / use-after-return
• Audit each removal for changed control flow
• Check the six recovery-boundary catches against their actual boundaries
• Post review

I traced every one of the 35 removals. No double-return, no use-after-return, and no control-flow regression — the two rewrites that changed shape (PayloadByNumberProtocol return null → break + guarded decode; BlockCachePreWarmer return → warmed flag under a catch-all) are both semantically identical to the originals. The InputExtractor case is the one with real aliasing potential and it's handled correctly: DecodeInput returns the buffer before throwing FormatException, and because the tuple deconstruction never runs on that path the caller's buffer stays null, so the new if (buffer is not null) Return(buffer) in the catch cannot double-return.

The SnapshotDownloader catch filters are the best-audited pair in the PR — catch (Exception e) when (e is IOException or HttpRequestException) matches DownloadWithRetryAsync's retry filter (InitDatabaseSnapshot.cs:131) exactly.

Two of the remaining four don't match their boundary.

---

Findings

Medium — CappedArrayJsonConverter: catch (JsonException) misses the common malformed-element cases

CappedArrayJsonConverter.cs:37 · inline

The only registered instantiation is CappedArrayJsonConverter<int> (EthereumJsonSerializer.cs:137), so elementConverter is the BCL Int32Converter → Utf8JsonReader.GetInt32(), which throws InvalidOperationException on a non-number token ([1, true]) and FormatException on an out-of-range value ([1, 99999999999999]). S.T.Json rewrites those to JsonException only in JsonSerializer.ReadCore, outside this converter — so they escape the filter. Only reader.Read() itself throws JsonException inside the try.

This is the untrusted, per-request, recover-and-continue path the catch was added for, and the buffer doubles (Rent(buffer.Length * 2)), so a long valid prefix followed by one bad element abandons the largest rental rather than the initial 16.

Medium — SSZ generator output is compiled with EnableZkEvm=true, contradicting the new carve-out

SszGenerator.cs:625 · inline

Nethermind.Stateless.ZiskGuest/Makefile:44 builds with -p:EnableZkEvm=true; that project references Nethermind.Stateless.Executor, whose csproj carries an unconditioned analyzer reference to Nethermind.Serialization.SszGenerator (line 11). Global properties flow to project references, so SszCodecHelpers and every generated Merkleize*/Decode compiles into an EnableZkEvm=true assembly — where "the abandoned array remains GC-reclaimable" is false ("zkVM is single-threaded with no GC. Allocations are forever"), and where these call ArrayPool<T>.Shared directly, so the SafeArrayPool carve-out doesn't cover them either.

The PR description says "I retained the zkVM-compatible Core/Trie/SSZ cases" — Core/Trie yes, but 12 SSZ generator finally blocks were removed. Probably harmless in practice (a guest-side exception aborts the proof run, so retention is moot), but the rule and the change currently disagree. Either restore them or write that rationale into robustness.md and correct the description.

Low — ChannelDecoder: filter narrower than the boundary

ChannelDecoder.cs:52 · inline

DecodingPipeline.cs:71 recovers with a bare catch (Exception e) and continues the loop, so every exception from CopyDataWithLimit is expected-and-repeated against untrusted L1 DA data. InvalidDataException is the realistic case for both DeflateStream and BrotliStream and the rental is 4 KiB, so exposure is small — noting it only because the audit claims boundary-matching.

Same shape in the SSZ sequence decode: SszMiddleware.cs:245 recovers InvalidDataException or EndOfStreamException, and :264 recovers everything else with a 500.

Low — leftover bare block reads as a deletion typo

InitDatabaseSnapshot.cs:223-224 · inline

byte[] checksum; followed by a naked { … } where the try was. Compiles, behaviour unchanged (the await using would dispose at method end anyway), but a bare block will stop every future reader.

Low — large rentals on recovered RPC paths

PayloadBodiesDirectResponseWriter rents TxDecoder.GetLength(tx), which exceeds 85 KB (LOH) for blob-carrying txs, and HexWriter.WriteHexString documents its input as unbounded. Both sit under per-request JSON-RPC error handling. Utf8JsonWriter.WriteRawValue buffers rather than doing I/O, so a client disconnect surfaces on Flush rather than here — which is why I've scored this Low rather than folding it into the Medium above. Worth a sentence in the audit confirming nothing recoverable throws between the rent and the return.

Low — diff shape

PersistentBlobTxDistinctSortedPool (−83/+71) and EraWriter (−64/+58) are almost entirely re-indentation with a two-line semantic change buried in each. Not wrong, just expensive to review against AGENTS.md's reviewer-fatigue guidance — worth calling out in the PR body which hunks are pure re-indent.

---

Notes

The rule text itself reads well now — the EnableZkEvm carve-out, the using-helper permission, and the double-return warning all landed, and the "unexpected aborts vs. expected-and-recovered" split is the right axis to cut on.

I could not run dotnet build in this environment (permission denied), so I verified definite-assignment by inspection only: checksum, warmed, parsed, payload, and cappedArray are each assigned on every path reaching their use, and incomingMessage is assigned before the try in EthStatsMessageParser. The PR's reported clean build is consistent with that.

Verdict: no leak, no corruption, no consensus or control-flow risk. The two Mediums are audit-accuracy issues rather than defects — the CappedArrayJsonConverter filter is the one I'd actually change before merge.
· feature/arraypool-shared-return-guidance

[benaadams]

Addressed in d734042.

The recoverable/retried paths now keep their original finally blocks instead of replacing them with targeted catches. That restores complete coverage for malformed JSON, channel decompression, snapshot retry, and multi-segment SSZ failures without duplicating recovery-boundary exception filters. All generated SSZ returns were also restored because the output is compiled with EnableZkEvm=true.

The rule now explicitly says not to replace an existing recovery-path finally with catches solely for Return, and the checksum method's bare scope is now an explicit await using block. There are no added catches relative to master.

Current-head validation:

• main solution: 0 warnings, 0 errors
• StatelessInputGen: 0 errors, five pre-existing assembly-version warnings
• whitespace verification passed

@claude review

[claude]

Claude finished @benaadams's task in 5m 46s —— View job

---

Review: ArrayPool.Shared finally-block removals (head d734042)

• Re-read the rule text and the full diff at current head
• Verify the four previously-flagged items are actually restored
• Audit each remaining removal for double-return / use-after-return / control-flow change
• Check the recovery-boundary claim for every removal that sits under per-request error handling
• Post review

The diff shrank from 19 files to 12. SszGenerator (12 finally blocks), CappedArrayJsonConverter, ChannelDecoder, SnapshotDownloader, BlockCachePreWarmer, PayloadByNumberProtocol and EthStatsMessageParser are all back to their master shapes, and git diff origin/master...HEAD | grep "^[+-].*catch" returns exactly one line — the removed bare catch in InputExtractor. The claim of no added catches relative to master holds.

Eleven finally blocks removed: 10 in src/Nethermind production code, 1 in SszCodecTests. That matches the description.

---

Verification

No double-return, no use-after-return. Every added Return sits on exactly one reachable path. The only ordering that needed checking was PayloadBodiesDirectResponseWriter — HexWriter.WriteHexString(writer, buffer.AsSpan(0, length), …) completes before the Return, and Utf8JsonWriter.WriteRawValue copies rather than retaining the span, so the chunked path is safe.

No control-flow change. Two rewrites changed scope shape and both are equivalent:

• InitDatabaseSnapshot.ComputeChecksumAsync — the bare block is now await using (FileStream …); checksum is assigned unconditionally at the end of that scope, and the FileStream disposes at the same point as the old await using declaration did.
• EraWriter.Finalize — using ArrayPoolList<byte> indexBytes moved from try-scope to method-scope, so it now disposes after FinalizeChecksum(). Harmless: its only consumer (WriteEntry(…, indexBytes.AsMemory(), …)) is awaited well before.

PersistentBlobTxDistinctSortedPool.TryGetBlobsAndProofsV1 has no early return between the three rents and the three returns — only continue/break — so the de-indent is behaviour-preserving.

The "nothing expected throws in the window" claim checks out for the three removals that do sit under a live per-request recovery boundary:

• HexWriter.WriteHexString — pure computation plus Utf8JsonWriter writes. Nethermind's StreamPipeWriter buffers into segments; no I/O in GetSpan/Advance, so a client disconnect surfaces on flush, not here.
• PayloadBodiesDirectResponseWriter — TxDecoder.Encode into a buffer sized by GetLength on the same tx.
• TryGetBlobsAndProofsV1 — GetBlobsHandlerV4's outer catch rethrows to the JSON-RPC layer, so this is a repeated boundary, but the only throw sources are RocksDB TryGetMany and index arithmetic on already-validated wrappers, and MaxRequest = 128 caps the rentals at ~12 KB + 2×2 KB. Well under LOH.

InputExtractor is the one case with real aliasing potential and it's correct: DecodeInput still returns the buffer immediately before throw new FormatException for invalid hex, and the caller's try/finally around the file write is retained — correctly, since that sits inside the per-block catch (Exception ex) when (ex is IOException or FormatException or …) recovery loop. The removed bare catch only covered non-FormatException escapes from Bytes.FromUtf8HexString, which propagate past the caller's filter and abort the tool.

Session.Invoke drops the guaranteed Array.Clear alongside the Return, but only on the path where the array is abandoned and therefore unreachable — no delegate rooting, so no Session retention. The skipped activityObserver?.OnSessionDisconnected on a throwing handler is pre-existing behaviour; the old finally didn't swallow either.

---

Findings — all Low

Low — SlotFile.TryRead is the widest window opened here · SlotFile.cs:56-58 · inline

size is bounded only by 64 * MiB, and action is a caller-supplied callback on a public API — so whether the escape is "unexpected and aborting" is decided outside this file. For today's single caller it holds narrowly: RecordedBalStore.Get catches RlpException inside the callback. But BlockAccessListDecoder raises RlpException only for its own ordering check (:214) and otherwise leans on Rlp.ValueDecoderContext, which doesn't uniformly normalise overruns — a truncated slot can surface as IndexOutOfRangeException and escape, abandoning up to 64 MiB on a per-block replay path. Widening that inner filter to catch (Exception) closes it and restores the logged-warning behaviour the callback clearly intends.

Low — two hunks are ~95% re-indentation

PersistentBlobTxDistinctSortedPool (−83/+71) and EraWriter (−64/+58) move ~190 lines to delete two finally blocks. Correct, but AGENTS.md ranks minimal diff and reviewer fatigue as first-order, and these two are where the guidance is most expensive to apply for the least benefit (12 KB and 64 KB rentals on paths that abort anyway). Worth noting which hunks are pure de-indent in the PR body so a reviewer can skip them, or considering leaving these two alone under the "do not add" reading.

Low — the rule is now the largest topic in robustness.md

Three bullets, ~6 sentences, in a file loaded on nearly every task. The content is right and the carve-outs all landed, but the ratio is now inverted relative to async void and IByteBuffer ownership, which cause worse bugs. Consider compressing bullets 1 and 2 into one — they're the same rule cut along the expected/unexpected axis, and the "may remain in a finally already required for other cleanup" clause duplicates bullet 2's "keep an existing finally".

---

Notes

git diff --check is clean. I could not run dotnet build in this environment (command requires approval), so definite assignment was verified by inspection: checksum, nodes, and chunk-free locals are each assigned on every path reaching their use, and tdOffsets is [] when !needsTd with the Return correspondingly guarded. Your reported clean build is consistent with that.

Verdict: no leak, no pool corruption, no control-flow or consensus risk. The two Mediums from the previous round are resolved at the source rather than papered over, and the four inline items are all restored to master shape. Nothing blocking.
· feature/arraypool-shared-return-guidance

[claude]

Low — this is the widest abandonment window the PR opens: size is bounded only by the 64 * MiB guard on line 54, so a single escape drops an up-to-64 MiB LOH rental, and action is a caller-supplied ReadOnlySpanAction<byte, TArg> on a public API, so the guidance's "unexpected and aborts the operation" premise depends on every future caller rather than on code visible here.

For the one caller today it holds by a hair. RecordedBalStore.Get (RecordedBalStore.cs:36-41) wraps the decode in catch (RlpException) inside the callback, so the expected corrupt-slot case never reaches this frame. But BlockAccessListDecoder throws RlpException only for its own semantic check (BlockAccessListDecoder.cs:214) and otherwise relies on Rlp.ValueDecoderContext for bounds, which does not uniformly normalise overruns to RlpException — a truncated slot can surface as IndexOutOfRangeException/ArgumentOutOfRangeException, escape the inner filter, and abandon the rental on a path that repeats per block during replay.

Nothing to change in this PR necessarily — but widening that inner filter to catch (Exception) would both close this and turn "corrupt slot kills replay" back into the logged warning it was clearly meant to be.

[benaadams]

Fixed in 57efe16 by restoring SlotFile.TryRead to its original finally shape. A broad callback catch would hide arbitrary caller bugs, while this public callback boundary and its possible 64 MiB rental make guaranteed return the clearer ownership contract.

[benaadams]

Addressed the follow-up review in 57efe16.

• Restored SlotFile.TryRead's original finally; the public callback boundary and possible 64 MiB rental make guaranteed return appropriate, while widening the callback catch would hide caller bugs.
• Compressed the ArrayPool guidance from three bullets to two without dropping the expected/unexpected split, recovery-path rule, EnableZkEvm and helper carve-outs, or double-return warning.
• Updated the PR description to identify the EraWriter and PersistentBlobTxDistinctSortedPool hunks as mechanical de-indentation around their two semantic return changes.

Formatting and diff checks pass. SlotFile is restored exactly to master, so no additional test was needed for this follow-up.

@claude review

[claude]

Claude finished @benaadams's task in 3m 4s —— View job

---

Review: ArrayPool.Shared finally-block removals (head 57efe16)

• Re-read the rule text at current head
• Verify SlotFile is restored to master exactly
• Audit the 10 remaining removals for double-return / use-after-return / control-flow change
• Post review

The diff is down to 11 files / +171 −231. git diff origin/master...HEAD -- Nethermind.BalRecorder/SlotFile.cs is empty — SlotFile is byte-identical to master, as claimed. git diff origin/master...HEAD | grep "^[+-].*catch" still returns exactly one code line, the removed bare catch in InputExtractor, so the "no added catches relative to master" property holds. git diff --check is clean.

Nine production finally blocks removed plus one in SszCodecTests and the InputExtractor bare catch.

---

Verification

No double-return, no use-after-return. Every added Return sits on exactly one reachable path, and no removal introduced a second Return for the same rental.

No control-flow change. Three scopes changed shape; all three are equivalent:

• InitDatabaseSnapshot.ComputeChecksumAsync — the naked block from the previous round is now an explicit await using (FileStream …). checksum is assigned as the last statement inside that scope, so definite assignment holds, and the FileStream disposes at the same point the old declaration-form await using did.
• EraWriter.Finalize — using ArrayPoolList<byte> indexBytes moved to method scope, so it now disposes after FinalizeChecksum(). Harmless: WriteEntry(…, indexBytes.AsMemory(), …) is awaited ~4 lines earlier. tdOffsets is [] when !needsTd, and the Return is correspondingly guarded.
• PersistentBlobTxDistinctSortedPool.TryGetBlobsAndProofsV1 — I re-read the full method at head: between the three Rent calls and the three Return calls there is no return, only continue/break inside the nested loops, and the inner dbResults return sits at the end of the if (missCount > 0) block with nothing escaping it. De-indent is behaviour-preserving.

Rentals stay off the LOH on the recovered paths. TryGetBlobsAndProofsV1 is capped by MaxRequest = 128 → maxMisses = 512 → ~12 KB + 2×2 KB. EraWriter's tdOffsets is blockCount * 8 ≤ 64 KB for an 8192-block epoch, and that path aborts the archive write anyway. Session.Invoke rents count = subscriber count.

Session.Invoke drops the guaranteed Array.Clear along with the Return, but only on the throwing path where the array is abandoned and therefore unreachable — no delegate rooting, no Session retention. The skipped activityObserver?.OnSessionDisconnected on a throwing handler is pre-existing (Session.cs:534); the old finally didn't swallow either.

InputExtractor is the only case with real aliasing potential and it's still correct: DecodeInput returns the buffer immediately before throw new FormatException for invalid hex, and the caller's try/finally around the file write is retained — correctly, since it sits inside the per-block catch (Exception ex) when (ex is IOException or FormatException or …) recovery loop. The removed bare catch only covered non-FormatException escapes from Bytes.FromUtf8HexString, which fall through that filter and abort the tool.

Rule text. Three bullets → two, and nothing was dropped: the EnableZkEvm scope, the expected/unexpected split, "don't replace an existing recovery finally with catches", the return-before-deliberate-throw clause, the SafeArrayPool / ArrayPoolDisposableReturn / ArrayPoolList carve-outs, and the double-return warning all survive.

---

Findings — both Low

Low — bullet 2's because clause reads backwards · robustness.md:16 · inline

"using-based helpers remain permitted because other contracts may track ownership, use reference counting, or hold non-GC resources." The ownership/refcount/non-GC clause is the rationale for the non-generalisation list, not for permitting the helpers — as compressed it says the helpers are allowed because they might hold non-GC resources. Swapping the clause order fixes it at equal length. This file is machine-consumed, so the inverted rationale is worth a token.

Low — CapabilityConverter trips the PR's own "deliberate throw" clause · CapabilityConverter.cs:70 · inline

WriteToBuffer ends in ThrowJsonException() (:87 → :127) when Utf8Formatter.TryFormat fails, so rented is deliberately abandoned at a throw the code chooses — the one shape the new rule says to return before. Unreachable in practice (MaxIntegerDigits is sized for the int version, and this branch needs a protocol code longer than StackAllocThreshold), so it's an accuracy note: the PR body cites InputExtractor as the only deliberate-throw case.

---

Notes

Previously-flagged items I re-confirmed as resolved at this head: SszGenerator (12 blocks), CappedArrayJsonConverter, ChannelDecoder, SnapshotDownloader, BlockCachePreWarmer, PayloadByNumberProtocol, EthStatsMessageParser, and now SlotFile are all back to master shape.

I could not run dotnet build in this environment (command requires approval), so definite assignment was verified by inspection: checksum, nodes, and accumulatorRoot/checksum in EraWriter are each assigned on every path reaching their use. Your reported clean build is consistent with that.

Verdict: no leak, no pool corruption, no control-flow or consensus risk. Both findings are Low and neither blocks merge.
· feature/arraypool-shared-return-guidance

[claude]

Low — the compression left a non-sequitur: "using-based helpers remain permitted because other contracts may track ownership, use reference counting, or hold non-GC resources." The ownership/refcount/non-GC clause is the reason for the non-generalisation list, not for permitting the using helpers. Read literally it says the helpers are allowed because they might hold non-GC resources, which inverts the argument. Swapping the clause order fixes it without adding length:

Suggested change

- Do not generalize this exception to `SafeArrayPool<T>.Shared`, `ArrayPoolDisposableReturn`, `ArrayPoolList<T>`, custom pools, or other `Return`, `Release`, or `Dispose` contracts; `using`-based helpers remain permitted because other contracts may track ownership, use reference counting, or hold non-GC resources. Never return an array that may already have been returned or handed off — a double return silently corrupts the pool.

- Do not generalize this exception to `SafeArrayPool<T>.Shared`, `ArrayPoolDisposableReturn`, `ArrayPoolList<T>`, custom pools, or other `Return`, `Release`, or `Dispose` contracts, which may track ownership, use reference counting, or hold non-GC resources; `using`-based helpers remain permitted. Never return an array that may already have been returned or handed off — a double return silently corrupts the pool.

[benaadams]

Agreed. I swapped the clause order in 09a236a so the ownership/ref-counting/non-GC rationale clearly applies to the non-generalization rule, while using-based helpers remain permitted.

[claude]

Low — this is the one removal in the PR that trips the new rule's own "deliberate throw" clause. WriteToBuffer ends in ThrowJsonException() (:87 → :127) when Utf8Formatter.TryFormat fails, so the code deliberately throws while rented is still owned locally, and the rule says to return it immediately first.

Practically unreachable — MaxIntegerDigits is sized for the int version and this branch only runs for a protocol code longer than StackAllocThreshold — so this is an accuracy note rather than a defect. Worth mentioning because the PR body claims the "return before deliberate throws" case is handled everywhere (InputExtractor is cited as the only instance).

[benaadams]

Agreed. In 09a236a, WriteToBuffer is now TryWriteToBuffer. Its known formatting failure returns the rental before calling the throw helper; unrelated encoding or writer exceptions still abandon the array as intended.

[flcl42]

Found 1 low-severity issue in src/Nethermind/Nethermind.TxPool/Collections/PersistentBlobTxDistinctSortedPool.cs.

[benaadams]

@claude review

[claude]

Claude finished @benaadams's task in 4m 5s —— View job

---

Review: ArrayPool.Shared finally-block removals (head 09a236a)

• Re-read the rule text and full diff at current head
• Verify the three items from the previous round are resolved
• Audit the CapabilityConverter TryWriteToBuffer refactor
• Audit remaining removals for double-return / use-after-return / control-flow change
• Post review

The diff is down to 10 files, +110 −151. PersistentBlobTxDistinctSortedPool.cs no longer appears at all — git diff origin/master...HEAD --stat confirms it, so it is byte-identical to master as claimed. git diff origin/master...HEAD | grep "^[+-].*catch" still resolves to a single removed code line (the bare catch in InputExtractor), so "no added catches relative to master" holds. Nine production finally blocks removed plus one in SszCodecTests.

---

Verification

Previous-round items resolved. robustness.md:16 now reads "…which may track ownership, use reference counting, or hold non-GC resources; using-based helpers remain permitted" — the rationale attaches to the non-generalisation list, and nothing was dropped in the reorder. CapabilityConverter returns before ThrowJsonException. PersistentBlobTxDistinctSortedPool is reverted, which also removes the largest re-indent hunk from the diff.

No use-after-return. Every rental is consumed before its Return, and nothing retains a reference past that point:

• HexWriter.WriteHexStringValue — Utf8JsonWriter.WriteRawValue copies the span into the writer's own buffer.
• PayloadBodiesDirectResponseWriter — HexWriter.WriteHexString encodes into IBufferWriter.GetSpan, no aliasing.
• KBucketTree — GetAllAtDistanceRecursive only reads bucketEntries via CopyAllWithHash; result.Span.ToArray() materialises before the Return.
• EraWriter — tdOffsets is read into indexBytes before WriteEntry; Return is guarded by needsTd and tdOffsets is [] otherwise.
• SszCodecTests — decoded is built from encoded, so the returned cells[0]/proofs[0] are not aliased by the assertions.
• Session.Invoke — rentedHandlers is a private copy under _lock; handlers are invoked synchronously and the array is never handed off.

No double return. One Return per rental on every reachable path. The only two-call site is CapabilityConverter (see inline; correct, but load-bearing on [DoesNotReturn]).

No control-flow change. InitDatabaseSnapshot.ComputeChecksumAsync's naked block is now an explicit await using (FileStream …); checksum is the last statement in that scope so definite assignment holds, and the stream disposes at the same point the declaration form did. EraWriter.Finalize moves using ArrayPoolList<byte> indexBytes to method scope — it now disposes after FinalizeChecksum(), harmless since WriteEntry(…, indexBytes.AsMemory(), …) is awaited earlier. No early return/throw sits between any rent and its return.

Recovery-boundary claim re-checked at the source. The one I had not previously traced is ComputeChecksumAsync: InitDatabaseSnapshot.cs:82 awaits DownloadWithRetryAsync and :84 then calls VerifyChecksumAsync outside the retry loop, so a throw there propagates out of the init step once per process start. The 64 KiB ChecksumBufferSize rental is abandoned at most once and stays under the LOH threshold. EraWriter's tdOffsets is blockCount * 8 ≤ 64 KB for an 8192-block epoch on a path that aborts the archive write; Session.Invoke rents subscriber-count refs. Nothing here is a large rental on a repeated path.

InputExtractor remains the only case with real aliasing potential and is still correct: DecodeInput returns the buffer immediately before throw new FormatException for invalid hex, and because the tuple deconstruction never runs on that path the caller's local is never bound, so the retained try/finally around the file write cannot double-return. The removed bare catch only covered non-FormatException escapes from Bytes.FromUtf8HexString, which fall through the caller's catch (… IOException or FormatException or UnauthorizedAccessException) filter and abort the tool.

---

Findings — both Low

Low — CapabilityConverter's duplicated Return depends on [DoesNotReturn] for pool safety · CapabilityConverter.cs:72-79 · inline

The two Return(rented) calls are mutually exclusive only because ThrowJsonException is [DoesNotReturn] (:133). Removing that attribute — or swapping in a log-and-continue helper — turns this into the silent double return the new rule warns about. A bool written local gives one Return, one fewer line, and no dependency on the attribute. Worth taking since this file is the PR's worked example for the "return before deliberate throws" clause.

Low — unrelated blank-line deletion · InputExtractor.cs:155

The diff drops the blank line between (byte[] buffer, int dataLength) = DecodeInput(inputBytes); and the retained try. AGENTS.md asks that unrelated code be left verbatim; restoring it costs nothing and removes a line from the diff.

---

Notes

Cumulative status of everything flagged across the four rounds: SszGenerator (12 blocks), CappedArrayJsonConverter, ChannelDecoder, SnapshotDownloader, BlockCachePreWarmer, PayloadByNumberProtocol, EthStatsMessageParser, SlotFile, and PersistentBlobTxDistinctSortedPool are all back to master shape; the robustness.md clause order and the CapabilityConverter deliberate-throw case are fixed. Nothing from earlier rounds is outstanding.

git diff --check is clean. I could not run dotnet build in this environment (command requires approval), so definite assignment was verified by inspection: checksum (both files), nodes, accumulatorRoot, and written-equivalent locals are assigned on every path reaching their use. Your reported clean build is consistent with that.

Verdict: no leak, no pool corruption, no control-flow or consensus risk. No Critical/High/Medium findings. Both Lows are optional polish and neither blocks merge.
· feature/arraypool-shared-return-guidance

[claude]

Low — correct as written, but it's the one shape in this PR that is only correct because of an attribute on another method.

Two Return(rented) calls now exist for one rental. They're mutually exclusive solely because ThrowJsonException carries [DoesNotReturn] (:133). Drop that attribute, or replace the helper with something that logs-and-continues, and this becomes a double return — precisely the "silently corrupts the pool" failure the new rule in robustness.md warns about. Given this file is the worked example the PR body cites for "return before deliberate throws", a single-exit shape is worth preferring:

Suggested change

	byte[] rented = ArrayPool<byte>.Shared.Rent(totalLength);
	try
	{
	WriteToBuffer(writer, capability, rented.AsSpan(), protocolByteCount);
	}
	finally
	if (!TryWriteToBuffer(writer, capability, rented.AsSpan(), protocolByteCount))
	{
	ArrayPool<byte>.Shared.Return(rented);
	ThrowJsonException();
	}
	ArrayPool<byte>.Shared.Return(rented);
	byte[] rented = ArrayPool<byte>.Shared.Rent(totalLength);
	bool written = TryWriteToBuffer(writer, capability, rented.AsSpan(), protocolByteCount);
	ArrayPool<byte>.Shared.Return(rented);
	if (!written)
	{
	ThrowJsonException();
	}

One Return, one line shorter, still returns before the deliberate throw, and no longer depends on [DoesNotReturn] for pool safety.

[benaadams]

Agreed. c3e9972 now stores the result, returns the rental once, and only then calls the throw helper when needed, so pool safety no longer depends on DoesNotReturn.

[benaadams]

Addressed both Low findings in c3e9972: CapabilityConverter now has a single Return site before its deliberate throw, and the unrelated InputExtractor blank-line deletion is restored.

@claude review

[claude]

Claude finished @benaadams's task in 4m 39s —— View job

---

Review: ArrayPool.Shared finally-block removals (head c3e9972)

• Re-read the full diff at current head
• Verify the two Low findings from the previous round are resolved
• Audit remaining removals for double-return / use-after-return / control-flow change
• Trace which touched assemblies are actually built/linked under EnableZkEvm=true
• Post review

Diff unchanged in scope from the last round: 10 files, +110 −151, nine production finally blocks plus one in SszCodecTests plus the InputExtractor bare catch. git diff --check clean; git diff origin/master...HEAD | grep "^[+-].*catch" resolves to a single code line (the removed bare catch) — "no added catches relative to master" still holds.

---

Previous-round items — both resolved

CapabilityConverter (c3e9972) now has a single Return site: bool written = TryWriteToBuffer(...) → Return(rented) → if (!written) ThrowJsonException(). Pool safety no longer depends on [DoesNotReturn] on ThrowJsonException — flip that attribute and the worst outcome is a duplicate JsonException path, not a double return. The stackalloc branch throws from the same place it used to (inside the old WriteToBuffer), and the writer state on failure is identical, so observable behaviour is unchanged.

InputExtractor — the blank line before the retained try is back; the diff for that file is now +0 −5, exactly the bare catch removal.

Verification at this head

No double return, no use-after-return. Each rental has exactly one Return on exactly one reachable path, and every consumer completes first: Utf8JsonWriter.WriteRawValue copies the span (HexWriter); HexWriter.WriteHexString encodes into IBufferWriter.GetSpan (PayloadBodiesDirectResponseWriter); result.Span.ToArray() materialises before bucketEntries is returned, and GetAllAtDistanceRecursive only reads it via CopyAllWithHash (KBucketTree, with the IsReferenceOrContainsReferences clear flag preserved); indexBytes is written from tdOffsets and WriteEntry awaited well before the Return (EraWriter).

No control-flow change. ComputeChecksumAsync's scope is now an explicit await using (FileStream …) with checksum assigned as the block's last statement — definite assignment holds and the stream disposes where the declaration form disposed it. EraWriter.Finalize has no early return/continue between the Rent and the Return, accumulatorRoot is default-initialised, the if (needsTd) guard matches the conditional rent ([] otherwise), and using ArrayPoolList<byte> indexBytes moving to method scope only defers its dispose past FinalizeChecksum(), after its last use.

Abandonment windows re-checked against real boundaries. EraExporter.cs:163 calls Finalize once with no retry, inside the per-epoch using (EraWriter …), so the ≤64 KB tdOffsets is dropped at most once per failed epoch. ComputeChecksumAsync is called at InitDatabaseSnapshot.cs:84, outside DownloadWithRetryAsync's retry loop — one 64 KiB abandonment per process start at worst. Session.Invoke rents subscriber-count refs and only skips the Array.Clear+Return on the throwing path, where the array is unreachable anyway (no delegate rooting, so no Session retention); the skipped OnSessionDisconnected on a throwing handler is pre-existing (Session.cs:534).

InputExtractor remains the only case with real aliasing potential and is still correct: DecodeInput returns the buffer immediately before throw new FormatException, and because the tuple deconstruction never runs on that path the caller's local is never bound, so the retained try/finally around the file write cannot double-return. The removed bare catch only covered non-FormatException escapes from Bytes.FromUtf8HexString, which fall through the caller's catch (… IOException or FormatException or UnauthorizedAccessException) filter and abort the tool.

---

Findings — one Low

Low — two touched files are compiled with EnableZkEvm=true, so they sit inside the rule's own carve-out by the letter · robustness.md:15 · inline

ZiskGuest/Makefile:44 builds with -p:EnableZkEvm=true and the property flows down the ProjectReference closure: Stateless.Executor → Consensus → Evm → Specs → Serialization.Json (Nethermind.Specs.csproj:10), i.e. HexWriter.cs; and Consensus → Blockchain → Network.Stats (Nethermind.Blockchain.csproj:14), i.e. CapabilityConverter.cs. Same chain that got the SSZ-generator blocks restored in d734042 — but the exposure is much smaller: neither assembly is in BFLAT_REFS (Makefile:15-40), so neither is linked into the guest binary, whereas Serialization.Ssz.dll and Stateless.Executor.dll are. The change is fine; it's the rule wording that's broader than intended. Scoping it to code that reaches the guest binary makes the boundary checkable and stops a future agent reverting these two hunks on a literal reading.

---

Notes

Cumulative status across all five rounds — nothing outstanding: SszGenerator (12 blocks), CappedArrayJsonConverter, ChannelDecoder, SnapshotDownloader, BlockCachePreWarmer, PayloadByNumberProtocol, EthStatsMessageParser, SlotFile, and PersistentBlobTxDistinctSortedPool are all back to master shape; the robustness.md clause order, the CapabilityConverter deliberate-throw case and its single-Return shape, and the InputExtractor blank line are all fixed.

dotnet build is not permitted in this environment, so definite assignment was verified by inspection (checksum in both files, nodes, accumulatorRoot/checksum in EraWriter, written in CapabilityConverter — each assigned on every path reaching its use). Your reported clean build is consistent with that.

Verdict: no leak, no pool corruption, no control-flow or consensus risk. No Critical/High/Medium findings. The single Low is a rule-wording precision point and does not block merge.
· feature/arraypool-shared-return-guidance

[claude]

Low — two of the files this PR touches are compiled with EnableZkEvm=true, so by the letter of this bullet they fall inside its own carve-out.

Nethermind.Stateless.ZiskGuest/Makefile:44 builds with -p:EnableZkEvm=true, and the global property flows through the whole ProjectReference closure:

• ZiskGuest → Stateless.Executor → Consensus → Evm → Specs → Serialization.Json (Nethermind.Specs.csproj:10) — that's HexWriter.cs
• ZiskGuest → Stateless.Executor → Consensus → Blockchain → Network.Stats (Nethermind.Blockchain.csproj:14) — that's CapabilityConverter.cs

This is the same chain that got the 12 SSZ-generator blocks restored in d734042, but the exposure is materially smaller here: neither Nethermind.Serialization.Json.dll nor Nethermind.Network.Stats.dll appears in BFLAT_REFS (Makefile:15-40), so neither is linked into the guest binary — the AOT build would fail if guest-reachable code touched them. Serialization.Ssz.dll and Stateless.Executor.dll, by contrast, are listed.

So the change is fine in practice and it's the rule text that's imprecise: "compiled with EnableZkEvm=true" is broader than "runs in the zkVM". Scoping it to code that reaches the guest binary (i.e. an assembly in BFLAT_REFS) would make the boundary checkable rather than requiring a reader to trace the reference graph — and would stop a future agent from reverting these two hunks on a literal reading.

[benaadams]

Agreed. 7b7eb34 narrows the carve-out to zkVM guest entry-point code and assemblies explicitly listed in BFLAT_REFS, rather than every transitive project compiled with EnableZkEvm. The PR description now uses the same boundary.

[benaadams]

Narrowed the zkVM exception in 7b7eb34 to guest entry-point code and BFLAT_REFS assemblies, and updated the PR description accordingly.

@claude review

[claude]

Claude finished @benaadams's task in 2m 23s —— View job

---

Review: ArrayPool.Shared finally-block removals (head 7b7eb34)

• Read the rule text and full diff at current head
• Verify the narrowed zkVM carve-out against the actual BFLAT_REFS / guest boundary
• Re-audit the nine production removals for double-return / use-after-return / control-flow change
• Post review

Diff unchanged in scope from the last round: 10 files, +110 −151. git diff --check clean; git diff origin/master...HEAD | grep "^[+-].*catch" resolves to one doc line plus the single removed catch in InputExtractor — "no added catches relative to master" still holds.

---

The narrowing checks out

7b7eb34 replaced "Outside code compiled with EnableZkEvm=true" with "Except in zkVM guest entry-point code and assemblies listed in its BFLAT_REFS", and I verified every touched file against Makefile:15-40:

Touched assembly	In BFLAT_REFS?
Nethermind.EraE, Init.Snapshot, Kademlia, Merge.Plugin(+.Test), Network, Network.Stats, Serialization.Json, StatelessInputGen	no

So all ten hunks are outside the carve-out by the letter of the rule — the two cases I flagged last round (HexWriter.cs in Serialization.Json, CapabilityConverter.cs in Network.Stats, both reachable via the EnableZkEvm=true ProjectReference closure but absent from BFLAT_REFS) are now unambiguously in scope for removal, and the 12 restored SSZ-generator blocks stay covered because Serialization.Ssz.dll and Stateless.Executor.dll are listed. Rule and change now agree without needing a reference-graph trace.

Nothing else in the rule text moved: the expected/unexpected split, "don't replace an existing recovery finally with catches", the return-before-deliberate-throw clause, the SafeArrayPool / ArrayPoolDisposableReturn / ArrayPoolList non-generalisation list with its rationale in the right position, and the double-return warning all survive the rewrite.

Code re-audit at this head

No double return, no use-after-return. One Return per rental on every reachable path, and each consumer completes first: Utf8JsonWriter.WriteRawValue copies the span (HexWriter:452); HexWriter.WriteHexString encodes into IBufferWriter.GetSpan (PayloadBodiesDirectResponseWriter:248); result.Span.ToArray() materialises into nodes before bucketEntries is returned, with the IsReferenceOrContainsReferences clear flag preserved (KBucketTree:190-192); indexBytes is filled from tdOffsets and WriteEntry awaited before the guarded Return (EraWriter:248-251); the SSZ test's decoded is built from encoded, not aliased to cells[0]/proofs[0].

No control-flow change. ComputeChecksumAsync uses the explicit await using (FileStream …) block with checksum assigned as its last statement — definite assignment holds and the stream disposes where the declaration form disposed it. EraWriter.Finalize has no early return/throw between the conditional Rent and the if (needsTd)-guarded Return (tdOffsets is [] otherwise), accumulatorRoot is default-initialised, and moving using ArrayPoolList<byte> indexBytes to method scope only defers its dispose past FinalizeChecksum(), after its last use.

CapabilityConverter (c3e9972) still has the single-Return shape: bool written = TryWriteToBuffer(...) → Return(rented) → if (!written) ThrowJsonException(). [DoesNotReturn, StackTraceHidden] is still on the helper (:132) but pool safety no longer depends on it. The stackalloc branch throws from the same condition it did inside the old WriteToBuffer, with identical writer state on failure.

Abandonment windows. EraExporter.cs:163 calls Finalize once per epoch with no retry (≤64 KB tdOffsets, sub-LOH). ComputeChecksumAsync runs at InitDatabaseSnapshot.cs:84, outside DownloadWithRetryAsync's retry loop — one 64 KiB abandonment per process start at worst. Session.Invoke rents subscriber-count refs and only skips Array.Clear+Return on the throwing path, where the array is unreachable anyway (no delegate rooting → no Session retention); the skipped OnSessionDisconnected on a throwing handler is pre-existing (Session.cs:534).

InputExtractor remains the only case with real aliasing potential and is still correct: DecodeInput returns the buffer immediately before throw new FormatException, and because the tuple deconstruction never runs on that path the caller's local is never bound, so the retained try/finally around the file write cannot double-return. The removed bare catch only covered non-FormatException escapes from Bytes.FromUtf8HexString, which fall through the caller's catch (… IOException or FormatException or UnauthorizedAccessException) filter and abort the tool. File diff is +0 −5, and the blank line before the retained try is back.

---

Findings — one Low

Low — the carve-out boundary is checkable now, but not self-locating, and it exempts most of the hot core · robustness.md:15 · inline

grep -rl BFLAT_REFS returns exactly two files — Nethermind.Stateless.ZiskGuest/Makefile and this rule — so an agent that doesn't already know the token has nothing to anchor on; naming the path makes the bullet self-contained. Separately, worth being deliberate about the resulting scope: BFLAT_REFS lists Core, Evm, Evm.Precompiles, State, Trie, TxPool, Blockchain, Consensus, Specs, Serialization.Rlp, Serialization.Ssz, Db, Crypto, Abi, Config, Logging — most of the hot path, i.e. where the guidance's own performance argument is strongest. Per-callsite guest reachability isn't statically checkable, so an assembly-granular boundary is the right trade; I'm flagging it so the width is a choice rather than a surprise, not asking for a change.

---

Notes

Cumulative status across six rounds — nothing outstanding. Restored to master shape: SszGenerator (12 blocks), CappedArrayJsonConverter, ChannelDecoder, SnapshotDownloader, BlockCachePreWarmer, PayloadByNumberProtocol, EthStatsMessageParser, SlotFile, PersistentBlobTxDistinctSortedPool. Fixed: the robustness.md clause order, the CapabilityConverter deliberate-throw case and its single-Return shape, the InputExtractor blank line, and now the zkVM carve-out boundary.

dotnet build is not permitted in this environment, so definite assignment was verified by inspection (checksum in InitDatabaseSnapshot, nodes in KBucketTree, accumulatorRoot/checksum in EraWriter, written in CapabilityConverter — each assigned on every path reaching its use). Your reported clean build is consistent with that.

Verdict: no leak, no pool corruption, no control-flow or consensus risk. No Critical/High/Medium findings. The single Low is a rule-wording precision point and does not block merge.
· feature/arraypool-shared-return-guidance

[benaadams]

@claude review

[claude]

Claude finished @benaadams's task in 2m 38s —— View job

---

Review: ArrayPool.Shared finally-block removals (head 9600aed)

• Read the rule text and full diff at current head
• Verify the BFLAT_REFS link resolves and the boundary still matches every touched file
• Re-audit the nine production removals for double-return / use-after-return / control-flow change
• Post review

Diff unchanged in scope from the last round: 10 files, +110 −151. Nine production finally blocks removed, plus one in SszCodecTests and the bare catch in InputExtractor. git diff --check clean; git diff origin/master...HEAD | grep "^[+-].*catch" still resolves to one doc line plus that single removed catch, so "no added catches relative to master" holds.

---

The link lands

9600aed turned "assemblies listed in its BFLAT_REFS" into a link to Nethermind.Stateless.ZiskGuest/Makefile. The relative path resolves (.agents/rules/ → ../../src/Nethermind/…; the file exists at that path), and the rule is now self-locating — a reader who has never seen the token can find the list without grepping. Nothing else in the bullet moved: the expected/unexpected split, "retain an existing finally rather than replacing it with catches", the return-before-deliberate-throw clause, the SafeArrayPool / ArrayPoolDisposableReturn / ArrayPoolList<T> non-generalisation list with its rationale in the right position, and the double-return warning all survive verbatim.

I re-checked every touched assembly against Makefile:15-40: Nethermind.EraE, Init.Snapshot, Kademlia, Merge.Plugin(+.Test), Network, Network.Stats, Serialization.Json, StatelessInputGen — none is listed, so all ten hunks sit outside the carve-out by the letter of the rule, and the 12 restored SSZ-generator blocks stay inside it (Serialization.Ssz.dll and Stateless.Executor.dll are both listed). Rule and change agree without needing a ProjectReference trace.

Code re-audit at this head

No double return, no use-after-return. One Return per rental on every reachable path, and each consumer completes first:

• HexWriter:452 — Utf8JsonWriter.WriteRawValue copies the span into the writer's own buffer before the Return; the rented is not null guard mirrors the conditional rent exactly.
• PayloadBodiesDirectResponseWriter:248 — HexWriter.WriteHexString encodes into IBufferWriter.GetSpan, no span retained.
• KBucketTree:186-191 — result.Span.ToArray() materialises into nodes before bucketEntries is returned, and the RuntimeHelpers.IsReferenceOrContainsReferences clear flag is preserved. PooledList<TNode> result disposes after the Return, same relative order as master.
• EraWriter:248-251 — indexBytes is filled from tdOffsets and WriteEntry awaited before the if (needsTd)-guarded Return; tdOffsets is [] when !needsTd.
• SszCodecTests — decoded is built from encoded, so the assertions don't alias cells[0]/proofs[0].
• Session.cs:866-871 — rentedHandlers is a private copy taken under _lock, handlers run synchronously, and the array is never handed off.

No control-flow change. I read EraWriter.Finalize end to end at this head: between the conditional Rent (:181) and the Return (:250) there is no return, throw, continue or break at method level, accumulatorRoot is default-initialised, and moving using ArrayPoolList<byte> indexBytes to method scope only defers its dispose past FinalizeChecksum() — well after its last use. ComputeChecksumAsync keeps the explicit await using (FileStream …) block with checksum assigned as its last statement, so definite assignment holds and the stream disposes where the declaration form disposed it.

CapabilityConverter retains the single-Return shape from c3e9972: bool written = TryWriteToBuffer(…) → Return(rented) → if (!written) ThrowJsonException(). Pool safety no longer depends on [DoesNotReturn] (:133). TryWriteToBuffer writes to the Utf8JsonWriter only on the success path, so a false return leaves writer state exactly as the old else { ThrowJsonException(); } branch did, and the stackalloc path throws from the same condition. ThrowJsonException still has three other call sites, so no dead-code warning.

Abandonment windows are unchanged and all bounded: EraExporter.cs:163 calls Finalize once per epoch with no retry (≤64 KB tdOffsets, sub-LOH); ComputeChecksumAsync runs at InitDatabaseSnapshot.cs:84, outside DownloadWithRetryAsync's retry loop, so at most one 64 KiB abandonment per process start; Session.Invoke rents subscriber-count refs and only skips Array.Clear+Return on the throwing path, where the array is unreachable anyway (no delegate rooting → no Session retention). The skipped OnSessionDisconnected on a throwing handler is pre-existing (Session.cs:534).

InputExtractor is still the only case with real aliasing potential and still correct: DecodeInput returns the buffer immediately before throw new FormatException for invalid hex (:210-214), and because the tuple deconstruction at :155 never runs on that path the caller's local is never bound, so the retained try/finally around the file write cannot double-return. The removed bare catch only covered non-FormatException escapes from Bytes.FromUtf8HexString, which fall through the caller's catch (… IOException or FormatException or UnauthorizedAccessException) filter and abort the tool. File diff is +0 −5.

---

Findings

No Critical / High / Medium. Two cosmetic nits, take or leave:

• EraWriter.cs:181-183 — de-indenting removed the brace that used to separate the rent from the // Write sections in EraE spec order: comment, so the comment now reads as annotating the rental rather than the section loops. A blank line after the tdOffsets line restores the original reading.
• Session.cs:869 — Array.Clear now butts directly against the closing brace of the invoke loop; a blank line before it separates cleanup from dispatch, matching the spacing the finally gave it.

---

Notes

Cumulative status across seven rounds — nothing outstanding. Restored to master shape: SszGenerator (12 blocks), CappedArrayJsonConverter, ChannelDecoder, SnapshotDownloader, BlockCachePreWarmer, PayloadByNumberProtocol, EthStatsMessageParser, SlotFile, PersistentBlobTxDistinctSortedPool. Fixed: the clause order in robustness.md, the CapabilityConverter deliberate-throw case and its single-Return shape, the InputExtractor blank line, the zkVM carve-out boundary, and now its link.

dotnet build requires approval in this environment, so definite assignment was verified by inspection (checksum in InitDatabaseSnapshot, nodes in KBucketTree, accumulatorRoot/checksum in EraWriter, written in CapabilityConverter — each assigned on every path reaching its use). Your reported clean build is consistent with that. The EXPB comparison on this branch shows no regression (AVG −0.47% / −1.66% / −2.46% across the three payload sets, within noise).

Verdict: no leak, no pool corruption, no control-flow or consensus risk. Nothing blocking; the two nits are whitespace only.
· feature/arraypool-shared-return-guidance