Skip to content

perf(rpc): resolve block records by binary search, not by walking the chain - #87

Open
rabbitson87 wants to merge 3 commits into
mainfrom
perf/rpc-record-lookup
Open

perf(rpc): resolve block records by binary search, not by walking the chain#87
rabbitson87 wants to merge 3 commits into
mainfrom
perf/rpc-record-lookup

Conversation

@rabbitson87

@rabbitson87 rabbitson87 commented Aug 19, 2026

Copy link
Copy Markdown
Member

Found while auditing #85 and #86, and larger than either of them.

The problem

Context::record_for_hash walked every record in the block log to find one. The log holds one entry per applied block and nothing removes it, so at a mainnet tip that is ~963k comparisons per call — and getblock, getblockheader and the index read path's block_at_height all land there.

It is the same shape as the gettxoutproof scan that #85 removes, on a hotter path.

The fix

Step 1 already knows the height, because the block tree gave it. That makes the lookup a binary search over a height-ordered log.

The log is append-only in height order — add_block pushes, and the only removal is the tail pop a disconnect performs on the applied tip. The codebase already relied on this: crates/node/src/block_source.rs had record_at_height and record_at_height_hash doing exactly this search, with tests.

Rather than copy them, they move to crates/rpc beside BlockRecord and node imports them — one implementation for both, and 40 lines of duplicate deleted. Context::block_by_height and Context::block_hash_at_height were scanning the same way and now use them too.

Step 2 stays linear on purpose. Without the tree there is no height to search on, and a hash-keyed index would have to be maintained for every block to serve a path only legacy state reaches. The comment says so.

Tests

Three cover what the rpc side never had:

  • a hash is matched within a duplicate-height run, rather than the run's first record being assumed — a reorg leaves the losing block in the log beside the winner
  • a hash absent from that run does not resolve to a sibling
  • a log that does not start at height zero still resolves

The duplicate-height test is shaped deliberately: heights [1, 1, 2] put the dense fast path's index straight onto the second duplicate, where the height check alone would accept it and only the preceding-record guard rejects it.

An earlier version of that test used heights starting at zero. It passed — and the mutation audit showed it passed with the guard removed, because an index past the end of a 3-record log never enters the fast path at all. The test was not testing what its name claimed. This is the shape that does.

Mutation audit

Mutation rpc node
baseline 177 passed 442 passed
ignore the hash, return the first record at the height 2 failed — the new test and the pre-existing block_by_height_prefers_tree_identity_over_stale_cache 1 failed
dense fast path without its preceding-record guard 1 failed 1 failed
restored 177 passed 442 passed

An earlier run of this audit is not reported here because it was invalid, not because it passed: a second audit was editing the same file concurrently, and its baseline disagreed with its own restored run. Re-run serially.

Not in this PR

BlockRecord still carries an 80-byte header the block tree also holds, and the tree itself never drops a node. Both are per-block memory costs; separate work.

🤖 Generated with Claude Code


Update: the measurement this PR was missing

This PR argued for a change without a number. It has one now — and the audit was
re-run at seven mutations instead of two.

crates/rpc/benches/blocklookup.rs, both arms over one fixture in one process:

Records Hash at before_scan after_search ratio
10,000 tip 12.17 µs 39.6 ns 307x
100,000 tip 415.9 µs 50.2 ns 8,285x
500,000 tip 3.737 ms 46.1 ns 81,092x
963,124 tip 7.707 ms 48.3 ns 159,569x
963,124 middle 3.328 ms 48.6 ns 68,528x

Flat at 37–50 ns across 96x the records: ~20 binary-search steps at a mainnet
tip. Two lookup positions are measured because reporting one would flatter
the scan — the tip is its worst case, the middle costs it half.

The linear scan is written out in the benchmark and in the new tests rather than
called through the crate: it is two lines, and an oracle that shares code with
the implementation cannot disagree with it.

Three more tests

Two sweep the search against that scan over every height in and around the
fixture and every hash in it — including hashes at the wrong height, which
must find nothing. A search is wrong at its boundaries, and a test that picks one
pair picks whether it visits them.

The third closes a real gap: block_by_height with no applied tip was
uncovered.
Replacing that fallback's whole body with "the last record in the
log" turned nothing red. block_by_height_without_an_applied_tip_reads_the_log
is the only test that kills that mutation.

The shared fixture starts at height 1 rather than 0, and that is load-bearing:
the preceding-record guard only matters when index h holds a record at height
h that is not the first at that height, and a log starting at zero can never
be in that state.

Re-audited

Mutation Result
the search stops at the first record of the height run 3 tests failed
record_at_height_hash skips the rewind to the run head 3 tests failed
record_at_height skips the rewind to the run head 3 tests failed
record_at_height trusts the direct index unconditionally 4 tests failed
record_at_height drops only the preceding-record guard 3 tests failed
record_for_hash ignores the hash and takes the run head 1 test failed
block_by_height without a tip answers the last record 1 test failed

Baseline and restored: 181 passed, 0 failed.

Provenance

This change was written a second time on perf/chaininfo-fold (#89) before this
PR was noticed. That copy is reverted there; the measurement, the tests and the
audit from it are ported here. #89 keeps only the chain-info fold.

Full write-up: docs/benchmarks/block-record-lookup.md.

… chain

Context::record_for_hash walked every record in the block log to find one. The
log holds one entry per applied block and nothing removes it, so at a mainnet tip
that is ~963k comparisons per call -- and getblock, getblockheader and the index
read path all land there. It is the same shape as the gettxoutproof scan, on a
hotter path.

Step 1 already knows the height, because the block tree gave it. That makes the
lookup a binary search over a height-ordered log. The log is append-only in
height order -- add_block pushes, and the only removal is the tail pop a
disconnect performs on the applied tip -- and the codebase already relied on that:
crates/node had record_at_height and record_at_height_hash doing exactly this.

Rather than copy them, they move to crates/rpc beside BlockRecord and node imports
them, so one implementation serves both and forty lines of duplicate go away.
Context::block_by_height and Context::block_hash_at_height were scanning the same
way and now use them too.

Step 2 stays linear on purpose. Without the tree there is no height to search on,
and a hash-keyed index would have to be maintained for every block to serve a path
only legacy state reaches. The comment says so.

Three tests cover the parts the rpc side never had: that a hash is matched within
a duplicate-height run rather than the run's first record being assumed, that a
hash absent from the run does not resolve to a sibling, and that a log which does
not start at height zero still resolves.

The duplicate-height test is shaped deliberately. Heights [1, 1, 2] put the dense
fast path's index straight onto the second duplicate, where the height check alone
would accept it and only the preceding-record guard rejects it. An earlier version
used heights starting at zero, passed, and did not touch the guard at all -- the
mutation audit caught that the test was not testing what its name claimed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 02a71dd6-c2c0-40be-982e-d9da9708861c


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…factor

Both conflicts were additive, not semantic: the crate root export list gained
entries on each side, and the block-source file gained a BlockTreeAdapter impl on
main where this branch had deleted the duplicate lookup helpers. The merge
resurrected those helpers alongside the import that replaced them, so they are
deleted again here.

The premise still holds after the refactor: record_for_hash on the new main still
walks the log linearly in all four places this branch replaces.

Re-ran the mutation audit rather than trusting the tests for staying green
through a merge that moved the code they pin. Both mutations still turn the
intended tests red. One change is worth noting: under the hash-ignoring mutation
node used to fail a test too, and no longer does -- main routing that path through
BlockTreeAdapter cost node its detector. The rpc side still catches it twice.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rabbitson87 added a commit that referenced this pull request Aug 21, 2026
…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>
The PR claimed a performance win with no number attached. This adds the
refactor-set benchmark, both arms over one fixture in one process.

  10,000 records, hash at tip:     12.17 us -> 39.6 ns       307x
  100,000 records, hash at tip:   415.90 us -> 50.2 ns     8,285x
  500,000 records, hash at tip:     3.737 ms -> 46.1 ns   81,092x
  963,124 records, hash at tip:     7.707 ms -> 48.3 ns  159,569x
  963,124 records, hash in middle:  3.328 ms -> 48.6 ns   68,528x

The new arm is flat at 37-50 ns across 96x the records: a binary search is ~20
steps at a mainnet tip. Two lookup positions are measured because reporting one
would flatter the scan - the tip is its worst case, the middle costs it half.

The linear scan is written out in the benchmark and in the new tests rather than
called through the crate: it is two lines, and an oracle that shares code with
the implementation cannot disagree with it.

Three tests are added on top of the three already here. Two sweep the search
against that scan over every height in and around the fixture and every hash in
it - a search is wrong at its boundaries, and a test that picks one pair picks
whether it visits them. The third covers `block_by_height` with no applied tip,
which was a gap: replacing that fallback's whole body with "the last record in
the log" turned nothing red, and the new test is the only one that kills it.

The shared fixture starts at height 1 rather than 0. That is load-bearing: the
preceding-record guard in the direct-index fast path only matters when index `h`
holds a record at height `h` that is not the first at that height, and a log
starting at zero can never be in that state.

Re-audited at seven mutations, all killed, baseline and restored green. Write-up
in docs/benchmarks/block-record-lookup.md.

Ported from perf/chaininfo-fold, where the same change was written a second time
before this PR was noticed. That copy is reverted; this is the one that stays.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant