Skip to content

Commit f8edac8

Browse files
committed
docs: survey the optimization candidates the campaign never looked at
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.
1 parent fe8c9e9 commit f8edac8

1 file changed

Lines changed: 258 additions & 0 deletions

File tree

Lines changed: 258 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,258 @@
1+
# Optimization candidates not yet taken
2+
3+
The two-phase performance campaign finished what `PLAN.md` scheduled. Everything
4+
in it either landed — the index read path, the v5 record codec — or was
5+
**rejected on its own measurement** and recorded as such: the decoded-block cache
6+
(`docs/benchmarks/index-read-path.md`) and the `bumpalo` arena (`DEVIATIONS.md`
7+
§9, `docs/benchmarks/utxo-memory.md`).
8+
9+
That leaves a question the campaign never asked: **what was never looked at?** It
10+
only examined crates that already had benchmarks — `consensus`, `coinstats`,
11+
`storage`, `utxo`, `node` — plus the two it added them to. This page surveys the
12+
ones that have none: `rpc` (9,137 LOC), `mempool` (4,254), `p2p` (3,295),
13+
`chain` (3,010), `filters` (771).
14+
15+
Six candidates, and the negative results, so the next reader does not re-derive
16+
either. **Nothing here is implemented.** Each item names what would have to be
17+
measured before it is worth doing, because two of them are structural findings
18+
whose magnitude is unknown, and one is a projection from a number that has not
19+
converged.
20+
21+
Every `file:line` below is against the tree this page lands on and was
22+
re-verified there. The cross-references *out* of it are forward-looking:
23+
`docs/benchmarks/index-read-path.md` arrives with the read-path PR, and
24+
`docs/benchmarks/utxo-memory.md`, `DEVIATIONS.md` §9 and the benchmark-shape
25+
best practice arrive with the UTXO-memory PR. The findings themselves depend on
26+
neither.
27+
28+
Of the three G14 budgets only **tip RSS ≤ 16 GiB** is anywhere near binding: UTXO
29+
commit p95 measures 2.4 ms against 50 ms, and the Electrum path measures 33x
30+
under its 30 ms budget on synthetic fixtures. The ranking reflects that.
31+
32+
## Ranking
33+
34+
| | Candidate | Evidence | Expected gain | Budget touched |
35+
|---|---|---|---|---|
36+
| 1 | A — `gettxoutproof` scans the chain | structural, **not timed** | O(chain) reads -> one index lookup | none, but unbounded work per call |
37+
| 2 | B — block-record log never shrinks | 264 B/block, arithmetic below | 241 MiB at tip | **tip RSS** |
38+
| 3 | F — remaining record encoding | 6 B/output, **projected** | ~1.0 GiB at tip | **tip RSS** |
39+
| 4 | C — mempool priority index | O(n² log n) rebuild, **not timed** | unknown | none |
40+
| 5 | D — `dbcache_mb` reaches nothing | verified code absence | unlocks tuning | **tip RSS**, indirectly |
41+
| 6 | E — one file open per position | 12.00 µs x P, **measured** | ~50 µs per 5-position height | Electrum p95 (headroom) |
42+
43+
Only **B** and **E** carry measured per-unit costs. **A** and **C** are shapes,
44+
not numbers. **F** rests on 3.626 outputs per record, which has not converged.
45+
46+
---
47+
48+
## A — `gettxoutproof` reads the whole chain when given no block hash
49+
50+
`crates/rpc/src/handlers/tx.rs:175`:
51+
52+
```rust
53+
None => ctx.blocks.read().clone(),
54+
```
55+
56+
With the optional `blockhash` argument absent, the handler deep-copies **every**
57+
`BlockRecord`, then for each one loads the body, deserializes the whole block,
58+
computes every txid into a `HashSet`, and tests whether it contains the wanted
59+
set. At tip that is ~957,600 block loads and full deserializations to answer one
60+
call, almost all of it discarded.
61+
62+
This is unbounded work for one authenticated RPC call, not a remote
63+
denial-of-service. It still stalls the node for the duration and evicts
64+
everything else from cache.
65+
66+
**The fix is already in the tree.** `Context.indexer`
67+
(`crates/rpc/src/context.rs:302`) is the txindex, and the read-path campaign's
68+
`resolve_transaction` answers exactly "which block contains this txid" in one
69+
lookup. Bitcoin Core takes the same route: its `gettxoutproof` requires a block
70+
hash *unless* txindex is enabled.
71+
72+
**Before implementing:** time the current handler on a fixture of a few thousand
73+
blocks and extrapolate, so the fix has a `before` arm. The refactor-set contract
74+
needs one, and "obviously faster" is the claim this campaign was wrong about
75+
twice.
76+
77+
## B — the block-record log grows one entry per block, forever
78+
79+
`crates/rpc/src/context.rs:534` pushes a record per applied block:
80+
81+
```rust
82+
pub fn add_block(&self, record: BlockRecord) {
83+
self.blocks.write().push(record);
84+
}
85+
```
86+
87+
Nothing removes one. `NodePruneService::prune_to_height`
88+
(`crates/node/src/state.rs:566`) walks the log and blanks `record.block_hex`, but
89+
leaves the record itself. The only removal in the crate is the single-record
90+
reorg undo at `crates/node/src/apply.rs:1167`, which pops one entry when a
91+
disconnect matches the tail.
92+
93+
`applied_block_record` (`crates/node/src/apply.rs:3475`) makes `block_hex`
94+
conditional on `cache_block_bodies_in_memory` — which production sets to `false`
95+
(`crates/node/src/state.rs:1111`) — but `header_hex` is computed unconditionally
96+
and the record is always pushed.
97+
98+
Per record, with the body already blanked:
99+
100+
| Field | Bytes |
101+
|---|---:|
102+
| `hash: Hash256` (`[u8; 32]`, `crates/primitives/src/hash.rs:27`) | 32 |
103+
| `height: u32` | 4 |
104+
| `block_hex: String` (empty in production: struct only) | 24 |
105+
| `body_size: usize` | 8 |
106+
| `header_hex: String` struct | 24 |
107+
| `tx_count: usize` | 8 |
108+
| `time: u32` | 4 |
109+
| **struct total** | **104** |
110+
| `header_hex` heap — an 80-byte header as lowercase hex | 160 |
111+
| **per block** | **264** |
112+
113+
That is **103.9 MiB at height 412,732** and **241.1 MiB at 957,600**. The
114+
attribution run at 412,732 left 0.64 GiB of RSS unattributed and named the
115+
block-record log as one of four suspects (`docs/benchmarks/utxo-memory.md`); this
116+
sizes it at **15.9% of that residual**, on the one budget that is at risk.
117+
118+
**`header_hex` is not dead — do not delete it.** It is returned directly at
119+
`crates/rpc/src/handlers/chain.rs:286` and `:291`, and hex-decoded back to bytes
120+
at `:886`. The candidate is to store the 80 raw bytes and encode on read: the
121+
read is one RPC call, the storage is every block for the life of the process.
122+
That alone is 160 B/block of the 264.
123+
124+
The second half — bounding the log, or dropping records below the prune height
125+
the way `block_hex` already is — is a behaviour change, because
126+
`crates/rpc/src/handlers/chain.rs:45` and `:171` scan the log. Establish what
127+
those two need before removing anything.
128+
129+
## C — the mempool priority index re-sorts on every insert
130+
131+
`crates/mempool/src/pareto.rs:29-38`:
132+
133+
```rust
134+
pub fn insert(&mut self, id: EntryId, entry: &MempoolEntry) {
135+
self.remove(id);
136+
self.entries.push(ParetoKey { .. });
137+
self.entries.sort_by(compare_keys);
138+
}
139+
```
140+
141+
`remove` is a linear scan (`pareto.rs:42`), and the sort covers the whole
142+
`TinyVec`. It runs on every acceptance, via `crates/mempool/src/pool.rs:419`.
143+
144+
The rebuild is worse. Ancestor/descendant recomputation at
145+
`crates/mempool/src/pool.rs:620-626` discards the front and re-inserts entry by
146+
entry:
147+
148+
```rust
149+
self.pareto = ParetoFront::new();
150+
for (id, entry) in pareto_entries {
151+
self.pareto.insert(id, &entry);
152+
}
153+
```
154+
155+
Each of those inserts sorts everything inserted so far, so rebuilding *n* entries
156+
is **O(n² log n)**.
157+
158+
**Not on a G14 budget**, and its real cost is unknown — a full mempool is tens of
159+
thousands of entries, not millions, and `TinyVec` sorting is cache-friendly.
160+
`mempool` has never been benchmarked at all, which is the actual finding here.
161+
162+
**Before implementing:** build the missing benchmark first. A sorted structure
163+
with O(log n) insert is the obvious replacement, but the campaign's own lesson
164+
(`docs/solutions/best-practices/benchmark-the-operation-the-workload-performs-not-the-one-the-api-exposes.md`)
165+
is that the obvious replacement is worth nothing until the harness is shaped like
166+
the workload — here, acceptance and template building, not `insert` in isolation.
167+
168+
## D — `dbcache_mb` never reaches a storage backend
169+
170+
Parsed from CLI and `bitcoin.conf` (`crates/node/src/config.rs:162`,
171+
`crates/node/src/bitcoin_conf_compat.rs:64`), carried through config layering
172+
(`config.rs:574`), and referenced **nowhere in `crates/storage`** — verified by
173+
search, zero hits. Tracked as issue #51.
174+
175+
It has already cost this campaign once: `docs/benchmarks/utxo-memory.md` had to
176+
retract a claim that 450 MB of residual RSS was the configured `dbcache`, because
177+
the setting reaches no constructor. fjall takes builder defaults and RocksDB a
178+
fixed 256 MiB block cache.
179+
180+
The consequence for everything else on this page: **there is no lever between
181+
configuration and the backends**, so the one budget that is at risk cannot be
182+
tuned, and the non-UTXO residual cannot be attributed by turning a knob and
183+
re-measuring.
184+
185+
This is a design question before it is an optimization — `dbcache_mb` means
186+
different things to four backends — so it does not fit the refactor-set contract
187+
as stated.
188+
189+
## E — `load_range` opens the file once per position
190+
191+
Measured during the read-path campaign and deliberately deferred, with the
192+
numbers already published in `docs/benchmarks/index-read-path.md`:
193+
194+
| Operation | Cost |
195+
|---|---:|
196+
| `load` — whole 250 KB body | 23.44 µs |
197+
| `load_range` — 250 bytes | 12.00 µs |
198+
| `load_range` x5 — five positions at one height | 62.34 µs |
199+
200+
The cost is the open/`fstat`/seek/read sequence, not the bytes: a 250-byte read
201+
costs half a 250 KB one. The optimized resolver arm is now syscall-bound at ~13
202+
µs per height, so a `load_ranges` that opens once per height recovers most of the
203+
62 µs.
204+
205+
**This is headroom, not a regression.** The position path already beats the scan
206+
path by 63-77x end to end. It only matters if the real G14 Electrum measurement
207+
lands closer to the 30 ms budget than the synthetic fixtures suggest — and that
208+
measurement has not been run.
209+
210+
## F — the record-encoding savings not taken
211+
212+
`PLAN.md` projected 17 B/output from four changes. Three shipped and measured
213+
11.75 (`docs/benchmarks/utxo-memory.md`). The two remaining, ~3 B/output each:
214+
215+
- **Core-style scriptPubKey compression** — P2PKH 25→21, P2SH 23→21, P2PK
216+
35/67→33. No blocker known. Independent of the other half and could go first.
217+
- **Hoisting `height` into the record header.** Needs "every output of a record
218+
shares one height" to hold, and BIP30's duplicate coinbase txids (mainnet
219+
blocks 91,842 and 91,880) are precisely where it might not. **The
220+
investigation is the prerequisite, not the encoding** — the invariant has never
221+
been checked, and the codec change is trivial once it is.
222+
223+
Together about 1.0 GiB at tip, on the same 3.626-outputs-per-record projection
224+
that has not converged (2.296 at height 183k, 4.056 at 390k). Same caveat as the
225+
v5 verdict: this is worth doing if tip RSS is actually near budget, and that has
226+
never been measured.
227+
228+
---
229+
230+
## Negative results
231+
232+
Checked during the survey and cleared. Recorded so nobody re-checks them.
233+
234+
- **`crates/chain/src/tree.rs:100-118`** — the fork-point walk builds a `HashSet`
235+
of one side's ancestors and walks the other against it. O(depth), not
236+
quadratic.
237+
- **`Context.transactions`** (`crates/rpc/src/context.rs:289`) — a
238+
`HashMap<Txid, Transaction>` that looks like candidate B, but the apply path
239+
never populates it. It is fed by `add_transaction` on the submission path and
240+
cleaned by the prune service. Not a per-block leak.
241+
- **`crates/p2p/src/banlist.rs:138`** — the `retain` is a periodic expiry sweep,
242+
not per-message work.
243+
244+
## What blocks what
245+
246+
- **A, B, C** — implementable now, each under the refactor-set contract: keep the
247+
old path as the oracle and the benchmark's `before` arm, prove equivalence,
248+
prove the win in one run.
249+
- **D** — needs a decision on what `dbcache_mb` should mean per backend first.
250+
- **E** — gated on the real G14 Electrum measurement. Do not build it against
251+
synthetic fixtures.
252+
- **F** — the height half is gated on the BIP30 investigation; the script half is
253+
not gated on anything.
254+
255+
Three of the six point at the same missing measurement: **G14 tip RSS on a synced
256+
mainnet-tip node with `txindex` and `blockfilterindex`.** That run decides
257+
whether B and F are worth their complexity, and it is the same run that decides
258+
whether the v5 codec is kept or reverted (`DEVIATIONS.md` §9).

0 commit comments

Comments
 (0)