Fix undisposed array-pool list leaks

[batrr]

Changes

Fix leaks that were showing in logs as undisposed array-pool lists.

This PR fixes three cleanup cases:

1. AccountChanges was not disposed when block processing failed on an invalid block. In this case, ownership is not passed to the normal valid-block flow, so it must be disposed before throwing.

2. MessageDictionary could receive a response after the request was already cancelled or timed out. When TrySetResult failed, the response data was not used and was not disposed.

3. Transaction decoding can create prehash data backed by MemoryPool<byte>.Shared. This is normally cleaned up when the transaction is submitted to the txpool. If processing failed before submission, some decoded transactions could keep this memory. Now the prehash is cleared when those transactions are dropped.

[claude]

Claude finished @batrr's task in 4m 52s —— View job

---

Review: Fix undisposed array-pool list leaks

• Load repo rules
• Read changed files
• Review correctness and edge cases
• Review security and performance
• Post findings

---

Summary

Three targeted leak fixes, all logically correct. The ClearPreHash() method is idempotent (guards on _preHash.Length > 0), and DisposeAccountChanges() nulls the field after disposing, so neither can double-free. No security or performance concerns on hot paths — all fixes apply only to error/cancellation paths. One medium finding (missing regression tests, required by AGENTS.md); two low findings.

---

Findings

Medium — No regression tests added

AGENTS.md states: "When fixing a bug, always add a regression test." None of the five changed files has a corresponding test addition, despite existing test infrastructure that makes them straightforward to write:

• MessageDictionary — MessageDictionaryTests.cs already mocks IOwnedReadOnlyList<BlockHeader> with NSubstitute. A test should: pre-cancel the CompletionSource (e.g. via TrySetCanceled()), call Handle(id, mockData, size) successfully, and assert mockData.Received().Dispose() (or TryDispose()). This exercises the new if (!TrySetResult) data?.TryDispose() branch.

• BlockProcessor — Verify block.AccountChanges is null after ValidateProcessedBlock throws InvalidBlockException (mock ValidateProcessedBlock to return false). The DisposeAccountChanges_should_dispose_and_null_account_changes test in BlockTests.cs demonstrates the pattern.

• Eth62ProtocolHandler.HandleSlow — A pre-cancelled CancellationToken test can verify that all transaction pre-hashes are cleared (via ClearPreHash() tracking) and the list is disposed, even in the early-cancellation path.

• TransactionsMessageSerializer.DeserializeTxs — Feed malformed RLP mid-sequence; verify pre-hashes on already-decoded transactions are cleared before the exception propagates.

• Eth68ProtocolHandler.HandleSlow — Inject a transaction that fails ValidateSizeAndType; verify all pre-hashes from startIdx onwards are cleared.

Low — Stale comment in HandleSlow (Eth62, line 265)

"Cancelled before processing any transaction — dispose and bail out."

The phrase "dispose and bail out" implies disposal happens at the return site, but in the refactored code it happens in the finally block. See inline comment.

Low — startIdx range in Eth68 cleanup (line 263)

The for (int j = startIdx; ...) range is correct (transactions [0, startIdx) were already submitted by a prior scheduler slot), but the bound may look surprising without a comment. See inline comment.

---

Per-file notes

File	Assessment
BlockProcessor.cs	Correct — DisposeAccountChanges() is idempotent; no double-dispose risk; placed before the throw so the exception propagates cleanly.
MessageDictionary.cs	Correct — TrySetResult is atomic; if it returns false the caller already owns nothing, so disposing here is safe and complete.
Eth62ProtocolHandler.cs	Correct — finally block reliably clears pre-hashes for unprocessed transactions in all paths (cancellation, exception, scheduling failure, normal completion). The isTransferred flag correctly prevents double-disposal when ownership is handed off to a rescheduled task.
TransactionsMessageSerializer.cs	Correct — result only contains successfully decoded transactions, so the foreach loop cannot touch an uninitialized object.
Eth68ProtocolHandler.cs	Correct — the validation loop runs before base.HandleSlow, so no transactions have been submitted yet, making startIdx-to-end the right cleanup range.

[claude]

Low — stale comment

The comment says "dispose and bail out" but disposal no longer happens here — it happens in the finally block below. The phrase could mislead a reader into thinking the resource is freed at this return site.

Suggested change

	// Rescheduling would just loop (cancelled again immediately).
	// Rescheduling would just loop (cancelled again immediately); disposal handled in finally.

[claude]

Low — loop range starts at startIdx, but already-processed transactions from prior invocations are skipped

This is a rescheduled-task scenario where startIdx > 0 is possible. Transactions [0, startIdx) were submitted by the prior invocation of HandleSlow, so they are correctly excluded from this cleanup loop. This is fine as-is, but worth a short comment explaining the invariant, since for (int j = startIdx; ...) rather than for (int j = 0; ...) may look surprising:

Suggested change

	for (int j = startIdx; j < transactionsSpan.Length; j++)
	for (int j = startIdx; j < transactionsSpan.Length; j++) // [0, startIdx) were already handled in a prior scheduler slot

[asdacap]

@claude double check this one

[Marchhill]

possible to add unit tests for this?

[batrr]

@claude review pls

Added regression tests for the MessageDictionary and Eth62 cancellation paths. Skipped BlockProcessor.DisposeAccountChanges and TransactionsMessageSerializer.DeserializeTxs - asserting the pooled buffer is actually returned is
hard to verify in a test, and since disposal is idempotent (guarded against double disposal) regression risk is low.

Also double-check the AccountChanges leak: confirm the DisposeAccountChanges() fix in ValidateProcessedBlock is correct.

[claude]

Claude finished @batrr's task in 4m 56s —— View job

---

Review: Fix undisposed array-pool list leaks (updated)

• Load repo rules
• Read changed files (new tests + AccountChanges fix)
• Review new regression tests (MessageDictionary + Eth62)
• Double-check AccountChanges leak fix in ValidateProcessedBlock
• Post findings

---

Summary

Tests were added for the two highest-risk paths (MessageDictionary timeout-race and Eth62 cancellation). The BlockProcessor and TransactionsMessageSerializer paths remain untested; the author's rationale (idempotency guards, hard to observe pool return) is reasonable. The AccountChanges fix is confirmed correct (see below). One carry-over Low finding (stale comment) remains.

---

AccountChanges leak — confirmed correct (@asdacap's question)

PrepareBlockForProcessing always returns a new Block object (it constructs a fresh BlockHeader and Block), so block and suggestedBlock are distinct. The lifecycle is:

Step	State
ProcessBlock(block, ...)	block.AccountChanges set via SetAccountChanges(block) at line 169
Validation fails	block.DisposeAccountChanges() disposes and nulls block.AccountChanges → throw
Validation passes	suggestedBlock.AccountChanges = block.AccountChanges transfers ownership

• suggestedBlock.AccountChanges is never set in the failure path → no leak on suggestedBlock.
• DisposeAccountChanges is null-safe (block.AccountChanges?.Dispose()) and idempotent (nulls the field after disposing), so no double-free risk.
• The call site is before the throw, so the exception propagates with the resource already freed. ✓

---

New test review

MessageDictionaryTests.Test_Send_MessageDisposing_OnOldRequest ✓

Correctly exercises the new branch: TrySetException completes the source, then Handle calls TrySetResult which returns false, triggering data?.TryDispose(). Since TData = IOwnedReadOnlyList<BlockHeader> is not constrained to IDisposable, the TryDispose(this object? item) overload is used, which checks item is IDisposable and calls .Dispose() — caught by NSubstitute.

MessageDictionaryTests.Handle_disposes_tuple_disposable_component_on_unmatched_request ✓

Verifies the tuple overload path in the else branch: TryDispose on (IDisposable, long) iterates the ITuple elements and disposes the IDisposable member.

Eth62ProtocolHandlerTests.Cancelled_mid_processing_releases_transactions_unless_rescheduled ✓

Covers both paths cleanly:

• rescheduleSucceeds = false: TryScheduleBackgroundTask returns false → isTransferred = false → finally block clears txs[1].PreHash and disposes the list. txs[1].Hash is null (preHash cleared, _hash was already nulled by SetPreHashNoLock).
• rescheduleSucceeds = true: callback manually clears and disposes (simulating transferred ownership) → isTransferred = true → finally is a no-op. Same end-state.

The SetPreHashNoLock([(byte)(i+1)]) sets _hash = null, so txs[0].Hash is computed only when the SubmitTx mock accesses it (_ = tx.Hash); txs[1].Hash stays null because its preHash is cleared before the hash is ever accessed. The assertions are consistent with this.

Not tested (acknowledged by author):

• BlockProcessor.DisposeAccountChanges — disposal is idempotent and the fix is one-liner; pool-return observability is genuinely hard without a custom ArrayPool shim.
• TransactionsMessageSerializer.DeserializeTxs — similarly hard to assert on pooled memory.

These omissions are acceptable given the reasoning; no Medium finding.

---

Remaining Low findings (carry-over, unaddressed)

1. Stale comment Eth62ProtocolHandler.cs:265-266 — "dispose and bail out" implies disposal happens at the return site; it actually happens in the finally block. See inline suggestion.
2. startIdx loop Eth68ProtocolHandler.cs:263 — for (int j = startIdx; ...) rather than for (int j = 0; ...) can look surprising without a comment. Previous suggestion still stands.

---