diff --git a/docs/manual/src/configuration.md b/docs/manual/src/configuration.md index 7bd0311be..010694ca1 100644 --- a/docs/manual/src/configuration.md +++ b/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. | diff --git a/node/src/mempool/orphanage.rs b/node/src/mempool/orphanage.rs index 54ef75594..4999455a9 100644 --- a/node/src/mempool/orphanage.rs +++ b/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,6 +81,8 @@ pub struct OrphanEntry { pub added_at: Instant, pub missing_parents: HashSet, pub bytes: usize, + /// Its key in the orphanage's arrival-order queue. + seq: u64, } struct OrphanageInner { @@ -88,10 +90,13 @@ struct OrphanageInner { by_parent: HashMap>, by_peer: HashMap>, 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, + /// 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, + /// 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 { - 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 { + 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 { - 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::*; diff --git a/node/src/mempool/orphanage_order_tests.rs b/node/src/mempool/orphanage_order_tests.rs new file mode 100644 index 000000000..bbf919e07 --- /dev/null +++ b/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) { + 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 = (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); +} diff --git a/node/src/mempool/pool.rs b/node/src/mempool/pool.rs index 9056ab32d..6060cd761 100644 --- a/node/src/mempool/pool.rs +++ b/node/src/mempool/pool.rs @@ -1564,6 +1564,17 @@ impl Mempool { } } + /// `LeaveEvicted` for each evicted entry that was acting. A held entry + /// never emitted an `Enter`, so a `Leave` for it would be a phantom that + /// leaks a withheld txid (§10). + fn emit_evicted(&self, evicted: &[(Txid, QuarantineScope)], reason: EvictReason) { + for (txid, scope) in evicted { + if scope.is_acting() { + self.emit(MempoolEvent::LeaveEvicted { txid: *txid, reason }); + } + } + } + /// Emit a quarantine-class event on the separate channel (§10). Best-effort, /// no-op when no sender is wired. fn emit_quarantine(&self, event: QuarantineEvent) { @@ -1634,9 +1645,31 @@ impl Mempool { /// Swap in a new mempool/relay policy live (SIGHUP config reload). Takes /// effect on the next `accept_transaction` call; already-admitted entries - /// are not re-evaluated. + /// are not re-evaluated, except that a lower size limit trims the pool to + /// it at once. pub fn reload_policy(&self, new: MempoolConfig) { + let (max_size_bytes, quarantine_max_bytes, incremental_relay_fee) = + (new.max_size_bytes, new.quarantine_max_bytes, new.incremental_relay_fee); *self.config.write() = new; + + // Core never leaves the pool over `-maxmempool`: every admission ends + // in `LimitMempoolSize` → `TrimToSize` (src/validation.cpp). Left over a + // lowered limit, the pool refused every admission until it drained. + let none = HashSet::new(); + let (acting, held) = { + let mut inner = self.inner.write(); + let over = inner.acting_bytes().saturating_sub(max_size_bytes); + let acting = + Self::evict_lowest_fee_entries_except(&mut inner, over, false, incremental_relay_fee, &none); + let over = inner.quarantine_bytes.saturating_sub(quarantine_max_bytes); + let held = + Self::evict_lowest_fee_entries_except(&mut inner, over, true, incremental_relay_fee, &none); + self.sync_unbroadcast_len(&inner); + (acting, held) + }; + // Reported as admission reports them: only what was acting. + self.emit_evicted(&acting, EvictReason::FullPool); + self.emit_evicted(&held, EvictReason::Policy); } /// Load (or replace) the transaction-filtering ruleset from a file. Returns @@ -2993,21 +3026,54 @@ impl Mempool { } else { EvictReason::FullPool }; - let mut evicted_full_pool: Vec = Vec::new(); - if class_bytes + tx_size > class_budget { - // Only evict if the new tx outbids the cheapest entry *in its own class*. - let min_class_fee_rate = inner + let needs_room = class_bytes + tx_size > class_budget; + if needs_room { + // Only evict if the new tx outbids the cheapest entry *in its own + // class* that it may evict. Its own in-pool ancestors stay (see the + // eviction below), so they are not that bar. + let Some(min_class_fee_rate) = inner .entries - .values() - .filter(|e| e.scope.is_quarantined() == quarantined) - .map(|e| e.fee_rate) + .iter() + .filter(|(t, e)| e.scope.is_quarantined() == quarantined && !ancestors.contains(*t)) + .map(|(_, e)| e.fee_rate) .min() - .unwrap_or(0); + else { + return Err(MempoolError::MempoolFull); + }; if fee_rate <= min_class_fee_rate { return Err(MempoolError::MempoolFull); } - // Evict enough lowest-fee-rate entries *of this class* to make room. - evicted_full_pool = Self::evict_lowest_fee_entries(&mut inner, tx_size, quarantined, cfg.incremental_relay_fee); + } + + // Script verification (all inputs at once for taproot), before + // anything leaves the pool to make room. Core runs PolicyScriptChecks + // and ConsensusScriptChecks before it adds the transaction and trims + // the pool (src/validation.cpp, AcceptSingleTransactionInternal), so a + // transaction whose scripts fail changes nothing. + // Use tip_height + 1 since the tx will be mined in the next block + if let Err(e) = script_verifier.verify_transaction_unnamed(&tx, &prev_outputs, tip_height + 1) { + // Naming the script error can mean running the second engine over + // the input. Nothing has changed in the pool, so release it first. + drop(inner); + let e = script_verifier.name_failure(&tx, &prev_outputs, tip_height + 1, e); + return Err(MempoolError::Script(e.reason().to_string(), e.core_debug_string(&tx))); + } + + let mut evicted_full_pool: Vec<(Txid, QuarantineScope)> = Vec::new(); + if needs_room { + // Evict lowest-fee-rate entries *of this class* until the pool, + // with this transaction in it, is back within its limit: Core's + // `TrimToSize` (src/txmempool.cpp). Freeing only this transaction's + // size left a pool that was already over (a lowered `-maxmempool`) + // over, and refused everything. Never this transaction's own + // ancestors. + evicted_full_pool = Self::evict_lowest_fee_entries_except( + &mut inner, + class_bytes + tx_size - class_budget, + quarantined, + cfg.incremental_relay_fee, + &ancestors, + ); self.sync_unbroadcast_len(&inner); // If still not enough room after eviction, reject. let class_bytes_after = if quarantined { @@ -3016,18 +3082,13 @@ impl Mempool { inner.acting_bytes() }; if class_bytes_after + tx_size > class_budget { + drop(inner); + // The evicted entries are gone whether or not this one gets in. + self.emit_evicted(&evicted_full_pool, evict_reason); return Err(MempoolError::MempoolFull); } } - // Script verification (all inputs at once for taproot) - // Use tip_height + 1 since the tx will be mined in the next block - script_verifier - .verify_transaction(&tx, &prev_outputs, tip_height + 1) - .map_err(|e| { - MempoolError::Script(e.reason().to_string(), e.core_debug_string(&tx)) - })?; - // RBF: remove conflicted transactions before inserting replacement. // Collect replaced txids so we can emit LeaveReplaced events after the // write lock is dropped. Only *acting* conflicts go on the standard @@ -3108,19 +3169,10 @@ impl Mempool { // are best-effort but keeping the lock duration tight is the rule. drop(inner); - // `evicted_full_pool` is always drawn from the incoming tx's own class - // (`evict_lowest_fee_entries` filters to `quarantined`). Acting-class - // evictions go on the standard stream as before; quarantine-class - // evictions never emitted an `Enter`, so a `LeaveEvicted` for them would - // be a phantom Leave leaking a withheld txid (§10) — suppress it. - if !quarantined { - for evicted_txid in &evicted_full_pool { - self.emit(MempoolEvent::LeaveEvicted { - txid: *evicted_txid, - reason: evict_reason, - }); - } - } + // Acting-class evictions go on the standard stream as before; + // quarantine-class ones never emitted an `Enter`, so a `LeaveEvicted` + // for them would be a phantom Leave leaking a withheld txid (§10). + self.emit_evicted(&evicted_full_pool, evict_reason); for conflict_txid in &replaced { self.emit(MempoolEvent::LeaveReplaced { txid: *conflict_txid, @@ -6062,12 +6114,13 @@ impl Mempool { // Skip fee rate check — this is the bypass. - // Script verification - script_verifier - .verify_transaction(&tx, &prev_outputs, tip_height + 1) - .map_err(|e| { - MempoolError::Script(e.reason().to_string(), e.core_debug_string(&tx)) - })?; + // Script verification; the failure is named once the pool is released, + // as in `accept_transaction`. + if let Err(e) = script_verifier.verify_transaction_unnamed(&tx, &prev_outputs, tip_height + 1) { + drop(inner); + let e = script_verifier.name_failure(&tx, &prev_outputs, tip_height + 1, e); + return Err(MempoolError::Script(e.reason().to_string(), e.core_debug_string(&tx))); + } let fee_rate = policy::fee_rate_sat_per_kvb(fee, weight as u64); @@ -6131,7 +6184,43 @@ impl Mempool { want_quarantined: bool, incremental_relay_fee: u64, ) -> Vec { - // Sort *this class's* entries by fee rate ascending; the other class is + Self::evict_lowest_fee_entries_except( + inner, + bytes_needed, + want_quarantined, + incremental_relay_fee, + &HashSet::new(), + ) + .into_iter() + .map(|(txid, _)| txid) + .collect() + } + + /// [`evict_lowest_fee_entries`](Self::evict_lowest_fee_entries), never + /// choosing an entry in `keep`. Admission passes the incoming + /// transaction's in-pool ancestors: evicting one would leave that + /// transaction spending an output nothing provides. An ancestor set is + /// closed under "parent of", so no kept entry descends from an evicted one + /// either. + /// + /// The cost follows what is evicted, not the size of the pool: one pass + /// builds a heap of the class, each eviction pops it, and descendants come + /// from the `spends` index. + /// + /// Each evicted txid comes with the scope it had: a held descendant of an + /// evicted acting entry goes with it, but never emitted an `Enter`, so it + /// gets no `LeaveEvicted` (§10). + fn evict_lowest_fee_entries_except( + inner: &mut MempoolInner, + bytes_needed: usize, + want_quarantined: bool, + incremental_relay_fee: u64, + keep: &HashSet, + ) -> Vec<(Txid, QuarantineScope)> { + if bytes_needed == 0 { + return Vec::new(); + } + // *This class's* entries, lowest fee rate first; the other class is // never an eviction candidate (its own budget governs it). // // On the *modified* fee rate. `entry.fee_rate` is the rate as of @@ -6140,55 +6229,49 @@ impl Mempool { // delta did not exist — which defeats the point of the RPC. Core // evicts on the modified feerate (`CTxMemPool::TrimToSize` sorts by // descendant-modified feerate). - let mut by_fee_rate: Vec<(Txid, u64)> = inner + let mut by_fee_rate: std::collections::BinaryHeap> = inner .entries .iter() - .filter(|(_, entry)| entry.scope.is_quarantined() == want_quarantined) + .filter(|(txid, entry)| { + entry.scope.is_quarantined() == want_quarantined && !keep.contains(*txid) + }) .map(|(txid, entry)| { - ( - *txid, + std::cmp::Reverse(( policy::fee_rate_sat_per_kvb( modified_fee(entry.fee, entry.fee_delta), entry.weight as u64, ), - ) + *txid, + )) }) .collect(); - by_fee_rate.sort_by_key(|(_, rate)| *rate); let mut freed = 0usize; let mut to_remove: Vec = Vec::new(); + // What `to_remove` holds, for membership tests. + let mut removing: HashSet = HashSet::new(); + let mut children: Vec = Vec::new(); - for (txid, _) in &by_fee_rate { - if freed >= bytes_needed { + while freed < bytes_needed { + let Some(std::cmp::Reverse((_, txid))) = by_fee_rate.pop() else { break; - } - if to_remove.contains(txid) { + }; + if !removing.insert(txid) { continue; } let package_start = to_remove.len(); - to_remove.push(*txid); - if let Some(entry) = inner.entries.get(txid) { + to_remove.push(txid); + if let Some(entry) = inner.entries.get(&txid) { freed += bitcoin::consensus::serialize(&entry.tx).len(); } // Also collect descendants of the evicted entry - let mut desc_queue = vec![*txid]; + let mut desc_queue = vec![txid]; while let Some(current) = desc_queue.pop() { - let current_txid_for_search = current; - let children: Vec = inner - .entries - .iter() - .filter(|(child_txid, child_entry)| { - !to_remove.contains(child_txid) - && child_entry - .tx - .input - .iter() - .any(|i| i.previous_output.txid == current_txid_for_search) - }) - .map(|(child_txid, _)| *child_txid) - .collect(); - for child in children { + Self::collect_children(inner, ¤t, &mut children); + for child in children.drain(..) { + if !removing.insert(child) { + continue; + } if let Some(child_entry) = inner.entries.get(&child) { // A cross-class descendant (e.g. a quarantined child of an // evicted acting parent) must still be removed for graph @@ -6220,6 +6303,7 @@ impl Mempool { } } + let mut evicted = Vec::with_capacity(to_remove.len()); for txid in &to_remove { if let Some(entry) = inner.entries.remove(txid) { let tx_size = bitcoin::consensus::serialize(&entry.tx).len(); @@ -6229,13 +6313,14 @@ impl Mempool { } inner.unbroadcast.remove(txid); tracing::debug!(%txid, fee_rate = entry.fee_rate, "Evicted low-fee tx from mempool"); + evicted.push((*txid, entry.scope)); } } - if !to_remove.is_empty() { - tracing::info!(evicted = to_remove.len(), "Mempool eviction complete"); + if !evicted.is_empty() { + tracing::info!(evicted = evicted.len(), "Mempool eviction complete"); } - to_remove + evicted } /// Keep the lock-free `unbroadcast_len` mirror coherent with the map. @@ -6430,6 +6515,11 @@ impl Mempool { #[cfg(test)] pub(crate) fn insert_entry_for_test(&self, txid: Txid, tx: Transaction, fee_rate: u64) { let mut inner = self.inner.write(); + // Every admission indexes the entry's spends, and eviction finds + // descendants through them. + for input in &tx.input { + inner.spends.insert(input.previous_output, txid); + } inner.entries.insert( txid, MempoolEntry { @@ -6607,6 +6697,11 @@ impl Mempool { let txid = tx.compute_txid(); let tx_size = bitcoin::consensus::serialize(&tx).len(); let mut inner = self.inner.write(); + // Every admission indexes the entry's spends, and eviction finds + // descendants through them. + for input in &tx.input { + inner.spends.insert(input.previous_output, txid); + } inner.entries.insert( txid, MempoolEntry { @@ -6635,6 +6730,10 @@ impl Mempool { #[path = "pool_sigops_tests.rs"] mod sigops_tests; +#[cfg(test)] +#[path = "pool_fullpool_tests.rs"] +mod fullpool_tests; + #[cfg(test)] mod tests { @@ -8802,6 +8901,11 @@ mod tests { let txid = tx.compute_txid(); let tx_size = bitcoin::consensus::serialize(&tx).len(); let mut inner = mp.inner.write(); + // Index the spends as every admission does: eviction finds a + // descendant through them. + for i in &tx.input { + inner.spends.insert(i.previous_output, txid); + } inner.entries.insert( txid, MempoolEntry { diff --git a/node/src/mempool/pool_fullpool_tests.rs b/node/src/mempool/pool_fullpool_tests.rs new file mode 100644 index 000000000..b90f767bd --- /dev/null +++ b/node/src/mempool/pool_fullpool_tests.rs @@ -0,0 +1,473 @@ +//! Admission into a full pool, checked against Bitcoin Core's +//! `MemPoolAccept` (src/validation.cpp, `AcceptSingleTransactionInternal`): +//! the scripts are checked (`PolicyScriptChecks`, `ConsensusScriptChecks`) +//! before anything leaves the pool, and the pool is then trimmed to its +//! limit (`LimitMempoolSize` → `CTxMemPool::TrimToSize`). +//! +//! satd frees room before it inserts rather than trimming after, so it also +//! has to keep the incoming transaction's own ancestors: Core's trim takes a +//! parent together with the child that pays for it, never the parent alone. + +use super::*; +use crate::mining::template::tests::make_funded_template_env_with; +use crate::storage::coinview::Coin; +use crate::validation::script::{NoopVerifier, ScriptError}; +use bitcoin::hashes::Hash; +use bitcoin::{Amount, Sequence, TxIn, Witness}; + +const COIN_VALUE: u64 = 100_000; + +fn prev(tag: u8) -> OutPoint { + OutPoint { txid: Txid::from_byte_array([tag; 32]), vout: 0 } +} + +fn p2wpkh(tag: u8) -> ScriptBuf { + let mut spk = vec![0x00, 0x14]; + spk.extend_from_slice(&[tag; 20]); + ScriptBuf::from_bytes(spk) +} + +fn coin() -> Coin { + Coin { + amount: COIN_VALUE, + script_pubkey: p2wpkh(0x11), + height: 1, + coinbase: false, + txseq: node_index::TXSEQ_UNKNOWN, + } +} + +/// Spend `prevs` (worth `in_value` together), paying `fee` and splitting the +/// rest over `outs` P2WPKH outputs. One input and one output is 82 bytes; +/// each further output adds 31. +fn spend(prevs: &[OutPoint], in_value: u64, fee: u64, outs: u64, tag: u8) -> Transaction { + let each = (in_value - fee) / outs; + let mut output: Vec = (0..outs) + .map(|_| TxOut { value: Amount::from_sat(each), script_pubkey: p2wpkh(tag) }) + .collect(); + output[0].value = Amount::from_sat(in_value - fee - each * (outs - 1)); + Transaction { + version: bitcoin::transaction::Version(2), + lock_time: bitcoin::absolute::LockTime::ZERO, + input: prevs + .iter() + .map(|p| TxIn { + previous_output: *p, + script_sig: ScriptBuf::new(), + sequence: Sequence::MAX, + witness: Witness::new(), + }) + .collect(), + output, + } +} + +/// A regtest chain holding one coin per tag, and an empty pool with no relay +/// floor. +fn env(tags: &[u8]) -> (ChainState, Mempool, std::path::PathBuf) { + let coins: Vec<(OutPoint, Coin)> = tags.iter().map(|t| (prev(*t), coin())).collect(); + let (cs, mp, dir) = make_funded_template_env_with(&coins, Box::new(NoopVerifier)); + *mp.config.write() = + MempoolConfig { max_size_bytes: 1_000_000, min_fee_rate: 0, ..Default::default() }; + (cs, mp, dir) +} + +/// Set the size limit without going through `reload_policy`. +fn set_max(mp: &Mempool, max: usize) { + mp.config.write().max_size_bytes = max; +} + +fn admit(mp: &Mempool, cs: &ChainState, tx: Transaction) -> Txid { + mp.accept_transaction(tx, cs, &NoopVerifier, TxSource::P2p, false).expect("admitted") +} + +fn pool(mp: &Mempool) -> HashSet { + mp.inner.read().entries.keys().copied().collect() +} + +fn events(mp: &Mempool) -> broadcast::Receiver { + let (tx, rx) = broadcast::channel(1024); + mp.set_event_sender(tx); + rx +} + +/// The `LeaveEvicted { FullPool }` events sent so far. +fn evicted(rx: &mut broadcast::Receiver) -> Vec { + let mut out = Vec::new(); + while let Ok(ev) = rx.try_recv() { + if let MempoolEvent::LeaveEvicted { txid, reason: EvictReason::FullPool } = ev { + out.push(txid); + } + } + out +} + +/// Fails every transaction's scripts, as libconsensus does a forged signature. +struct Refuses; + +impl ScriptVerifier for Refuses { + fn verify_transaction( + &self, + _tx: &Transaction, + _prev_outputs: &[TxOut], + _height: u32, + ) -> Result<(), ScriptError> { + Err(ScriptError::VerifyFailed { input: 0, reason: "Invalid Schnorr signature".into() }) + } +} + +/// A transaction that outbids the pool but fails its scripts takes nothing +/// out of it: no entry, no rise in the minimum fee, no `LeaveEvicted`. The +/// same transaction with valid scripts then takes the one entry it needs. +#[test] +fn a_script_failure_on_a_full_pool_evicts_nothing() { + let (cs, mp, dir) = env(&[1, 2, 3, 4]); + for t in 1..=3 { + admit(&mp, &cs, spend(&[prev(t)], COIN_VALUE, 100, 1, t)); + } + let before = pool(&mp); + set_max(&mp, mp.acting_bytes()); + let floor = mp.info().mempool_min_fee; + let mut rx = events(&mp); + + let rich = spend(&[prev(4)], COIN_VALUE, 5_000, 1, 4); + let err = mp + .accept_transaction(rich.clone(), &cs, &Refuses, TxSource::P2p, false) + .expect_err("its scripts fail"); + assert!(matches!(err, MempoolError::Script(..)), "{err:?}"); + assert_eq!(pool(&mp), before, "a transaction whose scripts fail evicted entries"); + assert_eq!(mp.info().mempool_min_fee, floor, "a transaction whose scripts fail raised the floor"); + assert!(evicted(&mut rx).is_empty()); + + let txid = mp + .accept_transaction(rich, &cs, &NoopVerifier, TxSource::P2p, false) + .expect("admitted"); + let gone = evicted(&mut rx); + assert_eq!(gone.len(), 1, "one 82-byte entry makes room for one 82-byte transaction"); + assert!(before.contains(&gone[0])); + assert!(pool(&mp).contains(&txid)); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// A child whose parent is the cheapest entry keeps its parent: the room +/// comes from the cheapest entry that is not one of its ancestors. Evicting +/// the parent left the child in the pool with an input nothing provides. +#[test] +fn a_child_keeps_its_parent_in_a_full_pool() { + let (cs, mp, dir) = env(&[1, 2, 3]); + let parent = admit(&mp, &cs, spend(&[prev(1)], COIN_VALUE, 100, 1, 1)); + let second = admit(&mp, &cs, spend(&[prev(2)], COIN_VALUE, 200, 1, 2)); + let third = admit(&mp, &cs, spend(&[prev(3)], COIN_VALUE, 300, 1, 3)); + set_max(&mp, mp.acting_bytes()); + + let child = admit( + &mp, + &cs, + spend(&[OutPoint { txid: parent, vout: 0 }], COIN_VALUE - 100, 10_000, 1, 4), + ); + let now = pool(&mp); + assert!(now.contains(&parent), "the child's own parent was evicted to make room for it"); + assert!(!now.contains(&second), "the cheapest entry that is not an ancestor makes the room"); + assert!(now.contains(&third)); + assert!(now.contains(&child)); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// A child that outbids only its own parent has nothing it may evict, so it +/// is refused and the pool is unchanged. (Core trims the parent and the child +/// together as the worst-paying chunk and answers "mempool full".) +#[test] +fn a_child_that_outbids_only_its_parent_is_refused() { + let (cs, mp, dir) = env(&[1, 2, 3]); + let parent = admit(&mp, &cs, spend(&[prev(1)], COIN_VALUE, 100, 1, 1)); + admit(&mp, &cs, spend(&[prev(2)], COIN_VALUE, 2_000, 1, 2)); + admit(&mp, &cs, spend(&[prev(3)], COIN_VALUE, 2_000, 1, 3)); + set_max(&mp, mp.acting_bytes()); + let before = pool(&mp); + + let child = spend(&[OutPoint { txid: parent, vout: 0 }], COIN_VALUE - 100, 1_000, 1, 4); + let err = mp + .accept_transaction(child, &cs, &NoopVerifier, TxSource::P2p, false) + .expect_err("it pays less than every entry it could evict"); + assert!(matches!(err, MempoolError::MempoolFull), "{err:?}"); + assert_eq!(pool(&mp), before); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// When the room still runs short after eviction, the transaction is refused, +/// and what was evicted on the way is reported: those entries are gone. +#[test] +fn a_full_pool_refusal_reports_its_evictions() { + let (cs, mp, dir) = env(&[1, 2]); + let parent = admit(&mp, &cs, spend(&[prev(1)], COIN_VALUE, 200, 1, 1)); + let cheap = admit(&mp, &cs, spend(&[prev(2)], COIN_VALUE, 100, 1, 2)); + set_max(&mp, mp.acting_bytes()); + let mut rx = events(&mp); + + // 144 bytes, and only the 82-byte `cheap` can go. + let child = spend(&[OutPoint { txid: parent, vout: 0 }], COIN_VALUE - 200, 10_000, 3, 3); + let err = mp + .accept_transaction(child, &cs, &NoopVerifier, TxSource::P2p, false) + .expect_err("evicting everything but its parent does not make enough room"); + assert!(matches!(err, MempoolError::MempoolFull), "{err:?}"); + assert_eq!(pool(&mp), HashSet::from([parent])); + assert_eq!(evicted(&mut rx), vec![cheap], "an eviction was never reported"); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// Lowering `-maxmempool` on a live node trims the pool to the new limit at +/// once, lowest feerate first, and reports each eviction. Left over the limit, +/// every later admission freed one transaction's worth and was refused. +#[test] +fn lowering_maxmempool_trims_the_pool_at_once() { + let (cs, mp, dir) = env(&[1, 2, 3, 4, 5]); + let ids: Vec = (1..=4u8) + .map(|t| admit(&mp, &cs, spend(&[prev(t)], COIN_VALUE, 100 * t as u64, 1, t))) + .collect(); + let two = mp.acting_bytes() / 2; + let mut rx = events(&mp); + + let cfg = MempoolConfig { max_size_bytes: two, ..mp.config.read().clone() }; + mp.reload_policy(cfg); + assert!(mp.acting_bytes() <= two, "{} bytes over a {two}-byte limit", mp.acting_bytes()); + assert_eq!(pool(&mp), HashSet::from([ids[2], ids[3]])); + let mut gone = evicted(&mut rx); + gone.sort(); + let mut lowest = vec![ids[0], ids[1]]; + lowest.sort(); + assert_eq!(gone, lowest); + + let txid = admit(&mp, &cs, spend(&[prev(5)], COIN_VALUE, 10_000, 1, 5)); + assert!(pool(&mp).contains(&txid)); + assert!(mp.acting_bytes() <= two); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// An admission into a pool that is over its limit trims it to the limit, +/// as Core's `TrimToSize` does, instead of freeing only its own size and +/// refusing because the pool is still over. +#[test] +fn an_admission_trims_an_over_limit_pool() { + let (cs, mp, dir) = env(&[1, 2, 3, 4, 5]); + let ids: Vec = (1..=4u8) + .map(|t| admit(&mp, &cs, spend(&[prev(t)], COIN_VALUE, 100 * t as u64, 1, t))) + .collect(); + let two = mp.acting_bytes() / 2; + set_max(&mp, two); + + let txid = mp + .accept_transaction(spend(&[prev(5)], COIN_VALUE, 10_000, 1, 5), &cs, &NoopVerifier, TxSource::P2p, false) + .expect("the pool is trimmed to its limit and the transaction admitted"); + assert_eq!(pool(&mp), HashSet::from([ids[3], txid])); + assert!(mp.acting_bytes() <= two); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// Evicting a parent takes every descendant once: a child spending two of +/// its outputs, a child spending the third, and a grandchild. The spend +/// index and the byte count follow. +#[test] +fn evicting_a_parent_takes_every_descendant() { + let (cs, mp, dir) = env(&[1, 2]); + let parent = admit(&mp, &cs, spend(&[prev(1)], COIN_VALUE, 100, 3, 1)); + let out = |txid, vout| OutPoint { txid, vout }; + let per = (COIN_VALUE - 100) / 3; + let two_outs = admit(&mp, &cs, spend(&[out(parent, 1), out(parent, 2)], 2 * per, 5_000, 1, 2)); + let one_out = admit(&mp, &cs, spend(&[out(parent, 0)], COIN_VALUE - 100 - 2 * per, 5_000, 1, 3)); + let grandchild = admit(&mp, &cs, spend(&[out(two_outs, 0)], 2 * per - 5_000, 5_000, 1, 4)); + let other = admit(&mp, &cs, spend(&[prev(2)], COIN_VALUE, 1_000, 1, 5)); + let other_size = bitcoin::consensus::serialize(&mp.get(&other).unwrap().tx).len(); + + let mut inner = mp.inner.write(); + let mut gone = Mempool::evict_lowest_fee_entries(&mut inner, 1, false, 0); + assert_eq!(inner.entries.keys().copied().collect::>(), HashSet::from([other])); + assert_eq!(inner.spends.len(), 1); + assert_eq!(inner.acting_bytes(), other_size); + drop(inner); + gone.sort(); + let mut family = vec![parent, two_outs, one_out, grandchild]; + family.sort(); + assert_eq!(gone, family, "each descendant is evicted, and only once"); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// The cost of eviction follows what it evicts, not the pool size times the +/// evictions. 600 evictions from a pool of 20,000 scanned the whole pool once +/// per eviction, each scan checking a growing list of what was already +/// chosen: billions of comparisons under the pool's write lock. +#[test] +fn eviction_cost_follows_what_it_evicts() { + const N: u32 = 20_000; + const EVICT: usize = 600; + let mp = Mempool::with_config(MempoolConfig { + max_size_bytes: usize::MAX, + min_fee_rate: 0, + ..Default::default() + }); + let mut inner = mp.inner.write(); + let mut by_rate: Vec = Vec::new(); + let mut size = 0; + for i in 0..N { + let mut bytes = [0u8; 32]; + bytes[..4].copy_from_slice(&i.to_le_bytes()); + let tx = spend(&[OutPoint { txid: Txid::from_byte_array(bytes), vout: 0 }], COIN_VALUE, 1_000, 1, 7); + let txid = tx.compute_txid(); + size = bitcoin::consensus::serialize(&tx).len(); + inner.spends.insert(tx.input[0].previous_output, txid); + // A distinct feerate per entry, highest first. + let fee = 1_000 + (N - i) as u64; + inner.entries.insert( + txid, + MempoolEntry { + tx, + fee, + weight: 4 * size, + fee_rate: policy::fee_rate_sat_per_kvb(fee, 4 * size as u64), + time: 0, + fee_delta: 0, + sigop_cost: 0, + prev_scripthashes: Vec::new(), + prev_amounts: Vec::new(), + prev_scripts: Vec::new(), + sp_tweak: None, + source: TxSource::P2p, + scope: QuarantineScope::acting(), + quarantine_rule: None, + }, + ); + inner.account_insert(QuarantineScope::acting(), size); + by_rate.push(txid); + } + + let start = std::time::Instant::now(); + let gone = Mempool::evict_lowest_fee_entries(&mut inner, EVICT * size, false, 0); + let took = start.elapsed(); + drop(inner); + + let mut expected: Vec = by_rate[by_rate.len() - EVICT..].to_vec(); + expected.sort(); + let mut got = gone.clone(); + got.sort(); + assert_eq!(got, expected, "the {EVICT} lowest feerates go"); + assert!(took < std::time::Duration::from_secs(2), "evicting {EVICT} of {N} took {took:?}"); +} + +/// Fails every transaction with libbitcoinconsensus's coarse reason, and +/// records whether the pool was free when asked to name the failure. +struct NamesWhenAsked<'a> { + mp: &'a Mempool, + pool_free: Mutex>, +} + +impl ScriptVerifier for NamesWhenAsked<'_> { + fn verify_transaction( + &self, + _tx: &Transaction, + _prev_outputs: &[TxOut], + _height: u32, + ) -> Result<(), ScriptError> { + Err(ScriptError::VerifyFailed { input: 0, reason: "ERR_SCRIPT".into() }) + } + + fn name_failure( + &self, + _tx: &Transaction, + _prev_outputs: &[TxOut], + _height: u32, + err: ScriptError, + ) -> ScriptError { + *self.pool_free.lock() = Some(self.mp.inner.try_write().is_some()); + ScriptError::VerifyFailed { input: err.input(), reason: "Invalid Schnorr signature".into() } + } +} + +/// The rejection carries the specific script error, and finding it (which +/// can mean running the second script engine over the input) happens after +/// the pool is released, so block connection and template building do not +/// wait on it. +#[test] +fn a_script_failure_is_named_after_the_pool_is_released() { + let (cs, mp, dir) = env(&[1]); + let verifier = NamesWhenAsked { mp: &mp, pool_free: Mutex::new(None) }; + let err = mp + .accept_transaction(spend(&[prev(1)], COIN_VALUE, 1_000, 1, 1), &cs, &verifier, TxSource::P2p, false) + .expect_err("its scripts fail"); + match err { + MempoolError::Script(reason, _) => assert_eq!(reason, "Invalid Schnorr signature"), + other => panic!("{other:?}"), + } + assert_eq!(*verifier.pool_free.lock(), Some(true), "the failure was named under the pool's write lock"); + assert!(pool(&mp).is_empty()); + + let _ = std::fs::remove_dir_all(&dir); +} + +const HELD: QuarantineScope = QuarantineScope { relay: true, template: true }; + +/// An acting parent with a held child, and a better-paying acting entry. +/// The held child never emitted an `Enter`, so it must not be reported as +/// leaving the standard stream when it goes with its parent. +fn parent_with_held_child(tags: &[u8]) -> (ChainState, Mempool, std::path::PathBuf, Txid, Txid) { + let (cs, mp, dir) = env(tags); + let parent = admit(&mp, &cs, spend(&[prev(tags[0])], COIN_VALUE, 100, 1, tags[0])); + admit(&mp, &cs, spend(&[prev(tags[1])], COIN_VALUE, 2_000, 1, tags[1])); + let child = mp.insert_tx_scoped_for_test( + spend(&[OutPoint { txid: parent, vout: 0 }], COIN_VALUE - 100, 1_000, 1, 0x40), + HELD, + ); + // Unexpired, so the expiry sweep after an admission leaves it alone. + mp.inner.write().entries.get_mut(&child).unwrap().time = crate::time::now_secs(); + (cs, mp, dir, parent, child) +} + +/// Admission evicts the acting parent, and the held child with it, and +/// reports only the parent. +#[test] +fn a_held_descendant_evicted_on_admission_is_not_reported() { + let (cs, mp, dir, parent, child) = parent_with_held_child(&[1, 2, 3]); + set_max(&mp, mp.acting_bytes()); + let mut rx = events(&mp); + + admit(&mp, &cs, spend(&[prev(3)], COIN_VALUE, 10_000, 1, 3)); + let now = pool(&mp); + assert!(!now.contains(&parent) && !now.contains(&child)); + assert_eq!(evicted(&mut rx), vec![parent], "a held entry was reported leaving the standard stream"); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// The same on a live `-maxmempool` decrease. +#[test] +fn a_held_descendant_trimmed_on_reload_is_not_reported() { + let (_cs, mp, dir, parent, child) = parent_with_held_child(&[1, 2]); + let mut rx = events(&mp); + + let cfg = MempoolConfig { max_size_bytes: mp.acting_bytes() - 1, ..mp.config.read().clone() }; + mp.reload_policy(cfg); + let now = pool(&mp); + assert!(!now.contains(&parent) && !now.contains(&child)); + assert_eq!(evicted(&mut rx), vec![parent], "a held entry was reported leaving the standard stream"); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// The test insert helpers index spends as every admission does, so a pool +/// built with them has the shape eviction expects. +#[test] +fn test_inserted_entries_index_their_spends() { + let mp = Mempool::new(1_000_000, 0); + let parent = mp.insert_tx_scoped_for_test(spend(&[prev(1)], COIN_VALUE, 100, 1, 1), QuarantineScope::acting()); + let child = spend(&[OutPoint { txid: parent, vout: 0 }], COIN_VALUE - 100, 100, 1, 2); + let child_txid = child.compute_txid(); + mp.insert_entry_for_test(child_txid, child, 1); + let inner = mp.inner.read(); + assert_eq!(inner.spends.get(&prev(1)), Some(&parent)); + assert_eq!(inner.spends.get(&OutPoint { txid: parent, vout: 0 }), Some(&child_txid)); +} diff --git a/node/src/validation/script.rs b/node/src/validation/script.rs index 8f03bf38c..f8892c0f0 100644 --- a/node/src/validation/script.rs +++ b/node/src/validation/script.rs @@ -74,6 +74,34 @@ pub trait ScriptVerifier: Send + Sync { height: u32, ) -> Result<(), ScriptError>; + /// [`verify_transaction`](Self::verify_transaction), except that a + /// failure may come back with a coarse reason. Pass it to + /// [`name_failure`](Self::name_failure) for the specific one. Finding + /// that name can mean running a second script engine over the input, so + /// a caller holding a lock can verify with this, release the lock, and + /// name the failure afterwards. + fn verify_transaction_unnamed( + &self, + tx: &Transaction, + prev_outputs: &[TxOut], + height: u32, + ) -> Result<(), ScriptError> { + self.verify_transaction(tx, prev_outputs, height) + } + + /// Give a failure from + /// [`verify_transaction_unnamed`](Self::verify_transaction_unnamed) its + /// specific reason. The verdict never changes; only the reason can. + fn name_failure( + &self, + _tx: &Transaction, + _prev_outputs: &[TxOut], + _height: u32, + err: ScriptError, + ) -> ScriptError { + err + } + /// If this verifier runs in shadow mode, return the shadow engine. /// Currently unused — ShadowVerifier handles async dispatch internally. fn shadow_verifier(&self) -> Option<&dyn ScriptVerifier> { @@ -309,8 +337,12 @@ impl ConsensusVerifier { /// revisited: libconsensus has already rejected this input and that stands. /// This runs only on the failure path, and only to write the message. /// +/// Only the input that failed is run. libconsensus has already passed every +/// input before it, so running those again would repeat that work for +/// nothing. +/// /// If the two engines disagree about this input — the native one finding it -/// valid — there is no name to borrow, and the coarse error is reported as +/// valid — there is no name to borrow, and `None` leaves the coarse error as /// it was. That disagreement is a parity bug worth knowing about, so it is /// logged rather than papered over. fn name_script_failure( @@ -318,17 +350,29 @@ fn name_script_failure( prev_outputs: &[TxOut], flags: u32, input: usize, - coarse: bitcoinconsensus::Error, -) -> String { - match consensus::verify_transaction(tx, prev_outputs, flags) { - Err((idx, consensus::Error::ErrScript(e))) if idx == input => e.as_str().to_string(), - _ => { +) -> Option { + let (txin, prev) = (tx.input.get(input)?, prev_outputs.get(input)?); + // The BIP 341 signature hash commits to every spent output; nothing + // else reads them (as in `consensus::verify_transaction`). + let checker_prevs: &[TxOut] = + if flags & consensus::VERIFY_TAPROOT != 0 { prev_outputs } else { &[] }; + let checker = consensus::sighash::TxSignatureChecker::new(tx, input, prev.value, checker_prevs); + let witness: Vec> = txin.witness.iter().map(|w| w.to_vec()).collect(); + match consensus::verify::verify_script( + txin.script_sig.as_bytes(), + prev.script_pubkey.as_bytes(), + &witness, + flags, + &checker, + ) { + Err(e) => Some(e.as_str().to_string()), + Ok(()) => { tracing::debug!( input, "the native engine does not agree that this input fails; \ reporting the coarse error" ); - format!("{coarse:?}") + None } } } @@ -339,6 +383,17 @@ impl ScriptVerifier for ConsensusVerifier { tx: &Transaction, prev_outputs: &[TxOut], height: u32, + ) -> Result<(), ScriptError> { + self.verify_transaction_unnamed(tx, prev_outputs, height) + .map_err(|e| self.name_failure(tx, prev_outputs, height, e)) + } + + /// libconsensus alone. A failure carries its coarse `ERR_SCRIPT`. + fn verify_transaction_unnamed( + &self, + tx: &Transaction, + prev_outputs: &[TxOut], + height: u32, ) -> Result<(), ScriptError> { let tx_bytes = bitcoin::consensus::serialize(tx); @@ -374,12 +429,27 @@ impl ScriptVerifier for ConsensusVerifier { ) .map_err(|e| ScriptError::VerifyFailed { input: input_index, - reason: name_script_failure(tx, prev_outputs, flags, input_index, e), + reason: format!("{e:?}"), })?; } Ok(()) } + + fn name_failure( + &self, + tx: &Transaction, + prev_outputs: &[TxOut], + height: u32, + err: ScriptError, + ) -> ScriptError { + let input = err.input(); + let flags = script_verify_flags(self.network, height); + match name_script_failure(tx, prev_outputs, flags, input) { + Some(reason) => ScriptError::VerifyFailed { input, reason }, + None => err, + } + } } /// No-op verifier for tests that don't need script checking. @@ -647,6 +717,30 @@ impl ScriptVerifier for ShadowVerifier { result } + /// The primary engine's unnamed verify, with the same shadow dispatch. + fn verify_transaction_unnamed( + &self, + tx: &Transaction, + prev_outputs: &[TxOut], + height: u32, + ) -> Result<(), ScriptError> { + let result = self.primary.verify_transaction_unnamed(tx, prev_outputs, height); + if result.is_ok() { + self.dispatch_shadow(tx, prev_outputs, height); + } + result + } + + fn name_failure( + &self, + tx: &Transaction, + prev_outputs: &[TxOut], + height: u32, + err: ScriptError, + ) -> ScriptError { + self.primary.name_failure(tx, prev_outputs, height, err) + } + fn dispatch_shadow(&self, tx: &Transaction, prev_outputs: &[TxOut], height: u32) { self.enqueue(ShadowWork { tx_bytes: bitcoin::consensus::serialize(tx), @@ -679,6 +773,10 @@ impl Drop for ShadowVerifier { } } +#[cfg(test)] +#[path = "script_naming_tests.rs"] +mod naming_tests; + #[cfg(test)] mod tests { use super::*; diff --git a/node/src/validation/script_naming_tests.rs b/node/src/validation/script_naming_tests.rs new file mode 100644 index 000000000..5e044da3f --- /dev/null +++ b/node/src/validation/script_naming_tests.rs @@ -0,0 +1,111 @@ +//! Naming a libbitcoinconsensus rejection. +//! +//! libbitcoinconsensus reports a coarse `ERR_SCRIPT`; Bitcoin Core, calling +//! `VerifyScript` directly, names the script error (`ScriptErrorString`, +//! src/script/script_error.cpp). satd asks its own engine for the name. That +//! takes only the input that failed, and the caller decides when to do it. + +use super::*; +use bitcoin::hashes::Hash; +use bitcoin::opcodes::all::{OP_PUSHNUM_1, OP_RETURN}; +use bitcoin::opcodes::OP_0; +use bitcoin::{Amount, OutPoint, ScriptBuf, Sequence, TxIn, Witness}; + +const HEIGHT: u32 = 1_000; +const EVAL_FALSE: &str = "Script evaluated without error but finished with a false/empty top stack element"; +const OP_RETURN_HIT: &str = "OP_RETURN was encountered"; + +/// A P2WSH input per witness script, each revealing its script. +fn p2wsh_spend(scripts: &[ScriptBuf]) -> (Transaction, Vec) { + let prevs: Vec = scripts + .iter() + .map(|ws| TxOut { + value: Amount::from_sat(10_000), + script_pubkey: ScriptBuf::new_p2wsh(&ws.wscript_hash()), + }) + .collect(); + let tx = Transaction { + version: bitcoin::transaction::Version(2), + lock_time: bitcoin::absolute::LockTime::ZERO, + input: scripts + .iter() + .enumerate() + .map(|(i, ws)| TxIn { + previous_output: OutPoint { txid: bitcoin::Txid::from_byte_array([i as u8 + 1; 32]), vout: 0 }, + script_sig: ScriptBuf::new(), + sequence: Sequence::MAX, + witness: Witness::from_slice(&[ws.as_bytes()]), + }) + .collect(), + output: vec![TxOut { value: Amount::from_sat(1_000), script_pubkey: ScriptBuf::new_op_return([]) }], + }; + (tx, prevs) +} + +fn script(op: u8) -> ScriptBuf { + ScriptBuf::from_bytes(vec![op]) +} + +/// Only the input that failed is run to name it. Input 0 here fails too, +/// but naming input 1's failure must not depend on it: libbitcoinconsensus +/// already passed every input before the one it rejected, and running them +/// again only repeats that work. +#[test] +fn naming_runs_only_the_failing_input() { + let (tx, prevs) = p2wsh_spend(&[script(OP_0.to_u8()), script(OP_RETURN.to_u8())]); + let cpp = ConsensusVerifier::new(Network::Regtest); + let coarse = ScriptError::VerifyFailed { input: 1, reason: "ERR_SCRIPT".into() }; + let named = cpp.name_failure(&tx, &prevs, HEIGHT, coarse); + assert_eq!(named.input(), 1); + assert_eq!(named.reason(), OP_RETURN_HIT); +} + +/// `verify_transaction` names the failure as Core does, on the input that +/// failed. +#[test] +fn verify_transaction_names_the_failure() { + let (tx, prevs) = p2wsh_spend(&[script(OP_PUSHNUM_1.to_u8()), script(OP_RETURN.to_u8())]); + let err = ConsensusVerifier::new(Network::Regtest) + .verify_transaction(&tx, &prevs, HEIGHT) + .expect_err("input 1 fails"); + assert_eq!((err.input(), err.reason()), (1, OP_RETURN_HIT)); + + let (tx, prevs) = p2wsh_spend(&[script(OP_0.to_u8())]); + let err = ConsensusVerifier::new(Network::Regtest) + .verify_transaction(&tx, &prevs, HEIGHT) + .expect_err("input 0 fails"); + assert_eq!((err.input(), err.reason()), (0, EVAL_FALSE)); + + let (tx, prevs) = p2wsh_spend(&[script(OP_PUSHNUM_1.to_u8())]); + ConsensusVerifier::new(Network::Regtest) + .verify_transaction(&tx, &prevs, HEIGHT) + .expect("a true script passes"); +} + +/// The unnamed verify gives the same verdict and input with the coarse +/// reason, and leaves the name to `name_failure`. The shadow verifier passes +/// both through to its primary engine. +#[test] +fn the_unnamed_verify_leaves_the_name_for_later() { + let (tx, prevs) = p2wsh_spend(&[script(OP_PUSHNUM_1.to_u8()), script(OP_RETURN.to_u8())]); + let shadow = ShadowVerifier::new( + Box::new(ConsensusVerifier::new(Network::Regtest)), + Box::new(RustVerifier::new(Network::Regtest)), + "cpp", + "rust", + 16, + 1, + ); + let verifiers: [(&str, &dyn ScriptVerifier); 2] = + [("cpp", &ConsensusVerifier::new(Network::Regtest)), ("rust-shadow", &shadow)]; + for (name, v) in verifiers { + let err = v.verify_transaction_unnamed(&tx, &prevs, HEIGHT).expect_err("input 1 fails"); + assert_eq!(err.input(), 1, "{name}"); + assert_eq!(err.reason(), "ERR_SCRIPT", "{name}: named under the caller's lock"); + let named = v.name_failure(&tx, &prevs, HEIGHT, err); + assert_eq!((named.input(), named.reason()), (1, OP_RETURN_HIT), "{name}"); + + v.verify_transaction_unnamed(&p2wsh_spend(&[script(OP_PUSHNUM_1.to_u8())]).0, &prevs[..1], HEIGHT) + .expect("a true script passes"); + } +}