feat(builder): record build-loop state-read latency - #5025
Conversation
🟡 Heimdall Review Status
|
|
✅ All benchmarks green — 14 within ±2% (deterministic instruction counts). View run Benchmark details (14)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| predicate_eval_hard_cutoff: self.predicate_eval_hard_cutoff, | ||
| resource_metering: self.resource_metering, | ||
| rejection_cache: self.rejection_cache, | ||
| state_provider_metrics: false, |
There was a problem hiding this comment.
Low: state_provider_metrics not resolved from node config on this path.
The multiplex service builder reads ctx.config().engine.state_provider_metrics to propagate the flag, but this standalone build_payload_builder path hardcodes false. If someone uses BasePayloadBuilder without the multiplex layer, --engine.state-provider-metrics will have no effect on the builder.
Consider resolving from ctx here too:
| state_provider_metrics: false, | |
| state_provider_metrics: ctx.config().engine.state_provider_metrics, |
| reth-eth-wire-types = { git = "https://github.com/base/reth", tag = "base-v2.5.2.3" } | ||
| reth-payload-builder = { git = "https://github.com/base/reth", tag = "base-v2.5.2.3" } | ||
| reth-execution-types = { git = "https://github.com/base/reth", tag = "base-v2.5.2.3" } | ||
| reth-engine-tree = { git = "https://github.com/base/reth", tag = "base-v2.5.2.3" } |
There was a problem hiding this comment.
Nit: reth-engine-tree is a heavyweight dependency for a single type.
reth-engine-tree pulls in reth-db, reth-stages, reth-static-file, reth-prune, rayon, moka, and ~40 other transitive crates. It's being added to base-builder-core and base-execution-payload solely for InstrumentedStateProvider, which is likely a thin timing wrapper.
If InstrumentedStateProvider could be re-exported from a lighter leaf crate (e.g., reth-provider or a dedicated reth-state-instrumentation crate), both builder crates would avoid inheriting the full engine-tree dependency graph. Not a blocker, but worth tracking upstream.
| reth-trie-parallel.workspace = true | ||
| reth-execution-types.workspace = true | ||
| reth-execution-cache.workspace = true | ||
| reth-engine-tree.workspace = true |
There was a problem hiding this comment.
Nit: reth-engine-tree breaks the waterfall sort order.
Per project conventions, Cargo.toml dependencies are sorted by line length. reth-engine-tree (34 chars) is shorter than its neighbors reth-execution-cache (38 chars) and reth-payload-builder (38 chars). Same applies in crates/builder/core/Cargo.toml and the workspace root.
Review Summary —
|
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | Low | crates/execution/node/src/node.rs:1182 |
state_provider_metrics hardcoded to false in the standalone build_payload_builder path. The multiplex path reads from ctx.config().engine.state_provider_metrics, but this path doesn't — so the flag has no effect when BasePayloadBuilder is used without the multiplex layer. |
| 2 | Nit | Cargo.toml, crates/builder/core/Cargo.toml, crates/execution/payload/Cargo.toml |
reth-engine-tree is a heavyweight dependency (~40 transitive crates including reth-db, reth-stages, rayon) pulled in for a single type. Worth tracking upstream to re-export InstrumentedStateProvider from a lighter crate. |
| 3 | Nit | crates/execution/payload/Cargo.toml:23, crates/builder/core/Cargo.toml:31, Cargo.toml:395 |
reth-engine-tree breaks the waterfall (sort-by-line-length) ordering convention in all three manifests. |
No critical or block-production findings.
3175a9b to
bb4b743
Compare
Review Summary —
|
bb4b743 to
89a00c6
Compare
Review SummaryThis PR adds Block-production sensitivityThis PR touches both payload builder paths (flashblocks and native), which are block-production-sensitive. However, the instrumentation is:
No block-production halt/stall risk when disabled. When enabled, the wrapper adds timing overhead but does not introduce new error paths, panics, or failure modes beyond what the underlying state provider already produces. FindingsNo new findings beyond what has already been raised in existing inline comments. The prior comments on Wrapping order noteThe |
89a00c6 to
d400ac7
Compare
Review SummaryPR: feat(builder): record build-loop state-read latency This PR adds optional Block-production sensitivityThis PR touches payload builder paths, making it block-production-sensitive. However, no Critical findings were identified:
Correctness
No new findingsThe existing inline comments from prior review runs cover the relevant observations (heavyweight dependency, sort order). No additional issues identified. |
Times the payload builder's account, storage, and code reads so time blocked on state during block building can be measured. Off by default. Reth's InstrumentedStateProvider is constructed in exactly one place, the engine's newPayload validation path, hardcoded to a source label of "engine". Enabling --engine.state-provider-metrics on a sequencer therefore measures block validation and reports nothing about the loop that builds blocks. Its constructor already accepts a source label, so both Base payload builders now wrap their own state provider and report under sync.state_provider with a "builder" label. The wrapper is applied outside CachedStateProvider so the recorded latency is the build loop's total per-read cost, cache hits included. Wrapping it inside would measure only reads that fall through to the database, and a working cache would then show no improvement. Scope, deliberately narrow. The reth per-job read cache (CachedReads) sits above this wrapper, so what is timed is reads that miss that cache rather than every state read the build loop issues. Those are the reads that can actually touch storage, but the measured population shrinks as CachedReads warms, so means are not directly comparable across configurations without accounting for it. The consumer today is the snapshot benchmark harness, which scrapes the Prometheus endpoint directly and can group on the source label. Datadog currently applies an empty tag allowlist to the reth metric families for cardinality control, so builder and engine series would aggregate together there; separating them in dashboards needs a tag-configuration change that is not part of this commit. Driven by reth's existing --engine.state-provider-metrics rather than a new flag, so the builder and the validation path are instrumented by one switch. Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
d400ac7 to
3ad4a73
Compare
| Arc, Mutex, | ||
| atomic::{AtomicUsize, Ordering}, |
There was a problem hiding this comment.
Low: Unused imports will fail CI.
AtomicUsize and Ordering are imported but never used in the test module. With -D warnings in CI, this will cause a compile error.
| Arc, Mutex, | |
| atomic::{AtomicUsize, Ordering}, | |
| sync::Arc, |
(Keep Mutex on a separate line if it's used elsewhere in the block, or merge as appropriate.)
|
<!-- CLAUDE_REVIEW_SUMMARY --> Review SummaryPR: feat(builder): record build-loop state-read latency This PR adds optional Findings
Block Production AssessmentThis PR touches block-production-sensitive paths (both payload builders' state provider setup). However, the change is low-risk:
No critical block-production findings. |
|
Caution This PR may regress performance. 1 benchmark(s) slower by more than 15% beyond the noise band: Benchmark results (advisory)Median time on the PR head versus the base branch, measured on the same host. Wall-clock, so a change is only flagged when it clears ±15% and the confidence intervals do not overlap. Only benchmarks past the ±15% threshold (plus new or dropped ones) are listed. This check never blocks a merge.
9 benchmark(s) within ±15% omitted. |
2 participants
Time spent blocked on state reads during block building was not measurable. Reth's existing state-provider instrumentation is constructed only on the engine's block-validation path, so enabling it on a sequencer reports validation cost and nothing about the loop that builds blocks.
What changed
InstrumentedStateProvider, reporting undersync.state_providerwith abuildersource label to distinguish it from the engine'senginelabel.--engine.state-provider-metricsrather than a new flag, so the builder and the validation path are instrumented by one switch. Off by default.CachedStateProvider, so the recorded latency is the build loop's total per-read cost including cache hits. Inside, a working cache would show no improvement.state_provider_metricstoBuilderConfigandBaseBuilderConfig, resolved from the node config when the payload service is built.Notes for reviewers
Scope is narrower than "all build-loop IO". Reth's per-job read cache (
CachedReads) sits above this wrapper, so what is timed is reads that miss that cache rather than every state read the build loop issues. Those are the reads that can actually reach storage, but the measured population shrinks asCachedReadswarms, so means are not directly comparable across configurations without accounting for it.No production effect as merged.
--engine.state-provider-metricsis currently off across the fleet, so this emits nothing until it is enabled. The gate is evaluated once per payload build, not per read.