Claude finished @Marchhill's task in 3m 35s —— View job

PR Review: consensus: standardize slow block JSON output for cross-client metrics

Tasks:

• Post initial comment
• Read rule files and gather context
• Read changed files and assess current state
• Evaluate remaining blockers from Round 5 (LukaszRozmej's items)
• Check for new issues
• Finalize review summary

Summary

All five blockers from LukaszRozmej's 2026-05-15 review have been addressed in the latest commit (c8e6afa). No new Medium or High issues found. This PR is ready to merge.

LukaszRozmej Blockers — All Resolved

Also fixed from Round 5 (Low):

• ProcessingStats ctor default discrepancy — lower-level ctor now defaults to slowBlockThresholdMs = -1 (disabled) with an explanatory <remarks>, consistent with production defaults.

All Prior Findings — Confirmed Fixed

Outstanding Low Items (acknowledged, non-blocking)

• PerTxTimingCollector statics not volatile — _enabled, _ticksPerTx, and _count are plain static fields. Prepare() sets _ticksPerTx on the block-processing thread; parallel workers read it in Record() without a formal barrier. Safe on x86/.NET (TSO hardware + ThreadPool dispatch guarantees ordering). Class doc now explicitly states the single-block assumption.
• Thread.Sleep(50) polling in ProcessingStatsTests.cs:46–50 — polling up to 5s for each assertion; fragile under load. A ManualResetEventSlim would be deterministic but is a test-quality concern only.
• Missing parallel path test — no coverage of ProcessTransactionsParallel for the per-tx timing collector; the static-per-block assumption is not tested under concurrent workers.
• MetricsIntegrationTests uses combined Metrics.AccountReads (not MainThread*) — susceptible to counter contamination from parallel test runs; Is.GreaterThanOrEqualTo softens this but weakens the spec.
• Test fixture duplication — MetricsIntegrationTests and SlowBlockIntegrationTests share ~30 lines of setup boilerplate; per AGENTS.md this should be factored into a shared helper.

Positive Notes

All structural quality from prior reviews remains fully intact:

• ExecutionMetricsFlag + MetricsTimer<TSink> pattern: JIT-foldable, clean zero-overhead compile path.
• ArrayPoolList<long> for per-tx ticks with ownership transfer and pool return in BlockDataPolicy.Return.
• ArrayBufferWriter<byte> + Utf8JsonWriter zero-allocation JSON path.
• Object pool with proper reference nulling in Return.
• Deltas pre-computed on block-processing thread before UnsafeQueueUserWorkItem dispatch.
• StopTxTimer in finally — timing captured even for failing transactions.
• Default interface methods for timing hooks — no-op inheritance without stub boilerplate.

| Branch

Review — focused on correctness and performance

Verdict

Feature is well-designed and test coverage is excellent. However, the PR description's claim that "there is zero overhead on block processing when disabled" is not quite accurate. The new EVM-side metric counters fire unconditionally in default builds (gated only by compile-time ExecutionMetricsFlag, not by SlowBlockThresholdMs). On hot paths (SSTORE, SLOAD, account reads/writes), this adds one atomic Interlocked op per state operation, always on, even when the feature is configured to -1.

Performance concerns

1. Hot-path atomics added regardless of SlowBlockThresholdMs (HIGH)

ExecutionMetricsFlag.IsActive is a compile-time-folded constant, true by default. Only a recompile with NETHERMIND_NO_EXECUTION_METRICS disables it. So in production builds, even with SlowBlockThresholdMs = -1:

• PersistentStorageProvider.Set → IncrementStorageWrites() on every SSTORE that mutates
• EvmInstructions.Storage.cs (both InstructionSStoreMetered and InstructionSStoreUnmetered) → IncrementStorageDeleted() on every SSTORE-to-zero
• PersistentStorageProvider.LoadFromTree → IncrementStorageReads() on every uncached SLOAD
• StateProvider.GetState (via WarmUp) → IncrementAccountReads() on every uncached account touch
• StateProvider.SetState → IncrementAccountWrites() (+ IncrementAccountDeleted() when null) on every state mutation
• CodeInfoRepository.GetCodeInfo → IncrementCodeReads() + IncrementCodeBytesRead() on every code DB fetch

Each is Interlocked.Increment(ref IsBlockProcessingThread ? ref _main... : ref _other...). On x64 that's ~10–15 ns per call (uncontested atomic) plus a thread-static read. For the example block in the PR description (~1800 SSTOREs, ~3200 SLOADs, ~2000 account changes), that's ~70–100 µs/block of overhead even when the feature is "disabled" by config. The PR description quotes ~31 µs/block for slow-block capture; the real per-block tax in default builds is larger and unavoidable without recompiling.

Suggestion: Gate Increment* calls behind a runtime flag (e.g. _blocksConfig.SlowBlockThresholdMs >= 0 exposed as a volatile static), or set ExecutionMetricsFlag.IsActive from config at startup. If you keep the compile-time flag, please correct the PR description.

2. Per-tx try/finally is added unconditionally

BlockProcessor.BlockValidationTransactionsExecutor.ProcessTransaction now wraps transactionProcessor.ProcessTransaction(...) in try/finally. Even with _enableTxTimingMetrics = false, the JIT still emits the exception-filter table. Tiny per-tx cost but always on.

In the parallel executor, inner.StartTxTimer() / inner.StopTxTimer() are interface calls through IBlockProcessor.IBlockTransactionsExecutor (default interface methods). The JIT likely cannot devirtualize since inner is interface-typed at the call site — 2 vcalls per tx in the parallel hot path.

3. LogSlowBlock recomputes blob count

Transaction.GetBlobCount() is summed both in UpdateStats (existing) and again in LogSlowBlock (new). Cheap, but redundant — store the blob count on BlockData and reuse.

Correctness concerns

1. PerTxTimingCollector is process-wide static

_ticksPerTx is a static field, written on the block-processing thread (Prepare), read/written on parallel ThreadPool workers (Record). It is not volatile. Workers see writes only because ParallelUnbalancedWork.For provides the implicit synchronisation barrier. This works today but is fragile — please document the dependency explicitly. Same applies to BlockValidationTransactionsExecutor._enableTxTimingMetrics.

2. PerTxTimingCollector.SetEnabled(true) is sticky

CaptureStartStats calls SetEnabled(true) per block when _slowBlockPerTxThresholdMs >= 0, but there is no corresponding SetEnabled(false). If the config is hot-reloaded or in a multi-ProcessingStats scenario, the static stays true. The Prepare/Snapshot flow tolerates this, but the API is asymmetric.

3. Stopwatch ticks vs TimeSpan ticks

MetricsTimer<T>.Dispose uses Stopwatch.GetElapsedTime(_start).Ticks which returns TimeSpan ticks (100 ns) — dividing by TimeSpan.TicksPerMillisecond later is correct. Worth a doc comment on IncrementStateHashTime(long ticks) clarifying the unit is TimeSpan ticks, not Stopwatch ticks, to avoid future regressions.

4. Negative-time fallback hides a real bug

double executionMs = totalMs - stateHashMs - commitMs;
if (executionMs < 0) executionMs = totalMs;

If this branch is ever taken, the log entry silently lies (claims execution ≥ total). At minimum, debug-log when clamping so the bug surfaces.

5. BlockData pool — new Delta* fields not reset on Return

BlockDataPolicy.Return resets PerTxTicks and the existing fields but not the new DeltaAccountReads, DeltaStorageReads, etc. Safe only because UpdateStats either writes all of them (threshold ≥ 0) or never reads them (threshold < 0). If the threshold ever changes at runtime, stale deltas leak. Belt-and-braces fix: reset them too.

6. Cross-client metric semantics under parallel execution

The LogSlowBlock <remarks> correctly notes that under ParallelExecution the EVM and state-read/write counters sum across workers while wall-clock timings remain per-block. Good documentation, but this is a real caveat for "cross-client" comparability — Geth/Reth don't run txs in parallel by default. Consumers will see inflated counter values relative to wall-clock ms. Worth surfacing in the JSON itself (e.g. parallel_execution: true) so analysers can normalise.

Minor

• private bool _enableTxTimingMetrics = false; — = false is redundant.
• Double blank line after StopTxTimer.
• IBlockProcessor.IBlockTransactionsExecutor gains 3 new default interface methods — public-API change and a small ABI risk for plugins implementing the interface.
• MetricsTimer<T> as a parameterless readonly ref struct is nice — stack-only, inlinable, scope-based timing.

Suggested fixes before merge

1. Either gate hot-path Increment* by a runtime flag tied to SlowBlockThresholdMs >= 0, or correct the "zero overhead when disabled" claim in the PR body and docs.
2. Document the synchronisation dependency of PerTxTimingCollector / _enableTxTimingMetrics on ParallelUnbalancedWork.For.
3. Reset the new Delta* fields in BlockDataPolicy.Return.
4. Add a parallel_execution boolean to the JSON so cross-client analyses can normalise counter values.
5. Tiny: store blob count in BlockData; remove = false init; debug-log the executionMs < 0 fallback.

@claude review

Claude finished @Marchhill's task in 5m 50s —— View job

PR Review: consensus: standardize slow block JSON output for cross-client metrics

Tasks:

• Post initial comment
• Read rule files and gather context
• Read changed files and assess current state
• Evaluate LukaszRozmej's blocker items (from 2026-05-20 review)
• Check for new issues
• Finalize review summary

Summary

This is round 7. LukaszRozmej posted a review at 2026-05-20 14:15 UTC with 5 "Suggested fixes before merge." Marchhill committed two fixes today (56776b6 at 11:46 and ebd66dd at 16:30, the latter post-review) that address all 5 blockers. No new Critical or High issues found.

LukaszRozmej's Blockers (from 2026-05-20) — All Resolved

Additional items addressed this round

• _slowBlockThresholdMs doc comment now says "Defaults to -1 (disabled)" instead of the previously inaccurate "default: 1000ms" (56776b6).
• TimeSpan-ticks unit clarified with block comment in Metrics.cs (lines 310–313) explaining that these are 100 ns TimeSpan ticks, not raw Stopwatch timestamp ticks.
• CodeReads and StorageReads semantics documented: both count only DB-backed reads (cache misses), and the inline comments now say so explicitly (56776b6).

All Prior Findings — Confirmed Fixed

Remaining Low Items (non-blocking, acknowledged)

• Test fixture duplication — MetricsIntegrationTests and SlowBlockIntegrationTests share ~30 lines of fixture setup (per AGENTS.md, this should be factored into a helper). Low impact since these are test files only.
• Thread.Sleep(50) polling in tests — Fragile under load; a ManualResetEventSlim would be deterministic. Non-blocking test quality concern.
• Missing parallel path test — No test covers ProcessTransactionsParallel for per-tx timing; the static-collector design and join-barrier contract are not tested under actual concurrent workers.
• Exception logging drops stack trace — LogSlowBlock catch block logs only ex.Message at Debug level (ProcessingStats.cs:760). Acceptable as a "don't fail block processing" guard, but a regressions from a serialization bug would be harder to diagnose.

Positive Notes (all still valid)

• ExecutionMetricsFlag + MetricsTimer<TSink> pattern: JIT-foldable; overhead truly zero when built with NETHERMIND_NO_EXECUTION_METRICS.
• ArrayPoolList<long> for per-tx ticks with ownership transfer and pool return in BlockDataPolicy.Return.
• ArrayBufferWriter<byte> + Utf8JsonWriter zero-allocation JSON path.
• Object pool with all field resets in Return (including all new Delta* fields).
• Deltas pre-computed on block-processing thread before UnsafeQueueUserWorkItem dispatch.
• StopTxTimer in finally: timing captured even for failing transactions.
• Default interface methods for timing hooks: no stub boilerplate on implementors.

| Branch

@claude re-review

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

PR Re-Review (Round 8): consensus: standardize slow block JSON output for cross-client metrics

Tasks:

• Post initial comment
• Read rule files and gather context
• Read changed files and assess current state
• Verify all prior findings are addressed
• Analyze a03e6d1 (benaadams' new commit: "Remove duplicate execution read metrics")
• Finalize review summary

Summary

All High/Medium findings from Rounds 1–7 remain confirmed fixed. The latest commit (a03e6d1b) by benaadams removes dedicated AccountReads, StorageReads, and CodeReads counter increments from Evm.Metrics and derives state_reads.* from existing cache statistics instead — a good performance improvement. This commit introduces one new Medium concern (PR description documentation now out of sync with implementation) and two Low items.

New Findings from a03e6d1b

Medium — PR description example is stale after read-counter removal

a03e6d1b made state_reads.* entirely derived from cache.* values:

// ProcessingStats.cs — post-commit
blockData.DeltaAccountReads = blockData.DeltaAccountCacheMisses;          // always == cache.account.misses
blockData.DeltaStorageReads = blockData.DeltaStorageCacheHits + blockData.DeltaStorageCacheMisses;
blockData.DeltaCodeReads    = blockData.DeltaCodeCacheMisses;              // always == cache.code.misses

The PR description example still shows:

"state_reads": { "accounts": 1842, ... },
"cache": { "account": { "hits": 1520, "misses": 322, ... } }

With the current implementation state_reads.accounts will always equal cache.account.misses (322 in that example, not 1842). The example implies state_reads.accounts = hits + misses = total logical reads, but the implementation outputs only DB-backed reads (misses). Since this PR's description functions as the cross-client specification, the example should be updated to reflect actual semantics — or state_reads.accounts / state_reads.code should be removed (they're now redundant with cache.account.misses / cache.code.misses).

Low — Asymmetric semantics across state_reads.* sub-fields

After the change:

• state_reads.accounts = account trie DB cache misses (reads that go to the trie/DB)
• state_reads.code = code DB cache misses
• state_reads.storage_slots = all storage trie node accesses (hits + misses)

The three fields use different counting methods. The PR description doesn't document this asymmetry and consumers will likely expect consistent semantics across all three.

Low — MetricsIntegrationTests uses the combined counter after read-counter removal

The test was updated in a03e6d1b to use DbMetrics.StateTreeReads (= _main + _other) instead of the now-removed Metrics.MainThreadAccountReads:

// MetricsIntegrationTests.cs:80
long startReads = DbMetrics.StateTreeReads;  // combined, not MainThread*
...
Assert.That(DbMetrics.StateTreeReads - startReads, Is.GreaterThanOrEqualTo(2));

The test sets ProcessingThread.IsBlockProcessingThread = true, so the increments land in _main*. But another parallel test's prewarmer/non-main-thread activity could increment _other*, making the combined delta spuriously large. DbMetrics.MainThreadStateTreeReads would give a tighter, isolated delta.

Prior Findings — All Confirmed Fixed

Acknowledged Low (non-blocking, unchanged)

• PerTxTimingCollector statics not volatile — class <remarks> explicitly documents the join-barrier dependency on ParallelUnbalancedWork.For; safe on x86/.NET.
• Missing parallel path test — no coverage of ProcessTransactionsParallel for per-tx timing; single-block assumption is undocumented under actual concurrent workers.
• Test fixture duplication — MetricsIntegrationTests / SlowBlockIntegrationTests share ~30 lines of setup boilerplate; AGENTS.md recommends factoring this out.

Positive Notes

Everything praised in prior rounds still holds. The a03e6d1b commit is a net improvement: removing three hot-path Interlocked.Increment calls per state access (two for account reads, one for storage reads) reduces per-block overhead with no loss of accuracy, since the DB-level Db.Metrics trie counters already track this information. WaitableTestLogger with Monitor.Wait/PulseAll (replacing the old Thread.Sleep(50) polling) is a clean deterministic test fix.

| Branch

@claude re-review

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

PR Re-Review (Round 9): consensus: standardize slow block JSON output for cross-client metrics

Tasks:

• Post initial comment
• Read rule files and gather context
• Read changed files and assess current state vs Round 8
• Evaluate new commits (68d6f79, 43a0b97, 2c66f53)
• Check for new issues
• Finalize review summary

Summary

Three new commits since Round 8. All prior High/Medium findings remain fixed. The new commits are improvements; no new High or Medium issues found. This PR is ready to merge.

New Commits — Analysis

2c66f538 — Use main-thread DB metrics in EVM metric tests ✅

Fixes the Round 8 Low finding. MetricsIntegrationTests now uses DbMetrics.MainThreadStateTreeReads and DbMetrics.MainThreadStorageTreeReads + MainThreadStorageTreeCache instead of combined counters, eliminating susceptibility to parallel-test-run contamination. Also adds InternalsVisibleTo("Nethermind.Evm.Test") to Nethermind.Db.

43a0b97c — Restore cached contracts slow block metric ✅

Restores evm.cached_contracts_used in the slow-block JSON. Both evm.cached_contracts_used and cache.code.hits still use the MainThreadCodeDbCache delta, so they remain equal at runtime. The PR description's "EVM field definitions" table explicitly documents this equality ("retained because it is part of the original cross-client metrics spec"), which is the right place for that note.

Minor: The JSON example in the PR body shows cached_contracts_used: 97 and cache.code.hits: 180 — these should be equal but differ in the example. The text in the table explains they're always equal, so consumers who read that table will understand, but the inconsistent example could confuse cross-client implementors who parse the example first.

68d6f79e — Use thread-static block processing attribution ✅

Key improvement. Switches ProcessingThread.IsBlockProcessingThread from AsyncLocal<bool> to [ThreadStatic], with a new test PreWarmCaches_DoesNotFlowIsBlockProcessingThread_IntoTask that validates the critical invariant.

Correctness: With AsyncLocal, tasks spawned by the prewarmer (via BlockCachePreWarmer.PreWarmCaches) inherited IsBlockProcessingThread = true from the calling thread's ExecutionContext. That caused prewarmer EVM metric increments to land in _main* counters — the root cause of the prewarmer contamination that previous rounds worked around with MainThread* accessor additions. With [ThreadStatic], the flag is not inherited by new tasks or threads: prewarmer tasks correctly see IsBlockProcessingThread = false and route their increments to _other*. The new test pins this behavior.

Performance: [ThreadStatic] reads are ~1–2 ns vs ~5–20 ns for AsyncLocal, benefiting all existing and new Increment* calls across every hot-path state operation.

BlockchainProcessor safety: The loop correctly saves/restores the flag in each iteration (bool previousMainThread = IsBlockProcessingThread; ... finally { IsBlockProcessingThread = previousMainThread; }). After await _blockQueue.Reader.WaitToReadAsync, the continuation may run on a fresh thread with IsBlockProcessingThread = false; the code immediately sets it to IsMainProcessor before any processing — no stale state window.

Round 8 Findings — Status

All Prior Findings — All Confirmed Fixed

Remaining Low Items (non-blocking, acknowledged)

• evm.cached_contracts_used example inconsistency — the JSON example in the PR body shows cached_contracts_used: 97 and cache.code.hits: 180, but at runtime they are always equal (same MainThreadCodeDbCache delta). The "EVM field definitions" table documents the equivalence; consider updating the example to show equal values.
• PerTxTimingCollector statics not volatile — documented in class <remarks> as relying on the ParallelUnbalancedWork.For join-barrier; safe on x86/.NET.
• Missing parallel path test — no test covers ProcessTransactionsParallel for per-tx timing under actual concurrent workers.
• Test fixture duplication — MetricsIntegrationTests / SlowBlockIntegrationTests share ~30 lines of setup boilerplate.

Positive Notes

All structural quality from prior reviews still holds, and the [ThreadStatic] change in 68d6f79e makes the whole metrics attribution model materially cleaner: the MainThread* accessors now truly reflect main-thread-only work at the source, not just at the query boundary.
| Branch