Skip to content

Commit 539d8da

Browse files
rabbitson87claude
andcommitted
perf(rpc): search the block log for a record instead of scanning it
`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>
1 parent ae689f5 commit 539d8da

5 files changed

Lines changed: 455 additions & 52 deletions

File tree

crates/node/src/block_source.rs

Lines changed: 5 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use bitcoin::hex::FromHex as _;
1414
use bitcoin_rs_chain::{BlockTree, NodeId, TipSnapshot};
1515
use bitcoin_rs_index::BlockSource;
1616
use bitcoin_rs_primitives::Hash256;
17-
use bitcoin_rs_rpc::{BlockBodySource, BlockLog, BlockRecord};
17+
use bitcoin_rs_rpc::{BlockBodySource, BlockLog};
1818
use parking_lot::RwLock;
1919

2020
/// Reads decoded Bitcoin blocks from the shared in-memory log.
@@ -82,7 +82,7 @@ impl BlockSource for NodeBlockSource {
8282
tree.read().active_node_at_height(height)?.hash
8383
} else {
8484
let guard = self.blocks.read();
85-
record_at_height(&guard, height)?.hash
85+
guard.record_at_height(height)?.hash
8686
};
8787
self.resolve_block_by_hash(height, active_hash)
8888
}
@@ -98,7 +98,7 @@ impl BlockSource for NodeBlockSource {
9898
tree.read().active_node_at_height(height)?.hash
9999
} else {
100100
let guard = self.blocks.read();
101-
record_at_height(&guard, height)?.hash
101+
guard.record_at_height(height)?.hash
102102
};
103103
source.block_body_range(height, hash, offset, len)
104104
}
@@ -138,7 +138,7 @@ impl NodeBlockSource {
138138
fn cached_body_bytes(&self, height: u32, hash: Hash256) -> Option<Vec<u8>> {
139139
let block_hex = {
140140
let guard = self.blocks.read();
141-
let record = record_at_height_hash(&guard, height, hash)?;
141+
let record = guard.record_at_height_hash(height, hash)?;
142142
(!record.block_hex.is_empty()).then(|| record.block_hex.clone())
143143
}?;
144144
Vec::<u8>::from_hex(&block_hex).ok()
@@ -298,47 +298,6 @@ fn serialized_header(
298298
})
299299
}
300300

301-
fn record_at_height(records: &[BlockRecord], height: u32) -> Option<&BlockRecord> {
302-
if let Ok(index) = usize::try_from(height)
303-
&& let Some(record) = records.get(index)
304-
&& record.height == height
305-
&& index
306-
.checked_sub(1)
307-
.and_then(|previous| records.get(previous))
308-
.is_none_or(|previous| previous.height < height)
309-
{
310-
return Some(record);
311-
}
312-
313-
let mut index = records
314-
.binary_search_by_key(&height, |record| record.height)
315-
.ok()?;
316-
while index > 0 && records[index.saturating_sub(1)].height == height {
317-
index = index.saturating_sub(1);
318-
}
319-
records.get(index)
320-
}
321-
322-
fn record_at_height_hash(
323-
records: &[BlockRecord],
324-
height: u32,
325-
hash: Hash256,
326-
) -> Option<&BlockRecord> {
327-
let mut index = records
328-
.binary_search_by_key(&height, |record| record.height)
329-
.ok()?;
330-
while index > 0 && records[index.saturating_sub(1)].height == height {
331-
index = index.saturating_sub(1);
332-
}
333-
while index < records.len() && records[index].height == height {
334-
if records[index].hash == hash {
335-
return Some(&records[index]);
336-
}
337-
index += 1;
338-
}
339-
None
340-
}
341-
342301
#[cfg(test)]
343302
mod tests {
344303
use super::*;
@@ -347,6 +306,7 @@ mod tests {
347306
use bitcoin::consensus::encode::serialize;
348307
use bitcoin_rs_chain::NodeStatus;
349308
use bitcoin_rs_primitives::Hash256;
309+
use bitcoin_rs_rpc::BlockRecord;
350310
use std::error::Error;
351311

352312
type TestResult = Result<(), Box<dyn Error>>;

crates/rpc/Cargo.toml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,3 +52,7 @@ criterion.workspace = true
5252
[[bench]]
5353
name = "chaininfo"
5454
harness = false
55+
56+
[[bench]]
57+
name = "blocklookup"
58+
harness = false

crates/rpc/benches/blocklookup.rs

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
//! Block-record lookup cost under `getblock` and `getblockheader`.
2+
//!
3+
//! `Context::record_for_hash` resolves a hash to a block record. Step 1 asks the
4+
//! block tree, which answers with the height — and then scans the block-record
5+
//! log linearly for the matching `(hash, height)` pair, even though the log is
6+
//! ordered by height and the height is in hand.
7+
//!
8+
//! That log grows one entry per block forever, so `getblock`,
9+
//! `getblockheader`, `getblockstats`, `getrawtransaction` with a blockhash, the
10+
//! REST block endpoint and `gettxoutproof`'s explicit-hash path each pay a walk
11+
//! proportional to chain length. `verifychain` pays one per block it checks.
12+
//!
13+
//! Both arms of the refactor set run here over one fixture in one process, so
14+
//! the ratio cannot be confounded by the rebuild and baseline drift recorded in
15+
//! `docs/solutions/best-practices/criterion-bench-trust-rebuild-drift-baselines-allocator.md`.
16+
//! `before_scan` is the linear find that was there; `after_search` is the
17+
//! height-keyed binary search plus duplicate-height walk that replaced it.
18+
//!
19+
//! Two lookup positions are measured. A hash at the *end* of the log is the
20+
//! best case for a forward scan and the one a tip-following client asks for; a
21+
//! hash in the *middle* is what a wallet rescanning history asks for. Measuring
22+
//! only the tip would report the linear scan at its most flattering.
23+
// PERF: Criterion emits public harness items whose docs are irrelevant here.
24+
#![allow(missing_docs)]
25+
// A fixture that fails to build has no meaningful degraded mode.
26+
#![allow(clippy::expect_used)]
27+
28+
use std::hint::black_box;
29+
use std::sync::Arc;
30+
31+
use bitcoin_rs_primitives::Hash256;
32+
use bitcoin_rs_rpc::{BlockLog, BlockRecord, Context};
33+
use criterion::{Criterion, criterion_group, criterion_main};
34+
35+
/// Log lengths to measure. The last is a mainnet tip at the time of writing.
36+
const LOG_LENGTHS: [u32; 4] = [10_000, 100_000, 500_000, 963_124];
37+
38+
fn hash_for(height: u32) -> Hash256 {
39+
let mut hash = [0_u8; 32];
40+
hash[..4].copy_from_slice(&height.to_le_bytes());
41+
Hash256::from_le_bytes(&hash)
42+
}
43+
44+
fn log_with_records(count: u32) -> Arc<Context> {
45+
let ctx = Arc::new(Context::new());
46+
{
47+
let mut blocks = ctx.blocks.write();
48+
blocks.reserve(count as usize);
49+
for height in 0..count {
50+
let mut record = BlockRecord::synthetic(height, hash_for(height));
51+
record.body_size = 1_000_000 + (height as usize % 400_000);
52+
record.tx_count = 1 + (height as usize % 3_000);
53+
record.time = 1_231_006_505 + height * 600;
54+
blocks.push(record);
55+
}
56+
}
57+
ctx
58+
}
59+
60+
/// The scan that was in `record_for_hash`, kept here as the `before` arm and as
61+
/// the oracle the search is checked against.
62+
///
63+
/// Written out rather than called through the crate: it is three lines, and an
64+
/// oracle that shares code with the implementation cannot disagree with it.
65+
fn scan_for(log: &BlockLog, height: u32, hash: Hash256) -> Option<&BlockRecord> {
66+
log.iter()
67+
.find(|candidate| candidate.hash == hash && candidate.height == height)
68+
}
69+
70+
fn bench_lookup(c: &mut Criterion) {
71+
let mut group = c.benchmark_group("block_record_lookup");
72+
group.sample_size(20);
73+
74+
for count in LOG_LENGTHS {
75+
let ctx = log_with_records(count);
76+
let log = ctx.blocks.read();
77+
78+
for (label, height) in [("tip", count.saturating_sub(1)), ("middle", count / 2)] {
79+
let hash = hash_for(height);
80+
81+
// Prove both arms find the same record before timing either. An arm
82+
// that found nothing would be timed as a spectacular, empty win.
83+
assert_eq!(
84+
scan_for(&log, height, hash).map(|record| record.height),
85+
log.record_at_height_hash(height, hash)
86+
.map(|record| record.height),
87+
"the arms disagree at {label}; the benchmark would be meaningless"
88+
);
89+
90+
group.bench_function(format!("before_scan/{label}/{count}"), |b| {
91+
b.iter(|| black_box(scan_for(&log, height, hash).map(|record| record.time)));
92+
});
93+
group.bench_function(format!("after_search/{label}/{count}"), |b| {
94+
b.iter(|| {
95+
black_box(
96+
log.record_at_height_hash(height, hash)
97+
.map(|record| record.time),
98+
)
99+
});
100+
});
101+
}
102+
}
103+
104+
group.finish();
105+
}
106+
107+
criterion_group! {
108+
name = benches;
109+
config = Criterion::default();
110+
targets = bench_lookup
111+
}
112+
criterion_main!(benches);

0 commit comments

Comments
 (0)