perf(consensus): fix mainnet script verification and make apply 2.89x faster - #11
Merged
Conversation
- txindex: batch index writes across apply_buffered_blocks (begin/end_batch) - sync: pipeline header requests from the best ready peer instead of fanout-only - sync: tighten SYNC_TICK from 5s to 1s for faster header/data turnarounds - apply: evaluate assume-valid trust gate before startup so assume-valid is honored - apply: allow float-to-int metric casts in verify_block_transactions - benches: use body_blocks variable in sync_pipeline synthetic chain setup
Add ResolvedPrevoutView: for blocks below assume-valid height that do not need a same-block UTXO overlay, resolve all prevouts once into a flat hashbrown HashMap and run the non-script verification against that map. This avoids repeated per-input shard lock acquisitions on the live UtxoSet inside verify_transaction_borrowed_non_script_with_mtp. Also fix clippy nits: semicolon in index begin_batch wrapper, explicit deref in state.rs, and too-many-lines allow on drain_inbound_headers.
…t/maturity/bip68 checks
…tate sync pipeline
…ctions Compute txids with rayon when a block has more than 32 transactions; smaller blocks keep the sequential path to avoid thread-pool overhead. The sequential scan that tracks same-block spends and BIP68 state is unchanged and still reads the precomputed txids in order. [UNMEASURED] The synthetic sync_apply_metrics workloads have <=32 transactions per block, so they exercise only the sequential path. The parallel branch targets real mainnet blocks (hundreds to thousands of transactions) where txid hashing is a meaningful share of plan_block_transactions; the win is a complexity/scheduling argument, not a synthetic-benchmark measurement.
…counted apply gap Processing-bound 0->150k replay f76d43a 178.93s vs 389.7s baseline (838 blk/s) vs Core 67s (2.67x gap halved from 5.8x). Add histograms txid_plan_seconds + utxo_resolve_seconds to attribute 41.5s uninstrumented gap before next lever. Clarify CCheckQueue is already input-level parallel (verify_tx.rs:394) — next target is prepare 18s serial phase.
Prepare per-tx checks (prevout resolution, kernel setup, value/sigop finalization) previously ran serially (18s in 150k replay, 10% of apply time) before the per-input par_iter fan-out. For blocks >=16 txs, run prepare in parallel via resolved.par_iter_mut().enumerate() — each tx touches only its own resolved[tx_index] (disjoint), matching the thread-safety contract of the existing check_input parallelization. Build checks in block order and truncate at first pre/post error to preserve serial break semantics. Expected to cut prepare from 18s toward ~2-3s on 128 cores.
Parallel prepare regressed 150k replay 178.93s -> 195.89s (prepare saved only 0.27s, total +16s). Early blocks have small tx counts (<16) so parallel threshold rarely triggers, but per-block rayon overhead and extra truncation logic add cost. Keep 68bbb2f instrumentation (txid_plan + utxo_resolve) for profiling, revert to serial prepare until a more targeted overlap is designed.
Instrumentation added at 68bbb2f (quanta::Instant + histogram per block, 150k invocations) regressed 150k replay 178.93s -> 201.84s (+22.9s). The histograms themselves cost more than the insight they provide at this scale. Keep serial prepare (already reverted at 53feecb) and the txid parallelization at f76d43a; profiling will be done via off-line sampling, not per-block histograms.
…regressions unconfirmed 53feecb instrumented run showed utxo_resolve 25.69s + txid_plan 21.63s dominate the 41.5s gap, not prepare 18s alone. Update guidance to prioritize UTXO cache/lookup batching. Note that 1126cab (195.89s) and 53feecb (201.84s) regressions were observed under contaminated conditions (active full-tip IBD blk00817->867, background rustc) and root causes (dual pools, quanta overhead) are unconfirmed — taskset-pinned 3x median at e540b91 is the reliable post-opt: 173.1s (2.58x gap).
Re-applies 1126cab (parallel prepare_block_input_checks for >=16 txs) on top of e540b91 (serial baseline, no per-block histograms) to test in isolation without IBD/rustc contamination. Previous 195.89s measurement at 1126cab was contaminated (active full-tip IBD blk00817->867 + background rustc), so revert at 53feecb was based on noise. This commit will be measured clean with taskset -c 0-31 3x median before deciding to keep or revert.
Clean taskset -c 0-31 3x median at 0302a0c (parallel prepare, no per-block histograms, no IBD) = 173.5s (170.6, 173.5, 173.9) vs e540b91 serial prepare clean median 173.1s (166.4, 173.1, 177.8) — delta 0.4s within 6% noise, no win. Previous 195.89s contaminated run at 1126cab was IBD contention (blk00817->867), not rayon pool contention. Revert to serial for simplicity; next lever remains UTXO-resolve batching per 53feecb breakdown.
…I boundary Two candidate optimizations measured clean (taskset -c 0-31, 3x median, no IBD/rustc contention) against the 173.1s baseline: parallel prepare_block_input_checks 173.5s 1.00x rejected thread-local tx serialize buffer 173.2s 1.00x rejected The serialize-buffer change cut script_prepare 20.4s -> 18.2s yet moved the total 0.1s, which locates the cost inside the C++ parse and PrecomputedTransactionData construction rather than the Rust allocation. Correct the prior guidance: ResolvedUtxoView::resolve is already into_par_iter (apply.rs:1136), so UTXO batching is not the lever either. Every Rust-side stage in the apply path is now parallel; the residual 173.1s vs Core 67s is FFI boundary cost (per-tx serialize round-trip, per-input ScriptPubkey allocation).
Adds two more rejected candidates to the levers table: skip kernel_txout for witness-free txs rejected by tests feed kernel pre-sliced tx byte ranges rejected on cost (~1.01x) The witness-free skip is instructive: Core's PrecomputedTransactionData Init computes no midstates without a witness, but btck_script_pubkey_verify still gates on m_spent_outputs_ready, so the prevout TxOut objects must be constructed even though nothing ever hashes them. Redirect guidance away from prepare entirely: prepare is 20.4s of 173.1s, so zeroing it still leaves ~153s vs Core's 67s. The deciding cost is script_parallel at 63-65s over 67,891 blocks (~0.93ms/block), which is either real secp256k1 work or per-block rayon dispatch overhead. Those need opposite fixes, so attribution must come first -- and needs a sampling profiler (perf unavailable: perf_event_paranoid=4, no sudo).
…k replay) The pool was capped at 16 on the belief that SMT siblings slow secp256k1 down past that width. A matched-validation replay of mainnet 0..150000 (assume_valid_height=0, fjall, 3x medians) measures otherwise on this 80-CPU host. Attribution first: forcing every block through the serial path (MIN_PARALLEL_SCRIPT_CHECKS = usize::MAX) took the replay from 173.1s to 313.3s and the parallel input-check stage from 63s to 227.6s. That stage is genuine secp256k1 work, not rayon dispatch overhead -- but 227.6s -> 63s is only 3.6x from a 16-thread pool while the benchmark pinned 32 CPUs. The pool width, not the crypto, was the limit. 16 threads, 32 physical cores (-c 0-31) 173.1s 867 blk/s 32 threads, 32 physical cores (-c 0-31) 157.8s 950 blk/s 32 threads, 16 physical + SMT (-c 0-15,40-55) 158.9s The last two agree within noise while the third uses half the physical cores, so the gain tracks thread count and SMT pairing costs nothing measurable here -- the original comment's premise does not hold. Note this also changes the unpinned default, where available_parallelism() reports 80 and the pool goes 16 -> 32. Win: 1.097x (8.8%), script_parallel 63s -> 55.6s Gap to Core 31.0 (67s): 2.58x -> 2.36x Total vs 4700c25 (389.7s): 2.47x Tests: 72 node apply, 60 consensus lib, 5 vector, 3 kernel -- green.
…ution method The open question was whether script_parallel's ~0.93ms/block was real secp256k1 work or rayon dispatch overhead. perf was unavailable (perf_event_paranoid=4, no sudo), but the hypothesis is binary, so forcing MIN_PARALLEL_SCRIPT_CHECKS = usize::MAX settled it: 173.1s -> 313.3s, stage 63s -> 227.6s. Genuine crypto -- and 227.6s -> 63s is only 3.6x from a 16-thread pool, which identified pool width as the binding constraint and led to 0e2dda5 (157.8s, 1.10x). Records the three-way pool measurement including the SMT-sibling config, replaces the now-answered 'needs a profiler' guidance with the disable-the-stage technique, and retitles the note around the finding that actually mattered.
…T fetch Sweep is non-monotonic -- 64 threads (177.9s) is worse than 16 (173.1s), with 32 the optimum at 157.8s. Coordination cost on ~22-input blocks outweighs the extra width. Recorded so nobody raises the cap on the assumption that more is better. Also replaces the stage decomposition with clean numbers from the 32-thread run, superseding the contaminated 53feecb attribution. Two corrections fall out: - 22.4s of the 157.8s is REST fetch, which Core's -reindex-chainstate never pays (it reads local blk files). Apply-only is 128.9s vs Core 67s = 1.92x, so the 2.36x headline overstates the engine gap. - script_verify alone (84.9s) exceeds Core's entire 67s run, and non-script apply (44.0s) is two-thirds of it. Neither half closes the gap on its own.
Ran the shipped config unpinned across all 80 CPUs to close the claim-scope gap (every prior number was taskset -c 0-31). Result: 190.7s median (188.0/190.7/191.5) -- 21% SLOWER than the pinned 157.8s, with load average 7.7-18.5 from other tenants during the run. This refutes the hypothesis that pinning starved the replay of cores Core had: more CPUs made it slower because they were contended, not faster. taskset -c 0-31 is therefore the reproducible measurement. Corollary now recorded: Core's 67s reference was captured at another time under unknown load, so the 2.36x and 1.92x figures carry that uncertainty on both sides and need a back-to-back idle-host pair before being treated as final.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+131
to
+139
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: dtolnay/rust-toolchain@1.95.0 | ||
| - uses: Swatinem/rust-cache@v2 | ||
| - run: cargo check -p bitcoin-rs-consensus --no-default-features | ||
| - run: cargo test -p bitcoin-rs-consensus --no-default-features | ||
| - run: cargo test -p bitcoin-rs-script | ||
| - run: cargo check -p bitcoin-rs-node --no-default-features --features fjall |
Retaining kernel TxOut objects in PreparedKernelTx and borrowing script_pubkey()/value() per input removes ~3.3M FFI ScriptPubkey allocations over the 0-150k window. Measured 160.3s against the 157.8s baseline (0.98x) with script_parallel unchanged at 56.1s and RSS up 14 MB, so object residency costs more than the allocations save. This retires the FFI-boundary hypothesis for its allocation half and downgrades the proposed block-level kernel object reuse: that lever was partly tested here and did not pay.
MIN_PARALLEL_SCRIPT_CHECKS gated the script-verify pool at 16 checks, so every block with fewer inputs verified serially and never reached the pool. On mainnet 0-150k most blocks are below that line, which left the 32-thread pool idle for the bulk of the window. Sweeping the threshold on a pinned 3x median puts the optimum at 4: 4 -> 139.4s 8 -> 145.1s 16 -> 155.8s 48 -> 185.0s 128 -> 234.6s 512 -> 297.3s The 4/8/16 runs were interleaved round-robin so page-cache warming hit each equally; the ordering held in all three rounds. Threshold 2 measures 143.8s, so 4 is an interior minimum rather than a floor. Shipped constant measures 135.0s median (135.0, 134.0, 140.6), 1111 blk/s, with script_verify down 84.0s to 66.5s. That is 1.15x over the 32-thread pool alone and 2.89x over the 4700c25 baseline.
metaphorics
changed the title
The threshold and the pool width interact, so width was re-swept at threshold 4 rather than carried over. It did not move: 16 -> 152.7s, 32 -> 139.6s on taskset -c 0-31. Also records the measurement trap that makes a wider constant look inert. Under taskset -c 0-31 available_parallelism() reports 32 and available.min(cap) clamps anything above it, so 48/64/80 are replicates of 32 (141.0/147.2/144.0s, which sets single-run noise near 5%). On taskset -c 0-63 width 32 and 64 tie and both lose ~15% to the narrower pin from host contention.
Sweeping plan_block_transactions the same way the script-check threshold was swept finds nothing: 1 -> 139.8s, 4 -> 138.1s, 16 -> 141.9s, 32 -> 136.4s, all inside single-run noise. A txid is one SHA256d against a script check's ~100us, so per-item work, not the threshold shape, decides whether a fan-out pays.
Contributor
Author
|
@coderabbitai ultrareview |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Mainnet full-validation sync no longer wedges, and processing-bound apply is 2.89× faster than at the branch point. Two consensus defects blocked real mainnet chains before this branch: taproot key-path spends were verified without the full prevout set, and the default verifier could not validate the script classes mainnet actually contains. Both are fixed,
bitcoinkernelis now the default verifier, and mainnet nodes ship a hash-pinned assume-valid anchor so historical script checks are skipped only against a verified chain.The performance work is measured, not asserted: every candidate was benchmarked on a pinned 3× median, and the five that did not clear a 1.05× noise floor were reverted rather than kept.
Correctness
fix(script): pass all prevouts to taproot key spendsfix(consensus): make bitcoinkernel the default verifierlibbitcoinkernel(Core's own engine) is the default; the Rust interpreter remains for differential testingfeat(node): hash-pinned assume-valid default on mainnetfix(node): send getheaders after getdata within a ticktick, inverting message order and breaking 43 sync testsfix(index): dedup block rows before counting themfix(bench): model the Electrum node as a single process--pid, so it could not satisfy the script's new tip and socket-ownership checksbitcoin-rsserves Electrum and owns the socketPerformance
Processing-bound replay, mainnet 0→150,000,
assume_valid_height=0(full script verification), fjall,taskset -c 0-31, 3× medians:4700c25)Supporting wins: deferred redb body-index durability, active-prefix ancestry and locator indexes in
chain, MTP heap-allocation removal, batched txindex writes acrossapply_buffered_blocks, and inbound block byte reuse insync.Bitcoin Core 31.0 runs the same window in 67s, so bitcoin-rs remains ~2.01× slower in this regime. That gap is documented rather than papered over, including the caveat that ~22s of the run is REST fetch the Core harness never pays, which puts the defensible apply-only ratio nearer 1.7×.
Design decisions
prepare_block_input_checks(1.00×), a thread-local serialize buffer (1.00×), per-block profiling histograms (+23s overhead), and a 64-thread pool (177.9s, worse than 16) were each measured and backed out. The branch keeps the measurements and drops the code.available_parallelismso verification cannot starve the rest of the apply pipeline.MIN_PARALLEL_SCRIPT_CHECKSrouted every block under 16 input checks to the serial branch, which on 0→150k is most of them. Lowering it to 4 bought 1.15× on its own. That threshold sweep is also an interior minimum (2 → 143.8s, 4 → 139.4s, 8 → 145.1s), so it is pinned by measurement in both directions.script_parallelwas real secp256k1 work or rayon overhead was settled by disabling the stage (all-serial: 313.3s) rather than guessing — which is what identified pool width as the binding constraint.Test plan
cargo test --workspace --release --no-fail-fast— 0 failures. This is the gate that matters and it was not run earlier in the branch's life: subset runs hid 47 real failures (43 sync, 3 Electrum evidence, 1 index), each fixed above and each verified againstorigin/mainbefore being called a regression.cargo check --workspace --all-targets— clean, both with and without thekernelfeature.cargo test -p bitcoin-rs-node --lib --release— 341 pass (was 298 passed / 43 failed).cargo test -p bitcoin-rs-consensus --release— 60 lib + 5 vector + 3 kernel tests pass, includingscript_verdict_parityand the taproot script-path vectors.cargo test -p bitcoin-rs-node --lib apply::consensus_rule_tests --release— 72 pass, covering BIP30/BIP34, DAA retarget boundaries, BIP68, coinbase maturity, and the assume-valid skip/no-skip paths.crates/storage/tests/backend_equivalence.rsextended so the redb durability change is checked against the other backends rather than in isolation.mainnet_prefix_replayagainst a localbitcoind -rest, 3× medians on pinned CPUs; the JSON artifacts are referenced from the solution note.Durable findings are recorded under
docs/solutions/(script-verify pool sweep, redb durability boundary, at-scale benchmark fidelity) and the assume-valid vocabulary is reconciled inCONCEPTS.md, perAGENTS.md.