Parallel transaction execution with BALs

[Marchhill]

Depends on BALs #9114

Types of changes

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

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[github-actions]

EVM Opcode Benchmark Diff

Aggregated runs: base=1, pr=1

No significant regressions or improvements detected.

[LukaszRozmej]

@Marchhill let me handle last few open comments

[Marchhill]

@Marchhill let me handle last few open comments

Thanks but I think all addressed now except for ref readonly, and maybe using composition which we could talk through and make a separate PR. Just need to check nothing broke from the changes

[benaadams]

@claude re-review

[claude]

Claude finished @benaadams's task in 6m 30s —— View job

---

Re-Review: Parallel Transaction Execution with BALs

• Load repository rules
• Read key changed files
• Review open issues from previous reviews
• Check for new issues from April 24–27 commits
• Post final review

---

Fixed Since April 20 Re-Review

• NullReferenceException risk in BlockAccessListBasedWorldState.Get/GetOriginal — null checks now added ✓
• Exception wrapping via ParallelExecutionException class preserving Reason — addresses the stack-trace concern ✓
• BlockCachePreWarmer — ShouldPreWarm(), IsBalReadWarmingEnabled(), WarmupFromBal(), WarmupBalAccount() helpers extracted ✓
• Test parametrization — EngineModuleTests and AuRaMergeEngineModuleTests both use [TestFixture(true)] / [TestFixture(false)], Sequential* derived classes deleted ✓
• Enabled=false → ParallelExecutionEnabled=false invariant holds: ParallelExecutionEnabled = Enabled && ... is mathematically impossible to be true when Enabled is false, so LukaszRozmej's query is answered ✓

---

New Issue

Medium — Thread-pool thread leak when non-InvalidBlockException escapes parallel worker

BlockProcessor.ParallelBlockValidationTransactionsExecutor.cs:102–118

The lambda inside ParallelUnbalancedWork.For catches only InvalidBlockException. If any other exception escapes (NullReferenceException, OperationCanceledException, StackOverflowException etc.), gasResults[txIndex] is never completed. IncrementalValidation is then permanently blocked at the inner gasResults[j].Task.GetAwaiter().GetResult() loop, leaking a thread-pool thread indefinitely. Because ParallelUnbalancedWork.For propagates the original exception on the calling thread, incrementalValidationTask.GetAwaiter().GetResult() is never reached, leaving the stuck task with no cancellation path.

Fix: add a bare catch after catch (InvalidBlockException) that calls TrySetCanceled() and rethrows — see inline suggestion. Fix this →

---

Still Open — Acknowledged as Future TODOs

High — Enumerable.SequenceEqual + ChangeAtIndex.SlotChanges yield-return iterator

BlockAccessListManager.cs:338 and BlockAccessList.cs:379 (inline comment added)

Both still present. Acknowledged by the author as "same as master + will be resolved in future todo" and benaadams has approved. Tracking here for completeness — the feature defaults to false so the hot path is only exercised when explicitly enabled.

Medium — _lastLoadedBal guard semantics

BlockAccessListManager.cs:63, 89–93 — _lastLoadedBal = Hash256.Zero on construction; BlockAccessListManager is AddScoped (one per DI scope = one per block, per BlockProcessingModule.cs:67). The cache guard always passes on first call. If PrepareForProcessing can be called multiple times within the same scope (e.g. a retry), this is intentional — but that invariant is not documented. Either add a comment explaining the retry scenario, or remove the guard and call LoadPreStateToSuggestedBlockAccessList unconditionally.

Low / Tracked as TODO

• N VirtualMachine allocations per block (BlockAccessListManager.cs:487) — // todo: will optimize to allocate ~16 worldstates upfront present
• GetNonce/GetBalance O(N) linear scan (AccountChanges.cs:271) — // todo: binary search present
• SlotChanges.Get allocates byte[] per storage read (SlotChanges.cs:67, 73) — [.. tmp.WithoutLeadingZeros()] still allocates per SLOAD in parallel path
• AccountChanges.Equals/SlotChanges.Equals use LINQ SequenceEqual — not a hot path but violates the no-LINQ rule
• IBlockAccessListManager split — // todo: maybe split into smaller classes still in interface; 12 methods spanning tx processing, BAL tracking, system contracts, gas validation, AuRa preprocessing
• ref readonly on GetBalance/GetNonce — LukaszRozmej's analysis (April 27 09:48) proposed a List<T> + CollectionsMarshal.AsSpan approach; acknowledged as out of scope for this PR

---

Summary

The April 27 commit by LukaszRozmej addressed the last round of outstanding comments cleanly. The one new issue is the thread-pool leak when a non-InvalidBlockException escapes a parallel worker (Medium, one-line fix suggested inline). The two hot-path allocation issues remain acknowledged as future work. The PR is close to ready — the thread leak fix is the only unresolved item that should land before merge.

[LukaszRozmej]

@claude re-review. Everything with TODO's is out of scope of this PR and will be done in next ones

[claude]

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

---

Re-Review: Parallel Transaction Execution with BALs

• Load repository rules
• Review recent commits since last review
• Check thread-pool leak fix status
• Review LukaszRozmej's recent changes (composition, exception wrapping, test parametrization)
• Check for new issues
• Post final review

---

Fixed Since April 27 Review

• Thread-pool leak: Fixed ✓ — TrySetCanceled() in the bare catch block unblocks IncrementalValidation, and the outer catch observes the background task before rethrowing the original exception (BlockProcessor.ParallelBlockValidationTransactionsExecutor.cs:117–134)
• Composition over inheritance: Implemented ✓ — ParallelBlockValidationTransactionsExecutor now takes IBlockProcessor.IBlockTransactionsExecutor inner and delegates via AddDecorator in BlockProcessingModule (matching SimulateBlockValidationTransactionsExecutor pattern)
• Exception wrapping: Improved ✓ — ParallelExecutionException now extends InvalidTransactionException preserving the Reason field from the underlying InvalidTransactionException (or synthesising a fallback reason for plain InvalidBlockException)
• Test parametrization: Cleaned up ✓ — [TestFixture(true)]/[TestFixture(false)] on base classes, Sequential* derived classes deleted
• BlockCachePreWarmer helper methods: Extracted ✓ — ShouldPreWarm, IsBalReadWarmingEnabled, WarmupFromBal, WarmupBalAccount helpers

---

Minor Observations

1. BranchProcessor.ShouldPreWarmOrBatchRead name is inverted

BranchProcessor.cs:203, 213

private Task? PreWarmTransactions(...) =>
    ShouldPreWarmOrBatchRead(suggestedBlock, spec)
        ? null             // <-- null = skip prewarming
        : preWarmer?.PreWarmCaches(...);

private bool ShouldPreWarmOrBatchRead(Block suggestedBlock, IReleaseSpec spec)
    => suggestedBlock.Transactions.Length < 3 && !(_parallelExecutionBatchRead && ...);

The method returns true when we should skip prewarming (fewer than 3 txs + no BAL batch read). The name implies the opposite — "should prewarm or batch read". Consider renaming to ShouldSkipPreWarming or inverting the ternary.

2. _lastLoadedBal caching intent is undocumented

BlockAccessListManager.cs:63, 89–93 — The _lastLoadedBal guard correctly prevents redundant LoadPreStateToSuggestedBlockAccessList calls when PrepareForProcessing is called multiple times for the same block within the same DI scope (e.g. on retry). However, the comment says only "check last loaded bal to avoid loading prestate again" without explaining when that scenario occurs. Worth adding a one-liner about the retry case so future readers don't remove it as "dead code" (as three prior reviews did).

---

Acknowledged as Future TODOs (out of scope per trigger comment)

• Enumerable.SequenceEqual on ChangeAtIndex.SlotChanges (BlockAccessListManager.cs:338)
• ChangeAtIndex.SlotChanges yield-return iterator allocation per account (BlockAccessList.cs:379)
• GetNonce/GetBalance O(N) linear scan (AccountChanges.cs:272)
• N VirtualMachine + TxProcessorWithWorldState allocations per block (BlockAccessListManager.cs:487)
• IBlockAccessListManager split into smaller interfaces
• LINQ SequenceEqual in AccountChanges.Equals / SlotChanges.Equals
• SlotChanges.Get byte[] allocation per SLOAD

---

Summary

The two most recent commits (20ae9675, 14668967) cleanly address all the open blocking items from the April 27 review. The thread-pool leak fix is correct and complete, composition over inheritance is properly wired through AddDecorator, and the test suite correctly exercises both parallel and sequential paths via [TestFixture(true)]/[TestFixture(false)] parametrization. The two observations above are low-severity naming/documentation nits, not blockers. With parallel execution defaulting to false and both benaadams and LukaszRozmej having approved, the PR is mergeable.

[LukaszRozmej]

@claude one more review

[claude]

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

---

Re-Review: Parallel Transaction Execution with BALs

• Load repository rules
• Read key changed files (post-April 27 commits)
• Check thread-pool leak fix and recent LukaszRozmej changes
• Review remaining open issues
• Check for new issues
• Post final review

---

Fixed Since April 27 13:07 Review

• Thread-pool leak: bare catch { TrySetCanceled(); throw; } is in place; outer catch observes the background task and logs independent secondary faults at Error ✓
• Composition over inheritance: ParallelBlockValidationTransactionsExecutor now takes IBlockProcessor.IBlockTransactionsExecutor inner and delegates via AddDecorator in BlockProcessingModule (StandardBlockValidationModule) ✓
• ShouldPreWarmOrBatchRead → ShouldSkipPreWarming: renamed, logic clarified, split into ShouldSkipPreWarming + ShouldBalReadWarm named helpers ✓
• _lastLoadedBal comment: documents that PrepareForProcessing can be called multiple times in one DI scope on the same block (e.g. retry) — explains why the guard is not dead code ✓
• BranchProcessor removes duplicated BAL predicate: delegates to preWarmer.IsBalReadWarmingEnabled(spec) (now exposed on IBlockCachePreWarmer) rather than re-deriving internally; drops unused _parallelExecutionBatchRead field ✓
• SystemContractHandler sealed ✓
• BlockAccessListSystemContractHandler sealed, directly implements ISystemContractHandler (no intermediate class hierarchy) ✓

---

Minor Issues

1. BlockValidationTransactionsExecutor.SetBlockAccessListManager is dead code

BlockValidationTransactionsExecutor.cs:30

public virtual void SetBlockAccessListManager(in IBlockAccessListManager balManager) { }

This virtual no-op is never called — BlockProcessor.cs only calls SetBlockExecutionContext on the executor. It is also not on IBlockProcessor.IBlockTransactionsExecutor. The corresponding stub in SimulateBlockValidationTransactionsExecutor.cs:60 throws NotImplementedException. If the method is dead code, both should be deleted; if it has a future use, remove the NotImplementedException stub. Fix this →

2. Restore method inconsistency — balance uses change.BlockAccessIndex, code/nonce/storage use Index

BlockAccessList.cs:323, 332, 341, 351

accountChanges.PopBalanceChange(change.BlockAccessIndex, out _);  // line 323
accountChanges.PopCodeChange(Index, out _);                       // line 332
accountChanges.PopNonceChange(Index, out _);                      // line 341
slotChanges.TryPopStorageChange(Index, out _);                    // line 351

change.BlockAccessIndex and Index are equal during a restore (you only restore changes from the current tx), but the inconsistency is a latent trap for future maintainers. Standardise to change.BlockAccessIndex throughout. Fix this →

---

Acknowledged as Future TODOs (out of scope per LukaszRozmej)

• Enumerable.SequenceEqual on ChangeAtIndex.SlotChanges (BlockAccessListManager.cs:344) — yield-return iterator allocations per matched account per ValidateBlockAccessList call
• ChangeAtIndex.SlotChanges field stores IEnumerable<SlotChanges> from SlotChangesAtIndex(index) (BlockAccessList.cs:379) — iterator state machine allocated per account unconditionally in GetChangesAtIndex
• N VirtualMachine + TxProcessorWithWorldState allocations per block (BlockAccessListManager.cs:493) — // todo: will optimize to allocate ~16 worldstates upfront
• GetNonce/GetBalance O(N) linear scan (AccountChanges.cs:271) — // todo: binary search
• IBlockAccessListManager God object — // todo: maybe split into smaller classes

---

Summary

All prior blocking items are resolved. The two minor items above (#1 dead code, #2 Restore inconsistency) are clean-up nits that do not affect correctness or safety. The acknowledged performance TODOs are tracked in-code and deferred. With ParallelExecution defaulting to false, the feature is safely gated. The PR is mergeable.

[LukaszRozmej]

@claude one more review

[claude]

Claude finished @LukaszRozmej's task in 3m 31s —— View job

---

Re-Review: Parallel Transaction Execution with BALs

• Load repository rules
• Read recent commits and changed files
• Check status of issues from previous review
• Look for new issues
• Post final review

---

Fixed Since April 27 13:40 Review

All two minor items from that review are confirmed fixed:

• SetBlockAccessListManager dead code — virtual no-op removed from BlockValidationTransactionsExecutor.cs, corresponding NotImplementedException stub removed from SimulateBlockValidationTransactionsExecutor.cs ✓
• BlockAccessList.Restore inconsistency — balance branch was the odd one out using change.BlockAccessIndex; all four cases now uniformly use Index (correct since snapshot/restore is per-tx and Index doesn't change within a snapshot boundary) ✓

---

Acknowledged TODOs (out of scope per LukaszRozmej)

These items persist but are explicitly deferred to follow-up PRs:

• Enumerable.SequenceEqual on ChangeAtIndex.SlotChanges (BlockAccessListManager.cs:344) — two yield-return enumerator allocations per matched account per ValidateBlockAccessList call
• ChangeAtIndex.SlotChanges stores IEnumerable<SlotChanges> from SlotChangesAtIndex(index) (BlockAccessList.cs:379) — iterator state machine allocated per account unconditionally in GetChangesAtIndex
• N VirtualMachine + TxProcessorWithWorldState allocations per block (BlockAccessListManager.cs:493)
• GetNonce/GetBalance O(N) linear scan (AccountChanges.cs:271, 286)
• IBlockAccessListManager split (IBlockAccessListManager.cs:15)
• SlotChanges.Get byte[] allocation per SLOAD (SlotChanges.cs:67, 73)
• AccountChanges.Equals/SlotChanges.Equals using LINQ SequenceEqual

---

No New Issues Found

This pass found no new correctness, safety, or performance concerns beyond the acknowledged TODOs.

---

Overall Assessment

All blocking and minor items raised across multiple review rounds are now resolved. The two items fixed in today's commits (SetBlockAccessListManager removal, Restore normalization) were the last outstanding non-deferred findings. With ParallelExecution defaulting to false, the hot-path TODOs are safely gated behind a feature flag. The PR is mergeable.