perf(rpc): read the chain-info figures off the block log instead of folding it - #89
Open
rabbitson87 wants to merge 4 commits into
Open
perf(rpc): read the chain-info figures off the block log instead of folding it#89rabbitson87 wants to merge 4 commits into
rabbitson87 wants to merge 4 commits into
Conversation
Neither number existed before. Both are measured at the scale the code actually runs at, because both costs are superlinear and a small-fixture figure would have understated them. `chaininfo` folds the block-record log on `getblockchaininfo` and `getchaintxstats`. At a 963,124-record mainnet tip that is 7.9 ms and 7.2 ms per call, held under the log's read lock that block application needs to push to. `getblockchaininfo` walks the whole log to produce one scalar, `size_on_disk`. `pareto` fills the mempool priority index. `ParetoFront::insert` does a linear `remove` and then re-sorts the entire index, so a fill is quadratic: 4.9 ms at 1,000 entries, 4.57 s at 50,000 — a measured exponent of 2.05 across the last leg, or 91 us per transaction at 50,000 against 4.9 us at 1,000. No behaviour changes here. These are the measurements the fixes will be judged against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…olding it `Context::blocks` holds one record per applied block and grows for the life of the process — ~963k entries on a mainnet node. `getblockchaininfo` and `getchaintxstats` both walked all of it to produce five scalars, and `getblockchaininfo` used exactly one of them: it folded 963k records to report `size_on_disk` and discarded the rest. The walk ran under the log read lock, which is the lock `apply_block` takes to append the record for the block it just connected, so the call stalled block application for its duration and got slower with every block. `BlockLog` now maintains what the fold used to compute. `total_body_size` is a running sum and answers `size_on_disk` outright. `cumulative_tx_count[i]` is the sum over `records[..=i]` and answers both transaction counts as differences across two boundaries. It is a type rather than a `Vec<BlockRecord>` with totals kept beside it because the log is appended from `apply`, from `Context::add_block` and from tests; a total any of those could forget to update is a total that will drift. Reads are unchanged - it derefs to `[BlockRecord]`. `chain_stats` replaces the fold for `getchaintxstats`. The log is appended in height order and only ever popped from the tail, so it binary-searches the three boundaries it needs - the same property `Context::block_at_height` already relies on. Only the caller's window is then walked, and only for `earliest_window_time`: block timestamps are not monotonic, so no prefix sum can answer a minimum over them. Measured over one fixture in one process, `before_fold` against `after_indexed`: 10,000 records: 19.32 us -> 3.83 us 5.0x 100,000 records: 552.70 us -> 3.94 us 140x 500,000 records: 4.214 ms -> 4.24 us 993x 963,124 records: 7.450 ms -> 3.77 us 1,977x End to end the dispatch is flat: `getblockchaininfo` 2.79 us and `getchaintxstats` 4.43 us at 963k records, against 2.66 us and 4.88 us at 10k. Prefix sums rather than one more running total because the benchmark caught the cheaper version being a cliff. A single total answers `txcount` only when the applied tip is the log last record; anywhere else it subtracts the records above the tip, and with no applied tip that tail is the whole log. `getchaintxstats` measured 6.23 ms while its own reader measured 4.79 us. Prefix sums cost 8 bytes per record, ~7.7 MB at a mainnet tip against the ~254 MB the records occupy. `fold_block_records` is retained whole as the oracle and as the benchmark before arm. `chain_stats_matches_the_fold_it_replaced` sweeps every applied height against every window length over a log with a duplicate height and a backwards timestamp. Ten mutations, all killed; see docs/benchmarks/chain-info-fold.md, which also records that the first audit run misreported every kill as an invalid mutation because the harness could not tell a red test from a broken build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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 |
`Context::record_for_hash` asks the block tree for the hash, gets the height back, and then scanned the block-record log linearly for the matching `(hash, height)` pair. The height was already in hand and the log is ordered by height, so the scan was avoidable work proportional to chain length - on the path under `getblock`, `getblockheader`, `getblockstats`, `getrawtransaction` with a blockhash, the REST block endpoint, and `gettxoutproof`s explicit-hash path. `verifychain` calls it once per block it checks, so that RPC was quadratic. This is a hotter path than the chain-info fold: `getblockchaininfo` is a status call, `getblockheader` is what a tip-following client polls. Measured over one fixture in one process, `before_scan` against `after_search`: 10,000 records, hash at tip: 14.59 us -> 37.4 ns 390x 100,000 records, hash at tip: 433.40 us -> 38.5 ns 11,265x 500,000 records, hash at tip: 3.850 ms -> 44.0 ns 87,551x 963,124 records, hash at tip: 6.263 ms -> 42.2 ns 148,548x 963,124 records, hash in middle: 3.448 ms -> 46.9 ns 73,548x The new arm is flat at 37-47 ns across 96x the records: a binary search is ~20 steps at a mainnet tip. Both lookup positions are reported because measuring one would flatter the scan - the tip is its worst case, the middle costs it half. `BlockLog::record_at_height_hash` and `record_at_height` are not new code. `block_source.rs` already had both, private, over `&[BlockRecord]`; they now live on `BlockLog` beside the data and the node calls them, so this deletes a duplicate implementation rather than adding one. `record_at_height` keeps the direct-index shortcut, accepted only when the record and its predecessor agree, so a log with gaps or duplicate heights falls through to the search rather than answering with the wrong record. Step 2 of `record_for_hash` stays linear on purpose: when the tree has no node for the hash there is no height, so there is nothing to search on. Seven mutations, all killed. Two survived the first pass and both were the fixture's fault: it started at height 0, where the predecessor check can never matter, and nothing exercised `block_by_height` with no applied tip. See docs/benchmarks/block-record-lookup.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing it" This reverts 539d8da. The change duplicates #87, which was opened two days earlier and makes the same argument: `record_for_hash` has the height from the block tree, the log is ordered by height, so the lookup is a search. #87 also covers `Context::block_hash_at_height`, which 539d8da missed. What 539d8da had and #87 does not - a benchmark, the sweep-against-a-scan equivalence tests, `block_by_height_without_an_applied_tip_reads_the_log`, and five further mutations - moves to #87 rather than being dropped. This branch keeps only the chain-info fold, which nothing else covers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
getblockchaininfofolded ~963k block records to report one number(
size_on_disk) and threw the rest of the fold away.getchaintxstatsfoldedthe same log for four more.
The walk ran under the block log's read lock — the lock
apply_blocktakesto append the record for a block it has just connected. So the call stalled
block application for its duration, and got slower with every block.
Measured
Both arms of the refactor set in one Criterion run over one fixture:
before_foldafter_indexedEnd to end through
Handler::dispatch, now flat in chain length:getblockchaininfogetchaintxstatsThe old arm is worse than linear — 96.3x the records cost 385.6x the time, an
exponent of 1.30. A fold over 963k records touches ~100 MB, so it leaves
cache long before it leaves the log.
What replaced it
BlockLogmaintains what the fold used to compute.total_body_sizeis arunning sum and answers
size_on_diskoutright.cumulative_tx_count[i]— thesum over
records[..=i]— answers both transaction counts as differences acrosstwo boundaries.
A type rather than a
Vec<BlockRecord>with totals beside it: the log isappended from
apply, fromContext::add_blockand from tests, and a total anyof those could forget to update is a total that will drift. Reads are unchanged —
it derefs to
[BlockRecord], so every existing slice, index, iterator and binarysearch over the log still compiles and still means the same thing.
chain_statsreplaces the fold forgetchaintxstats. The log is appended inheight order and only ever popped from the tail, so it binary-searches its three
boundaries — the same property
Context::block_at_heightalready relies on. Onlythe caller's window is then walked, and only for
earliest_window_time: blocktimestamps are not monotonic, so no prefix sum can answer a minimum over them.
The benchmark caught the cheaper version being a cliff
The first implementation kept a single running
total_tx_countand subtractedthe records above the applied tip. That answers
txcountcorrectly when theapplied tip is the log's last record — which is where a production node keeps
it, so it would have been free in practice.
With no applied tip, the "tail above the tip" is the entire log.
getchaintxstatsmeasured 6.23 ms while the reader it was supposedly usingmeasured 4.79 µs. A cliff, not a bound. Prefix sums answer any prefix in constant
time, for 8 bytes per record — ~7.7 MB at a mainnet tip against the ~254 MB the
records already occupy.
The fixture was wrong too, and is fixed: it now publishes an applied tip at the
end of the log, which is the shape a node is actually in.
Correctness
fold_block_recordsis retained whole as the oracle and the benchmark'sbeforearm. It makes no assumption about the log's ordering, which is the point — the
replacement binary-searches, and an oracle sharing that assumption could not
catch it being wrong.
chain_stats_matches_the_fold_it_replacedsweeps every applied height from 0to past the end of the log against every window length from 0 to past the
whole log, comparing all four figures each time. The fixture records height 3
twice, as a reorg leaves it, and its timestamps dip at height 5.
Ten mutations, all killed, baseline and restored green.
The first audit run reported every one of them as an invalid mutation. The
harness decided a run was a broken build by grepping for
^error, andcargo testends a failing run witherror: test failed, to rerun pass ...— soevery kill was recorded as a build failure. An audit harness that cannot tell a
red test from a broken build reports the same thing for both, and the thing it
reports is not "killed".
The two
popmutations also died for the wrong reason at first: dropping theprefix
popleft the prefix vector longer than the records, and a clamp happenedto read out of the stale tail.
tx_count_beforenow asserts the two areparallel, so the mutation dies on what it actually broke.
Verification
cargo test -p bitcoin-rs-rpc -p bitcoin-rs-node --no-default-features --features bitcoin-rs-node/fjall --no-fail-fast— green (471 node lib + 176 rpc lib + integration suites)cargo fmt --check— clean (also fixes a pre-existing violation incrates/mempool/benches/pareto.rs)cargo clippy -p bitcoin-rs-rpc -p bitcoin-rs-node ... -- -D warnings— cleanNot in this change
Context::record_for_hashstill scans the log linearly, undergetblockand
getblockheader— a hotter path than either RPC here. It has a height fromthe block tree and should binary-search the same way. Separate change.
gettxoutproof's fallback still clones the whole log (tx.rs:175,~160 MB at tip). Belongs to perf(rpc): answer gettxoutproof from the txindex instead of scanning the chain #85.
size_on_diskstill reports recorded block sizes, not disk usage. That iswhat the fold reported; changing what the field means is a different question.
Full write-up:
docs/benchmarks/chain-info-fold.md.Second commit: block-record lookup (
539d8da)Auditing the first commit surfaced the same shape one level down, on a hotter
path.
Context::record_for_hashasks the block tree for the hash, gets theheight back, and then scanned the log linearly for the matching
(hash, height)pair — with the height in hand, over a log ordered by height.That is under
getblock,getblockheader,getblockstats,getrawtransactionwith a blockhash, the REST block endpoint, and
gettxoutproof's explicit-hashpath.
verifychaincalls it once per block it checks, so that RPC wasquadratic in chain length.
getblockchaininfois a status call;getblockheaderis what a tip-following client polls.before_scanafter_searchFlat at 37–47 ns across 96x the records: ~20 binary-search steps at a mainnet
tip. Both lookup positions are reported because measuring one would flatter the
scan — the tip is its worst case, the middle costs it half.
BlockLog::record_at_height_hashandrecord_at_heightare not new code.block_source.rsalready had both, private, over&[BlockRecord]. They now liveon
BlockLogbeside the data and the node calls them, so this deletes aduplicate implementation rather than adding one.
Step 2 of
record_for_hashstays linear on purpose: when the tree has no nodefor the hash there is no height, so there is nothing to search on.
Two mutations survived the first pass, and both were the fixture's fault
The fixture started at height 0. The predecessor check in the direct-index
shortcut only matters when index
hholds a record at heighththat is notthe first at that height — and a log starting at zero can never be in that state.
It now starts at height 1, which puts a duplicate at index 3 and the run head at
index 2. A fixture that cannot reach the state a check defends against tests
the check by not reaching it.
Nothing exercised
block_by_heightwith no applied tip. Replacing its wholebody with "the last record in the log" turned nothing red. The new test covering
it is among the killers for four of the seven mutations.
Seven mutations, all killed, baseline and restored green. One invalid mutation is
recorded as invalid: an anchor stopped matching after
cargo fmtrewrapped theexpression.
Full write-up:
docs/benchmarks/block-record-lookup.md.Correction: the second commit was a duplicate and is reverted
The block-record lookup change (
539d8da) duplicated #87, opened two daysearlier with the same argument:
record_for_hashhas the height from the blocktree, the log is ordered by height, so the lookup is a search. #87 also covers
Context::block_hash_at_height, which the duplicate missed.539d8dais reverted here. What it had that #87 did not — the benchmark, thesweep-against-a-scan equivalence tests,
block_by_height_without_an_applied_tip_reads_the_log,and five further mutations — is ported to #87 rather than dropped.
The
gettxoutproofwhole-log copy was the same story: #85's branch alreadywalks the log by index, with the same reasoning, so nothing was added here for
it either.
This PR is now the chain-info fold only, which nothing else covers.