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}
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. |
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. |
3ad4a73 to
be9102d
Compare
Review SummaryClean, well-scoped instrumentation change. No correctness, safety, or block-production concerns. Block-production sensitivity: The PR touches both payload builder paths (flashblocks and native), but the change is gated behind Wrapping order is correct: Config propagation is consistent: All three entry points ( No new findings beyond what has already been noted (dependency weight of |
be9102d to
e536a67
Compare
| reth-rpc-layer.workspace = true | ||
| reth-storage-api.workspace = true | ||
| reth-chain-state.workspace = true | ||
| reth-engine-tree.workspace = true |
There was a problem hiding this comment.
Low: reth-engine-tree is unused in base-builder-core.
This crate was previously importing CachedStateProvider etc. from reth-execution-cache directly in flashblocks/payload.rs, but that PR change delegates to base_execution_payload_builder::wrap_builder_state_provider instead. No source file in crates/builder/core/src/ imports reth_engine_tree, so this dependency is unused and will likely be flagged by cargo-udeps in CI.
| reth-engine-tree.workspace = true |
Review SummaryThis PR adds build-loop state-read latency instrumentation by wrapping state providers in FindingsLow: Unused The PR moves state-provider wrapping into Block-production assessmentThis PR is block-production-sensitive (touches payload builder state-provider setup in both the native and flashblocks paths). No critical findings:
Notes on prior bot comments
|
| } | ||
|
|
||
| /// Wraps a builder's state provider in the decorators the build loop needs. | ||
| pub fn wrap_builder_state_provider<S>( |
There was a problem hiding this comment.
Nice helper fn to make constructing these easier and enforcing instrumentation is after-cache latency.
Wdyt about making this also a StateProvider wrapper like BuilderStateProvider::new(provider, cache, instrument)?
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>
e536a67 to
4540a1e
Compare
Review SummaryThis PR adds Block-production safety: No concerns. The instrumentation is off by default, evaluated once per payload build (not per read), and adds only timing measurement. All existing error paths pass through unchanged. FindingsAll substantive issues were already raised in prior inline comments. No new findings beyond those. Notes on prior comments
|
3 participants
A sequencer both builds blocks and validates them, and reth's state-provider instrumentation is wired only into the validation path. Enabling
--engine.state-provider-metricsthere reports the cost of checking a finished block, not the cost of assembling one — so time spent blocked on state reads during building was never measurable.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
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.