diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ed18daa5..7b106189f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,10 @@ The format is loosely based on [Keep a Changelog](https://keepachangelog.com/en/ (The actual consensus tightening will happen after a fork, the height is yet to be decided.) + - Node RPC: + - The method `mempool_transactions` now returns transactions in the same order in which they were originally inserted + into the mempool, rather than sorting them by descendant score. + ### Fixed - Mempool: - Fixed an issue where the mempool wouldn't track non-UTXO dependencies between transactions, which could diff --git a/mempool/src/interface/mempool_interface.rs b/mempool/src/interface/mempool_interface.rs index 2d25b93e3..5935d63bc 100644 --- a/mempool/src/interface/mempool_interface.rs +++ b/mempool/src/interface/mempool_interface.rs @@ -45,8 +45,8 @@ pub trait MempoolInterface: Send + Sync { options: TxOptions, ) -> Result; - /// Get all transactions from mempool - fn get_all(&self) -> Vec; + /// Get all transactions from mempool, in the insertion order. + fn get_all_in_insertion_order(&self) -> Vec; /// Get a specific transaction from the main mempool (non-orphan) fn transaction(&self, id: &Id) -> Option; diff --git a/mempool/src/interface/mempool_interface_impl.rs b/mempool/src/interface/mempool_interface_impl.rs index 004897eda..2f4f307d5 100644 --- a/mempool/src/interface/mempool_interface_impl.rs +++ b/mempool/src/interface/mempool_interface_impl.rs @@ -135,8 +135,8 @@ impl MempoolInterface for Mempool { self.add_transaction(tx) } - fn get_all(&self) -> Vec { - self.get_all() + fn get_all_in_insertion_order(&self) -> Vec { + self.get_all_in_insertion_order() } fn contains_transaction(&self, tx_id: &Id) -> bool { diff --git a/mempool/src/pool/mod.rs b/mempool/src/pool/mod.rs index fe275ae99..43563c7bf 100644 --- a/mempool/src/pool/mod.rs +++ b/mempool/src/pool/mod.rs @@ -245,10 +245,17 @@ impl Mempool { } } - pub fn get_all(&self) -> Vec { + pub fn get_all_in_insertion_order(&self) -> Vec { match &self.0 { MempoolState::InIbd(_) => Vec::new(), - MempoolState::AfterIbd(state) => state.tx_pool.get_all(), + MempoolState::AfterIbd(state) => state.tx_pool.get_all_in_insertion_order(), + } + } + + pub fn get_all_by_descendant_score(&self) -> Vec { + match &self.0 { + MempoolState::InIbd(_) => Vec::new(), + MempoolState::AfterIbd(state) => state.tx_pool.get_all_by_descendant_score(), } } diff --git a/mempool/src/pool/tests/basic.rs b/mempool/src/pool/tests/basic.rs index 748cc2135..b0af38feb 100644 --- a/mempool/src/pool/tests/basic.rs +++ b/mempool/src/pool/tests/basic.rs @@ -20,8 +20,10 @@ use tokio::sync::mpsc; use chainstate::BlockSource; use chainstate_test_framework::helpers::split_utxo; use common::chain::{ - AccountNonce, AccountOutPoint, AccountSpending, UtxoOutPoint, block::timestamp::BlockTimestamp, + self, AccountNonce, AccountOutPoint, AccountSpending, Genesis, UtxoOutPoint, + block::timestamp::BlockTimestamp, }; +use test_utils::BasicTestTimeGetter; use crate::event::NewTipEvent; @@ -516,3 +518,69 @@ async fn non_utxo_orphan_dependency_can_be_resolved_by_mempool_parent(#[case] se mempool.tx_store().assert_valid(); } + +// Check that `get_all_in_insertion_order` actually returns all transactions in the insertion order. +#[rstest] +#[case(Seed::from_entropy())] +#[trace] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn get_all_txs_in_insertion_order(#[case] seed: Seed) { + let mut rng = make_seedable_rng(seed); + + let tx_count = rng.random_range(50..100); + let genesis_txo_amount = Amount::from_atoms(1_000_000_000); + + let genesis_txos = (0..tx_count) + .map(|_| { + TxOutput::Transfer( + OutputValue::Coin(genesis_txo_amount), + Destination::AnyoneCanSpend, + ) + }) + .collect(); + + let time_getter = + BasicTestTimeGetter::with_secs_since_epoch(rng.random_range(100_000_000..100_000_000_000)) + .get_time_getter(); + let genesis = Genesis::new( + "test".into(), + BlockTimestamp::from_time(time_getter.get_time()), + genesis_txos, + ); + + let chain_config = + chain::config::create_unit_test_config_builder().genesis_custom(genesis).build(); + let genesis_id = chain_config.genesis_block_id(); + + let tf = TestFramework::builder(&mut rng) + .with_chain_config(chain_config) + .with_time_getter(time_getter.clone()) + .build(); + let mempool_config = MempoolConfig { + min_tx_relay_fee_rate: FeeRate::from_amount_per_kb(Amount::ZERO).into(), + max_cluster_tx_count: Default::default(), + max_cluster_size_bytes: Default::default(), + }; + let mut mempool = setup_with_chainstate_generic(tf.chainstate(), mempool_config, time_getter); + + let mut expected_txs = Vec::new(); + + for i in 0..tx_count { + let genesis_outpoint = UtxoOutPoint::new(genesis_id.into(), i); + let fee = Amount::from_atoms(rng.random_range(10..10_000_000)); + let tx = TransactionBuilder::new() + .add_input(genesis_outpoint.into(), empty_witness(&mut rng)) + .add_output(TxOutput::Transfer( + OutputValue::Coin((genesis_txo_amount - fee).unwrap()), + Destination::AnyoneCanSpend, + )) + .build(); + + expected_txs.push(tx.clone()); + + mempool.add_transaction_test(tx).unwrap().assert_in_mempool(); + } + + let actual_txs = mempool.get_all_in_insertion_order(); + assert_eq!(actual_txs, expected_txs); +} diff --git a/mempool/src/pool/tests/relatives.rs b/mempool/src/pool/tests/relatives.rs index c9bb1eb21..af403b703 100644 --- a/mempool/src/pool/tests/relatives.rs +++ b/mempool/src/pool/tests/relatives.rs @@ -873,7 +873,7 @@ fn add_tx_expect_cluster_byte_size_failure(mempool: &mut MempoolType, tx: &Signe fn assert_txs(mempool: &MempoolType, txs: &[&SignedTransaction]) { let actual_tx_ids = mempool - .get_all() + .get_all_by_descendant_score() .iter() .map(|tx| tx.transaction().get_id()) .collect::>(); diff --git a/mempool/src/pool/tx_pool/mod.rs b/mempool/src/pool/tx_pool/mod.rs index 182fbbe71..658b0aa08 100644 --- a/mempool/src/pool/tx_pool/mod.rs +++ b/mempool/src/pool/tx_pool/mod.rs @@ -173,12 +173,12 @@ impl TxPool { .expect("IBD state query failed") } - pub fn get_all(&self) -> Vec { - self.store - .txs_by_descendant_score() - .iter() - .map(|(_score, id)| self.store.get_entry(id).expect("entry").transaction().clone()) - .collect() + pub fn get_all_in_insertion_order(&self) -> Vec { + self.store.txs_iter_in_insertion_order().cloned().collect() + } + + pub fn get_all_by_descendant_score(&self) -> Vec { + self.store.txs_iter_by_descendant_score().cloned().collect() } #[cfg(test)] diff --git a/mempool/src/pool/tx_pool/store/mod.rs b/mempool/src/pool/tx_pool/store/mod.rs index f67e9072d..502403a44 100644 --- a/mempool/src/pool/tx_pool/store/mod.rs +++ b/mempool/src/pool/tx_pool/store/mod.rs @@ -28,7 +28,7 @@ use common::{ primitives::Id, }; use logging::log; -use utils::{debug_assert_or_log, ensure, newtype}; +use utils::{debug_assert_or_log, debug_panic_or_log, ensure, newtype}; use super::{Fee, Time, TxEntry, TxEntryWithFee}; @@ -268,6 +268,28 @@ impl MempoolStore { &self.txs_by_creation_time } + pub fn txs_iter_in_insertion_order(&self) -> impl Iterator { + self.txs_by_seq_no.values().filter_map(move |tx_id| { + if let Some(entry) = self.txs_by_id.get(tx_id) { + Some(entry.transaction()) + } else { + debug_panic_or_log!("Missing tx entry for tx {tx_id:x}"); + None + } + }) + } + + pub fn txs_iter_by_descendant_score(&self) -> impl Iterator { + self.txs_by_descendant_score.iter().filter_map(move |(_, tx_id)| { + if let Some(entry) = self.txs_by_id.get(tx_id) { + Some(entry.transaction()) + } else { + debug_panic_or_log!("Missing tx entry for tx {tx_id:x}"); + None + } + }) + } + #[cfg(test)] pub fn seq_nos_by_tx(&self) -> &TrackedHashMap, usize> { &self.seq_nos_by_tx diff --git a/mempool/src/pool/tx_pool/tests/basic.rs b/mempool/src/pool/tx_pool/tests/basic.rs index e1e23b426..4ea44ed41 100644 --- a/mempool/src/pool/tx_pool/tests/basic.rs +++ b/mempool/src/pool/tx_pool/tests/basic.rs @@ -75,11 +75,11 @@ async fn add_single_tx() -> anyhow::Result<()> { let tx_id = tx.transaction().get_id(); mempool.add_transaction_test(tx)?.assert_in_mempool(); assert!(mempool.contains_transaction(&tx_id)); - let all_txs = mempool.get_all(); + let all_txs = mempool.get_all_by_descendant_score(); assert_eq!(all_txs, vec![tx_clone]); mempool.store.remove_tx(&tx_id, MempoolRemovalReason::Block); assert!(!mempool.contains_transaction(&tx_id)); - let all_txs = mempool.get_all(); + let all_txs = mempool.get_all_by_descendant_score(); assert_eq!(all_txs, Vec::::new()); mempool.store.assert_valid(); Ok(()) @@ -170,7 +170,7 @@ async fn txs_sorted(#[case] seed: Seed) -> anyhow::Result<()> { } let mut fees = Vec::new(); - for tx in mempool.get_all() { + for tx in mempool.get_all_by_descendant_score() { fees.push(try_get_fee(&mempool, &tx).await) } let mut fees_sorted = fees.clone(); @@ -1714,7 +1714,7 @@ fn stack_overflow_on_transaction_addition( } let actual_tx_ids = mempool - .get_all() + .get_all_by_descendant_score() .iter() .map(|tx| tx.transaction().get_id()) .collect::>(); diff --git a/mempool/src/rpc.rs b/mempool/src/rpc.rs index ec5244f7a..5e9518f59 100644 --- a/mempool/src/rpc.rs +++ b/mempool/src/rpc.rs @@ -56,12 +56,13 @@ trait MempoolRpc { #[method(name = "get_transaction")] async fn get_transaction(&self, tx_id: Id) -> RpcResult>; - /// Get all mempool transactions in a Vec/List, with hex-encoding. + /// Get all mempool transactions, in the insertion order. /// - /// Notice that this call may be expensive. Use it with caution. - /// This function is mostly used for testing purposes. + /// Note that this call may be expensive. #[method(name = "transactions")] - async fn get_all_transactions(&self) -> RpcResult>>; + async fn get_all_transactions_in_insertion_order( + &self, + ) -> RpcResult>>; /// Submit a transaction to the mempool. /// @@ -124,10 +125,12 @@ impl MempoolRpcServer for super::MempoolHandle { rpc::handle_result(self.call(move |this| this.contains_orphan_transaction(&tx_id)).await) } - async fn get_all_transactions(&self) -> rpc::RpcResult>> { + async fn get_all_transactions_in_insertion_order( + &self, + ) -> rpc::RpcResult>> { rpc::handle_result( self.call(move |this| -> Vec> { - this.get_all().into_iter().map(HexEncoded::new).collect() + this.get_all_in_insertion_order().into_iter().map(HexEncoded::new).collect() }) .await, ) diff --git a/mocks/src/mempool.rs b/mocks/src/mempool.rs index eb416781d..b3a0ce963 100644 --- a/mocks/src/mempool.rs +++ b/mocks/src/mempool.rs @@ -48,7 +48,7 @@ mockall::mock! { options: TxOptions, ) -> Result; - fn get_all(&self) -> Vec; + fn get_all_in_insertion_order(&self) -> Vec; fn transaction(&self, id: &Id) -> Option; fn orphan_transaction(&self, id: &Id) -> Option; fn contains_transaction(&self, tx: &Id) -> bool; diff --git a/node-daemon/docs/RPC.md b/node-daemon/docs/RPC.md index 741633fd4..d7d8654a8 100644 --- a/node-daemon/docs/RPC.md +++ b/node-daemon/docs/RPC.md @@ -863,10 +863,9 @@ EITHER OF ### Method `mempool_transactions` -Get all mempool transactions in a Vec/List, with hex-encoding. +Get all mempool transactions, in the insertion order. -Notice that this call may be expensive. Use it with caution. -This function is mostly used for testing purposes. +Note that this call may be expensive. Parameters: diff --git a/wallet/wallet-node-client/src/handles_client/mod.rs b/wallet/wallet-node-client/src/handles_client/mod.rs index 32ce9ffc6..0105ce525 100644 --- a/wallet/wallet-node-client/src/handles_client/mod.rs +++ b/wallet/wallet-node-client/src/handles_client/mod.rs @@ -528,7 +528,7 @@ impl NodeInterface for WalletHandlesClient { } async fn mempool_get_transactions(&self) -> Result, Self::Error> { - let res = self.mempool.call(move |this| this.get_all()).await?; + let res = self.mempool.call(move |this| this.get_all_in_insertion_order()).await?; Ok(res) } diff --git a/wallet/wallet-node-client/src/rpc_client/client_impl.rs b/wallet/wallet-node-client/src/rpc_client/client_impl.rs index 3f42478c0..09e9ba782 100644 --- a/wallet/wallet-node-client/src/rpc_client/client_impl.rs +++ b/wallet/wallet-node-client/src/rpc_client/client_impl.rs @@ -420,7 +420,7 @@ impl NodeInterface for NodeRpcClient { } async fn mempool_get_transactions(&self) -> Result, Self::Error> { - MempoolRpcClient::get_all_transactions(&*self.rpc_client) + MempoolRpcClient::get_all_transactions_in_insertion_order(&*self.rpc_client) .await .map_err(NodeRpcError::ResponseError) .map(|txs| txs.into_iter().map(|tx| tx.take()).collect())