Merge pull request #12 from gosuda/docs/e2e-sync-benchmarks perf: parse each block once, drop unpaid fan-outs, match the benchmark harness (4.6x) - #15
Closed
metaphorics wants to merge 619 commits into
Conversation
Add a G14 bitcoin-rs daemon IBD wrapper that launches the real node, polls applied-height RPC progress, validates start/stop hashes, and emits canonical bitcoin-rs/mainnet-ibd Criterion output for the existing artifact pipeline. Op: correct Restores: spec:G14 bitcoin-rs evidence must measure end-to-end daemon IBD, not only mainnet prefix replay
Add a hash-bound UTXO commit p95 measurement artifact and require Criterion G14 manifests, the collector, and the final G14 gate to validate commit, window, sample source SHA, sample count, and recomputed p95 before accepting the budget scalar. Op: correct Restores: spec:G14 UTXO commit p95 evidence must be derived from hash-bound per-block samples, not an untrusted scalar
Bind bitcoin-rs Criterion G14 evidence to the daemon IBD adapter at every ingress: runner argv validation, raw-output proof adapter, artifact and manifest validation, collector exports, and final gate command/proof checks. Op: correct Restores: spec:G14 bitcoin-rs IBD evidence must be true daemon sync evidence, not replay output using the same benchmark label
Tighten G14 adapter identity test fixtures after the daemon-adapter evidence gate so the full portable clippy gate remains clean without weakening replay rejection coverage. Op: correct Restores: spec:G14 evidence tests must satisfy workspace lint gates while preserving daemon-adapter identity checks
Add a daemon-side G14 UTXO commit sampler and wire the mainnet IBD adapter to produce hash-bound sample and measurement artifacts for the final G14 budget gate. Op: extend
Buffer G14 UTXO commit samples during the IBD window, materialize the JSON sample source once at the stop height before applied-tip publication, and keep sampler write failures best-effort so evidence fails closed without corrupting block apply state. Op: correct Restores: spec:G14 UTXO sample emission must not distort IBD timing or race RPC-visible stop-height evidence
Require daemon adapter command windows to match the outer G14 Criterion window, and require UTXO sample sources to contain boundary hashes matching the measured IBD start and stop blocks. Op: correct Restores: spec:G14 evidence must bind measured daemon window and UTXO sample boundaries to the same live IBD interval
Make direct final-gate evidence validate that G14_BITCOIN_RS_COMMAND carries daemon adapter IBD start/stop heights and hashes matching the G14 evidence window. Op: correct Restores: spec:direct G14 gate evidence must bind daemon command window to the asserted IBD interval
Let the G14 Bitcoin Core adapter use startup timeout only for pre-RPC startup, then use a separate IBD timeout for stop-height progress. Keep successful shutdown on a fixed bounded timeout so short startup probes do not kill valid long IBD runs. Op: correct Restores: spec:G14 Core IBD adapter must not use RPC startup timeout as the full benchmark deadline
Use startup timeout only before the daemon adapter has observed valid bitcoin-rs RPC state. After that, use the IBD timeout for stop-height progress and post-start RPC drops, and expose the long default in adapter help. Op: correct Restores: spec:G14 bitcoin-rs daemon adapter must not use RPC startup timeout as the post-start benchmark deadline
Match Bitcoin Core getblockhash semantics for a fresh node by falling back to the selected network genesis hash when height 0 is requested before any block record has been published. Op: correct Restores: spec:JSON-RPC getblockhash 0 must return the network genesis hash
tick() re-sent an identical getheaders to the sync peer on every tick whenever the peer's best height exceeded the local header tip, with no in-flight request tracking. During IBD this floods the peer with redundant 2000-header batches that the node then re-validates and rejects as duplicates, wasting bandwidth and CPU. Track the outstanding request (peer, locator tip hash, target height) and suppress a new getheaders while an identical one is in flight, retrying only after a 5s timeout. The gate releases implicitly when the header tip advances (the locator changes) rather than on any inbound batch, so an unrelated or stale batch from another peer cannot falsely clear it while the original response is still pending. Tests: no-resend while pending, release after a tip-advancing response, stale/orphan batch keeps the gate pending; three inflight tests updated to the new no-spam contract.
…tion
During IBD with block_body_store or txindex enabled, apply_block
reserialized every decoded block via consensus::encode::serialize to
get the bytes for body persistence and tx-index ingestion. The P2P
layer had already allocated and checksum-validated those exact wire
bytes, then discarded them. Under assume_valid (scripts skipped) this
redundant per-block serialization is a meaningful share of apply time
and scales with block size.
Preserve the raw block payload: wire::read_message now returns
(Message, Bytes); a new bitcoin_rs_p2p::InboundBlock { block, serialized }
carries both through the listener, the node inbound channel, and the
BlockSync staging buffer to apply_block_with_serialized, which reuses
the preserved bytes when needs_body holds and their length matches
block.total_size() (a cheap, release-observable guard that self-heals
to a fresh serialize otherwise; a debug_assert checks full equality).
Public apply_block(&Block) is unchanged and still serializes; non-P2P
injectors (submitblock, tests, benches) use InboundBlock::from_decoded.
Behavior-preserving: the decoded block and all consensus validation are
identical; only the source of the body bytes changes. The preserved
payload is byte-identical to the canonical serialization because the
decoder rejects every non-canonical encoding.
Tests: wire roundtrip asserts payload == serialize(block); a node test
asserts apply_block_with_serialized persists the same body as apply_block.
The inbound block channel between the per-peer P2P listener threads and the single-threaded BlockSync::tick drain was unbounded, while the outbound channel has always been bounded (P2P_OUTBOUND_QUEUE_LIMIT). Decoded inbound blocks carry the full Block plus the preserved wire bytes (up to ~4 MiB each), so a fast or flooding peer could enqueue blocks faster than they drain and grow the channel without limit -- an OOM / availability vector that block-download fan-out would amplify. Bound the channel to INBOUND_BLOCK_CHANNEL_LIMIT (256). A full channel applies TCP backpressure to the sending peer's listener thread; tick drains independently and holds no lock a listener needs, so the bound cannot deadlock. The limit sits well above the in-flight request window (PENDING_BUDGET = 128) and every block arrival wakes the drain, so honest delivery is never throttled -- backpressure engages only under sustained overload, shedding the offending peer. Tests: inbound_blocks_channel_is_bounded_against_flood fills the production channel to the limit and asserts the next send is rejected.
Now that the inbound-block channel is bounded, submitblock's blocking send could park the RPC connection thread during a sustained peer-driven inbound-block flood. Use send_timeout (2s) so a locally submitted block is still enqueued under normal load (the drain frees a slot within a tick) but a flood reports "inbound-busy" instead of blocking the worker indefinitely. Also correct the channel test to read InboundBlock.block (the channel has carried InboundBlock since the wire-bytes-reuse change), which had left the rpc test suite uncompilable.
Lockfile-only refresh of 30 infrastructure/utility crates to their latest Rust 1.95.0-compatible versions within existing manifest ranges, including the mimalloc allocator (0.1.50 -> 0.1.52, libmimalloc-sys 0.1.47 -> 0.1.49), hyper 1.9.0 -> 1.10.1, bitflags 2.11.1 -> 2.13.0, log 0.4.29 -> 0.4.32, serde_json 1.0.149 -> 1.0.150, socket2 0.6.3 -> 0.6.4, zerocopy 0.8.48 -> 0.8.50, and the wasm-bindgen 0.2.121 -> 0.2.122 set. Consensus-critical crates are deliberately held at their pinned versions (bitcoin 0.32.9, bitcoin-io, bitcoin-units, base58ck, bitcoin-consensus- encoding, bitcoinkernel, libbitcoinkernel-sys, hex-conservative, miniscript). The parallel Rust validation path must stay byte-identical to bitcoinkernel, so these are not bumped without the isolated kernel-vs-consensus equivalence gate. The blanket in-range `cargo update` is additionally unsafe right now: it pulls bitcoin-consensus-encoding 1.0.0, which renamed the Decodable/Encodable traits to Decode/Encode and fails to compile against transitive bitcoin-io 0.3.0 (an upstream packaging incompatibility). No CVEs before or after (cargo audit clean). Gates: node lib 194, binary gate 174 passed / 10 ignored, clippy -D warnings (rocksdb,fjall,redb,mdbx, bitcoinconsensus), fmt.
Adds a connect option (CLI --connect, env BITCOIN_RS_CONNECT, TOML connect) holding a list of fixed peer SocketAddrs. When non-empty, DNS seed bootstrap is skipped and the node dials only these addresses, re-queueing any that drop so the link is re-established (Bitcoin Core -connect semantics). Enables controlled single-peer topologies for reproducible IBD benchmarking and private-network operation.
Adds docs/solutions/architecture-patterns/ knowledge doc on why multi-peer block download requires Core-style stalling-disconnect, seeds CONCEPTS.md with IBD/sync domain vocabulary, and surfaces both via AGENTS.md so agents discover the knowledge store.
The per-connection message loop drained the outbound queue and then did a blocking read with a 1s timeout. When a peer went briefly silent (the IBD download window momentarily drained), the read blocked up to 1s before the loop could send the next queued getdata, stalling block download. A fair single-peer mainnet IBD (239,581 blocks, loopback) spent ~1100s (58% of wall time) in these ~1s stalls. Move outbound writes to a dedicated writer thread per connection (reader and writer share the socket via try_clone), so a queued getdata is sent immediately regardless of the blocking read. The read timeout stays at 1s, preserving message-framing integrity against slow peers. Inbound responses are queued to the writer instead of written from the read loop. Measured: genesis->239,581 IBD drops from 1897s to 815s (2.33x), eliminating the download stalls; rs is now apply-bound.
Surfaces block_body_persist_us and block_record_us in the apply_block profile log so the per-block apply breakdown is complete for IBD perf work.
The coin-stats UtxoChangeListener ran per-coin MuHash + event-collection work on the block-apply hot path for every block. Bitcoin Core does not maintain rolling UTXO-set statistics during IBD by default (coinstatsindex is opt-in), so rs paid large per-coin work Core skips. Register the listener only when G2 MuHash sampling needs the rolling accumulator; otherwise gettxoutsetinfo derives total_amount/bogo_size/muhash via an on-demand stable-view scan (new UtxoSetView::for_each_coin + coinstats::scan_coin_stats), matching Core's scan-on-demand model. A unit test confirms the scan reproduces the rolling listener's stats and MuHash exactly. Measured: fair single-peer mainnet IBD (genesis->239,581, loopback, matched dbcache/assumevalid) drops from 815s to ~610s, eliminating the per-coin listener cost on the apply path (~25% of apply time).
The no-listener multi-shard commit path spawned a rayon scope per block for any block touching 2+ shards (i.e. almost every block). For a small block the scope setup, per-shard task spawn, and join latency dwarf the handful of hash-table ops, and serialize against the rest of the apply pipeline's rayon use. Gate parallelism on combined add+remove volume: commit serially below 2048 ops, fan out across shards only for larger blocks. Shard commits are independent, so serial and parallel produce identical state (commit-roundtrip and listener-parity tests unchanged). Measured: utxo_commit drops 287->99us/block mid-height; fair single-peer mainnet IBD (genesis->239,581, loopback, matched config) drops from ~610s to ~408s -- the per-small-block rayon scope latency dominated across ~200k early blocks.
cargo update floats every workspace dep to the highest version inside its declared range (bitcoin 0.32.9->0.32.100, bitcoin-io 0.1.4->0.1.100, bitcoin-internals/units ->0.1.100, fjall/lsm-tree 3.1.4->3.1.5, etc.). It also floated bitcoin-consensus-encoding, an unconstrained transitive of bitcoin-io 0.3.0 (via rustreexo 0.5.0), from 1.0.0-rc.3 to the final 1.0.0. bitcoin-io 0.3.0 declares 'encoding ^1.0.0-rc.2' but its source still uses the rc-era Decodable/Encodable traits that 1.0.0 renamed to Decode/Encode, so the final release fails to compile. No reachable bitcoin-io fixes this: 0.4.0-rc.0 needs bitcoin_hashes 0.19 and 0.5.0 needs 0.20, both past the <0.15 wall, and rustreexo is capped <0.6. Pin bitcoin-consensus-encoding to 1.0.0-rc.3 in the lock and document the tripwire in Cargo.toml. Validated: fmt --check, clippy -D warnings (full feature set), cargo test --workspace, and cargo test -p bitcoin-rs with rocksdb,fjall,redb,mdbx,bitcoinconsensus all green.
Bumped all dependencies in Cargo.toml and Cargo.lock to version 0.3.0, reflecting the latest updates for the bitcoin-rs project. Updated repository URL and authors in Cargo.toml. Adjusted user agent strings in the electrum and p2p crates to match the new version. Ensured compatibility with bitcoin-consensus-encoding by pinning it in the utreexo crate. Validated changes with successful builds and tests.
Updated user-agent strings in the electrum and p2p crates to use the new USER_AGENT constant for consistency. Introduced a new version module in the primitives crate to centralize versioning information, including the current package version and user-agent string. This change enhances maintainability and ensures accurate version reporting across the project.
Introduced a new test (g15_workspace_version_sync) to ensure that all internal path dependencies in the workspace have the same version as the `[workspace.package].version`. Updated AGENTS.md to reflect the new gate tests and added workspace metadata guidelines, emphasizing the importance of version consistency across internal dependencies. Adjusted the range of gate tests to include the new test case.
Updated all dependencies in Cargo.toml and Cargo.lock to version 0.3.1, reflecting the latest updates for the bitcoin-rs project. Ensured consistency in versioning across all internal crates to maintain compatibility and stability. Validated changes with successful builds and tests.
Serial (<2048 ops) and parallel (rayon) UTXO commit must converge to byte-identical state; this guards every Phase-1 optimization that touches commit parallelism. Asserts the result is invariant under several chunkings and matches an independent HashMap oracle, across uniform/two-shard/concentrated shard shapes. Threshold-agnostic (no stale mirror). Op: extend
The assume-valid fast path skips script verification but MUST still enforce non-script consensus rules. Existing tests cover duplicate-spend and coinbase-scriptSig under assume_valid_height; this adds the input/output value-balance case (outputs exceeding inputs -> InputsLessThanOutputs) so a Phase-1 assume-valid optimization cannot silently drop it. Sigop-cost and finality cases remain as follow-ups. Op: extend
The verify hot path routes non-taproot inputs to bitcoinconsensus — Bitcoin Core's own extracted C script engine, enabled by default — so that path is byte-identical and equal-speed to Core. "Faster than Core" can only come from the non-script paths (UTXO cache, parallelism, download, storage). Grounded by a measured +2.7% regression when the duplicate-input set was swapped BTreeSet -> hashbrown::HashSet (reverted): the set is tiny and the per-input cost lives inside the C call, so the container choice cannot move it. Scoped to the non-taproot path: taproot inputs run the Rust interpreter even on the default build. Seeds the consensus-validation vocabulary cluster and back-links the multi-peer-download sibling. Op: extend
Line 117 quoted 1.92x from Core's old 67s reference and the REST-fetch run; marked historical with a pointer to the 1.42x figure. The fairness note recomputed engine-only from the REST run (1.95x); now derived from the local-block-file measurement (80.4s vs 59.6s = 1.35x). The document now asserts one current ratio throughout.
The 'What the remaining apply time is' table still listed script_prepare at 18.6s and plan_block_transactions at 10.3s. The one-shot kernel parse cut the first to 4.29s and absorbed the second into Block::new, so it contradicted the stage-by-stage table one scroll below. Replaced with a marker rather than deleted: the txid analysis it carried bounded the bitcoin_slices idea at ~2.7s and called AVX2 SHA-256 out of reach, which the refactor then obtained for free. That reasoning error is worth keeping visible.
UtxoSet::commit fans out over active shards above a threshold of 8, which looked like a third instance of the pattern that paid twice earlier. Paired 3x medians say otherwise: parallel 84.2s vs serial 84.7s, utxo_commit 5.5s either way. Blocks in this window rarely touch 8 shards, so the serial path is already what runs. Closing the 3.51s against Core's flush needs a change to what the commit does, not how it is scheduled. Program drops to four items worth ~17s.
Probes split block_rules 4.59s into merkle 4.34s, block.weight() 0.18s, other 0.07s. The stage is the merkle root. Rejected: hashing via sha2 instead of bitcoin_hashes + Encodable (4.36s, identical); parallel fold at threshold 512 (4.11s, inside noise); at threshold 32 (6.52s, worse). Records a calibration that corrected my own reasoning: raw double-SHA256 of 64 bytes measures 873ns here, not the ~400ns assumed from theory, so the overhead ratio is 2.9x rather than the 6.4x an earlier draft claimed. The remaining overhead is per-node cost in small levels, which neither parallelism nor a faster hash can reach.
block_body_persist looked wrong at 164 MB/s on tmpfs. Probing found two real inefficiencies -- an idempotency KV read that is always None during linear replay, and a cacheable monotonic max-height read -- worth 0.66s together, 0.8% of the run. The rest is the write itself. It is a policy call, not an optimization. With utxo_commit and merkle also closed negative, the 20.6s arithmetic closed but its reachable portion did not. The two remaining items sit inside the FFI boundary where four marshalling attempts already measured 0.98-1.00x, and closing both perfectly reaches 1.26x, not parity. Parity needs the architectural change, not this program.
Formatting only, no behaviour change. Sweeps this branch's own diffs in g14_perf_evidence_script.rs, verify_tx.rs and apply.rs, plus the pre-existing index.rs diff in a file the branch already modifies. fmt and clippy are red on main independently of this work (main carries 3 fmt diffs and clippy errors in bitcoin-rs-chain, untouched here), so the CI UNSTABLE state on both PRs is repo baseline.
The closing paragraph said parity needs removing bitcoin::Transaction from the hot path, while this same note prices that change at 3.1s and warns against starting it. Both cannot be true. From 84.6s against 59.6s: the architectural change alone lands at 1.37x, every still-open item at 1.21x, and only doing literally everything -- including three items measured as irreducible -- reaches 1.04x. Parity is not one refactor away. Core is modestly faster across nearly every non-crypto stage at once, which is a broad engineering difference rather than a defect with a fix.
The section still stated 'The gap is 2.22x' and 'apply-only (1.60x) is the fair engine comparison' as current, below a note saying those very figures were superseded. Fifth instance of the same drift in this document. Both bitcoin-rs rows are now labelled superseded and point at the harness section. The Core row stands unchanged. The section's durable lesson -- interleave both nodes rather than comparing your best run to someone else's old one -- is kept, since that is what exposed the harness problem.
AGENTS.md requires reconciling CONCEPTS.md whenever docs/solutions/ changes; ~20 edits to the performance note landed without it. Adds three terms the note now depends on: - One-shot kernel block parse: the KernelBlock mechanism, and the costing lesson that a replacement must be priced by everything it subsumes rather than the line item that motivated it. - Parallel granularity: per-item work against dispatch decides whether a fan-out pays, with evidence in both directions, and gate on elapsed rather than the stage being targeted. - Matched-harness comparison: match every input that is not the thing under test before quoting a ratio. Reconciles two existing terms: bitcoinkernel now points at its role as the block parser, and sync regimes points at harness parity.
The goal names all performance metrics and only the processing-bound replay had been measured. All three nodes run as a P2P sync to 150k against the same fixture peer, each at its default posture (all skip historical script checks at this height, so postures match): Core 43.0s | bitcoin-rs 76.0s (1.77x slower) | GoCoin 195.8s Records the caveat that matters: a loopback fixture imposes no bandwidth limit, so this is not the download-bound regime despite being a P2P sync. The older full-tip figures were taken in the real regime and are not contradicted -- but they are months old and unverified, so they should not be quoted without re-deriving. Also flags that both bitcoin-rs runs logged quota errors at shutdown checkpoint publication.
The replay verifies every script on both sides; the P2P run skips historical scripts on both sides. Removing that shared work makes bitcoin-rs's position worse, 1.42x -> 1.77x. That is the signature of a non-crypto gap: the ~36s of secp256k1 is a measured tie, so including it drags both ratios toward 1.0. Future work should ignore script verification, which is already at parity, and target the surrounding apply path.
The previous revision divided the P2P ratio (1.77x) by the replay ratio (1.42x) and called the residual the non-crypto gap shown undiluted. That is wrong: the two runs share no baseline. The P2P sync also runs header sync, net message processing, download scheduling, staging, mempool bookkeeping and an RPC server, none of which the replay executes. The difference mixes script posture with whole subsystems. Both results stand independently and each is internally matched -- 1.42x for the apply path, 1.77x for the whole node -- but neither decomposes the other. Also strikes the recommendation to ignore script verification and target the apply path, which rested on that inference. Same mismatched-baseline error this note catches elsewhere in measurements, made here in the analysis.
The node.sync.* histograms exist and MetricsHandle::snapshot() renders them, but install_metrics ships without the Prometheus listener, so a running daemon has no external scrape path. Splitting the 1.77x whole-node gap into apply versus sync stack needs that plumbing added first. Recorded so the obstacle is not rediscovered.
The kernel-parity CI lane runs clippy with -D warnings, which is stricter than the workspace lint job and caught three regressions from the one-shot block parse: - a doc comment missing backticks around 0..150_000 - plan_block_transactions left dead in the lib build, since the kernel refactor moved production callers to plan_block_transactions_with_txids and only tests still call it - an intermediate Vec of outpoints that existed to feed into_par_iter, kept after prevout resolution went serial Gating the helper on cfg(test) and streaming the outpoints straight into the map also drops two allocations per block.
main() selected a REST or CLI source and then shadowed it with a file source, so passing --blocks-file still spawned the REST prefetch thread and immediately discarded it, starting an HTTP pipeline the run never read. Selecting once in open_block_source removes that, and takes main back under the line limit the -D warnings lane enforces.
Both cost real time this session and neither is discoverable from the code: a socket check that is correct but races the server, and a local verification sweep that cannot predict the stricter CI lanes.
The note said splitting the P2P gap needed a metrics listener. It did not: sampling utime+stime from /proc while polling height answers it. bitcoin-rs takes 76.3s wall and 318.4s CPU to sync 150k; Core takes 42.5s and 65.0s. A stalling node burns comparable CPU over more wall time, so the stall hypothesis is refuted in the opposite direction. Parallelism hides the cost on a 128-core host and would not on the 4-8 core machines most nodes run. Per-thread attribution pointed at the script pool spinning during an assume-valid sync, which would have been a one-constant fix. Sweeping pool width against both axes refutes that too: width 1 still burns 230.1s, 3.5x Core, while costing 13s of wall. Recorded as a closed negative so nobody re-runs the sweep.
rayon sizes its global pool at one worker per core. That pool runs only the short coarse jobs in apply — block txid hashing and shard commits — while script verification holds a separate pool of up to 32 threads and the node holds its own I/O threads. On a many-core host the process oversubscribes badly and the global workers spin looking for work that is not there. Loopback P2P sync to 150k, taskset -c 0-31, three interleaved pairs: one per core (32) 75.6s wall 314.4s CPU capped at 4 64.4s wall 162.4s CPU 1.17x faster and 1.94x less CPU at the same time, so this is not a wall-for-CPU trade. The compiled binary reproduces it without the env var used to find it: 64.6s wall, 160.0s CPU over three runs. The sweep is flat from 2 to 8 and climbs above it, so 4 sits in the middle of the plateau rather than on an edge. A full-verification replay of the same window is insensitive at every width (84-88s) because script verification dominates there and runs in its own pool, so the cap costs that path nothing. CPU matters independently of wall here: these benchmarks run on an idle many-core host, and 314 CPU-seconds becomes wall time on the 4-8 core machines most nodes run on.
The threshold of 4 was picked by a sweep run while the harness fetched
every block over REST from a second bitcoind competing for the same
cores. That contention inflated the serial path and made ever-finer
fan-out look free, so the sweep walked the threshold down to its floor.
Re-measured against local block files, three interleaved rounds,
taskset -c 0-31, wall and CPU together:
4 84.4s wall 946.6s CPU <- previous
16 80.1s wall 773.2s CPU
32 75.5s wall 649.6s CPU <- both optima
64 78.4s wall 533.6s CPU
128 94.0s wall 390.7s CPU
The ordering inverts once the harness stops stealing CPU: 4 is now the
worst point tested on both axes. 32 is the wall minimum and also beats
every smaller value on CPU, so it dominates rather than trades.
Compiled binary, no env var, three runs: 75.2s wall, 643.8s CPU, all
150001 blocks validated at assume_valid_height=0 to the correct tip
hash. Against the previous 84.6s that is 1.12x faster and 1.47x less
CPU.
Both thread-pool fixes measured together, three interleaved rounds each, wall and CPU captured in the same runs: replay Core 60.7s / 463.6s CPU rs 77.9s / 652.5s CPU 1.28x / 1.41x P2P Core 45.9s / 67.8s CPU rs 62.8s / 90.1s CPU 1.37x / 1.33x End to end the two constants were worth 3.53x less CPU on the P2P sync (318.4s to 90.1s) and 1.45x less on the replay, so the CPU gap against Core fell from 4.9x to 1.33x. Neither fix added an optimisation; both removed parallelism a contended harness had made look free. An isolated Core run measured 67.6s and would have flattered us. The interleaved pair puts Core at 60.7s, matching the 59.6s reference, which is why cross-session comparison stays banned in this note.
…harness MAX_SCRIPT_VERIFY_THREADS was the third constant tuned on the contended REST harness and on wall alone, the pattern that made the other two wrong. Re-measured against local block files at the corrected threshold, two rounds, taskset -c 0-31: 8 threads 130.6s wall 474.2s CPU 16 threads 97.7s wall 521.3s CPU 24 threads 83.7s wall 590.9s CPU 32 threads 78.4s wall 652.3s CPU The value survives. Wall falls monotonically with width while CPU rises sublinearly, so unlike the threshold this genuinely trades, and 32 buys 1.67x the wall of 8 for 1.38x the CPU. No code change: only the rationale, which cited 157.8s and 173.1s from the harness now known to have been stealing CPU from the node.
perf: parse each block once, drop unpaid fan-outs, match the benchmark harness (4.6x)
![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 |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50d0a50411
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+434
to
+439
| bitcoin_core_elapsed_seconds = run_criterion_command( | ||
| bitcoin_core_command, | ||
| bitcoin_core_raw_output, | ||
| BITCOIN_CORE_CRITERION_BENCHMARK_ID, | ||
| "--bitcoin-core-command", | ||
| ) |
There was a problem hiding this comment.
Unlike the bitcoin-rs command, bitcoin_core_command is neither restricted to run-g14-bitcoin-core-mainnet-ibd.sh nor checked for the requested height/hash flags. Any misconfigured command that exits successfully and prints a matching Criterion line is therefore accepted, after which this script itself appends the trusted completion proof; the producer's reference-node checks cannot establish that the measured command performed an IBD. Validate and bind the Core adapter before running it rather than synthesizing proof for arbitrary output.
Useful? React with 👍 / 👎.
Comment on lines
+505
to
+508
| let raw_block: bytes::Bytes = provided_serialized | ||
| .clone() | ||
| .unwrap_or_else(|| bitcoin::consensus::encode::serialize(block).into()); | ||
| let kernel_block = bitcoin_rs_consensus::kernel::KernelBlock::parse(&raw_block) |
There was a problem hiding this comment.
apply_block_with_serialized is public, but the supplied bytes are parsed by the kernel without being compared to block in release builds. A caller can provide a same-sized serialization with the same stripped transactions but different witness data: txids and the transaction-count check still match, kernel script checks validate the supplied witness, while witness-commitment checks and UTXO changes operate on the separate decoded block. This can accept and commit a block whose decoded inputs were never script-verified; reject mismatched serialization before using it for kernel validation.
Useful? React with 👍 / 👎.
Comment on lines
+69
to
+71
| if let Some(tree) = &self.block_tree { | ||
| let active_hash = tree.read().active_node_at_height(height)?.hash; | ||
| return self.resolve_block_by_hash(height, active_hash); |
There was a problem hiding this comment.
When header sync selects a higher-work fork before block application catches up, active_node_at_height follows the best header tip, while the txindex rows and UTXO state still describe the old applied-tip ancestry. The Electrum history reader wires this NodeBlockSource directly into transaction/history and merkle-proof resolution, so lookups can become missing or resolve against an unapplied fork during that interval. Carry the applied-tip handle into this source and resolve heights from that tip, as the RPC context already does.
Useful? React with 👍 / 👎.
Comment on lines
742
to
+750
| Err(error) => { | ||
| failed = failed.saturating_add(1); | ||
| failed_hash = Some(expected_hash); | ||
| failed_hash = Some(drained_block.hash); | ||
| tracing::warn!( | ||
| %expected_hash, | ||
| %drained_block.hash, | ||
| %error, | ||
| "block sync: failed to apply buffered block" | ||
| ); | ||
| self.block_stager.lock().restore_many(drained); |
There was a problem hiding this comment.
When a requested block reaches staging but fails consensus application, this branch discards only the hash and the stager has already dropped the delivering PeerSource; the download window then simply requeues the height without penalizing that peer. A malicious peer can repeatedly return a block with the requested header hash but an invalid merkle body and remain eligible to receive the same or subsequent requests, indefinitely stalling the apply frontier. Preserve the source through staging and disconnect or quarantine it when the failure is attributable to an invalid peer block, while retaining retry behavior for local/storage failures.
Useful? React with 👍 / 👎.
Comment on lines
57
to
62
| prune_prefixed_rows( | ||
| &*self.store, | ||
| BLOCK_DATA_CF, | ||
| BLOCK_BODY_PREFIX_BYTES, | ||
| current_tip_height, | ||
| self.policy, |
There was a problem hiding this comment.
BlockPruner::prune_step still uses the old value-size row pruner even though block-body values are now only 16-byte flat-file positions. With a nonzero size target, the measured total remains far below the actual block-file footprint and pruning may never start; with a zero target it deletes index rows but never removes the referenced files, making bodies inaccessible without reclaiming disk. Route this public pruner through the flat-file-aware staging and reclamation path, accounting for actual file sizes.
Useful? React with 👍 / 👎.
Comment on lines
+285
to
+288
| let path = block_file_path(&self.blocks_dir, file_no); | ||
| match fs::remove_file(path) { | ||
| Ok(()) => Ok(true), | ||
| Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(false), |
There was a problem hiding this comment.
On POSIX filesystems, a successful remove_file is not crash-durable until the containing directory is synced. The caller subsequently commits deletion of the per-file metadata, so a crash after that commit can resurrect the flat file while leaving no metadata row for a later prune to discover, producing a permanent disk leak. Sync blocks_dir after successful removals and before committing metadata deletion.
Useful? React with 👍 / 👎.
Comment on lines
+150
to
+153
| let block_stats = { | ||
| let blocks_guard = ctx.blocks.read(); | ||
| fold_block_records(&blocks_guard, applied_height, Some(lowest_window_height)) | ||
| }; |
There was a problem hiding this comment.
After a complete checkpoint restore, NodeState restores the applied tip and coin statistics but starts ctx.blocks empty, so this fold sees only blocks applied since the current process started. A restart at height N therefore makes getchaintxstats return zero transaction counts and timestamps until another block arrives, and subsequent responses still omit all restored history. Seed durable per-height metadata or use the restored statistics for totals and load the requested window from the block index.
Useful? React with 👍 / 👎.
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.
No description provided.