perf: move blocking work off the async runtime, and deep copies out of it

[flyq]

Summary

PR 4/6 of the #170 split. #196 has merged and main is merged in as of c24d6f3, so this now targets main directly.

Two pieces of synchronous, multi-millisecond work move off the async runtime (§1, §2) — and the one that #170 got wrong is fixed here rather than shipped. §4 is that same argument one level down: a deep copy that was riding along inside §2, doing nothing. §3 is the one rider: an executor-side gas_used derivation, the surviving half of a commit whose other half went up as #196.

vincent's review of #170 named this PR's whole reason for existing: "Moving verify_block_integrity into spawn_blocking is a good change on its own, but it also moved verification inside round_robin_with_backoff's per-attempt timeout, so a healthy provider serving a large block can be classified as stalled, retried, and leave the blocking task running behind it. As a standalone three-line PR that consequence is visible on sight; at position 2 of 6 in a 30-file cleanup it is not."

1. Block verification becomes the retry loop's finalize step

round_robin_with_backoff already splits each attempt into f (run under the per-attempt window) and finish (run after it, bounded by the caller's deadline alone). #182 introduced that seam for exactly this problem on the witness path: CPU-bound decode must neither burn the rotation reserve nor read as a provider stall, while a corrupt payload still rotates as that provider's error.

get_block_with_deadline now uses the same seam instead of call_with_deadline's identity finish:

• f = do_get_block_unchecked — the transport, timed by the attempt window.
• finish = verify_block_integrity on the blocking pool — per-transaction ECDSA recovery plus a re-encode of every envelope, outside the window.

So the timeout interaction #170 introduced never exists: a healthy provider serving a large block cannot be classified as stalled by its own verification, and no abandoned blocking task runs behind a retry. A verification failure is still RpcAttemptOutcome::Error, so a tampered block rotates exactly as a transport failure does — pinned by block_verification_failure_rotates_to_the_next_provider, which serves a wrong-hash block from provider 1 and a good one from provider 2 and asserts both were hit.

The seam is reached through the data path's own funnel rather than around it: call_with_finish owns the provider-rotation wiring — providers, labels, concurrency, backoff policy, attempt cap, and the per-call rr_start rotation now in RpcClient::next_data_rr_start — and call_with_deadline_at delegates to it with the identity finish it used to write inline. round_robin_with_backoff keeps exactly two call sites, so the data path has one definition of its rotation policy rather than two.

2. The advancer commits on the blocking pool

chain_advancer takes Arc<S> / Arc<H> and runs hooks.pre_advance + store.advance_chain inside spawn_blocking. These are redb commits — and, in the trace server, multi-MB block-data writes — on a runtime that also serves RPC handlers.

Panic semantics are preserved exactly: a panic in the store or the hooks comes back as a JoinError, and try_into_panic → resume_unwind re-raises it on this task, so a corrupted persistence layer still takes the process down the way it did inline. vincent called this handling "careful and correct" in #170; test_chain_advancer_propagates_hook_panics now pins it — a PipelineHooks whose pre_advance panics must make the advancer future panic, not return Err.

The metas lockstep vector is gone with it: the batch is moved into the blocking closure and the metas are built there from the items themselves, so the two can no longer drift.

3. gas_used is read from the executor's own result

replay_block derived the header check's gas_used by re-deriving mega-evm's own expression, receipts.last().cumulative_gas_used(), rather than reading the BlockExecutionResult::gas_used field that mega-evm's finish() sets from that exact expression (v1.7.0 30ce038, crates/mega-evm/src/block/executor.rs:712). It now reads the field (crates/stateless-core/src/executor.rs:501). Definitional, not behavioural: on the pinned mega-evm the two cannot disagree. What it buys is that a future mega-evm accounting gas outside the receipt chain changes one derivation instead of silently diverging from a copy of it.

Three things hold that claim down. The debug_assert_eq! at the derivation site pins the upstream definition on every debug/CI replay; replayed_gas_used_matches_the_mainnet_header (executor.rs:1096) pins the surviving value against what real mainnet headers claim; and in release the value still feeds the header check at executor.rs:659, so a divergence rejects the block rather than passing quietly.

This rode in on e62e3e1, whose other half — the verify_block_integrity re-encode — was upstreamed as #196 and is now in main; merging main in (c24d6f3) left only this half in the diff.

4. pre_advance stores block data by reference

Moving pre_advance onto the blocking pool surfaced work inside it that should not be there at all. TraceHooks::pre_advance cloned every block and every witness purely to reshape a slice for store_block_data, which only ever borrows — both fields go straight into encode_block_to_vec / encode_to_vec (bin/debug-trace-server/src/server_db.rs:86-87). The witness clone is the expensive half: a node-by-node BTreeMap copy of kvs plus the levels map, once per block, per advance batch.

BlockStore::store_block_data now takes &[(&Block<Transaction>, &LightWitness)], so pre_advance builds a 16-bytes-per-item Vec of references instead (bin/debug-trace-server/src/chain_sync.rs:238). This is §2's argument one level down: §2 moved the work off the runtime, this removes a piece of it that never needed doing. It also keeps the AGENTS.md sentence honest — pre_advance is described there as synchronous multi-millisecond disk work, and a meaningful slice of those milliseconds was memcpy, not disk.

Mechanical elsewhere: the trait declaration, the inherent and trait impls, the test stub, and five call sites. No behavioural change, and the existing server_db storage tests exercise the new signature unchanged.

Testing

cargo fmt --all --check, cargo clippy --workspace --all-targets --all-features (0 warnings), cargo sort --check, full workspace suite 484 passed / 0 failed, cargo test -p stateless-core --no-default-features --lib --no-run clean.

Both new tests were mutation-checked: forcing verify = false kills the rotation test, and replacing resume_unwind with an Err return kills the panic test.

Notes

§1 and §2 are the same argument applied to the two places blocking work sits on this runtime, and §4 is that argument one level down, so they are kept together — happy to split them apart if you would rather review them separately. AGENTS.md and README are updated for §1 and §2.

A quality pass over this PR's own diff landed in 333c283: get_block_with_deadline reaches the finalize seam through the data path's funnel instead of copying its argument list, §3's test calls verify_and_replay rather than hand-rolling it, the advancer frees its batch on the blocking pool instead of handing it back to the runtime, and two test helpers were deduplicated. Behaviour-preserving; the numbers above are from after it.

[mega-maxwell]

Claude review status

Living comment — rewritten in place. The review workflow keeps this single comment up to date instead of posting a new one each round, so it always describes the latest reviewed commit and the earlier text is intentionally gone. No reply is needed here; reply to a finding in its own review thread, and answer an open question in a reply on this PR. The next review round reconciles your answer.

🛠️ Review did not finish

Attempted 333c283f..20fc1fc3 · updated 2026-09-18T02:26:35+00:00

This round did not publish: PRIOR_QUESTION_INVALID in phase compile. Anything listed below is from the last round that did. Re-run the workflow or push a new commit to try again.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.6%. Comparing base (bb1de6a) to head (20fc1fc).

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mega-maxwell]

❓ Review complete — 1 open question(s)

Reviewed head c24d6f32.

Reviewed the two blocking-work-off-runtime moves (block verify as round_robin_with_backoff finalize, chain-advancer commit via spawn_blocking), the extracted next_data_rr_start, new tests, and README/AGENTS.md doc updates.

Open questions — answer them in a reply on this PR. Each one is marked answered here once a later review round confirms the answer, so this list stays current:

✅ **Answered** — flyq confirmed in the conversation the gas_used change is intentional, from commit e62e3e1, and added a '## 3.' section with the mega-evm citation to the PR description. This round's test refactor (using verify_and_replay) reinforces the claim.

• The executor.rs change in replay_block — switching gas_used to the BlockExecutionResult::gas_used field (guarded by a debug_assert_eq! and pinned by a new replayed_gas_used_matches_the_mainnet_header test) — isn't mentioned in the PR description. Is this refactor intentionally in this PR, or did it slip in from another slice of the #170 split?
• Why it matters: Only the debug_assert_eq! enforces gas_used == receipts.last().cumulative_gas_used() in debug/CI; release builds trust whatever mega-evm produces. A divergence would surface loudly (output.gas_used != header.gas_used would reject blocks), but the change alters the source of a consensus-critical value with no description, making the intent hard to attribute later.
• How to verify: Confirm with the author whether this hunk was meant to ride along with the perf changes; if not, move it to its own PR (or the correct slice of the #170 split) with its own description.

[flyq]

Intentional, and not from another slice — but you are right that the description never covered it.

The hunk comes from e62e3e1 ("perf: encode each transaction once in verify_block_integrity, read gas_used from the executor"), one of this branch's own two commits. That commit had two halves, and the verify_block_integrity re-encode half was upstreamed as #196; merging main in (c24d6f3) left only the executor half in the diff, which is why it now reads as an unexplained hunk. The gap is real and the fix belongs in the description — the PR body is the squash message, so that is where the attribution has to live. Added as ## 3., with the argument and the upstream citation (mega-evm v1.7.0 30ce038, crates/mega-evm/src/block/executor.rs:712, inside finish()).

[RealiCZ]

Reviewed the full diff and verified the claims it rests on:

• round_robin_with_backoff's finish seam classifies a finish error as that provider's Error (rotates) and a finish that outruns the deadline as DeadlineClamped, so §1's "a tampered block rotates like a transport failure" holds and verification no longer sits inside the per-attempt window.
• §3: mega-evm v1.7.0 (30ce038) finish() at crates/mega-evm/src/block/executor.rs:712 sets gas_used to receipts.last().cumulative_gas_used(), so reading the field is definitional on the pinned version.
• §2: run_with_signals only awaits the pipeline handle with a drain timeout and never aborts it, and a runtime drop waits for running blocking tasks, so a commit in flight on the blocking pool is never dropped mid-way. resume_unwind keeps a store/hook panic fatal.
• chain_advancer is pub(crate), run_pipeline already took Arcs, and BlockStore is trace-server-local, so nothing mega-reth consumes changes shape.
• Merges cleanly onto main and alongside #221.
• Ran locally: test_chain_advancer_propagates_hook_panics, replayed_gas_used_matches_the_mainnet_header, block_verification_failure_rotates_to_the_next_provider, and the server_db tests all pass.

Two non-blocking notes inline. Approving.

[RealiCZ]

Non-blocking, and pre-existing since #182 rather than introduced here: round_robin_with_backoff drops the concurrency permit after finish, so --data-max-concurrent-requests now also bounds how many ECDSA verifications run in parallel on the blocking pool, and attempt_elapsed (the on_rpc_attempt histogram) includes the verification CPU time. Consistent with how the witness decode already behaves, so fine for this PR; a follow-up could release the permit before finish so the cap counts only what is actually in flight against the gateway.

[RealiCZ]

Worth one sentence in this doc: a panic inside verify_block_integrity becomes this provider's Error and rotates (with deadline = None it retries forever), whereas the advancer half of this PR deliberately re-raises panics. The asymmetry is right (a store panic is a corrupted persistence layer, a verify panic is bad provider data) and matches decode_witness_wire, but the PR description only states the advancer half, so a reader may assume both hops preserve panics.