Mempool: rename get_all/get_all_transactions to get_all_in_insertion_order/get_all_transactions_in_insertion_order and make them return the txs in the insertion order

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

mempool/src/interface/mempool_interface.rs

@@ -45,8 +45,8 @@ pub trait MempoolInterface: Send + Sync {
         options: TxOptions,
     ) -> Result<TransactionDuplicateStatus, Error>;
 
-    /// Get all transactions from mempool
-    fn get_all(&self) -> Vec<SignedTransaction>;
+    /// Get all transactions from mempool, in the insertion order.
+    fn get_all_in_insertion_order(&self) -> Vec<SignedTransaction>;
 
     /// Get a specific transaction from the main mempool (non-orphan)
     fn transaction(&self, id: &Id<Transaction>) -> Option<SignedTransaction>;

mempool/src/interface/mempool_interface_impl.rs

@@ -135,8 +135,8 @@ impl MempoolInterface for Mempool {
         self.add_transaction(tx)
     }
 
-    fn get_all(&self) -> Vec<SignedTransaction> {
-        self.get_all()
+    fn get_all_in_insertion_order(&self) -> Vec<SignedTransaction> {
+        self.get_all_in_insertion_order()
     }
 
     fn contains_transaction(&self, tx_id: &Id<Transaction>) -> bool {

mempool/src/pool/mod.rs

@@ -245,10 +245,17 @@ impl<M> Mempool<M> {
         }
     }
 
-    pub fn get_all(&self) -> Vec<SignedTransaction> {
+    pub fn get_all_in_insertion_order(&self) -> Vec<SignedTransaction> {
         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<SignedTransaction> {
+        match &self.0 {
+            MempoolState::InIbd(_) => Vec::new(),
+            MempoolState::AfterIbd(state) => state.tx_pool.get_all_by_descendant_score(),
         }
     }
 

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);
+}

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::<BTreeSet<_>>();

mempool/src/pool/tx_pool/mod.rs

@@ -173,12 +173,12 @@ impl<M> TxPool<M> {
             .expect("IBD state query failed")
     }
 
-    pub fn get_all(&self) -> Vec<SignedTransaction> {
-        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<SignedTransaction> {
+        self.store.txs_iter_in_insertion_order().cloned().collect()
+    }
+
+    pub fn get_all_by_descendant_score(&self) -> Vec<SignedTransaction> {
+        self.store.txs_iter_by_descendant_score().cloned().collect()
     }
 
     #[cfg(test)]

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<Item = &'_ SignedTransaction> {
+        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<Item = &'_ SignedTransaction> {
+        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<Id<Transaction>, usize> {
         &self.seq_nos_by_tx

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::<SignedTransaction>::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::<BTreeSet<_>>();

mempool/src/rpc.rs

@@ -56,12 +56,13 @@ trait MempoolRpc {
     #[method(name = "get_transaction")]
     async fn get_transaction(&self, tx_id: Id<Transaction>) -> RpcResult<Option<GetTxResponse>>;
 
-    /// 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<Vec<HexEncoded<SignedTransaction>>>;
+    async fn get_all_transactions_in_insertion_order(
+        &self,
+    ) -> RpcResult<Vec<HexEncoded<SignedTransaction>>>;
 
     /// 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<Vec<HexEncoded<SignedTransaction>>> {
+    async fn get_all_transactions_in_insertion_order(
+        &self,
+    ) -> rpc::RpcResult<Vec<HexEncoded<SignedTransaction>>> {
         rpc::handle_result(
             self.call(move |this| -> Vec<HexEncoded<SignedTransaction>> {
-                this.get_all().into_iter().map(HexEncoded::new).collect()
+                this.get_all_in_insertion_order().into_iter().map(HexEncoded::new).collect()
             })
             .await,
         )

mocks/src/mempool.rs

@@ -48,7 +48,7 @@ mockall::mock! {
             options: TxOptions,
         ) -> Result<TxStatus, Error>;
 
-        fn get_all(&self) -> Vec<SignedTransaction>;
+        fn get_all_in_insertion_order(&self) -> Vec<SignedTransaction>;
         fn transaction(&self, id: &Id<Transaction>) -> Option<SignedTransaction>;
         fn orphan_transaction(&self, id: &Id<Transaction>) -> Option<SignedTransaction>;
         fn contains_transaction(&self, tx: &Id<Transaction>) -> bool;

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:

wallet/wallet-node-client/src/handles_client/mod.rs

@@ -528,7 +528,7 @@ impl NodeInterface for WalletHandlesClient {
     }
 
     async fn mempool_get_transactions(&self) -> Result<Vec<SignedTransaction>, 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)
     }
 

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<Vec<SignedTransaction>, 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())