perf(rpc): answer gettxoutproof from the txindex instead of scanning the chain - #85
perf(rpc): answer gettxoutproof from the txindex instead of scanning the chain#85rabbitson87 wants to merge 5 commits into
Conversation
…the chain Called without a block hash, the handler deep-copied every BlockRecord, then loaded, deserialized and hashed every block on the chain to answer one call. At tip that is roughly a million block loads, almost all of them discarded -- unbounded work for one authenticated RPC call, which stalls the node and evicts everything else from cache while it runs. Context.indexer already knows which block confirms a txid. Resolve the confirming height, build the proof from that block alone, and take the same route Bitcoin Core does: it requires the block hash unless txindex is enabled. The scan is kept whole as proof_from_records. It is the fallback whenever the index cannot answer -- no indexer, an unresolved or stale row, a pruned body, or a candidate block that does not hold every wanted txid -- and it is the oracle the equivalence tests compare against. That last fallback is not belt and braces: BIP30 duplicate coinbase txids mean a txid can confirm in more than one block, so a block chosen from a single txid is a candidate, never a verdict. IndexerLike gains resolve_transaction_height, defaulting to Ok(None), so every existing implementation keeps compiling and no caller may drop its fallback. Measured on an idle machine, both arms in one Criterion run over one fixture: 21.118 ms -> 32.983 us for the scan worst case, and within noise of each other for its best case, because the index arm is flat at 28-33 us regardless of position. Extrapolating the scan's 10.56 us per record to a mainnet-sized chain gives about 10 seconds per call, and that is a floor -- fixture blocks hold 8 small transactions where real ones hold thousands. See docs/benchmarks/gettxoutproof.md. The twelve tests were audited by mutation rather than trusted for being green. That audit found the three crates/index tests were not running at all, because their module is gated on a feature the crate does not enable by default, and that one of them passed vacuously when both resolvers returned None. Both are fixed here. 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 |
Review of the first commit turned up three defects in the path it added, plus a duplicated docstring it shipped. An index error failed the whole call. proof_via_index propagated it as RpcError::Internal, so a broken txindex turned a call that used to succeed into an error -- before this path existed the index was never consulted, and the scan answered. That is a behaviour regression introduced by an optimization, which is exactly what the refactor-set contract exists to prevent. The error is now logged and treated as a miss, and the function returns Option rather than Result so the regression cannot be reintroduced by accident. Only one wanted txid was probed, and `wanted` is a HashSet, so which one was whatever the hasher yielded. A single unresolvable txid therefore dropped the call into the full chain scan non-deterministically -- the precise cost this path exists to avoid. Every wanted txid is now probed before giving up. Five tests cover what was uncovered: several txids resolving through the index, an unresolvable probe not abandoning the path, an erroring index falling back to the scan, the explicit-blockhash path never consulting the index at all, and a counting indexer pinning that every wanted txid is probed. That last one exists because the outcome-based test is only a probabilistic detector: with a HashSet, a one-probe implementation picks the resolvable txid about half the time. Counting probes is deterministic, and the mutation audit confirms it -- reverting to one probe, and making a miss abandon the path, each turn it red. The "an index error must not fail the call" property is not mutation-detectable any more, because the Option return type enforces it. The test stays as a guard against anyone reintroducing the Result. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up review found three defects in the path this PR addsPushed as An index error failed the whole call. Only one wanted txid was probed, and A duplicated docstring on Five more tests
The last one exists because the outcome-based test is only a probabilistic detector: with a Mutation audit, second round
"An index error must not fail the call" is not mutation-detectable any more, because the Full suite: |
…al block shape Two follow-ups to the index path, both found by looking harder at what the first commit left behind. The fallback still cloned the whole block-record log to scan it. At tip that is about 963k records at 168 bytes each -- roughly 160 MB allocated and copied to answer one call, on the exact path taken when the index cannot answer. Passing the read guard through instead would be worse: the scan loads a block body from disk per record, and holding that lock across the loop stalls block application for the length of the scan. So the length is snapshotted once and each record is copied out under a momentary lock released before its body is read. Memory is O(1) and the lock never spans I/O. A test pins that directly, with a body source that fails if it cannot take the write lock while being called. proof_from_records then had one caller passing a one-element slice, so it is now proof_from_single_record with both of its error messages intact. The benchmark grew a second fixture shape, and it overturned a claim. With only 8-transaction blocks the per-record cost is the file read; at 500 it is the deserialize-and-hash a real block pays. Measured across both: 2,000 x 8tx scan 20.824 ms index 31.312 us last block 200 x 500tx scan 57.228 ms index 700.34 us last block and per record, 10.41 us against 286.1 us -- which brackets one call at a mainnet-sized chain between 10 seconds and 4.6 minutes rather than only giving a floor. It also showed the index arm is slower in the scan best case, by 10.6 us and 158.8 us respectively, where the previous single-shape run reported 1.04x and called it noise. That was wrong and the doc now says so. The cost is real -- a row lookup, a ranged read, a txid comparison -- and does not disappear, because the proof needs the block loaded either way. It is acceptable only because the scan best case means the transaction is in the first block of the chain, which on a real chain does not happen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more follow-ups, and a claim this PR had wrongThe fallback still copied the whole log
Passing the read guard through instead would be worse: the scan loads a block body from disk per record, and holding that lock across the loop stalls block application for the whole scan. So the length is snapshotted once and each record is copied out under a momentary lock, released before its body is read. Memory is O(1) and the lock never spans I/O. A test pins it directly — the body source tries to take the write lock while being called and fails if it cannot. Mutation-checked: restoring the held guard turns that test red and nothing else.
The benchmark had only one block shape, and it misled
Per record: 10.41 µs at 8 tx, 286.1 µs at 500. Against 963,124 records that brackets one call at 10 seconds to 4.6 minutes, where before there was only a floor. Correction: this PR previously reported the scan's best case as It is acceptable only because of what "the scan's best case" means: the scan walks from height 0, so it wins only when the wanted transaction is in the first block on the chain. The doc now says this rather than claiming parity. Noted, not fixed hereAt 200–2,000 records |
main replaced `Context.indexer: Option<Arc<Mutex<Box<dyn IndexerLike>>>>` with `Context.tx_index: Option<Arc<dyn TxIndexQuery>>` while this branch was adding a height lookup to `IndexerLike`. The merge was textually clean and compiled nowhere. This is the rework, not a fixup. `TxIndexQuery` gains `transaction_height`, defaulting to `Ok(None)` so the existing implementors keep compiling, and `TxIndexQueryEngine` implements it over the same row/position/full-block ladder `transaction` already walks — the two now share `locate_transaction_for` rather than keeping two copies of it. The height caller pays for a deserialization it does not use, which is the price of not answering from an unverified row: a row surviving a reorg would otherwise name a block that never held the transaction. `Indexer::resolve_transaction_height`, added earlier on this branch, is dropped. main already has `resolve_tx_with_height`, which does the same job, so the addition was redundant with the interface it was written against. `Retry` strengthens the fallback rather than complicating it. The index reconciles asynchronously, so `Retry` is the routine answer while it catches up, and a call the scan can answer today must not be refused because the index is behind. Every `TxQueryError` is a miss. The mutation audit found two coverage gaps and one invalid mutation: - `falls_back_when_the_indexed_block_lacks_some_wanted_txids` pinned the outcome but not the mechanism — its stub answered one height for every probe, so "keeps probing" and "gives up first" both landed in the fallback, which answers either way. Counting probes separates them. - `falls_back_to_the_scan_when_the_index_errors` asserted only that some string came back. It now compares against the scan's own answer, over all three error variants. - Replacing the error path's `return None` with `continue` killed nothing, but it does not remove the property either; recorded as invalid and reformulated. Six valid mutations, all killed on their named tests. rpc 186, node 473. The benchmark is re-run whole: the `after` arm now drives a real `TxIndexQuery` over the same RocksDB index and flat block files. Its absolute numbers are not comparable to the previous revision's — the unchanged `before_scan` arm moved 3.6x across this host's WSL memory cut — so the doc reports this run alone and says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes nothing; this is candidate A from the optimization-candidates survey (#84).
The problem
Called without a block hash,
gettxoutproofdeep-copies everyBlockRecord, then loads, deserializes and hashes every block on the chain to answer one call. At tip that is roughly a million block loads, almost all discarded.This is unbounded work for one authenticated RPC call, not a remote DoS. It still stalls the node for the duration and evicts everything else from cache.
The fix
Context.indexeralready knows which block confirms a txid. Resolve the confirming height, build the proof from that block alone. Bitcoin Core takes the same route: itsgettxoutproofrequires a block hash unless txindex is enabled.IndexerLikegainsresolve_transaction_height, defaulting toOk(None), so every existing implementation keeps compiling and no caller may quietly drop its fallback.What is preserved
The scan is kept whole as
proof_from_records. It is the fallback whenever the index cannot answer, and the oracle the equivalence tests compare against:That last one is not belt and braces. BIP30 duplicate coinbase txids mean a txid can confirm in more than one block, so a block chosen from a single txid is a candidate, never a verdict.
The explicit-
blockhashpath and every error code and message are unchanged.Measurement
Both arms in one Criterion run over one fixture, on an idle machine (the mainnet IBD sharing the host was stopped first and writeback allowed to drain).
first_blocklast_blockbefore_scanafter_indexTwo positions, because the scan is position-dependent and the index is not. In the scan's best case the arms are within noise — the index costs nothing it does not save. The index arm is flat at 28-33 us across both.
Extrapolating the scan's 10.56 us per record to a mainnet-sized chain gives about 10 seconds per call, and that is a floor: fixture blocks hold 8 small transactions where real ones hold thousands. Full method and caveats in
docs/benchmarks/gettxoutproof.md.crates/rpchad no benchmarks at all before this PR.Tests, and how they were checked
Twelve tests — nine in
crates/rpc, three incrates/index. They were then audited by mutation, because a green suite proves nothing until it is shown to fail when the behaviour it pins is removed:proof_via_indexalways returnsNoneresolve_transaction_heightalways returnsNoneNonecorrectly stayed greenThe audit found two real defects, both fixed here:
crates/indextests were not running at all — their module is#[cfg(all(test, feature = "rocksdb"))]and the crate defaults to no features, so they were filtered out while appearing to passresolve_transaction_height_agrees_with_the_transaction_resolverpassed vacuously: both resolvers returningNonesatisfied the equalityNot claimed
gettxoutproofis not a G14 budget item. The case is that one RPC call should not do O(chain) work. The fixture is synthetic and establishes the shape of the cost, not its absolute value on mainnet.🤖 Generated with Claude Code