feat(processing): parallel-safe gas-price aggregation, base-fee fallback#11542
feat(processing): parallel-safe gas-price aggregation, base-fee fallback#11542
Conversation
…ack, exec-mode emoji - Parallel BAL workers defer inline gas-price folds via Metrics.SuppressInlineGasPriceUpdate; the executor recomputes serially in AggregateBlockGasPriceMetrics after workers join, giving race-free min/max and deterministic avg/median. - SeedBlockGasPriceIfEmpty seeds all four aggregates with block.Header.BaseFeePerGas when no user tx contributed (empty / system-only blocks), so the "Processed" log line and Prometheus gauges show a meaningful value rather than blanking. - ProcessingStats auto-steps the gas-price unit GWei -> MWei -> KWei -> Wei when min would render 0.000 at :N3 precision, keeping low-base-fee values visible (e.g. 7 wei renders as "Gas KWei: 0.007" instead of "Gas GWei: 0.000"). - Append execution-mode indicator to the "Processed" log line: chains for parallel BAL, link for sequential. Emoji at end-of-line so cell-width variation across terminals cannot drift the mid-line pipes against the Blocks/Throughput rows below. - UpdateBlockGasPrice and SeedBlockGasPriceIfEmpty use IsUint64 / .u0 fast path to avoid the multi-limb UInt256 -> double conversion in the hot tx-execution path; single cvtsi2sd instead of an out-of-line helper call.
|
Claude finished @benaadams's task in 6m 38s —— View job PR Review: feat(processing): parallel-safe gas-price aggregation, base-fee fallback
SummaryThe design is sound. The race-free aggregation strategy (suppress inline updates via Medium
Before this PR, an empty block showed nothing (blank). The "meaningful value" goal of the PR is defeated for zero-baseFee blocks. A simple guard would fix it: internal static void SeedBlockGasPriceIfEmpty(in UInt256 baseFee)
{
if (BlockMinGasPrice != float.MaxValue) return;
if (!baseFee.IsUint64 || baseFee.IsZero) return; // ← add IsZero guard
...
}Low
// before
public static bool LastBlockUsedParallelExecution { get; private set; }
// after
public static volatile bool LastBlockUsedParallelExecution; // or use Volatile.Read/Write at call sites
Correctness confirmations
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…xec-mode flag - SeedBlockGasPriceIfEmpty: skip seeding when baseFee is zero (pre-EIP-1559 chains, some rollups, genesis). Without this guard, empty zero-baseFee blocks rendered "Gas Wei: 0.000 .. 0.000 (0.000) .. 0.000" in the "Processed" log line, defeating the PR's goal of showing a meaningful value vs the prior blank output. - LastBlockUsedParallelExecution: change auto-property to a volatile field. Written on the block-processing thread and read from ProcessingStats; cross-thread monitoring readers (Prometheus scrapers, external dashboards) need a memory barrier to avoid the JIT caching the field in a register.
|
@claude re-review |
|
Claude finished @benaadams's task in 3m 4s —— View job PR Re-Review: feat(processing): parallel-safe gas-price aggregation, base-fee fallback
SummaryBoth issues raised in the prior review have been addressed. The PR is ready to merge. Prior findings: resolved
Fresh review: no new findingsVerified end-to-end correctness:
|
|
Claude finished @benaadams's task in 3m 49s —— View job PR Re-Review: feat(processing): parallel-safe gas-price aggregation, base-fee fallback
SummaryAll findings from previous reviews are resolved or acknowledged. Fresh pass found no new Critical, High, or Medium issues. The PR is ready to merge. Prior findings: all resolved or acknowledged
Fresh review: no new issuesThread safety
Correctness
Performance
Gas-unit auto-scaling thresholds |
There was a problem hiding this comment.
Pull request overview
This PR adjusts block-processing telemetry to make per-block gas-price metrics deterministic under parallel Block Access List (BAL) execution, and improves reporting output for empty/system-only blocks and very low gas prices.
Changes:
- Suppress inline gas-price aggregation during parallel tx execution and recompute gas-price aggregates serially after workers join.
- Seed per-block gas-price aggregates from
BaseFeePerGaswhen no user txs contributed (to avoid blank metrics/log output). - Improve processing log output: adaptive gas-price units and an execution-mode indicator (parallel vs sequential).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs |
Routes per-tx metric updates through a suppression-aware helper (for parallel execution). |
src/Nethermind/Nethermind.Evm/Metrics.cs |
Adds thread-static suppression flag, fast-path gas-price folding, and empty-block base-fee seeding. |
src/Nethermind/Nethermind.Consensus/Processing/ProcessingStats.cs |
Updates “Processed” logging: adaptive gas units and an execution-mode marker. |
src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.ParallelBlockValidationTransactionsExecutor.cs |
Adds serial post-pass aggregation for gas-price metrics in the parallel path and exposes last execution mode. |
src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.BlockValidationTransactionsExecutor.cs |
Seeds gas-price metrics from base fee when no user tx updated the aggregates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| internal static void UpdateBlockGasPrice(in UInt256 effectiveGasPrice) | ||
| { | ||
| if (!effectiveGasPrice.IsUint64) return; | ||
| float gasPrice = (float)(effectiveGasPrice.u0 / 1_000_000_000.0); | ||
| BlockMinGasPrice = Math.Min(gasPrice, BlockMinGasPrice); | ||
| BlockMaxGasPrice = Math.Max(gasPrice, BlockMaxGasPrice); | ||
| BlockAveGasPrice = (BlockAveGasPrice * BlockTransactions + gasPrice) / (BlockTransactions + 1); | ||
| BlockEstMedianGasPrice += BlockAveGasPrice * 0.01f * float.Sign(gasPrice - BlockEstMedianGasPrice); | ||
| BlockTransactions++; |
There was a problem hiding this comment.
= 18.4 ETH per gas unit is really not valid for these metrics
That would mean an on target (gas price maintaining) block of 30M (for 60M max) would use 553402322 ETH which is x5 larger than the entire supply of ETH
Was appended to the end of the Processed row; now appended after the ops count on the Block throughput row (the lowest of the per-block rows). Same alignment rationale (variable-width emoji at end-of-line so it cannot drift mid-line pipes), but now the rows immediately above it (Block / Block throughput) cannot be drifted, instead of the rows below the Processed row. Reads as ", ... | ops 1,516,057 \U0001f517" or "... ⛓️".
…e, tests PR #11542 review feedback: - LastBlockUsedParallelExecution was public static volatile and shared across all BlockProcessor instances; concurrent processing pipelines (tests, multi-context hosts) would race on it. Switched to ThreadStatic with a private setter via property accessor. ProcessingStats reads on the same thread that writes (the block processing thread), so no read semantics regress; the only consumer was the log line, no Prometheus scraper read this field. - AggregateBlockGasPriceMetrics: added a cost note explaining why a serial post-pass (rather than per-worker thread-local accumulators merged after join) is the right tradeoff. avg/median don't compose losslessly across partitions, and the recompute cost is bounded: ~150 txs/block * (one Min/Add UInt256 op + a few float ops) ~ <20us vs ~100ms block work. - New tests in BlockProcessorTests covering the new paths: * Parallel_validation_aggregates_block_gas_price_metrics_after_processing - asserts Metrics.GetBlockGasPrices() is populated and LastBlockUsedParallelExecution is true after a parallel run. * Parallel_validation_seeds_block_gas_price_with_baseFee_for_empty_block - empty block + non-zero baseFee seeds aggregates to baseFee gwei. * Parallel_validation_does_not_seed_block_gas_price_for_zero_baseFee_empty_block - zero baseFee + empty block leaves aggregates uninitialized. All three tests are [NonParallelizable] because the fixture is Parallelizable(ParallelScope.All) and Metrics is static state.
|
@claude re-review |
|
Claude finished @benaadams's task in 4m 9s —— View job PR Re-Review: feat(processing): parallel-safe gas-price aggregation, base-fee fallback
SummaryOne new Medium finding from the latest commit (c904158). All earlier issues are resolved or acknowledged. Prior findings: all resolved or acknowledged
Medium —
|
| Concern | Verdict |
|---|---|
SuppressInlineGasPriceUpdate is [ThreadStatic] — each parallel worker has independent state |
✓ |
finally restores SuppressInlineGasPriceUpdate on the block-processing thread after ParallelUnbalancedWork.For |
✓ |
AggregateBlockGasPriceMetrics runs serially after all workers join, before ProcessTransactions returns |
✓ |
AggregateBlockGasPriceMetrics skips tx.IsSystem() txs, matching TransactionProcessor.UpdateMetrics |
✓ |
ResetBlockStats called exactly once per block across all three dispatch paths |
✓ |
opts is Commit or None or BuildUp pattern was already the existing behavior on master (unchanged by this PR) |
✓ |
Gas-unit auto-scaling thresholds 0.0005 / 5e-7 / 5e-10 prevent 0.000 at :N3 |
✓ |
IsZero guard in SeedBlockGasPriceIfEmpty prevents zeroed-out metrics on pre-EIP-1559 empty blocks |
✓ |
IsUint64 / .u0 fast path avoids multi-limb UInt256→double in hot tx-execution path |
✓ |
Three new [NonParallelizable] tests cover parallel aggregation, baseFee seeding, and zero-baseFee skip |
✓ |
…pool report The previous fix made LastBlockUsedParallelExecution [ThreadStatic] to avoid cross-instance races, but ProcessingStats.GenerateReport runs on a thread-pool thread (queued via UnsafeQueueUserWorkItem in CaptureReportData). The ThreadStatic read on that thread always returns the default false, so the emoji would permanently show sequential. Snapshot the flag in UpdateStats (block-processing thread, same thread the executor wrote on) into BlockData.UsedParallelExecution; GenerateReport reads from BlockData. Also clear the field in BlockDataPolicy.Return so pooled BlockData instances don't carry stale state across blocks. Caught by claude[bot] in PR #11542 review.
|
@claude re-review |
|
Claude finished @benaadams's task in 2m 17s —— View job PR Re-Review: feat(processing): parallel-safe gas-price aggregation, base-fee fallback
SummaryAll findings from previous reviews are resolved or acknowledged. The b72f3d0 fix correctly addresses the Medium issue from the last review. The PR is ready to merge. Prior findings: all resolved or acknowledged
Fix verification (b72f3d0)The
Fresh review: no new issues
|
Changes
... | ops 1,516,057 🔗(or ⛓️). Emoji at end-of-line of the lowest per-block row so cell-width variation across terminals cannot drift the mid-line pipes on the Block / Block-throughput rows above. The exec-mode flag is[ThreadStatic]so concurrent processing pipelines (tests, multi-context hosts) do not race on the same global; ProcessingStats reads on the same thread the executor wrote on.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Three new tests in
Nethermind.Blockchain.Test/BlockProcessorTests.cs(each[NonParallelizable]since the fixture isParallelizable(ParallelScope.All)and Metrics is static state):Parallel_validation_aggregates_block_gas_price_metrics_after_processing- assertsMetrics.GetBlockGasPrices()is populated andLastBlockUsedParallelExecutionis true after a parallel run with EIP-1559 txs at varied gas prices.Parallel_validation_seeds_block_gas_price_with_baseFee_for_empty_block- empty block + 2 gwei baseFee seeds aggregates to 2.0.Parallel_validation_does_not_seed_block_gas_price_for_zero_baseFee_empty_block- zero baseFee + empty block leaves aggregates uninitialized (GetBlockGasPrices()returns null).Nethermind.Blockchain.Test/BlockProcessorTests: 43/43 pass.