perf(mempool): order the priority index instead of re-sorting it per 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>

Author: rabbitson87

Cargo.lock

@@ -35,3 +35,9 @@ tracing.workspace = true
 [dev-dependencies]
 proptest.workspace = true
 serde_json.workspace = true
+
+criterion.workspace = true
+
+[[bench]]
+name = "pareto"
+harness = false

crates/mempool/Cargo.toml

@@ -0,0 +1,145 @@
+//! Mempool priority-index refactor-set benchmark.
+//!
+//! Both arms run over one identical fixture in one process, so the before/after
+//! ratio comes from a single run and cannot be confounded by the rebuild and
+//! baseline drift recorded in
+//! `docs/solutions/best-practices/criterion-bench-trust-rebuild-drift-baselines-allocator.md`.
+//!
+//! `before_sorted` is `SortedParetoFront`, the flat vector that did a linear
+//! `remove` and a full `sort_by` on every insert. `after_ordered` is
+//! `ParetoFront`, the ordered set that replaced it.
+//!
+//! `mempool_insert_entry` is the end-to-end path an attacker actually drives:
+//! `Mempool::insert_entry` calls `recompute_all_metadata`, which rebuilds the
+//! whole priority index for every accepted transaction. It is benchmarked at
+//! smaller sizes than the index arms because that outer rebuild is quadratic
+//! *independently* of the index — which is the point of measuring it separately.
+// PERF: Criterion emits public harness items whose docs are irrelevant here.
+#![allow(missing_docs)]
+// A fixture that fails to build has no meaningful degraded mode: a fill that
+// silently indexed nothing would be timed as a win.
+#![allow(clippy::expect_used)]
+
+use std::hint::black_box;
+use std::sync::Arc;
+
+use bitcoin::hashes::Hash as _;
+use bitcoin::{
+    Amount, OutPoint, ScriptBuf, Sequence, Transaction, TxIn, TxOut, Txid, Witness, absolute,
+    transaction,
+};
+use bitcoin_rs_mempool::{Mempool, MempoolEntry, MempoolLimits, ParetoFront, SortedParetoFront};
+use criterion::{Criterion, criterion_group, criterion_main};
+
+/// Index fill sizes. The largest is far below a Core-default mempool (~10^5
+/// transactions at `-maxmempool=300MB`); the quadratic arm cannot be measured
+/// there in reasonable time, which is itself the finding.
+const FILL_SIZES: [usize; 4] = [1_000, 4_000, 16_000, 50_000];
+
+/// End-to-end sizes, an order of magnitude smaller: `insert_entry` rebuilds the
+/// entire index per transaction, so its cost is the index fill cost multiplied
+/// by the number of transactions.
+const POOL_SIZES: [usize; 3] = [200, 800, 3_200];
+
+fn spread_fee(seed: u64) -> u64 {
+    // Not monotonic in the seed: an index fed entries already in priority order
+    // never has to reorder anything, and would benchmark the best case only.
+    (seed.wrapping_mul(2_654_435_761) % 100_000).saturating_add(1)
+}
+
+fn distinct_tx(seed: u64) -> Transaction {
+    let mut previous = [0_u8; 32];
+    previous[..8].copy_from_slice(&seed.to_le_bytes());
+    Transaction {
+        version: transaction::Version::TWO,
+        lock_time: absolute::LockTime::ZERO,
+        input: vec![TxIn {
+            // Distinct prevouts: entries that conflict would be rejected rather
+            // than accepted, and the fill would measure the rejection path.
+            previous_output: OutPoint::new(Txid::from_byte_array(previous), 0),
+            script_sig: ScriptBuf::new(),
+            sequence: Sequence::MAX,
+            witness: Witness::new(),
+        }],
+        output: vec![TxOut {
+            value: Amount::from_sat(10_000),
+            script_pubkey: ScriptBuf::from_bytes(seed.to_le_bytes().to_vec()),
+        }],
+    }
+}
+
+fn entry(seed: u64) -> MempoolEntry {
+    MempoolEntry::new(Arc::new(distinct_tx(seed)), 200, spread_fee(seed), seed, 0)
+}
+
+fn bench_index_fill(c: &mut Criterion) {
+    let mut group = c.benchmark_group("mempool_pareto");
+    group.sample_size(10);
+
+    for size in FILL_SIZES {
+        let entries = (0..size as u64).map(entry).collect::<Vec<_>>();
+
+        // Prove both arms index the same fixture before timing either. An arm
+        // that dropped entries would be timed as a spectacular, meaningless win.
+        let mut check_before = SortedParetoFront::new();
+        let mut check_after = ParetoFront::new();
+        for (index, item) in entries.iter().enumerate().take(1_000) {
+            let id = u32::try_from(index).expect("fixture id fits u32");
+            check_before.insert(id, item);
+            check_after.insert(id, item);
+        }
+        assert_eq!(
+            check_before.top_n(check_before.len()).collect::<Vec<_>>(),
+            check_after.top_n(check_after.len()).collect::<Vec<_>>(),
+            "the arms order differently; the benchmark would be meaningless"
+        );
+
+        group.bench_function(format!("before_sorted/fill/{size}"), |b| {
+            b.iter(|| {
+                let mut front = SortedParetoFront::new();
+                for (index, item) in entries.iter().enumerate() {
+                    front.insert(u32::try_from(index).unwrap_or(u32::MAX), item);
+                }
+                black_box(front.len())
+            });
+        });
+        group.bench_function(format!("after_ordered/fill/{size}"), |b| {
+            b.iter(|| {
+                let mut front = ParetoFront::new();
+                for (index, item) in entries.iter().enumerate() {
+                    front.insert(u32::try_from(index).unwrap_or(u32::MAX), item);
+                }
+                black_box(front.len())
+            });
+        });
+    }
+
+    group.finish();
+}
+
+fn bench_mempool_fill(c: &mut Criterion) {
+    let mut group = c.benchmark_group("mempool_insert_entry");
+    group.sample_size(10);
+
+    for size in POOL_SIZES {
+        let entries = (0..size as u64).map(entry).collect::<Vec<_>>();
+        group.bench_function(format!("fill/{size}"), |b| {
+            b.iter(|| {
+                let mut pool = Mempool::new(MempoolLimits::default());
+                for item in &entries {
+                    let _ = pool.insert_entry(item.clone());
+                }
+                black_box(pool.len())
+            });
+        });
+    }
+
+    group.finish();
+}
+
+criterion_group! {
+    name = benches;
+    config = Criterion::default();
+    targets = bench_index_fill, bench_mempool_fill
+}
+criterion_main!(benches);

crates/mempool/benches/pareto.rs

@@ -24,7 +24,7 @@ pub mod standardness;
 
 pub use entry::{EntryId, MempoolEntry};
 pub use eviction::evict_lowest_fee_packages;
-pub use pareto::ParetoFront;
+pub use pareto::{ParetoFront, SortedParetoFront};
 pub use policy::{MempoolLimits, PolicyError};
 pub use pool::{Mempool, MempoolError, MempoolStats, ScriptHash};
 pub use rbf::{RbfError, ReplacementCandidate, ReplacementPlan};

crates/mempool/src/lib.rs

@@ -1,11 +1,31 @@
+use alloc::collections::{BTreeMap, BTreeSet};
+
 use tinyvec::TinyVec;
 
 use crate::{EntryId, MempoolEntry};
 
 /// Priority index ordered by fee rate, ancestor fee rate, then age.
+///
+/// Ordering lives in [`ParetoKey`]'s [`Ord`], and the set is kept in that order
+/// rather than re-sorted. Insertion and removal are both `O(log n)`.
+///
+/// The previous implementation held the keys in a flat vector, and every
+/// `insert` did a linear `remove` followed by a full `sort_by`. Filling a
+/// mempool was therefore quadratic — 4.92 ms at 1,000 entries against 4.57 s at
+/// 50,000, a measured exponent of 2.05 — and the cost is paid on the path that
+/// accepts transactions from peers, so it was reachable by anyone who could fill
+/// the mempool. [`SortedParetoFront`] keeps that implementation as the oracle
+/// these tests compare against and as the benchmark's `before` arm.
 #[derive(Clone, Debug, Default)]
 pub struct ParetoFront {
-    entries: TinyVec<[ParetoKey; 256]>,
+    /// Keys in priority order.
+    order: BTreeSet<ParetoKey>,
+    /// The key currently indexed for each entry.
+    ///
+    /// A removal is given an id, and the ordered set is keyed by priority, so
+    /// without this a removal would have to search the set to find what to
+    /// remove — which is the linear scan this type exists to avoid.
+    keys: BTreeMap<EntryId, ParetoKey>,
 }
 
 #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
@@ -16,7 +36,111 @@ struct ParetoKey {
     time: u64,
 }
 
+impl Ord for ParetoKey {
+    /// Highest fee rate first, then highest ancestor fee rate, then oldest.
+    ///
+    /// The final tiebreak on `id` is what makes this a *total* order, and that
+    /// is load-bearing rather than cosmetic: the ordered set stores keys, so two
+    /// entries whose keys compared `Equal` would collapse into one and an entry
+    /// would silently vanish from the mempool's priority index. Entry ids are
+    /// unique, so no two distinct entries can compare equal.
+    fn cmp(&self, other: &Self) -> core::cmp::Ordering {
+        other
+            .fee_rate
+            .cmp(&self.fee_rate)
+            .then_with(|| other.ancestor_fee_rate.cmp(&self.ancestor_fee_rate))
+            .then_with(|| self.time.cmp(&other.time))
+            .then_with(|| self.id.cmp(&other.id))
+    }
+}
+
+impl PartialOrd for ParetoKey {
+    fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl ParetoKey {
+    fn new(id: EntryId, entry: &MempoolEntry) -> Self {
+        Self {
+            id,
+            fee_rate: entry.fee_rate,
+            ancestor_fee_rate: entry.ancestor_fee_rate(),
+            time: entry.time,
+        }
+    }
+}
+
 impl ParetoFront {
+    /// Creates an empty priority index.
+    #[must_use]
+    pub fn new() -> Self {
+        Self {
+            order: BTreeSet::new(),
+            keys: BTreeMap::new(),
+        }
+    }
+
+    /// Inserts or replaces an entry in priority order.
+    ///
+    /// Replacement is not a special case for the caller but is one here: an
+    /// entry whose ancestor fee rate changed has a different key, so the stale
+    /// key must leave the ordered set or the entry would be indexed twice.
+    pub fn insert(&mut self, id: EntryId, entry: &MempoolEntry) {
+        let key = ParetoKey::new(id, entry);
+        if let Some(previous) = self.keys.insert(id, key) {
+            let _ = self.order.remove(&previous);
+        }
+        let _ = self.order.insert(key);
+    }
+
+    /// Removes an entry from the priority index.
+    pub fn remove(&mut self, id: EntryId) -> bool {
+        let Some(key) = self.keys.remove(&id) else {
+            return false;
+        };
+        self.order.remove(&key)
+    }
+
+    /// Returns the highest-priority `n` entry identifiers.
+    pub fn top_n(&self, n: usize) -> impl Iterator<Item = EntryId> + '_ {
+        self.order.iter().take(n).map(|key| key.id)
+    }
+
+    /// Returns `true` if the front is empty.
+    #[must_use]
+    pub fn is_empty(&self) -> bool {
+        self.order.is_empty()
+    }
+
+    /// Returns the number of indexed entries.
+    #[must_use]
+    pub fn len(&self) -> usize {
+        self.order.len()
+    }
+}
+
+/// The flat-vector priority index [`ParetoFront`] replaced.
+///
+/// Retained deliberately, not left behind: it is the oracle the equivalence
+/// tests compare the replacement against, and the `before` arm of
+/// `benches/pareto.rs`. Both arms have to run in one process over one fixture
+/// for the ratio to mean anything, which they cannot do if this is deleted.
+///
+/// Nothing in the node uses it. It is quadratic to fill, which is the entire
+/// reason it was replaced.
+///
+/// It keeps its own copy of the comparison rather than borrowing
+/// [`ParetoKey`]'s [`Ord`]. Sharing it looked tidier and made the oracle
+/// worthless: a mutation that reversed the ordering left both implementations
+/// agreeing with each other, so the equivalence tests stayed green while the
+/// index was ordered backwards. An oracle has to be able to disagree.
+#[derive(Clone, Debug, Default)]
+pub struct SortedParetoFront {
+    entries: TinyVec<[ParetoKey; 256]>,
+}
+
+impl SortedParetoFront {
     /// Creates an empty priority index.
     #[must_use]
     pub fn new() -> Self {
@@ -28,21 +152,16 @@ impl ParetoFront {
     /// Inserts or replaces an entry in priority order.
     pub fn insert(&mut self, id: EntryId, entry: &MempoolEntry) {
         self.remove(id);
-        self.entries.push(ParetoKey {
-            id,
-            fee_rate: entry.fee_rate,
-            ancestor_fee_rate: entry.ancestor_fee_rate(),
-            time: entry.time,
-        });
-        self.entries.sort_by(compare_keys);
+        self.entries.push(ParetoKey::new(id, entry));
+        self.entries.sort_by(legacy_compare_keys);
     }
 
     /// Removes an entry from the priority index.
     pub fn remove(&mut self, id: EntryId) -> bool {
         let Some(index) = self.entries.iter().position(|entry| entry.id == id) else {
             return false;
         };
-        self.entries.remove(index);
+        let _ = self.entries.remove(index);
         true
     }
 
@@ -64,7 +183,11 @@ impl ParetoFront {
     }
 }
 
-fn compare_keys(left: &ParetoKey, right: &ParetoKey) -> core::cmp::Ordering {
+/// The ordering the flat-vector index sorted by, kept verbatim.
+///
+/// Deliberately a duplicate of [`ParetoKey`]'s [`Ord`] rather than a call to it.
+/// See [`SortedParetoFront`].
+fn legacy_compare_keys(left: &ParetoKey, right: &ParetoKey) -> core::cmp::Ordering {
     right
         .fee_rate
         .cmp(&left.fee_rate)