feat(consensus): BAL apply metrics, shadow root comparison + journal-bypassing parallel bulk apply (opt-in) - #12683
feat(consensus): BAL apply metrics, shadow root comparison + journal-bypassing parallel bulk apply (opt-in)#12683LukaszRozmej wants to merge 6 commits into
Conversation
…t comparison First step of the BAL-driven parallel state root workstream: make the parallel BAL apply pipeline observable and validate the future bulk post-value applier against the canonical root, with zero behavior change on the canonical path (all additions are metric timestamps or gated behind a new hidden, default-off config flag). Metrics (Nethermind.Evm.Metrics, ExecutionMetricsFlag-gated like the existing StateRootTime sinks): - BalWarmupWaitTime — time spent joining the HintBal read warmup - BalApplyTime — BAL journal replay in ApplyStateChanges, excluding commit - BalStateRootTime — the Commit + RecalculateStateRoot tail - BalRootReadyLagTime — wall-clock from last parallel tx worker finishing to the BAL-applied root being ready (0 when the root wins), telling us whether root work or execution binds block latency - BalShadowRootMismatches — shadow/canonical root divergences Shadow mode (Blocks.ParallelBalStateRootShadow, default false, hidden): after the parallel path merges the post-execution BAL row, a new BlockAccessListManager.StateRoot partial bulk-applies the suggested BAL's final values (account fields via the new pure BalPostState helper, including the EIP-158 touched-empty => absent rule; last value per changed slot; code inserts) on a read-only env at the parent root and compares the recomputed root with the canonical one. Mismatches and failures are reported via metric + error log only — the shadow body is fully caught and can never affect consensus results. Tests: BalPostStateTests (field fallbacks, EIP-158 absence, reads-only rows), BalShadowStateRootTests (shadow/canonical equivalence over real tries for account creation, balance/storage/code changes and EIP-158 sweeps; mismatch reporting; inert with the flag off; env failures and corrupt BALs never throw). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @LukaszRozmej's task in 9m 52s —— View job Code review — PR #12683
VerdictThe design is sound and the "no behaviour change on the default path" claim holds up under reading: the only edits to canonical code are Three Medium items, all about the instrumentation rather than consensus correctness — and two of them undercut the PR's own stated goals, which is why I'd like them addressed or explicitly waived before merge. Critical: 0 · High: 0 · Medium: 3 · Low: 6 Medium
Low
Notes
Nothing here blocks the design; items 1-3 are the ones I'd want resolved (or waived with rationale) since two of them affect whether the PR delivers the measurement it exists to produce. |
| // BalRootReadyLagTime tracking: Stopwatch timestamps of "all tx workers finished" and | ||
| // "BAL apply produced the root", captured per block only when detailed metrics are on. | ||
| private long _balRemainingTxWorkers; | ||
| private long _balWorkersDrainedAt; | ||
| private long _balRootReadyAt; |
There was a problem hiding this comment.
Medium — unpadded contended counter on the parallel hot path, and it is not actually opt-in.
ExecutionMetricsFlag.IsActive is a compile-time constant (Nethermind.Evm/Metrics.cs:38-46 — true unless NO_EXEC_METRICS is defined). So the comment "captured per block only when detailed metrics are on" is not accurate: in every default release build, OnBalTxWorkerFinished() performs an unconditional Interlocked.Decrement on _balRemainingTxWorkers once per transaction, on the parallel newPayload path.
Two consequences:
-
_balRemainingTxWorkersis contended by all tx workers while_balRootReadyAtis written concurrently by the iteration-0 BAL-apply worker — the threelongs share a cache line, so the atomic RMW invalidates the line the latency-critical BAL apply thread writes to. Depending on CLR field layout they may also share a line with the_receiptsTracerPool/_gasResultPool/_txExecutionOrderreferences that every worker reads. The repo already has the idiom for this (CacheLinePaddedLong, used throughoutMetrics.cs) — please use it here, or drop the shared counter entirely: stamping a completion timestamp into the existing per-tx slot and taking the max after the loop is contention-free. -
Since the gate is compile-time, there is no way for an operator to turn this measurement off short of a custom build — worth stating explicitly, because the PR's own framing suggests it is conditional.
Also note the metric's description says "last parallel BAL validation worker finishing", but the drain point is the last tx execution worker (IncrementalValidationWorkItem completion is separate). Worth aligning the wording so dashboards are not misread.
| /// unexpected failures are logged and swallowed so the canonical pipeline is unaffected. | ||
| /// Internal for tests. | ||
| /// </remarks> | ||
| internal void RunShadowStateRootComparison(Block block) | ||
| { | ||
| if (!blocksConfig.ParallelBalStateRootShadow || !ParallelExecutionEnabled) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| ReadOnlyBlockAccessList? bal = block.BlockAccessList; | ||
| Hash256? parentStateRoot = _parentStateRoot; | ||
| Hash256? canonicalRoot = block.StateRoot; | ||
| if (bal is null || parentStateRoot is null || canonicalRoot is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (readOnlyTxProcessingEnvFactory is null) | ||
| { | ||
| if (_logger.IsDebug) _logger.Debug("BAL shadow state root skipped: no read-only tx processing env factory available."); | ||
| return; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| _shadowRootEnv ??= readOnlyTxProcessingEnvFactory.Create(); | ||
| using IReadOnlyTxProcessingScope scope = _shadowRootEnv.Build(CreateParentStateHeader(block, parentStateRoot)); | ||
| Hash256 shadowRoot = ComputeShadowStateRoot(bal, scope.WorldState, specProvider.GetSpec(block.Header)); | ||
| if (shadowRoot != canonicalRoot) | ||
| { | ||
| Evm.Metrics.IncrementBalShadowRootMismatches(); | ||
| if (_logger.IsError) _logger.Error($"BAL shadow state root mismatch for block {block.Number} ({block.Hash}): shadow {shadowRoot}, canonical {canonicalRoot}."); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { |
There was a problem hiding this comment.
Medium — the soak can pass vacuously; there is no positive signal that the shadow ever ran.
BalShadowRootMismatches is the only metric. Every path that makes the shadow a no-op is silent or near-silent:
!ParallelExecutionEnabled(line 38) — returns without a word. Enable the flag on a node whereBlocks.ParallelExecutionhappens to be off and you get "0 mismatches" forever.bal is null/parentStateRoot is null/canonicalRoot is null(line 50) — silent.readOnlyTxProcessingEnvFactory is null(line 54) —Debugonly, which is off at default log levels.- the
catch(line 68) logs atError, but that log is the only trace; nothing counts it.
The stated acceptance criterion is "0 mismatches over ≥1 week before PR2 adopts the bulk applier on the canonical path". As written, an unnoticed early-return or a per-block exception produces exactly the same observable as a clean soak — and PR2 would then ship on the strength of a measurement that never happened. Please add a BalShadowRootComparisons counter (incremented only after a root was actually computed and compared) and a BalShadowRootFailures counter in the catch, so the soak conclusion is comparisons > 0 && mismatches == 0 && failures == 0.
Related: the catch (Exception ex) { _logger.Error(...) } and the mismatch _logger.Error(...) are both unbounded — one per block. A peer feeding invalid blocks via newPayload can drive a sustained error-log stream on a shadow-enabled node. Rate-limiting or Warn for the mismatch case would be safer.
|
|
||
| CheckInitialized(); | ||
| MergeAndReturnBal(uint.MaxValue); | ||
|
|
There was a problem hiding this comment.
Medium — the shadow runs synchronously on the block-processing critical path, and it confounds the very metrics this PR adds.
SetBlockAccessList is called from BlockProcessor.ProcessBlock after ComputeStateRoot, i.e. squarely inside newPayload latency. On a shadow-enabled node each block now pays, on the processing thread:
- a full BAL replay onto a fresh read-only world state,
- a second
Commit+RecalculateStateRootover the whole changed set, - an
Account+ twoHash256allocations per changed account (ComputeShadowStateRootline 88-90 materialisesAccountStructinto a class just to callBalPostState.Compute), so thousands of short-lived allocations per mainnet-size block.
The plan is to enable this flag on the same devnet canaries that are supposed to report BalApplyTime / BalStateRootTime / BalRootReadyLagTime. Those numbers will be measured on a node whose block processing has an extra state-root computation and a fresh GC load bolted on — which is the opposite of what PR1 is for. Either run the comparison off the processing thread (the shadow env is independent of stateProvider, so this looks feasible), or document clearly that shadow-enabled nodes must not be the ones producing the timing baseline.
Ordering nit while here: RunShadowStateRootComparison is invoked before ValidateBlockAccessList, so the shadow work is also spent on blocks that are about to be rejected. Moving it after validation costs nothing and removes that.
|
|
||
| UInt256 balance = hasBalanceChange ? changes.BalanceChanges[^1].Value : parent?.Balance ?? UInt256.Zero; | ||
| ulong nonce = hasNonceChange ? changes.NonceChanges[^1].Value : parent?.Nonce ?? 0; | ||
| Hash256 codeHash = hasCodeChange ? new Hash256(changes.CodeChanges[^1].CodeHash) : parent?.CodeHash ?? Keccak.OfAnEmptyString; |
There was a problem hiding this comment.
Low — CodeChange.CodeHash is the zero hash for null code, not Keccak.OfAnEmptyString.
CodeChange.CodeHash is code is null ? default : ValueKeccak.Compute(code) (CodeChange.cs:18), and default(ValueHash256) is 0x000…0. So a code change carrying null code yields codeHash == Hash256.Zero here, which (a) never trips the EIP-158 empty check below and (b) diverges from the canonical path, where InsertCode(address, null, spec) would either throw or settle on Keccak.OfAnEmptyString. Zero-length code is fine (ValueKeccak.Compute([]) is Keccak.OfAnEmptyString); it is specifically null that is inconsistent.
A decoded BAL should never carry null code, so this is not currently reachable — but BalPostState is explicitly billed as the "unit-testable core for the future bulk applier (PR2)", where it will run on the canonical path over peer-supplied data. Worth either normalising (changes.CodeChanges[^1].Code is null ? Keccak.OfAnEmptyString : …) or stating the precondition in <remarks>.
Also, forward-looking (Low): the signature takes and returns Account (a class), and ComputeShadowStateRoot has to allocate an Account plus two Hash256 from the AccountStruct it already read just to call it. For PR2's canonical bulk applier that is an allocation per changed account per block. Consider static bool TryCompute(in AccountStruct parent, bool parentExists, ReadOnlyAccountChanges changes, IReleaseSpec spec, out AccountStruct post) now, while there are no callers to migrate — .agents/rules/coding-style.md asks for low-allocation patterns on paths like this.
EVM Opcode Benchmark DiffAggregated runs: base=3, pr=3 No significant regressions or improvements detected. |
…ity, lag tracking without contention - BalRootReadyLag: replace the shared Interlocked worker countdown (three unpadded longs on one cache line, one atomic RMW per tx on the parallel hot path) with per-tx completion timestamps — contention-free per-slot stores, drain point taken as their max after the loop joins. Fix the comment claiming the tracking is conditional (ExecutionMetricsFlag is compile-time) and the metric description naming validation workers instead of tx execution workers. - Shadow root: run off the block-processing thread (queued after BAL validation passes, gates and parent root captured synchronously) so shadow-enabled canaries don't confound the BalApplyTime/BalStateRootTime/ BalRootReadyLagTime baselines; serialize runs over the shared env. - Soak can no longer pass vacuously: BalShadowRootComparisons counts only actually-completed comparisons, BalShadowRootFailures counts swallowed exceptions; soak criterion is comparisons > 0 && mismatches == 0 && failures == 0. Mismatch/failure error logs are sampled after the first occurrences so peer-fed blocks can't flood the log. - BalPostState: normalize a null-code CodeChange (zero hash) to Keccak.OfAnEmptyString so it can't dodge the EIP-158 empty check or diverge from the canonical InsertCode path; regression-tested. - Remove unused using (code lint failure). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review feedback addressed in 71b921b:
Deferred deliberately: the 🤖 Generated with Claude Code |
…cks.ParallelBalBulkApply
PR2 of the BAL-driven parallel state root workstream. On the parallel BAL
path, iteration 0 currently replays the BAL through the journaled world-
state operations (CreateAccountIfNotExists/AddToBalance/SetNonce/InsertCode/
Set + Commit), paying per-operation journal and bookkeeping costs before
the state root can be computed. This adds WorldState.BulkApplyBal
(IBalBulkWorldState): per account, the parent is read through the scope
(warm from the BAL read warmup), BalPostState derives the post-block
account, and the result goes straight into the backend scope's
StartWriteBatch — the same bulk seam WorldState.Commit funnels into, minus
the journal. Per-account storage batches are filled and disposed in
parallel; the batch dispose reconciles storage roots into the account
leaves.
Correctness notes:
- Block-level change tracking is preserved: bulk-applied accounts (and the
batch's storage-root fixups) are recorded via
StateProvider.TrackBulkAppliedState with Before == After, so
GetAccountChanges (TxPool cache invalidation) matches the journaled path
and a later FlushToTree skips them instead of clobbering reconciled
storage roots.
- Absent post-block accounts (EIP-158 sweep) go through Set(null); their
row's slot writes are skipped, mirroring the journaled commit's sweep.
- Zero slot values become empty bytes, i.e. slot deletes, matching
StorageTree semantics.
Dispatch is opt-in: BlockAccessListManager.ApplyBlockStateChanges routes to
the bulk applier only when Blocks.ParallelBalBulkApply (default false,
hidden) is set and the backend supports it; otherwise the journaled replay
runs unchanged. To be enabled on canaries after the PR1 shadow soak.
BalBulkApplyTests: 9 parity scenarios x {flat, trie} scope providers assert
the bulk root equals the journaled root and the account-change sets match,
plus manager dispatch tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every parallel-BAL test now exercises the bulk applier by default (the journaled replay stays available via --Blocks.ParallelBalBulkApply=false as the benchmark A/B baseline). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
EXPB Benchmark ComparisonRun: View workflow run superblocksScenario: Client Processing (SSE)
K6 TTFB
realblocksScenario: Client Processing (SSE)
K6 TTFB
fusakaScenario: Client Processing (SSE)
K6 TTFB
|
The AuRa post-merge path (RewriteContracts + ApplyAuRaPreprocessingChanges) materialises system accounts on the MAIN world state before the executor runs — real uncommitted journal writes. The bulk applier bypassed them: scope-level parent reads missed the materialised accounts and a later FlushToTree would have overwritten the bulk-applied values with the stale journal entries (caught by the bulk applier's debug assert, which crashed the [checked] CI test hosts). ApplyBlockStateChanges now commits the journal before dispatching to BulkApplyBal — a no-op on the clean PoS path, and on AuRa it lands the pre-block writes in the backend first so BAL finals overwrite exactly where they overlap, matching journaled semantics. The debug assert is relaxed to check pending WRITES only (read-through Before == After traces are expected and harmless), and is now a true invariant after the pre-commit. Regression test: pre-block journaled writes (in- and out-of- BAL accounts) produce identical roots and change sets on both paths. Also drop an unused using (code lint). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Benchmark A/B — state-actor suite (
|
| side | image | stateful (454) | compute (2,040) |
|---|---|---|---|
| base | master-3181e53 (= merge-base, #12681 included) |
31048503540 | 31052363733 |
| PR | pr-12683 |
31048989298 | 31055363877 |
All 2,494 tests pass on both images. Overall medians are neutral (stateful −0.6%, compute −0.1%), but the deltas are strongly structured:
Regressions — account-creation-heavy BAL blocks (consistent, well outside noise)
| test family | suite | n | Δ MGas/s |
|---|---|---|---|
bloatnet/test_create (create2 spam + immediate access) |
stateful | 18 | −21% (every variant down; −32…−35% for 32B contracts, −9…−17% for 1KB) |
bloatnet/test_call (call_value_to_empty) |
stateful | 2 | −35% |
scenario/contract_creation |
compute | 8 | −33% |
scenario/auth_transaction (EIP-7702) |
compute | 16 | −36% |
scenario/ether_transfers (to empty) |
compute | 14 | −6% |
Cross-run calibration: the create category moves only ±1–2% between independent runs of the same image, so −21% is unambiguous. The common denominator is new/mutated account identity — contract creations, 7702 delegations, touched-empty accounts — pointing at the bulk applier's account-creation path being slower than the journaled replay it bypasses.
Wins
bloatnet/test_sstore+4.6% (n=60) — existing-slot write-heavy blocks, the bulk batch's intended case- compute
instruction+2.0% average
(eip7928/deploy_then_interact shows +20% here, but an A/A control shows that family swings ±50% run-to-run under parallel execution — not readable at n=1.)
Suggestion
The applier is a net win on storage-write-heavy blocks but pays ~20–36% on creation/authorization-heavy ones. Worth either optimizing the creation path before it ships default-on, or flipping ParallelBalBulkApply default to false until then (numbers above are with the PR's default true).
Changes
BAL-driven parallel state root workstream: observability, a zero-risk shadow mode, and the journal-bypassing bulk applier itself (opt-in). No behavior change on the canonical path with default config.
Timing metrics for the parallel-path BAL apply pipeline, following the existing
StateRootTimeSinkpattern (CacheLinePaddedLong, gated byExecutionMetricsFlag):BalWarmupWaitTime— time iteration 0 spends joining theHintBalprefetchBalApplyTime/BalStateRootTime— the journal walk vs theCommit+RecalculateStateRoottail ofApplyStateChangesBalRootReadyLagTime— wall-clock from the last tx-execution worker finishing to root ready, instrumented entirely insideParallelBlockValidationTransactionsExecutorvia contention-free per-tx completion timestamps (no shared counter on the parallel hot path). This is the number that says whether root work or execution binds newPayload latency on BAL blocks.BalPostState(Nethermind.Core): pure static helper computing an account's post-block state from parent account +ReadOnlyAccountChanges— last-change-wins per field with parent fallback, EIP-158 touched-totally-empty ⇒ absent. The unit-testable core the bulk applier below builds on.Shadow-mode root comparison behind
Blocks.ParallelBalStateRootShadow(defaultfalse, hidden from docs): when enabled on the parallel BAL path, an independent read-only env at the parent state applies the BAL post-values (accounts viaBalPostState, last-value slot writes, code inserts), computes the state root, and compares with the canonical result. The comparison is queued off the block-processing thread (and only after BAL validation passes), so shadow-enabled canaries don't pollute the timing baselines above.BalShadowRootComparisonscounts completed comparisons — the soak's positive signal; mismatch ⇒BalShadowRootMismatches, swallowed failures ⇒BalShadowRootFailures; the error logs are sampled after the first occurrences so peer-fed blocks can't flood them. The canonical result is never affected.Journal-bypassing parallel BAL bulk apply behind
Blocks.ParallelBalBulkApply(defaulttrueon this branch so benchmarks and every parallel-BAL test exercise it; hidden from docs) — the optimization the metrics above measure. Iteration 0 currently replays the BAL through the journaled world-state operations (CreateAccountIfNotExists/AddToBalance/SetNonce/InsertCode/Set+Commit), paying per-operation journal and bookkeeping costs before the root can be computed. With the flag on,WorldState.BulkApplyBal(newIBalBulkWorldStateseam) instead reads each parent account through the scope (warm from the BAL read warmup), derives the post-block account viaBalPostState, and writes straight into the backend scope'sStartWriteBatch— the same bulk seamWorldState.Commitfunnels into, minus the journal — with per-account storage batches filled and disposed in parallel (storage-root computation happens inside each dispose; the outer batch reconciles the roots into the account leaves). Correctness notes:StateProvider.TrackBulkAppliedStatewithBefore == After, soGetAccountChanges(TxPool cache invalidation) matches the journaled path and a laterFlushToTreeskips them instead of clobbering reconciled storage roots.Set(null); their row's slot writes are skipped, mirroring the journaled commit's sweep. Zero slot values become empty bytes, i.e. slot deletes, matchingStorageTreesemantics.BlockAccessListManager.ApplyBlockStateChanges(interface default = journaled replay);--Blocks.ParallelBalBulkApply=falserestores byte-for-byte today's path and is the benchmark A/B baseline.Purpose: quantify where block latency goes on BAL blocks (warmup wait / apply / root tail), de-risk the bulk applier by soaking the BAL-derived root reconstruction on devnets with zero consensus exposure, and let benchmarks A/B the applier itself — bulk is the branch default,
--Blocks.ParallelBalBulkApply=falseis the journaled baseline (pair with--Blocks.ParallelBalStateRootShadow=trueso the shadow's journaled replay cross-checks the bulk root block by block).Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
BalPostStateTests(11): creation, parent fallbacks, last-change-wins, EIP-158 on/off, code change with unchanged nonce, null-code normalization, reads-only rows.BalBulkApplyTests(20): 9 parity scenarios × {flat, trie} scope providers assert the bulk-applied root equals the journaled root and the account-change sets match (created account, balance/nonce-only, storage overwrite/zero-out/create, code deploy, EIP-158 sweep with and without storage, storage-only change, multi-account mix, reads-only row), plus manager dispatch tests.BalShadowStateRootTests(9): shadow/canonical root equivalence over real tries (shared TrieStore mirroring production) for account creation, balance-only, storage overwrite/zero-out/create, code deploy, EIP-158 sweep; mismatch reported without throwing; flag-off fully inert (env never created); throwing env factory and corrupt BAL never crash. A capturing logger plus theBalShadowRootComparisons/BalShadowRootFailuresdeltas assert the shadow actually ran, guarding against vacuous passes.BlockAccessList*+BlockProcessorTests79/79,Eip7928Tests204/204,StandardConfigTests+ metrics-description tests,Nethermind.Consensus.TestProcessingStats/SlowBlock 17/17 — all green.BalShadowRootComparisons > 0with 0 mismatches and 0 failures over ≥1 week — withParallelBalBulkApplyalso enabled, the shadow's journaled replay cross-checks the bulk applier block by block — before the bulk applier's default is flipped on the canonical path.Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
Part of the BAL benchmark performance workstream (#12681, #12682). The bulk applier ships here behind
Blocks.ParallelBalBulkApply(default on for this branch's benchmark/soak phase — revisit the default before merge if a longer soak is wanted); an optional persist-tail overlap PR will follow, gated on the benchmark, metrics, and shadow-soak results from this PR.🤖 Generated with Claude Code