Conversation
Within one account the 52-byte flat storage key's address prefix/suffix are constant, so slotHash alone decides on-disk order. Sorting each account's prefetch jobs by slotHash before the parallel sweep turns scattered point gets into a sequential keyspace scan. Value path is unchanged - only read issue order changes - so sink contents (and consensus) are unaffected.
Generalize the per-account slot sort into one global sort keyed by (addrHash[0..4], slotHash), matching the flat storage key layout addrHash[0..4] ++ slotHash ++ addrHash[4..20]. The parallel prefetch sweep now covers a contiguous keyspace slice across all accounts, not just within one. The 16-byte addrHash suffix tie-break is dropped as negligible (~1-in-2^32 shared prefix). Value path unchanged.
Shared foundation for the BAL existing-key SLOAD levers: assigns every declared storage read a unique global ordinal g (0 <= g < TotalReads) via a per-account prefix-sum ReadBase, so prefetch/no-cache/coverage can key by ordinal without per-slot hashing. Ordinals are implied by position, never stored per slot. Slot lookup is tiered: a monotonic cursor makes ascending streams O(1), small sets scan linearly, larger ones binary-search the decoder-sorted StorageReads. No consumers yet - wiring (prefetch fill, execution consume) follows. slotHash[]/addrHash, RChargeable, an open-address tier, and pooling are deferred until a consumer needs them.
O(1) store/lookup indexed directly by the dense global read ordinal, sized once to the block's exact declared-read count - no hashing, no eviction, so it serves a 142k-read set the fixed-capacity associative cache would conflict-evict. Per-slot release/acquire on a state byte (single writer per ordinal, no CAS); a loaded zero is Ready-with-empty, distinct from not-loaded. Backing arrays pooled, released on Dispose. No consumers yet - prefetch fill + execution consume wiring follows.
Wire the dense read-ordinal foundation end to end for large-read-set blocks: - PreBlockCaches holds a per-block BalStorageReadPlan + BalStorageValueCache, built (BuildStorageReadDestination) when TotalStorageReads exceeds the associative StorageCache capacity, released on ClearCaches. - The prewarmer builds the destination before HintBal; CacheSink routes declared reads into it by global ordinal (changes/small blocks keep the StorageCache). - BlockAccessListBasedWorldState.Get serves a pure declared read from the destination when prefetched, byte-identical to the parent reader; a not-yet-loaded slot or small block falls through unchanged. Dual-path by design: small blocks and every miss degrade to today's path, so the change is additive. PreBlockCaches threaded into the parallel per-worker world state via the tx-processor pool.
"No-cache" was misleading - the Priority 2 read path does not skip a read cache; it skips PersistentStorageProvider's per-block change registry (PushToRegistryOnly) for a slot the BAL proves is a pure read.
Matches the read-storage word order used elsewhere (TryGetPureReadStorage). Pure rename - type, file, and test fixture; no behavior change.
…ads (P2) A BAL-declared storage read is read-only this block, so original == current == pre-state. Serve both Get and GetOriginal for the slotChanges-null branch via a journal-bypassing read (PersistentStorageProvider.GetPureRead) instead of parentReader.Get/GetOriginal, dropping the per-read _originalValues / _changes / _intraBlockCache bookkeeping those reads never use. Routing GetOriginal through the same pure read (not parentReader.GetOriginal) keeps the invalid-block path intact: an SSTORE on a read-declared slot still no-ops and is caught by BAL validation, never by GetOriginal throwing. IWorldState.TryGetPureReadStorage is a default-false method (keep the journal, use the normal path); only the backing WorldState overrides it.
Mirror the storage pure-read for accounts: a BAL-declared account read is read-only this block, so route GetBalance/GetNonce/GetCodeHash/GetCode and AccountExists' parent fallback through StateProvider.GetPureRead (GetState without PushJustCache), dropping the per-account ChangeType.JustCache entry in _changes/_intraTxCache the read-only parent never uses. IWorldState.TryGetPureReadAccount is default-false (keep the journal); only the backing WorldState overrides it. TryGetAccount stays on the normal path - it is not a VM hot path and reconstructs the storage root, which the pure read would complicate for no real gain.
The journal-bypassing read re-probes the SeqlockCache per call, which is slower than the normal upper-cache hit on a repeated same-slot read (the known same-key regression). Two fixes: - Read-through: on an ordinal-destination miss, cache the parent pure-read by ordinal (BalStorageValueCache.Set), so later repeats of a not-yet-prefetched declared slot hit O(1). Pure-read now returns byte[] so this stores the parent's array with no copy. - Gate on the destination: small blocks (no destination) keep the normal registered Get, whose upper cache is faster for repeated reads; the dropped journal saving is negligible there. Pure-read now applies only to large blocks, where repeats are served O(1) by the destination. Accounts are unaffected (their pure-read hits the _blockChanges dict on repeats, the same shape as the normal _intraTxCache hit).
Add a parameterized fixture that drives synthetic per-tx slices through the real generated validation index (RegisterGeneratedSlice) rather than the GeneratedBlockAccessList.Merge shortcut, which bypasses both the structural read-equivalence lane and the per-slice chargeable read budget. Locks the current consensus outcomes (declared-read coverage, content mismatch, untouched-account read, count shortfall, per-slice budget on repeated reads, and system-contract reads compared structurally but excluded from the budget) so they hold identically once read materialization is replaced by the dense-ordinal coverage bitmap. Exposes a minimal internal RegisterGeneratedSliceForTest seam to the test assembly, mirroring the existing HasGeneratedValidationIndexUpdates pattern.
…tion BalReadCoverage proves the suggested BAL's declared storage reads were all executed without materializing a generated read set. Two lanes over the dense ordinal space of BalReadStoragePlan: a structural coverage bitmap (every declared read, system-contract reads included) and a per-slice chargeable count (non-system reads, distinct within a slice, summed across slices) for the EIP-7928 read budget. Per SLOAD it is a single non-atomic bit OR plus at most one stamp write; at block end worker bitmaps OR-reduce via Absorb and TryFindFirstUncovered checks the implied all-bits-set expectation in O(R/64), replacing the O(R log R) sort-and-dedup of the materialized read set. Backing arrays are pooled.
…alization Replaces per-read generated read-set materialization on the verify-only parallel validation path with a per-slice read-coverage bitmap over the dense ordinal space of the shared read plan. On the SLOAD hot path, BlockAccessListBasedWorldState.Get marks a declared read's ordinal (folded onto an ascending-read cursor) instead of inserting into a per-account HashSet; TracedAccessWorldState skips storage-read materialization in this mode (the account read is still recorded for the presence lanes). At block end the workers' per-slice coverages are OR-reduced and every declared read must be covered (all bits set), replacing the O(R log R) sort-dedup + content compare. The per-slice chargeable count (non-system, distinct in slice) is captured from each slice's coverage at return and tallied incrementally, so the EIP-7928 read budget keeps its exact per-chunk timing. Two-lane semantics preserved: system-contract reads are covered structurally but excluded from the chargeable budget; undeclared execution reads still fail fast via ThrowMissingStorage; account-set equivalence is unchanged (the suggested index marks every account, and coverage-mode reads are always declared). Gated to verify-only parallel validation; recorder, sequential, and ForceConstructGeneratedBlockAccessList keep full read materialization. Per-slice coverage (fresh per tx, OR-reduced) avoids a per-worker block tag and a sliceStamp array; the registry is a ConcurrentQueue so the validator reliably drains every worker-enqueued coverage. Tests cover the bitmap primitive, the world-state marking/reduce/charge/gap, and the manager's end-to-end accept and uncovered-read reject (via a system read, caught by coverage not the budget).
The block-end coverage operations over ceil(R/64) words are SIMD-friendly: Absorb OR-reduces a slice's bitmap into the accumulator, and TryFindFirstUncovered must confirm every word is all-ones (the common valid- block case). Use fixed-width Vector512/Vector256/Vector128 (widest accelerated ISA, scalar tail) via LoadUnsafe/StoreUnsafe over the array data reference - no per-iteration bounds checks. TryFindFirstUncovered fast-skips fully-covered runs and drops to the scalar word scan only at the first vector block with a gap. MarkRead stays scalar (a single-bit OR on the hot path). Large-R tests exercise the vector paths and the scalar tail.
…d path Same-account SLOAD streams (the bloat loop walks one account in ascending slot order) re-probed the suggested BAL account dictionary on every read: ResolveContext -> GetAccountChangesOrThrow -> ReadOnlyBlockAccessList .GetAccountChanges is an uncached Dictionary.TryGetValue at the top of every Get/GetBalance/GetNonce/GetCodeHash/GetCode/TryGetAccount/AccountExists. Cache the resolved ReadOnlyAccountChanges under a single _contextAccount key and fold the P3 coverage cursor (account index + ascending-read cursor) into the same key, so a same-account stream resolves the BAL account dictionary once instead of once per read. The coverage plan index is resolved lazily so non-storage reads (balance/nonce/code) never probe the read plan. Invalidated on address change in ResolveContext and per slice in Setup / ClearParentReader. Cursor reset on a cross-account interleave falls back to the existing binary/linear search, so correctness is unchanged.
…e cell set Verify-only parallel BAL validation re-warms every declared (read-only) storage slot through JournalSet<StorageCell> on the SLOAD gas-charge path: a 52-byte StorageCell hash add + List append, with a HashSet remove per slot on sub-frame revert. Replace that, for the suggested BAL's declared reads, with a journaled ordinal bitset keyed by the dense read-ordinal plan (P1/P3). Routed inside StackAccessTracker.WarmUp(in StorageCell): ConsumeStorageAccessGas is unchanged - it already derives cold/warm from WarmUp's bool - so there is no gas-policy or SLOAD change. WarmUp(AccessList) routes per cell so EIP-2930 pre-warming hits the same lane. Warmth snapshot/restore fold into the tracker's TakeSnapshot/Restore under the existing tracing exception, so warm/cold revert is the same call as the cell-set revert and cannot diverge from the gas revert. The lane is enabled only for non-tracing verify-only txs (TransactionProcessor attaches a per-tx pooled BalReadWarmth when the world state exposes a declared-read plan); non-BAL execution leaves it null and takes the unchanged cell-set path. The lanes are disjoint (EIP-7928 reads are read-only, changes are written) so a cell is never in both, and IsCold(in StorageCell) has no callers. Also make StorageCell.Index a readonly field rather than a get-only property so it passes by `in` without copying the 32-byte UInt256, and pass it by `in` at the storage Get/HintSet and read-ordinal lookup call sites.
On a fast/empty block (e.g. during a gas-limit bump), BranchProcessor cancels the
background prewarm token right after ProcessOne, then drains the prewarm task via
WaitAndClear -> task.GetAwaiter().GetResult(). The BAL read-warming path returns the
HintBal task directly, and HintBal scheduled its body with Task.Run(body, token):
when the token is cancelled before the thread pool starts the body, the task ends in
the Cancelled state, so GetResult re-throws TaskCanceledException and fails the block:
System.Threading.Tasks.TaskCanceledException: A task was canceled.
at BranchProcessor.<Process>g__WaitAndClear|21_1 ... BranchProcessor.cs:line 194
The body already observes cancellation itself (early-return check + catch
OperationCanceledException), so passing the token to Task.Run is the only defect -
it converts an expected, benign cancellation into a faulted await. Drop the token
from Task.Run in both backends (FlatWorldStateScope, TrieStoreScopeProvider), matching
the documented pattern already used by BlockCachePreWarmer. The token is still observed
inside the body via ParallelOptions and the IsCancellationRequested check, so no warming
work runs after cancellation.
Regression test: ScopeProviderTests re-hints to cancel the first warming task's token,
then drains it as WaitAndClear does and asserts it neither throws nor ends Cancelled.
…ocations Addresses the storage_sload_same_key regression and two review findings (review.md 2-4) without changing warm/cold gas or coverage semantics. P6 (warm-bitset) regressed warm-read-heavy workloads ~23% because every verify-only EVM transaction allocated a block-sized BalReadWarmth (rent + clear of a ulong[] bitset and an int[TotalReads] journal) before knowing if it touched any declared read - undoing the per-access warm-read win from #11905. Fix: pool the warmth on the per-worker BlockAccessListBasedWorldState (GetDeclaredReadWarmth), rebuilt only when the block's read plan changes and reset per transaction; the tracker now only references it (the world state owns and disposes it). BalReadWarmth.Reset clears via the journal (O(warmed)) when cheaper than a full bitset clear, so a tx that warmed few of many declared reads no longer pays O(capacity). Also (review findings): - Coverage mode no longer pre-sizes the generated storage-read list it never materializes: resolve verify-only/coverage mode before building the generated validation index and pass zero read capacity when ReadCoverageEnabled, avoiding a large per-block backing allocation. - Reset ChargeableReadCount in BlockAccessListAtIndex.Clear so pooled slices carry no stale coverage-read count.
Benchmarks (engine_newPayloadV5) show the warm-bitset is a net loss: it regressed storage_sload_same_key ~15% by doing a per-access plan.TryGetGlobalReadOrdinal lookup on every SLOAD - dearer than the JournalSet<StorageCell> probe it replaced (which #11905 had already optimized) - and pooling the warmth per worker did not recover it (the cost is the lookup, not the allocation). Its target case, the warm-set on large declared-read blocks (sload_bloated existing_slots_True), was unmoved (+1.3%), and the existing_slots_False win (-21%) comes from the prefetch/pure-read paths, not this lever. Net: ~15% slower on the common warm-read path for no measurable gain, at the highest consensus risk of the series. Removes BalReadWarmth, the StackAccessTracker warmth routing/snapshot, the IWorldState.GetActiveDeclaredReadPlan/GetDeclaredReadWarmth seam, and the TransactionProcessor attach. Kept: the storage-value prefetch, parent pure-read path, read-coverage bitmap, per-account context-cache, the StorageCell.Index readonly field and in-call-site changes, the HintBal cancellation fix, and the coverage-allocation and ChargeableReadCount cleanups (all independent of the warm-bitset).
Completes the review finding: previously the BAL read-warming task observed only its own internal re-hint/dispose cancellation, so the block processor's tx-complete background cancel never reached it and the warming ran to completion in the background (ffe3c11 stopped it faulting WaitAndClear, but not the wasted work). Thread a CancellationToken through IWorldState.HintBal / IWorldStateScopeProvider.HintBal and all implementers; BlockCachePreWarmer passes the prewarm token. The two real warming impls (FlatWorldStateScope, TrieStoreScopeProvider) link the caller's token into their own _hintBalCts via CreateLinkedTokenSource, so the per-iteration IsCancellationRequested checks already in the warming loops stop promptly on either cancellation source. The Task.Run body still carries no token, so a benign cancel completes rather than faults. Consensus-neutral: HintBal only pre-populates caches/prefetch (the read path self-heals on miss), so stopping it earlier changes cache warmth, never state. Regression test added for the caller-token cancel path.
FlatWorldStateScope.HintBal pushed a WarmUpStateTrie job for every BAL account, read-only or written. In FlatDB account reads are served from the flat store and the post-block state-root recomputation only re-hashes changed accounts, so a read-only account's state-trie node is never needed - the warm job was wasted work competing for warmer threads on read-heavy blocks. ShouldQueuePrewarm only dedupes (a bloom filter), it does not filter read-only accounts. Gate PushAddressJob on ReadOnlyAccountChanges.HasStateChanges, mirroring the storage path (which already warms trie nodes only for StorageChanges). Read-only accounts still get their account and slot values prefetched from the flat store in phase 2; only the unnecessary state-trie warming is skipped. Consensus-neutral: warming only pre-populates caches, the root computation result is unchanged.
ShouldPrewarm's bloom-filter key used XxHash64 - for slots over a stack-allocated 52-byte address++slot buffer (with a big-endian conversion). Switch to the AES-based FastHash64 already used for StorageCell/Hash256: the address via Address.GetHashCode64 (FastHash64For20Bytes, tuned for zero-padded addresses) and the slot XORed in via FastHash64For32Bytes read in place - dropping the per-slot stackalloc, copy, and big-endian conversion. Both hashes are InstanceRandom-seeded and stable within a run, so the bloom's Add/MightContain agree; the bloom is per-scope/transient so nothing persists across runs. Consensus-neutral (the bloom only gates whether to prewarm). Address .GetHashCode64 is made public for the cross-assembly call.
…ewarm-dedup HintBal phase 1 must not early-return for read-only accounts: they still need the sink/prefetch path (GetAccount + OnAccountRead + accounts[i]) so phase 2 prefetches their declared StorageReads - the read-warming this whole path exists for. The HasStateChanges gate already skips their state-trie warming, and the storage-warming loop is gated on storageChangeCount, so read-only accounts do zero change-warming and zero StorageChanges iteration (the array is empty by construction) while still being read-prefetched. Restores two sink tests that the early return broke. Also skip slot entries with no recorded change in the warming loop. Split ShouldQueuePrewarm/ShouldPrewarm into non-nullable overloads - ShouldPrewarm(Address) and ShouldPrewarm(Address, in UInt256 slot) sharing a private ShouldPrewarm(ulong) - so the prewarm-dedup hash path drops the UInt256? wrapper and passes the slot by ref.
Two review fixes in the BAL read-warming path, both consensus-neutral (warming only pre-populates caches; the read path self-heals on a miss): - Swallow non-cancellation faults inside the HintBal Task.Run body so the returned task stays RanToCompletion. BranchProcessor.WaitAndClear drains it with GetResult() and would otherwise fail an already-executed block on a background prefetch error. - Observe cancellation in RunSinkSlotReads before counting, per-account during job materialization, and before the sort, so a block that finishes while warming is in that prologue stops hashing/sorting instead of running it out.
|
Claude finished @benaadams's task in 8m 53s —— View job PR Review: BAL Sload
OverviewThis PR adds the SLOAD-side of BAL parallel execution: dense-ordinal read coverage tracking (replacing read-set materialization on the hot path), an exact-sized ordinal-keyed prefetch destination for large read sets, and prefetch of declared read-only slots in High
Medium
The remarks assert a single writer per ordinal, but two concurrent writers are possible:
Both read from the same parent state root so they produce identical values, making the race benign. However, the documented invariant is wrong. A future change that diverges the two read paths (e.g., different snapshot roots) would silently corrupt values. The remarks should reflect the two-writer reality and the "same state root → same value" assumption. (
BalReadStoragePlan plan = preBlockCaches!.StorageReadPlan!;The invariant is correct — coverage is only active when the plan exists (
_generatedChargeableStorageReads += (int)slice.ChargeableReadCount;
ref byte ab = ref MemoryMarshal.GetReference(a.Bytes);
...
Unsafe.As<byte, Vector128<byte>>(ref ab) == ... // bytes 0–15
Unsafe.As<byte, uint>(ref Unsafe.Add(ref ab, 16)) == ... // bytes 16–19
Low
No test for The test suite covers prewarmer-populated reads and coverage bitmap logic. Missing is a test where the prewarmer has NOT yet filled an ordinal and the worker falls through to Positive observations
SummaryOne High finding (missing bounds guard in SIMD |
benaadams
changed the title
There was a problem hiding this comment.
Pull request overview
This PR optimizes Block Access List (BAL) SLOAD/read handling by introducing an ordinal-based storage-read plan, an exact-sized prefetch destination for large read sets, and a verify-only “read coverage” lane to avoid materializing generated read sets during parallel validation. It also threads cancellation through HintBal so background warming can stop promptly without surfacing task cancellation failures during block processing.
Changes:
- Add dense ordinal modeling for declared storage reads, plus an ordinal-indexed prefetch destination and per-slice read-coverage validation.
- Improve
HintBaloperational behavior (cancellation linking, avoiding pre-start task cancellation, best-effort fault handling) and move BAL warming initiation into the cache prewarmer path. - Reduce unnecessary warming work in FlatDB (e.g., skip state-trie warmup for read-only accounts) and add/extend tests to pin new semantics.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Nethermind/Nethermind.State/WorldStateScopeOperationLogger.cs | Propagates new HintBal cancellation token through scope logger wrapper. |
| src/Nethermind/Nethermind.State/WorldStateMetricsScopeProvider.cs | Propagates new HintBal cancellation token through metrics wrapper. |
| src/Nethermind/Nethermind.State/WorldState.cs | Adds pure-read APIs and threads cancellation into HintBal. |
| src/Nethermind/Nethermind.State/TrieStoreScopeProvider.cs | Links caller cancellation into background HintBal and avoids pre-start cancellation behavior. |
| src/Nethermind/Nethermind.State/TracedAccessWorldState.cs | Adds coverage-mode toggle and adjusts tracing for coverage-only reads. |
| src/Nethermind/Nethermind.State/StateProvider.cs | Adds account pure-read path that bypasses intra-tx journal entries. |
| src/Nethermind/Nethermind.State/PrewarmerScopeProvider.cs | Routes storage reads into ordinal destination when present; passes cancellation through. |
| src/Nethermind/Nethermind.State/PersistentStorageProvider.cs | Adds storage pure-read path bypassing registry/journal and updates in usage. |
| src/Nethermind/Nethermind.State/BlockAccessListBasedWorldState.cs | Implements declared-read pure-read serving + per-slice read coverage marking and context caching. |
| src/Nethermind/Nethermind.State.Test/StorageProviderTests.cs | Updates test scope decorator for new HintBal signature with cancellation. |
| src/Nethermind/Nethermind.State.Test/ScopeProviderTests.cs | Adds tests for re-hint cancellation and caller-token cancellation behavior. |
| src/Nethermind/Nethermind.State.Test/BlockAccessListBasedWorldStateTests.cs | Adds tests for ordinal destination serving, pure reads, and read coverage reduce behavior. |
| src/Nethermind/Nethermind.State.Flat/TransientResource.cs | Changes prewarm hashing and adds new overloads for slot-vs-no-slot prewarm keys. |
| src/Nethermind/Nethermind.State.Flat/SnapshotBundle.cs | Splits prewarm gating into address-only vs (address,slot) overloads. |
| src/Nethermind/Nethermind.State.Flat/ScopeProvider/FlatWorldStateScope.cs | Links cancellation into warming, avoids task pre-start cancellation, improves ordering of sink reads. |
| src/Nethermind/Nethermind.Evm/State/PreBlockCaches.cs | Adds read-plan/destination builders and coverage reduce queue; releases resources on clear. |
| src/Nethermind/Nethermind.Evm/State/IWorldStateScopeProvider.cs | Extends HintBal with cancellation token. |
| src/Nethermind/Nethermind.Evm/State/IWorldState.cs | Extends HintBal with cancellation and adds default pure-read APIs. |
| src/Nethermind/Nethermind.Core/StorageCell.cs | Changes storage index representation to allow in passing and adjusts hashing/equality accordingly. |
| src/Nethermind/Nethermind.Core/BlockAccessLists/BlockAccessListAtIndex.cs | Adds per-slice chargeable read count for coverage-based budget accounting. |
| src/Nethermind/Nethermind.Core/BlockAccessLists/BalStorageValueCache.cs | New: ordinal-indexed prefetch destination for large declared read sets. |
| src/Nethermind/Nethermind.Core/BlockAccessLists/BalReadStoragePlan.cs | New: dense ordinal model over declared BAL storage reads with tiered lookup. |
| src/Nethermind/Nethermind.Core/BlockAccessLists/BalReadCoverage.cs | New: per-slice coverage bitmap + chargeable count with SIMD-accelerated reduce/check. |
| src/Nethermind/Nethermind.Core/Address.cs | Exposes GetHashCode64() publicly for reuse in hashing optimizations. |
| src/Nethermind/Nethermind.Core.Test/BlockAccessLists/BalStorageValueCacheTests.cs | New: unit tests for ordinal destination behavior and semantics. |
| src/Nethermind/Nethermind.Core.Test/BlockAccessLists/BalReadStoragePlanTests.cs | New: unit tests for ordinal plan correctness and lookup behavior. |
| src/Nethermind/Nethermind.Core.Test/BlockAccessLists/BalReadCoverageTests.cs | New: unit tests for coverage bitmap and reduce semantics. |
| src/Nethermind/Nethermind.Consensus/Stateless/WitnessGeneratingWorldState.cs | Forwards new HintBal cancellation token to inner world state. |
| src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs | Removes direct BAL warmup call (now driven by cache prewarmer). |
| src/Nethermind/Nethermind.Consensus/Processing/BlockCachePreWarmer.cs | Drives BAL warming (and destination build) with caller cancellation token. |
| src/Nethermind/Nethermind.Consensus/Processing/BlockAccessListValidationIndex.cs | Adds option to skip read comparison when coverage lane validates reads. |
| src/Nethermind/Nethermind.Consensus/Processing/BlockAccessListManager.Validation.cs | Adds coverage-based read validation and coverage-path chargeable accounting. |
| src/Nethermind/Nethermind.Consensus/Processing/BlockAccessListManager.TxProcessorPool.cs | Plumbs PreBlockCaches to workers and captures slice chargeable reads on return. |
| src/Nethermind/Nethermind.Consensus/Processing/BlockAccessListManager.cs | Enables coverage mode earlier and avoids pre-sizing generated read buffers on coverage path. |
| src/Nethermind/Nethermind.Consensus.Test/BlockCachePreWarmerTests.cs | Updates test wiring for new prewarmer dependency on IWorldState. |
| src/Nethermind/Nethermind.Blockchain.Test/BlockProcessorTests.cs | Adds coverage that prewarmer is invoked even for tiny BAL blocks; updates token-capture helper. |
| src/Nethermind/Nethermind.Blockchain.Test/BlockAccessListReadCoverageValidationTests.cs | New: locks consensus behavior for verify-only read coverage + budget lane semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nTests The test's namespace is nested under Nethermind.Core, so the enclosing namespace is already in scope (IDE0005).
…che docs From the PR review: - BalReadCoverage.Absorb: Debug.Assert the two coverages share an ordinal space; the SIMD loop reads other._coverage up to this._wordCount, so a mismatch would overread. - BalStorageValueCache: the "exactly one prefetch writer" remark was wrong - the prefetch fill and a worker reading through on a destination miss can both write an ordinal. They resolve the same parent root so values are identical and the race is benign; docs now say so. - MarkDeclaredReadCoverage: Debug.Assert the storage read plan is present before the null-forgiving access (coverage is only enabled after the plan is built), so a broken invariant surfaces meaningfully rather than as an NRE.
|
@claude re-review |
|
Claude finished @benaadams's task in 6m 53s —— View job PR Re-Review: BAL Sload Optimization
Previous findings — statusAll four High/Medium findings from the first review have been addressed in the latest commits:
Remaining findingsMedium —
Low —
Low — Missing test for
New findings (this review pass)No new Critical, High, or Medium issues found. The concurrency model, release/acquire publication pattern, coverage-queue lifecycle, pool hygiene in Summary4 of 6 prior findings resolved. 1 Medium ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… self-heal - BalReadStoragePlan.MapOrdinalToAddress: Debug.Assert(false) before the Address.Zero sentinel. The sentinel is only reachable on a coverage-mismatch error path and Address.Zero is itself a valid address, so a real out-of-range ordinal would otherwise produce a misleading "0x0..0" in the error message. - RegisterGeneratedSlice: Debug.Assert the chargeable read count fits in int before the narrowing cast. The EIP-7928 budget stays far below int.MaxValue at any feasible gas limit, so the cast is kept (no checked, no widening). - BlockAccessListBasedWorldStateTests: extend the destination self-heal test to assert the value is cached in the destination after a miss read-through (and a second Get returns it), pinning that destination.Set fires on miss.
|
@claude re-review |
|
Claude finished @benaadams's task in 2m 20s —— View job PR Re-Review: BAL Sload Optimization (Pass 3)
Carry-over findings — status (commit
|
| Finding | Status |
|---|---|
Medium: (int)slice.ChargeableReadCount narrowing cast |
✅ Debug.Assert(slice.ChargeableReadCount <= int.MaxValue, ...) added at BlockAccessListManager.Validation.cs:288 with inline comment explaining safety at any feasible gas limit |
Low: MapOrdinalToAddress returns Address.Zero sentinel |
✅ Debug.Assert(false, "ordinal out of range") added at BalReadStoragePlan.cs:100; comment notes Address.Zero is a valid address and must not be used as a silent sentinel |
Low: No test for destination-miss → Set self-heal path |
✅ Get_DeclaredRead_ServedFromOrdinalDestination_ElseFallsBackToParent extended to assert TryGet(ordinal7) is true after the fallback read and that a second bws.Get(slot7) returns from the cached destination, not the parent |
New findings
None. No new Critical, High, or Medium issues found in this pass. The concurrency model, pool hygiene, SIMD coverage logic, and all prior positive observations remain intact.
Summary
All 6 findings raised across the two prior reviews are now resolved. The PR is mergeable.
| // Reads beyond the associative StorageCache's 2-way capacity (16384 sets x 2) conflict-evict, | ||
| // so a block declaring more than this many reads is served from an exact-sized ordinal | ||
| // destination instead. Below the threshold the StorageCache suffices and none is built. | ||
| public const int StorageReadDestinationThreshold = 32768; |
There was a problem hiding this comment.
I think mainnet never reach this yet. Can you remove the larger storage to reduce the risk of sudden untested consensus issue in the future? Or make this configurable so that its easy to test.
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
It seems it is very side-effecty and some things are not passed corrrectly?
| private bool ShouldPreWarm(Block suggestedBlock, IReleaseSpec spec) | ||
| => !_parallelExecutionEnabled | ||
| || !spec.BlockLevelAccessListsEnabled | ||
| || suggestedBlock.BlockAccessList is null |
There was a problem hiding this comment.
Is this correct? Shouldn't it be:
| || suggestedBlock.BlockAccessList is null | |
| || suggestedBlock.BlockAccessList is not null |
If it is correct then: https://github.com/NethermindEth/nethermind/pull/11920/changes?w=1#diff-1f8c02f97ace7dd6ea858679a438371fa7de947ba3f33e53d0dde1d1c4aa2fedR94
is always null?
This is confusing
| @@ -87,6 +92,15 @@ public Task PreWarmCaches(Block suggestedBlock, BlockHeader? parent, IReleaseSpe | |||
| // BAL makes speculative tx execution redundant — when BAL-based read warming | |||
| // is in use, drive warmup directly off the suggested block's access list. | |||
| ReadOnlyBlockAccessList? bal = IsBalReadWarmingEnabled(spec) ? suggestedBlock.BlockAccessList : null; | |||
There was a problem hiding this comment.
This is always null based on ShouldPreWarm?
| /// <summary> | ||
| /// Records a declared read at global <paramref name="ordinal"/>. Marks the structural coverage bit; | ||
| /// when <paramref name="chargeable"/> (a non-system-contract read) and this is the first time the | ||
| /// ordinal is seen this slice, counts it. | ||
| /// </summary> | ||
| public void MarkRead(int ordinal, bool chargeable) | ||
| { | ||
| ref ulong word = ref _coverage[ordinal >> 6]; | ||
| ulong mask = 1UL << (ordinal & 63); | ||
| if ((word & mask) == 0) | ||
| { | ||
| word |= mask; | ||
| if (chargeable) _chargeableCount++; | ||
| } | ||
| } |
There was a problem hiding this comment.
currently this mark's index-by-index? Can this be somehow speed up, agrregated in bulk-way? Does it matter?
| private readonly AccountEntry[] _accounts; | ||
| private readonly Dictionary<AddressAsKey, int> _addressToIndex; |
| /// <summary> | ||
| /// Reads a slot bypassing the change journal (no original-value / revert / cache entry the way | ||
| /// <see cref="Get"/> records). The value equals <see cref="Get"/>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Only safe for a slot that is never written or queried via <see cref="GetOriginal"/> this block, | ||
| /// since the skipped journal is what those rely on. The default returns <c>false</c> ("I keep the | ||
| /// journal - use the normal path"); a backing store that can read without journaling overrides it. | ||
| /// </remarks> | ||
| /// <returns><c>true</c> with the value when served without journaling; otherwise <c>false</c>.</returns> | ||
| bool TryGetPureReadStorage(in StorageCell cell, out byte[]? value) |
There was a problem hiding this comment.
Bit confusing, how does it differ Original value?
| // Don't pass token to Task.Run: the body observes cancellation itself (early-return + OCE | ||
| // catch). Passing it lets a pre-start cancel - a fast empty block cancels the background | ||
| // token before the pool starts this - mark the task Canceled, surfacing as | ||
| // TaskCanceledException in BranchProcessor.WaitAndClear and failing the block. |
| private bool _hasLastReadCell; | ||
| public BlockAccessListAtIndex? GetGeneratingBlockAccessList() => _generatingBlockAccessList; | ||
| public void SetGeneratingBlockAccessList(BlockAccessListAtIndex? bal) => _generatingBlockAccessList = bal; | ||
| public void SetReadCoverageActive(bool active) => _coverageActive = active; |
| catch (Exception ex) | ||
| { | ||
| // Warming is best-effort and self-heals on a read miss. Swallow non-cancellation faults | ||
| // here so the returned task stays RanToCompletion: BranchProcessor.WaitAndClear drains it | ||
| // with GetResult() and would otherwise fail an already-executed block on a prefetch error. | ||
| ILogger logger = _logManager.GetClassLogger<FlatWorldStateScope>(); | ||
| if (logger.IsError) logger.Error("HintBal read warming faulted; ignoring (reads self-heal)", ex); | ||
| } |
| // Don't pass token to Task.Run: the body observes cancellation itself (early-return + OCE | ||
| // catch). Passing it lets a pre-start cancel - a fast empty block cancels the background token | ||
| // before the pool starts this - mark the task Canceled, surfacing as TaskCanceledException in | ||
| // BranchProcessor.WaitAndClear and failing the block. |
| if (totalSlots == 0) return; | ||
|
|
||
| using ArrayPoolList<(Address Address, int SelfDestructIdx, UInt256 Slot)> jobs = new(totalSlots, totalSlots); | ||
| using ArrayPoolList<(Address Address, int SelfDestructIdx, UInt256 Slot, ValueHash256 SlotHash, uint AddrPrefix)> jobs = new(totalSlots, totalSlots); |
There was a problem hiding this comment.
Make custom struct for Job with IComparable
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
Hi @benaadams — follow-up to my review with the verified findings behind "very side-effecty and some things are not passed correctly". The 14 inline comments above are still the source of truth; this is the prioritized synthesis with reachability checks and recommended fixes.
Most important
1. ReadCoverageActive has three sources of truth. PreBlockCaches._readCoverageEnabled → BlockAccessListBasedWorldState._readCoverage → TracedAccessWorldState._coverageActive, last hop via the SetReadCoverageActive setter at BlockAccessListManager.TxProcessorPool.cs:416. A future wrapper that forgets to forward will silently take the wrong tracing path (RecordStorageReadAndGet materialising reads instead of the coverage-only RecordReadAndGet). This is the root cause of the "side-effecty" feel. Two cleaner shapes:
- Read-through:
TracedAccessWorldState._coverageActivebecomes(_innerWorldState as BlockAccessListBasedWorldState)?.ReadCoverageActive ?? false— one source of truth, no setter. - Per-block context object passed once through
Setup(block, context)carryingReadCoverageActive, the plan, the destination — wrappers receive the same context, no setters propagate.
2. FlatWorldStateScope.cs:267 swallows all non-cancellation exceptions. The rationale (don't fail BranchProcessor.WaitAndClear.GetResult()) is real, but the cure is broader than the disease. A corrupted flat-store read or unexpected disk fault is logged-and-ignored, and the block continues — relying on every consumer correctly routing through the journal-bypassing path so the "reads self-heal" claim holds. TrieStoreScopeProvider does not mirror this swallow (only OCE), so the two scope backends have divergent fault behaviour for the same HintBal contract. Either type-filter (MissingTrieNodeException / IOException) or fix the caller to tolerate faulted tasks; the current broad catch masks future regressions.
3. ShouldPreWarm conflates two orthogonal modes. Your suggested edit to is not null would break the non-BAL fall-through — the existing logic is technically reachable both ways. But the function genuinely fuses "speculative prewarm because BAL is unavailable" with "enter PreWarmCaches to drive HintBal", and a reader can't tell from the call site which mode wins. Splitting into ShouldRunSpeculativePrewarm(b, s) and ShouldRunBalReadWarming(s) makes the L94 ternary collapse into a clear if/else and resolves your "is bal always null?" question structurally.
Design
4. Extract BAL bookkeeping out of PreBlockCaches. What was a 60-line "four caches per block" container now also owns the BAL read plan, the ordinal destination, the coverage enable flag, and the coverage queue with reduce. Three new responsibilities. A BalBlockContext sibling on the same lifecycle keeps PreBlockCaches single-purpose and lets the BAL machinery be skipped at zero cost on non-BAL networks/forks. Also resolves your "both public?" concern — only the wrapper accessor needs to be public.
5. Drop the Dictionary<AddressAsKey, int> in BalReadStoragePlan. Accounts are already address-sorted in the BAL by decoder contract (you assert this for slot reads via AssertAscending). BinarySearch over _accounts[].Address recovers accountIndex in O(log n) without any allocation. TryGetAccountIndex is hit at most once per BAL account per worker (cached by _contextAccount), so log-n cost is negligible vs. dictionary build + rehash + retained capacity per block. For 10k+-account blocks at chain tip this is real allocation pressure.
6. PreBlockCaches.RentReadCoverage() is named "Rent" but allocates. Only the inner ulong[] is pooled (via ArrayPool in the BalReadCoverage ctor) — the wrapper object is new per slice per worker. Either pool the wrapper (ObjectPool<BalReadCoverage>) or rename to CreateReadCoverage so the API doesn't suggest pooling that isn't happening.
Polish
7. Shorten the duplicated Task.Run cancellation rationale. Same paragraph at TrieStoreScopeProvider.cs:199 and FlatWorldStateScope.cs:272. Suggested three-liner:
// Token not passed: a pre-start cancel (fast empty block cancels before the pool starts this)
// would mark the task Canceled, failing the block in BranchProcessor.WaitAndClear. Body
// observes cancellation itself via early-return + OCE catch.Factor into one place (XML doc on HintBal, or a helper wrapping the Task.Run pattern) so the rationale doesn't drift between the two scope backends.
8. Delete the ConcurrentBag comment (PreBlockCaches.cs:48) — nethermind never reaches for ConcurrentBag; the type choice is self-explanatory.
9. Clarify TryGetPureReadStorage vs GetOriginal. The current <remarks> says "no original-value… entry the way Get records" which naturally pairs "original" with GetOriginal. Better: "Reads the slot from the underlying flat store without journaling. Equivalent value to Get for slots the caller knows are never written this block; unlike Get it does not register a journal entry, so this slot cannot subsequently be passed to GetOriginal."
10. PrewarmJob struct with IComparable<> at FlatWorldStateScope.cs:299. Span<T>.Sort() with IComparable<T> is faster than the Comparison-delegate path (no virtual dispatch, inlined CompareTo), and named fields beat the current (address, selfDestructIdx, slot, _, _) destructuring that loses the slotHash/prefix names.
11. Parameterise the four declared-read tests in BlockAccessListBasedWorldStateTests (L66–L260). They share CreateBlockAccessListState + BAL/genesis/access-index pattern + assertion shape; deltas (which slot is prefetched, which is accessed, expected coverage gap) map cleanly to [TestCase] arguments. Aligns with .agents/rules/test-infrastructure.md.
Not actionable
BalReadCoverage.MarkRead bulk? Can't be batched — called per-SLOAD synchronously inside the EVM interpreter loop. Current implementation is one shift + one OR + one branch, already L1-resident. Doesn't matter.
Priority order
If you want to land this PR with the smallest follow-up surface: #1, #2, #3 directly address my top-level review note. #4 and #5 are the design follow-ups worth doing now while the API surface is still small. The polish items (#6–#11) can land in a follow-up or batched here.
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
Second pass — independent findings not raised by the prior reviews (yours, mine above, or the two Claude rounds). I dropped finding G from my notes after re-verifying it was a false alarm.
Higher-impact
A. HintBal fault behaviour diverges between scope backends (consensus-relevant)
FlatWorldStateScope.cs:260 catches all non-cancellation exceptions and logs them. TrieStoreScopeProvider.cs:194 only catches OperationCanceledException. Both implement the same HintBal contract, both feed BranchProcessor.WaitAndClear, and WaitAndClear calls GetResult() which propagates a faulted task into the block-processing thread.
So an IOException or MissingTrieNodeException raised during prefetch makes a TrieStore-backed sync fail the block, while a FlatStore-backed sync silently swallows it. The two backends produce different consensus outcomes under the same fault. The "reads self-heal" defence is defensible for the flat side if every consumer correctly takes the journal-bypassing path on a destination miss — but the asymmetry is undocumented and a real fault in flat prefetch is now invisible.
Pick one. Either tighten flat to type-filter (MissingTrieNodeException / IOException) and rethrow everything else, or extend trie to mirror flat. The same operation must fault identically across backends.
B. IsSystemContract duplicated and possibly incomplete
Two identical definitions at BlockAccessListManager.Validation.cs:326 and BlockAccessListBasedWorldState.cs:199, both checking only Eip7002Constants.WithdrawalRequestPredeployAddress and Eip7251Constants.ConsolidationRequestPredeployAddress.
EIP-7928 carves out "system contracts" from the read budget. The current check covers withdrawals (7002) and consolidations (7251). It doesn't mention EIP-4788 (beacon root) or EIP-2935 (historical block hashes), both of which the protocol calls every block. If those should also be exempt, the chargeable-read budget over-counts. If they shouldn't, the duplicated definitions are still a smell waiting for the next fork to add a third predeploy.
Single static helper + spec-check the predeploy list against EIP-7928.
C. System-contract read coverage attribution isn't explicit
ValidateReadCoverage at Validation.cs:425 demands every declared read ordinal be covered. Coverage is marked in BlockAccessListBasedWorldState.Get only when _readCoverage is non-null, which is set by SetupReadCoverage (line 88), which is called from the per-tx-slice Setup.
System contract reads at balIndex == 0 (pre-execution) and balIndex == txCount+1 (post-execution) happen on the main thread, not a worker slice. If those reads go through a different world state instance than the BAL world state with active coverage, their declared-read ordinals stay uncovered → TryFindFirstUncovered returns true → block rejected.
The pyspec passes 5514/5514, so apparently main-thread system reads do hit the same coverage-enabled world state in practice. But the chain of reasoning isn't in the diff. The Coverage_UncoveredSystemRead_ThrowsViaCoverageNotBudget test confirms enforcement, not attribution. Worth either a comment in ValidateReadCoverage naming which execution path covers the system slice's ordinals, or an integration test that drives a block with declared system reads through the actual block-handler pipeline rather than a synthetic RentReadCoverage.
D. RegisterGeneratedSlice has a fragile temporal coupling
Validation.cs:283 dispatches between the coverage path and the materialised path based on preBlockCaches?.ReadCoverageEnabled == true. This works only because EnableReadCoverage is called at BlockAccessListManager.cs:146 — strictly before the index is built and before any slice registers. No guard, just call-order discipline. A future contributor adding a slice-registration path that runs before EnableReadCoverage would silently switch back to materialisation, then crash because _generatedStorageReads capacity is 0 in coverage mode.
Debug.Assert on first slice register, or compute the dispatch flag once at PrepareForProcessing end and store it in a readonly bool _coverageMode field. Closes the coupling.
Medium
E. WorldState.TryGetPureReadStorage makes the interface default dead code
IWorldState.TryGetPureReadStorage defaults to (false, null) at IWorldState.cs:67. WorldState.cs:111 always returns true. No other implementer in the diff overrides it to false. So the default is effectively unreachable.
If the design intent is "some backings can't bypass the journal", we need a counterexample implementer. If it's "everyone can pure-read", remove the default and make the member non-defaulted. Current shape conveys uncertainty that isn't actually used.
F. Index field exposes a hidden type pun
StorageCell.cs:23 exposes Index as a public readonly field. For hash-constructed cells (_isHash == true), the field's type is UInt256 but its contents are ValueHash256 bytes (line 47 reinterprets). This was true pre-PR — _index had the same dual interpretation — but making it public invites consumers to treat cell.Index as a slot index without checking IsHash first. Any caller doing cell.Index.ToBigEndian(...) on a hash-constructed cell silently produces garbage.
Either keep the property accessor and provide a separate ref readonly UInt256 IndexRef helper for the in-passing optimization, or add an XML <remarks> clarifying the type-pun and pointing callers to check IsHash.
G. BalStorageValueCache.Set race with Dispose under reentrant HintBal
Set(ordinal, value) writes _values[ordinal] then Volatile.Write(_state[ordinal], Ready). Dispose replaces _values/_state with [] and returns them to pool. If a HintBal warming task is still running when ClearCaches → ReleaseReadResources → Dispose() fires, the warmer's next _values[ordinal] = value indexes into [] and the pool returns an array currently being written to.
Normal flow is safe — PreWarmCaches's returned task is awaited by BranchProcessor.WaitAndClear before ClearCaches. But IWorldState.HintBal is public, and anyone calling it outside the prewarmer is on their own. A future caller (RPC tooling proactively warming before a debug_traceCall) could create the race.
Doc-comment the lifecycle contract on IWorldState.HintBal ("returned task must be awaited before next ClearCaches"). Cheap and prevents the foot-gun.
H. Cancellation semantics diverge between BAL and non-BAL prewarm paths
BlockCachePreWarmer.cs:109 returns Task.Run(() => PreWarmCachesParallel(..., cancellationToken)) — the Task.Run-without-token pattern that the same PR adds elsewhere to prevent pre-start cancellation faulting the task. But line 102 (return _stateProvider.HintBal(bal, cancellationToken)) bypasses Task.Run entirely and returns the inner task directly. The inner task passes the token internally (linked CTS). So:
- Non-BAL path: pre-start cancel ⇒ task RanToCompletion (no fault).
- BAL path: pre-start cancel ⇒ linked source dies ⇒ task can fault before any sink read.
Should be uniform. Either pre-wrap the BAL path in Task.Run, or factor out a helper both paths use.
Lower / quality
I. RentReadCoverage allocates per slice; queue contention scales with txCount
PreBlockCaches.cs:92 does new BalReadCoverage(...) + _readCoverages.Enqueue(coverage) per slice. One new + one contended enqueue per tx per worker. For a 1000-tx block on 16 workers that's 1000 allocations and 1000 contended enqueues. Pool the wrapper (ObjectPool<BalReadCoverage>) and the queue contention can be replaced by per-worker accumulators drained at block-end. You already established the pooled-array pattern for BalReadCoverage._coverage's backing.
(Same shape as my synthesis-review #6, restated here because at chain-tip BAL sizes the cost is measurable.)
J. BlockAccessListAtIndex.ChargeableReadCount is publicly settable
BlockAccessListAtIndex.cs:37 — public long ChargeableReadCount { get; set; }. Pooled object (via IResettable), settable from outside. Reset() correctly zeroes it. But the public setter means anyone with a reference can stamp on it. Should be internal set, or assigned through a named method like CaptureChargeableReadCount(long) so the legitimate write path (TxProcessorPool capturing from _balWorldState.CurrentSliceChargeableReads) is auditable.
K. Two \ Note: lines instead of // Note: surfaced by the diff
PersistentStorageProvider.cs:527 and TrieStoreScopeProvider.cs:226 contain \ Note: … instead of // Note: …. They happen to compile because they sit on fresh lines but they're typos. Not introduced by this PR, but the PR touches both files; dotnet format won't catch them. Trivial fix while you're in there.
L. Missing test scenarios
- Two concurrent writers to
BalStorageValueCache.Setfor the same ordinal — Claude noted the writers exist and are benign-by-construction, but no test pins the post-race published value. - Block with declared reads but zero transactions (the path where
DrainAndReduceReadCoveragereturns null, hitting the throw atValidation.cs:436). - Warming task cancelled mid-flight by
WaitAndClear's token (re-hint cancellation is covered for TrieStore inScopeProviderTests, but inner-source cancellation during a single HintBal isn't). HintBalcalled twice in quick succession with overlapping tasks via the FlatWorldStateScope path (only the TrieStore re-hint is currently tested).
Priority
- A — fix FlatScope/TrieStore HintBal fault asymmetry; consensus-relevant.
- C — comment or integration-test the system-read coverage attribution.
- B — single
IsSystemContract+ spec-check vs EIP-7928. - D — close the temporal coupling in
RegisterGeneratedSlicedispatch. - E — decide whether
IWorldState.TryGetPureReadStoragehas a non-trivial default. - G — doc the
HintBallifecycle contract onIWorldState. - H — unify cancellation semantics between BAL and non-BAL prewarm paths.
- F, I, J, K, L — polish.
A, C, D are worth resolving before merge; the rest can ride a follow-up if you'd rather not expand this PR's diff.
| if (slotChanges is null) | ||
| { | ||
| MarkDeclaredReadCoverage(in storageCell); | ||
| if (TryReadDeclaredPureRead(in storageCell, out byte[]? value)) |
There was a problem hiding this comment.
For a declared read the ordinal is resolved twice here: MarkDeclaredReadCoverage already computes it via _coverageCursor, then TryReadDeclaredPureRead -> TryGetGlobalReadOrdinal redoes it from scratch (re-probes _addressToIndex, fresh cursor = -1 so it falls to BinarySearch for accounts with >16 reads). Same ordinal, and this is exactly the >32768-read block the destination targets. Could MarkDeclaredReadCoverage pass the ordinal into TryReadDeclaredPureRead? wdyt
| /// <summary>Rents a per-worker coverage sized to the block's read ordinal space and registers it for the block-end reduce.</summary> | ||
| public BalReadCoverage RentReadCoverage() | ||
| { | ||
| BalReadCoverage coverage = new(_storageReadPlan!.TotalReads); |
There was a problem hiding this comment.
Small thing: each slice rents a coverage sized to the whole block's TotalReads, and they're all kept until the block-end drain. So peak is ~txCount x full-block-width even though a slice touches few slots — on a big many-read block that can spill the ArrayPool into GC. Could each slice fold into one reduced coverage on return instead? Maybe worth a bench.
Changes
Optimizes the BAL (EIP-7928 Block Access List) storage-read path used by verify-only parallel block validation on FlatDb, targeting the existing-key
SLOADwall-clock gap.Read prefetch (warming) driven off the block access list
HintBalinBlockProcessor(kicked off at block-processing time) intoBlockCachePreWarmer, so warming off the suggested block's access list runs ahead of execution.BalReadStoragePlan) with an ordinal-keyed prefetch destination (BalStorageValueCache) onPreBlockCaches. The prefetch fills slots in on-disk key order while execution consumes them by bytecode order; a miss self-heals by falling back to the parent reader, so no frontier wait is needed.addrHash[0..4] ++ slotHash ++ addrHash[4..20]) and ordered across accounts so workers sweep a contiguous keyspace slice instead of scattering point gets.FastHash64instead ofXxHash64.Pure-read path (skip the change journal)
PersistentStorageProviderchange-registry, with a fast path for repeated same-slot reads.Verify-only validation by read coverage
BalReadCoverage) validates verify-only storage reads by coverage rather than materializing the generated read set; SIMD-vectorized OR-reduce and uncovered scan.Robustness / supporting changes
HintBalcancellation is plumbed end-to-end (caller token linked into the warming CTS) so warming stops promptly once a block's transactions have executed; the warming task is non-fatal (a background prefetch fault or a pre-start cancel can no longer fail an already-executed block).StorageCell.Indexis now areadonlyfield so it can be passed byin.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
BalReadStoragePlanTests,BalStorageValueCacheTests,BalReadCoverageTests,BlockAccessListReadCoverageValidationTests, plus additions toBlockAccessListBasedWorldStateTests,ScopeProviderTests(HintBal cancel/drain + sink), andBlockProcessorTests/BlockCachePreWarmerTests.eip7928pyspec consensus suite passes: 5514/5514 across all modes including verify-only parallel validation.Documentation
Requires documentation update
Requires explanation in Release Notes