docs: survey the optimization candidates the campaign never looked at - #84
Open
rabbitson87 wants to merge 2 commits into
Open
docs: survey the optimization candidates the campaign never looked at#84rabbitson87 wants to merge 2 commits into
rabbitson87 wants to merge 2 commits into
Conversation
The two-phase campaign finished what PLAN.md scheduled, but it only ever examined crates that already had benchmarks. Surveying the ones that do not — `rpc` (9,137 LOC), `mempool` (4,254), `p2p` (3,295), `chain` (3,010), `filters` (771) — found six candidates, two of them worse than anything the campaign itself fixed. No code changes. Each entry names what would have to be measured before it is worth doing, because two are structural findings whose magnitude is unknown and one is a projection from a number that has not converged. The two that stand out: - **`gettxoutproof` reads the whole chain** when called without a block hash (`crates/rpc/src/handlers/tx.rs:175`). It deep-copies every `BlockRecord`, then loads, deserializes and fully txid-hashes every block — ~957,600 of them at tip, to answer one call. The txindex is already reachable from the RPC context and the read-path campaign's `resolve_transaction` already answers exactly this question; Bitcoin Core takes the same route when txindex is on. - **The block-record log grows one entry per block, forever** (`crates/rpc/src/context.rs:534`). Pruning blanks `block_hex` but keeps the record, and the only removal is the single-entry reorg undo. At 264 B/record — 160 of which is an 80-byte header stored as a hex `String` — that is 103.9 MiB at the height the attribution run measured and 241.1 MiB at tip, which sizes it at 15.9% of the 0.64 GiB residual that run left unattributed. The arithmetic is shown per field rather than asserted. Also records the mempool priority index rebuilding in O(n² log n), `dbcache_mb` reaching no storage backend at all (issue #51, and the reason the residual cannot be attributed by tuning), the deferred `load_ranges` batch read, and the two record-encoding savings not taken. And the negative results, so nobody re-derives them: the chain fork-point walk is O(depth) rather than quadratic, `Context.transactions` only looks like the block-record log but is not populated per block, and the p2p banlist `retain` is a periodic sweep rather than per-message work. Three of the six point at the same missing measurement — G14 tip RSS on a synced mainnet-tip node — which is also the run that decides whether the v5 codec is kept or reverted.
|
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 |
This was referenced Aug 18, 2026
This was referenced Aug 19, 2026
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.
Documentation only. No code changes, no new tests, no new benchmarks.
The two-phase performance campaign finished what
PLAN.mdscheduled, but it only ever examined crates that already had benchmarks. This surveys the ones that do not —rpc(9,137 LOC),mempool(4,254),p2p(3,295),chain(3,010),filters(771) — and found six candidates, two of them worse than anything the campaign itself fixed.Independent of #80 and #83; branched from
main. Everyfile:linewas re-verified againstmainbefore this was written. The cross-references out of the page are forward-looking and land with those two PRs — the findings depend on neither, and the page says so.The two that stand out
A —
gettxoutproofreads the whole chain when given no block hash.crates/rpc/src/handlers/tx.rs:175doesctx.blocks.read().clone(), deep-copying everyBlockRecord, then for each one loads the body, deserializes the whole block and computes every txid into aHashSet. At tip that is ~957,600 block loads and full deserializations to answer one call, almost all of it discarded.The fix is already in the tree:
Context.indexer(crates/rpc/src/context.rs:302) is the txindex, and the read-path campaign'sresolve_transactionanswers exactly "which block contains this txid". Bitcoin Core takes the same route when txindex is on.B — the block-record log grows one entry per block, forever.
crates/rpc/src/context.rs:534pushes; nothing removes. Pruning (crates/node/src/state.rs:566) blanksblock_hexbut keeps the record, and the only removal is the single-entry reorg undo atcrates/node/src/apply.rs:1167.264 B per record, shown per field rather than asserted — 104 of struct plus 160 of heap for an 80-byte header stored as a hex
String. That is 103.9 MiB at height 412,732 and 241.1 MiB at tip, which sizes it at 15.9% of the 0.64 GiB the attribution run left unattributed, on the one G14 budget that is at risk.One correction worth flagging:
header_hexlooked dead and is not. It is returned atcrates/rpc/src/handlers/chain.rs:286and:291and decoded at:886. So the candidate is "store 80 raw bytes, encode on read" — the read is one RPC call, the storage is every block for the life of the process.The other four
pareto.rs:29-42,pool.rs:419,pool.rs:620-626dbcache_mbreaches no storage backendcrates/storage; issue #51load_rangeopens the file once per positionD is why the residual cannot be attributed. With no lever between config and the backends, fjall takes builder defaults and RocksDB a fixed 256 MiB block cache, so the one at-risk budget cannot be tuned and re-measured. It already cost the campaign one retracted claim.
Negative results are recorded too
So nobody re-derives them: the chain fork-point walk (
crates/chain/src/tree.rs:100-118) is O(depth) rather than quadratic;Context.transactionslooks like candidate B but is not populated per block by the apply path; the p2p banlistretainis a periodic expiry sweep, not per-message work.What is measured and what is not
The ranking table marks this explicitly, because the campaign's own recorded lesson is that a plausible speedup claim is worth nothing until the harness is shaped like the workload:
beforearm.What blocks what
A, B and C are implementable now under the refactor-set contract. D is a design question before it is an optimization. E is gated on the real G14 Electrum measurement — building it against synthetic fixtures would be the mistake this campaign already documented.
Three of the six point at the same missing measurement: G14 tip RSS on a synced mainnet-tip node with
txindexandblockfilterindex. That run decides whether B and F earn their complexity, and it is the same run that decides whether the v5 codec in #83 is kept or reverted.