perf(mempool): make transaction acceptance cost independent of mempool size - #88
Open
rabbitson87 wants to merge 3 commits into
Open
perf(mempool): make transaction acceptance cost independent of mempool size#88rabbitson87 wants to merge 3 commits into
rabbitson87 wants to merge 3 commits into
Conversation
…insert `ParetoFront::insert` held its keys in a flat vector and, on every insert, did a linear `remove` followed by `sort_by` over the whole index. Filling an index of n entries was O(n^2 log n). That is not an idle path. `Mempool::insert_entry` — the path that accepts a transaction from a peer — calls `recompute_all_metadata`, which discards the priority index and re-inserts every entry, so the quadratic cost was paid once per accepted transaction by anyone able to put transactions in the mempool. Measured across one Criterion run over one fixture, both arms in process: 1,000 entries 2.064 ms -> 151.0 us 13.7x 4,000 entries 45.65 ms -> 626.2 us 72.9x 16,000 entries 464.7 ms -> 3.046 ms 152.6x 50,000 entries 4.497 s -> 10.45 ms 430.4x Measured exponent 1.97 for the old arm across that span, i.e. n^2; the new arm takes 69x longer over 50x the entries against the 78x n log n predicts. The replacement is an ordered set keyed by the priority comparison plus a map from entry id to the key currently indexed for it. The map is not redundant: removals arrive as an id while the set is keyed by priority, so without it a removal would have to search the set and the linear scan would come back. The ordering's final tiebreak on entry id was cosmetic before and is now load-bearing. The index is a *set of keys*, so two entries whose keys compared equal would collapse into one and a transaction would silently leave the priority index. THIS DOES NOT CLOSE THE QUADRATIC. `recompute_all_metadata` still walks every entry per insert, so `Mempool::insert_entry` measures an exponent of 2.17 *with this change applied* — 2.572 ms at 200 transactions against 1.057 s at 3,200. docs/benchmarks/mempool-pareto.md reports that rather than claiming a fix, and names the follow-up. The flat-vector index is retained whole as `SortedParetoFront`: the oracle the equivalence tests compare against and the benchmark's `before` arm. Four mutations, all killed on their named tests. The audit found a defect in the tests themselves. The oracle originally shared `ParetoKey`'s `Ord` with the replacement, which made it worthless: under a reversed-ordering mutation both implementations agreed with each other and both equivalence tests stayed green while the index was ordered backwards. The oracle now keeps its own verbatim copy of the comparison and the same mutation kills all three ordering tests. An oracle that shares code with the implementation cannot disagree with it. 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 |
Replacing the priority index was not enough. `Mempool::insert_entry` called
`recompute_all_metadata`, which walked every entry, and then `total_vsize()`,
which folded every entry again. With the index already fixed, insertion still
measured an exponent of 2.17 -- 1.057 s to fill 3,200 transactions.
Three changes remove what was left.
The metadata refresh is incremental. Linking one transaction into the spend
graph changes package totals for its transitive ancestors, itself, and its
transitive descendants, and for nothing else: an entry outside that set gains no
new ancestor because it is not a descendant of the seed, and gains no new
descendant because every new path runs through the seed, which would have put it
among the seed's ancestors. `insert_entry` and `remove_entries` now recompute
exactly that closure, whose size is bounded by the ancestor and descendant
policy limits rather than by the pool.
The closure is taken after the entry enters the spend indexes on an insertion,
because a transaction can arrive after something that already spends its
outputs; and before the removal on a removal, because a removed entry's
ancestors cannot be walked once it is gone.
`total_vsize` and `aggregate_fees` are running sums. Both were folds over the
whole pool, and `insert_entry` consults `total_vsize()` on every acceptance, so
that fold alone made insertion quadratic.
Measured end to end, against the same fixture as the previous commit:
200 tx 2.572 ms -> 373.0 us 6.9x
800 tx 51.27 ms -> 1.760 ms 29.1x
3,200 tx 1.057 s -> 7.079 ms 149.3x
12,800 tx unmeasurable -> 37.87 ms
51,200 tx unmeasurable -> 211.5 ms
Per transaction 1.87 us -> 4.13 us across 256x the entries; measured exponent
1.24 over the final leg, which is n log n for the fill.
`prioritise` had a defect this fixes on the way. It applied its fee delta to
each descendant's `ancestor_fee` by hand and never reindexed those descendants,
so a descendant kept the priority key it had before its ancestor was bumped --
defeating `prioritisetransaction` for exactly the packages it aims at. The three
delta loops are now one call to the same refresh an insertion does.
`recompute_all_metadata` is retained under `cfg(test)` as the oracle. Nothing in
the pool reaches it, so production cannot drift away from it.
Nine mutations, all killed on their named tests. Two survived the first pass:
`aggregate_fees` guards itself with a debug_assert, but a guard only fires when
something calls it, and no test called it after a removal or a fee bump. Both
paths could lose their bookkeeping silently. Covered now, and compared against
an independent fold so the check survives a release build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Mempool::by_txid` is a `HashMap`, so its iteration order is randomized per process. Two of the tests added with the incremental metadata refresh picked their subject out of it — `victims.get(3)` and `by_txid.keys().next()` — and so removed or bumped a different transaction on every run. That made the mutation audit unsound rather than merely noisy. Rebuilding the "a removal takes its closure after the removal" mutant and running the test binary 60 times put the removal equivalence test at 36 red out of 60: the row recorded as a kill was a coin flip that had landed the right way. Every subject is now addressed by entry id, which is the slab index and so follows insertion order, and the removal test removes each of the six fixture entries in turn instead of one. Re-audited at 40 executions per mutant, every removal-side row is 40/40 red and the baseline 0/40. The exhaustive sweep also reaches a shape no single chosen victim could: removing `leaf` takes the fan-in `joined` with it while `sibling` — `joined`s other parent — survives and has to drop it from its descendant totals. Adds `eviction_during_insertion_leaves_metadata_a_rebuild_agrees_with` for the other gap the pass found. `enforce_size_limit` is a `remove_entries` caller reached from inside `insert_entry`, removing packages the caller never named, and nothing drove the refresh through it. It now kills both the forgets-ancestors and the skips-the-reindex mutations. No production code changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Two commits, both about the same thing: accepting a transaction cost time quadratic in mempool size, on the path that accepts transactions from peers. Anyone able to fill the mempool made every subsequent acceptance more expensive.
1. The priority index re-sorted itself on every insert
ParetoFront::insertheld its keys in a flat vector and did a linearremovefollowed bysort_byover the whole index. Filling an index ofnentries wasO(n² log n).Replaced with an ordered set keyed by the priority comparison, plus a map from entry id to the key currently indexed for it. Both operations are
O(log n).Measured exponent 1.97 for the old arm —
n². Both arms run in one process over one fixture.The id→key map is not redundant: removals arrive as an entry id while the ordered set is keyed by priority, so without it a removal would have to search the set. The ordering's final tiebreak on entry id was cosmetic before and is now load-bearing — the index is a set of keys, so two entries whose keys compared equal would collapse into one and a transaction would silently leave the priority index.
2. That alone did not close it
insert_entryalso calledrecompute_all_metadata, which walked every entry, and thentotal_vsize(), which folded every entry again. With the index fixed and nothing else changed, insertion still measured an exponent of 2.17.The metadata refresh is now incremental. Linking one transaction into the spend graph changes package totals for its transitive ancestors, itself, and its transitive descendants — and nothing else. An entry
xoutside that set gains no new ancestor, becausexis not a descendant of the seed; and gains no new descendant, because every new path runs through the seed, which would putxamong its ancestors. The closure's size is bounded by the ancestor and descendant policy limits (25 each by default), not by the pool.Order matters in both directions, and both are commented at the call sites: the closure is taken after the entry enters the spend indexes on an insertion (a transaction can arrive after something that already spends its outputs), and before the removal on a removal (a removed entry's ancestors cannot be walked once it is gone).
total_vsizeandaggregate_feesare now running sums, guarded bydebug_asserts against the folds they replaced.The last two sizes could not be measured before — at exponent 2.17, 51,200 transactions is about seven minutes per sample. Per-transaction cost is now nearly flat across 256x the entries, and the exponent over the final leg is 1.24:
n log nfor the fill, so aboutO(log n)per accepted transaction.A defect fixed on the way
prioritiseapplied its fee delta to each descendant'sancestor_feeby hand and then never reindexed those descendants, so a descendant kept the priority key it had from before its ancestor was bumped. Sinceprioritisetransactionexists to move transactions in the miner's template, leaving descendants ranked on the pre-bump figure defeated it for exactly the packages it was aimed at. The three delta loops are now one call to the same refresh an insertion does.Correctness
Two oracles, both retained rather than deleted:
SortedParetoFront— the flat-vector index, thebeforearm and the ordering oracle.recompute_all_metadata— kept undercfg(test). Nothing in the pool reaches it, so production cannot drift away from it. Each equivalence test drives the incremental path, then runs the full recompute, and asserts nothing moved.Mutation audit — thirteen mutations, all killed on their named tests:
removeforgets the ordered setprioritiseforgets the running fee totalThree of these survived a first pass and are why the audit was worth running.
The oracle originally shared
ParetoKey'sOrdwith the replacement, which looked tidy and made it worthless: under the reversed-ordering mutation both implementations agreed with each other, so both equivalence tests stayed green while the index was ordered backwards. The oracle now keeps its own verbatim copy of the comparison. An oracle that shares code with the implementation cannot disagree with it.The last two rows also survived.
total_vsizeandaggregate_feeseach guard themselves with adebug_assert, but a guard only fires when something calls it.insert_entryconsultstotal_vsize()on every acceptance, so its bookkeeping was covered by accident — the 29 failures are that accident.aggregate_fees()is only reached throughstats(), and no test called it after a removal or a fee bump, so deleting the bookkeeping in both paths turned nothing red. Now covered, and compared against an independent fold so the check survives a release build where thedebug_asserts are compiled out.Verification
cargo test -p bitcoin-rs-mempool -p bitcoin-rs-mining -p bitcoin-rs-electrum --no-fail-fastgreen (89 lib + 5 pareto_ordering + downstream).cargo fmt --checkandcargo clippy -p bitcoin-rs-mempool -p bitcoin-rs-mining -p bitcoin-rs-electrum -p bitcoin-rs-rpc -- -D warningsclean. The 22expect_usedfindings under--all-targetsare pre-existing onmain(verified by stashing).Full write-up:
docs/benchmarks/mempool-pareto.md.🤖 Generated with Claude Code