mempool: verify scripts before evicting, and make eviction linear

docs/manual/src/configuration.md

@@ -145,7 +145,7 @@ changed. Their values are redacted in the log, never printed.
 | `rpcextendederrors`, `rpcdefaultunits` | Switches the RPC error-payload shape and the default amount unit. |
 | `maxconnections`, `maxinboundperip` | New limits govern subsequent connections. Existing peers above a lowered cap are not dropped. |
 | `bantime` | New ban duration applies to bans created after the change. |
-| `minrelaytxfee`, `maxmempool`, `dustrelayfee`, `datacarrier`, `datacarriersize`, `mempoolfullrbf`, `limitclustercount`, `limitancestorcount`, `limitdescendantcount`, `mempoolexpiry`, `permitbaremultisig` | Mempool and relay policy is swapped atomically and governs subsequent transaction admissions. Already-admitted entries are not re-evaluated. |
+| `minrelaytxfee`, `maxmempool`, `dustrelayfee`, `datacarrier`, `datacarriersize`, `mempoolfullrbf`, `limitclustercount`, `limitancestorcount`, `limitdescendantcount`, `mempoolexpiry`, `permitbaremultisig` | Mempool and relay policy is swapped atomically and governs subsequent transaction admissions. Already-admitted entries are not re-evaluated, except that a lower `maxmempool` evicts the lowest-feerate entries at once, down to the new size. |
 | `connect`, `addnode`, `seednode` | Newly added peers are registered as at startup and dialed immediately; existing connections are untouched. A new `addnode` entry is an added node, listed by `getaddednodeinfo` and removable with `addnode remove`, and a new `addnode` or `connect` host name that does not resolve yet is kept and looked up again on every reconnect tick. Removing an entry does not disconnect that peer (use `disconnectnode`) or remove the added node (use `addnode remove`), matching Core's `-addnode`. The exclusivity of `-connect` (connect only to these peers, with automatic outbound and DNS seeding suppressed) is a startup-time decision and is not re-evaluated on reload. Adding `-connect` live dials the new peer but does not put a running node into connect-only mode; restart for that. |
 | `peerblockfilters` | Turns `NODE_COMPACT_FILTERS` advertisement on or off for new handshakes, still gated on a complete `blockfilterindex`. |
 | `addrindexsubscriptions` | New address-index subscription cap, applied to subsequent subscriptions. Lowering it does not evict existing subscribers. |

node/src/mempool/orphanage.rs

@@ -16,7 +16,7 @@
 
 use bitcoin::{Transaction, Txid};
 use parking_lot::Mutex;
-use std::collections::{HashMap, HashSet, VecDeque};
+use std::collections::{BTreeMap, HashMap, HashSet};
 use std::time::{Duration, Instant};
 
 use crate::mempool::policy::MAX_STANDARD_TX_WEIGHT;
@@ -81,17 +81,22 @@ pub struct OrphanEntry {
     pub added_at: Instant,
     pub missing_parents: HashSet<Txid>,
     pub bytes: usize,
+    /// Its key in the orphanage's arrival-order queue.
+    seq: u64,
 }
 
 struct OrphanageInner {
     by_txid: HashMap<Txid, OrphanEntry>,
     by_parent: HashMap<Txid, HashSet<Txid>>,
     by_peer: HashMap<PeerId, HashSet<Txid>>,
     total_bytes: usize,
-    // Insertion-order queue for FIFO eviction. Entries may refer to
-    // txids that have already been removed (via explicit remove or
-    // per-peer eviction); eviction skips stale entries at the front.
-    order: VecDeque<Txid>,
+    /// The live orphans in arrival order, for FIFO eviction, keyed by an
+    /// arrival sequence number as Core's orphanage keys its announcements
+    /// (`m_entry_sequence`, src/node/txorphanage.cpp). Every removal takes
+    /// its orphan's key out, so this holds exactly the orphans in `by_txid`.
+    order: BTreeMap<u64, Txid>,
+    /// The next arrival's key in `order`.
+    next_seq: u64,
 }
 
 pub struct TxOrphanage {
@@ -107,7 +112,8 @@ impl TxOrphanage {
                 by_parent: HashMap::new(),
                 by_peer: HashMap::new(),
                 total_bytes: 0,
-                order: VecDeque::new(),
+                order: BTreeMap::new(),
+                next_seq: 0,
             }),
             config,
         }
@@ -178,23 +184,26 @@ impl TxOrphanage {
 
         // Global cap: evict oldest (FIFO) until we have room.
         while inner.by_txid.len() >= self.config.max_count {
-            if let Some(victim) = Self::pop_oldest(&mut inner) {
+            if let Some(victim) = Self::oldest(&inner) {
                 Self::remove_locked(&mut inner, &victim);
             } else {
                 break;
             }
         }
 
+        let seq = inner.next_seq;
+        inner.next_seq += 1;
         let entry = OrphanEntry {
             tx,
             from_peer,
             added_at,
             missing_parents: missing_parents.clone(),
             bytes,
+            seq,
         };
         inner.by_txid.insert(txid, entry);
         inner.total_bytes += bytes;
-        inner.order.push_back(txid);
+        inner.order.insert(seq, txid);
         for parent in &missing_parents {
             inner.by_parent.entry(*parent).or_default().insert(txid);
         }
@@ -238,8 +247,7 @@ impl TxOrphanage {
                 inner.by_peer.remove(&entry.from_peer);
             }
         }
-        // We don't eagerly strip `order` — stale entries are skipped at
-        // eviction time. This keeps remove() O(1).
+        inner.order.remove(&entry.seq);
         Some(entry)
     }
 
@@ -276,29 +284,27 @@ impl TxOrphanage {
         self.inner.lock().by_txid.contains_key(txid)
     }
 
-    fn pop_oldest(inner: &mut OrphanageInner) -> Option<Txid> {
-        while let Some(txid) = inner.order.pop_front() {
-            if inner.by_txid.contains_key(&txid) {
-                return Some(txid);
-            }
-            // Stale order entry (already removed via some other path).
-        }
-        None
+    fn oldest(inner: &OrphanageInner) -> Option<Txid> {
+        inner.order.first_key_value().map(|(_, txid)| *txid)
     }
 
+    /// The peer's oldest orphan: a scan of that peer's own orphans, at most
+    /// `config.max_per_peer` of them.
     fn oldest_for_peer(inner: &OrphanageInner, peer: PeerId) -> Option<Txid> {
-        let peer_set = inner.by_peer.get(&peer)?;
-        // Linear scan of order queue, picking the first that still
-        // belongs to this peer. Bounded by config.max_count.
-        for txid in &inner.order {
-            if peer_set.contains(txid) {
-                return Some(*txid);
-            }
-        }
-        None
+        inner
+            .by_peer
+            .get(&peer)?
+            .iter()
+            .filter_map(|txid| inner.by_txid.get(txid).map(|e| (e.seq, *txid)))
+            .min()
+            .map(|(_, txid)| txid)
     }
 }
 
+#[cfg(test)]
+#[path = "orphanage_order_tests.rs"]
+mod order_tests;
+
 #[cfg(test)]
 mod tests {
     use super::*;

node/src/mempool/orphanage_order_tests.rs

@@ -0,0 +1,114 @@
+//! The orphanage's eviction queue holds each live orphan exactly once.
+//!
+//! It kept the txid of every orphan it had ever held: a removal or an
+//! expiry left the txid behind, and only a full pool popped stale ones off
+//! the front. A peer at its per-peer quota scans the queue on every add, so
+//! the scan grew with every orphan that peer had sent.
+
+use super::*;
+use bitcoin::hashes::Hash;
+
+fn parent(i: u32) -> Txid {
+    let mut bytes = [0u8; 32];
+    bytes[..4].copy_from_slice(&i.to_le_bytes());
+    Txid::from_byte_array(bytes)
+}
+
+/// An orphan spending output 0 of [`parent`]`(i)`.
+fn orphan(i: u32) -> (Transaction, HashSet<Txid>) {
+    let tx = Transaction {
+        version: bitcoin::transaction::Version::TWO,
+        lock_time: bitcoin::absolute::LockTime::ZERO,
+        input: vec![bitcoin::TxIn {
+            previous_output: bitcoin::OutPoint { txid: parent(i), vout: 0 },
+            script_sig: bitcoin::ScriptBuf::new(),
+            sequence: bitcoin::Sequence::MAX,
+            witness: bitcoin::Witness::new(),
+        }],
+        output: vec![bitcoin::TxOut {
+            value: bitcoin::Amount::from_sat(1_000),
+            script_pubkey: bitcoin::ScriptBuf::new(),
+        }],
+    };
+    (tx, HashSet::from([parent(i)]))
+}
+
+fn add(pool: &TxOrphanage, i: u32, peer: PeerId) -> Txid {
+    let (tx, missing) = orphan(i);
+    let txid = tx.compute_txid();
+    assert_eq!(pool.add(tx, peer, missing).unwrap(), AddOutcome::Added);
+    txid
+}
+
+fn queued(pool: &TxOrphanage) -> usize {
+    pool.inner.lock().order.len()
+}
+
+/// One peer streaming orphans past its quota of 50: each add evicts that
+/// peer's oldest, and the queue stays the size of the pool.
+#[test]
+fn a_peer_at_its_quota_does_not_grow_the_queue() {
+    let pool = TxOrphanage::with_defaults();
+    for i in 0..10_000 {
+        add(&pool, i, 1);
+    }
+    assert_eq!(pool.len(), DEFAULT_MAX_ORPHAN_PER_PEER);
+    assert_eq!(queued(&pool), pool.len(), "the queue kept evicted orphans");
+}
+
+/// Removal and expiry take the orphan out of the queue too.
+#[test]
+fn removed_and_expired_orphans_leave_the_queue() {
+    let pool = TxOrphanage::with_defaults();
+    let ids: Vec<Txid> = (0..20).map(|i| add(&pool, i, i as PeerId)).collect();
+    for txid in &ids[..10] {
+        pool.remove(txid).expect("present");
+    }
+    assert_eq!(queued(&pool), 10, "removed orphans stayed queued");
+    pool.expire(Instant::now() + DEFAULT_ORPHAN_EXPIRY);
+    assert!(pool.is_empty());
+    assert_eq!(queued(&pool), 0, "expired orphans stayed queued");
+}
+
+/// An orphan that is removed and sent again queues as the newest, once.
+/// With its first place still queued, the full pool evicted it in place of
+/// the orphan that really was the oldest.
+#[test]
+fn a_returning_orphan_queues_as_the_newest() {
+    let pool = TxOrphanage::new(OrphanageConfig {
+        max_count: 2,
+        max_per_peer: 10,
+        expiry: DEFAULT_ORPHAN_EXPIRY,
+    });
+    let a = add(&pool, 1, 1);
+    let b = add(&pool, 2, 1);
+    pool.remove(&a).expect("present");
+    assert_eq!(add(&pool, 1, 1), a);
+    assert_eq!(queued(&pool), 2);
+
+    let c = add(&pool, 3, 1);
+    assert!(!pool.contains(&b), "the oldest orphan should make the room");
+    assert!(pool.contains(&a), "the orphan that came back was evicted as if it were the oldest");
+    assert!(pool.contains(&c));
+}
+
+/// The per-peer quota still evicts that peer's oldest orphan, and no one
+/// else's.
+#[test]
+fn the_quota_evicts_the_peers_own_oldest() {
+    let pool = TxOrphanage::new(OrphanageConfig {
+        max_count: 100,
+        max_per_peer: 3,
+        expiry: DEFAULT_ORPHAN_EXPIRY,
+    });
+    let first = add(&pool, 1, 1);
+    let other = add(&pool, 2, 2);
+    let second = add(&pool, 3, 1);
+    let third = add(&pool, 4, 1);
+    let fourth = add(&pool, 5, 1);
+    assert!(!pool.contains(&first));
+    for txid in [other, second, third, fourth] {
+        assert!(pool.contains(&txid));
+    }
+    assert_eq!(queued(&pool), 4);
+}