Improve TxPool tests and documentation

CHANGELOG.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
 
+### Added
+- [2327](https://github.com/FuelLabs/fuel-core/pull/2327): Add more services tests and more checks of the pool. Also add an high level documentation for users of the pool and contributors.
+
 ## Fixed
 - [2320](https://github.com/FuelLabs/fuel-core/issues/2320): Prevent `/health` and `/v1/health` from being throttled by the concurrency limiter.
 

crates/services/txpool_v2/src/collision_manager/basic.rs

@@ -47,6 +47,9 @@ use super::{
     Collisions,
 };
 
+#[cfg(test)]
+use fuel_core_types::services::txpool::ArcPoolTx;
+
 pub struct BasicCollisionManager<StorageIndex> {
     /// Message -> Transaction that currently use the Message
     messages_spenders: HashMap<Nonce, StorageIndex>,
@@ -236,4 +239,71 @@ where
             };
         }
     }
+
+    #[cfg(test)]
+    fn assert_integrity(&self, expected_txs: &[ArcPoolTx]) {
+        use std::ops::Deref;
+
+        let mut message_spenders = HashMap::new();
+        let mut coins_spenders = BTreeMap::new();
+        let mut contracts_creators = HashMap::new();
+        let mut blobs_users = HashMap::new();
+        for tx in expected_txs {
+            if let PoolTransaction::Blob(checked_tx, _) = tx.deref() {
+                let blob_id = checked_tx.transaction().blob_id();
+                blobs_users.insert(*blob_id, tx.id());
+            }
+            for input in tx.inputs() {
+                match input {
+                    Input::CoinSigned(CoinSigned { utxo_id, .. })
+                    | Input::CoinPredicate(CoinPredicate { utxo_id, .. }) => {
+                        coins_spenders.insert(*utxo_id, tx.id());
+                    }
+                    Input::MessageCoinSigned(MessageCoinSigned { nonce, .. })
+                    | Input::MessageCoinPredicate(MessageCoinPredicate {
+                        nonce, ..
+                    })
+                    | Input::MessageDataSigned(MessageDataSigned { nonce, .. })
+                    | Input::MessageDataPredicate(MessageDataPredicate {
+                        nonce, ..
+                    }) => {
+                        message_spenders.insert(*nonce, tx.id());
+                    }
+                    _ => {}
+                }
+            }
+            for output in tx.outputs() {
+                match output {
+                    Output::Coin { .. }
+                    | Output::Change { .. }
+                    | Output::Variable { .. }
+                    | Output::Contract(_) => {}
+                    Output::ContractCreated { contract_id, .. } => {
+                        contracts_creators.insert(*contract_id, tx.id());
+                    }
+                }
+            }
+        }
+        for nonce in self.messages_spenders.keys() {
+            message_spenders.remove(nonce).expect("A message sender is present on the collision manager that shouldn't be there.");
+        }
+        assert!(
+            message_spenders.is_empty(),
+            "Some message senders are missing from the collision manager."
+        );
+        for utxo_id in self.coins_spenders.keys() {
+            coins_spenders.remove(utxo_id).expect("A coin sender is present on the collision manager that shouldn't be there.");
+        }
+        assert!(
+            coins_spenders.is_empty(),
+            "Some coin senders are missing from the collision manager."
+        );
+        for contract_id in self.contracts_creators.keys() {
+            contracts_creators.remove(contract_id).expect("A contract creator is present on the collision manager that shouldn't be there.");
+        }
+        assert!(
+            contracts_creators.is_empty(),
+            "Some contract creators are missing from the collision manager."
+        );
+    }
 }

crates/services/txpool_v2/src/collision_manager/mod.rs

@@ -10,6 +10,9 @@ use std::collections::HashMap;
 
 use crate::storage::StorageData;
 
+#[cfg(test)]
+use fuel_core_types::services::txpool::ArcPoolTx;
+
 pub mod basic;
 
 pub type Collisions<StorageIndex> = HashMap<StorageIndex, Vec<CollisionReason>>;
@@ -36,4 +39,8 @@ pub trait CollisionManager {
 
     /// Inform the collision manager that a transaction was removed.
     fn on_removed_transaction(&mut self, transaction: &PoolTransaction);
+
+    #[cfg(test)]
+    /// Asserts the integrity of the collision manager.
+    fn assert_integrity(&self, expected_txs: &[ArcPoolTx]);
 }

crates/services/txpool_v2/src/lib.rs

@@ -1,10 +1,44 @@
+//! This crate manage the verification, storage, organisation and selection of the transactions for the network.
+//! A transaction in Fuel has inputs and outputs. These inputs are outputs of previous transactions.
+//! In a case where one of the input is an output of a transaction that has not been executed in a committed block (transaction still in the pool),
+//! then the new transaction is considered dependent on that transaction.
+//!
+//! If a transaction has a dependency, it cannot be selected in a block until the dependent transaction has been selected.
+//! A transaction can have a dependency per input and this dependency transaction can also have its own dependencies.
+//! This creates a dependency tree between transactions inside the pool which can be very costly to compute for insertion/deletion etc...
+//! In order to avoid too much cost, the transaction pool only allow a maximum of transaction inside a dependency chain.
+//! There is others limits on the pool that prevent its size to grow too much: maximum gas in the pool, maximum bytes in the pool, maximum number of transactions in the pool.
+//! The pool also implements a TTL for the transactions, if a transaction is not selected in a block after a certain time, it is removed from the pool.
+//!
+//! All the transactions ordered by their ratio of gas/tip to be selected in a block.
+//! It's possible that a transaction is not profitable enough to be selected for now and so either it will be selected later or it will be removed from the pool.
+//! In order to make a transaction more likely to be selected, it's needed to submit a new collidng transaction (see below) with a higher tip/gas ratio.
+//!
+//! When a transaction is inserted it's possible that it use same inputs as one or multiple transactions already in the pool: this is what we call a collision.
+//! The pool has to choose which transaction to keep and which to remove.
+//! The pool will always try to maximize the number of transactions that can be selected in the next block and so
+//! during collision resolution it will prioritize transactions without dependencies.
+//! In a collision case, the transaction is considered a conflict and can be inserted under certain conditions :
+//! - The transaction has dependencies:
+//!     - Can collide only with one other transaction. So, the user can submit
+//!       the same transaction with a higher tip but not merge one or more
+//!         transactions into one.
+//!     - A new transaction can be accepted if its profitability is higher than
+//!         the collided subtree's.
+//! - A transaction doesn't have dependencies:
+//!     - A new transaction can be accepted if its profitability is higher
+//!         than the collided subtrees'.
+//!
+//! The pool provides a way to subscribe for updates on a transaction status.
+//! It usually stream one or two messages:
+//! - If the insertion of the transaction fails, you can expect only one message with the error.
+//! - If the transaction is inserted, you can expect two messages: one with the validation of the insertion and one when the transaction is selected in a block.
+
 #![deny(clippy::arithmetic_side_effects)]
 #![deny(clippy::cast_possible_truncation)]
 #![deny(unused_crate_dependencies)]
 #![deny(warnings)]
 
-// TODO: Rename the folder from `txpool_v2` to `txpool` after the migration is complete.
-
 mod collision_manager;
 pub mod config;
 pub mod error;

crates/services/txpool_v2/src/pool.rs

@@ -493,6 +493,37 @@ where
                 .on_removed_transaction(storage_entry);
         }
     }
+
+    #[cfg(test)]
+    pub fn assert_integrity(&self, expected_txs: &[ArcPoolTx]) {
+        let storage_ids_dependencies = self.storage.assert_integrity(expected_txs);
+        let txs_without_dependencies = expected_txs
+            .iter()
+            .zip(storage_ids_dependencies)
+            .filter_map(|(tx, (_, has_dependencies))| {
+                if !has_dependencies {
+                    Some(tx.clone())
+                } else {
+                    None
+                }
+            })
+            .collect::<Vec<_>>();
+        self.selection_algorithm
+            .assert_integrity(&txs_without_dependencies);
+        self.collision_manager.assert_integrity(expected_txs);
+        let mut txs: HashMap<TxId, ArcPoolTx> = expected_txs
+            .iter()
+            .map(|tx| (tx.id(), tx.clone()))
+            .collect();
+        for tx in &self.tx_id_to_storage_id {
+            txs.remove(tx.0)
+                .expect("Transaction not found in the expected transactions");
+        }
+        assert!(
+            txs.is_empty(),
+            "Some transactions are not found in the pool"
+        );
+    }
 }
 
 pub struct NotEnoughSpace {

crates/services/txpool_v2/src/selection_algorithms/mod.rs

@@ -3,6 +3,9 @@ use crate::storage::{
     StorageData,
 };
 
+#[cfg(test)]
+use fuel_core_types::services::txpool::ArcPoolTx;
+
 pub mod ratio_tip_gas;
 
 /// Constraints that the selection algorithm has to respect.
@@ -42,4 +45,8 @@ pub trait SelectionAlgorithm {
 
     /// Inform the selection algorithm that a transaction was removed from the pool.
     fn on_removed_transaction(&mut self, storage_entry: &StorageData);
+
+    #[cfg(test)]
+    /// Asserts the integrity of the selection algorithm.
+    fn assert_integrity(&self, expected_txs: &[ArcPoolTx]);
 }

crates/services/txpool_v2/src/selection_algorithms/ratio_tip_gas.rs

@@ -21,6 +21,12 @@ use super::{
     SelectionAlgorithm,
 };
 
+#[cfg(test)]
+use fuel_core_types::services::txpool::ArcPoolTx;
+
+#[cfg(test)]
+use std::collections::HashMap;
+
 pub trait RatioTipGasSelectionAlgorithmStorage {
     type StorageIndex: Debug;
 
@@ -252,4 +258,19 @@ where
         let key = Self::key(storage_entry);
         self.on_removed_transaction_inner(key)
     }
+
+    #[cfg(test)]
+    fn assert_integrity(&self, expected_txs: &[ArcPoolTx]) {
+        let mut expected_txs: HashMap<TxId, ArcPoolTx> = expected_txs
+            .iter()
+            .map(|tx| (tx.id(), tx.clone()))
+            .collect();
+        for key in self.executable_transactions_sorted_tip_gas_ratio.keys() {
+            expected_txs.remove(&key.0.tx_id).expect("A transaction is present on the selection algorithm that shouldn't be there.");
+        }
+        assert!(
+            expected_txs.is_empty(),
+            "Some transactions are missing from the selection algorithm."
+        );
+    }
 }

crates/services/txpool_v2/src/storage/graph.rs

@@ -619,6 +619,93 @@ impl Storage for GraphStorage {
             self.clear_cache(storage_entry);
         })
     }
+
+    #[cfg(test)]
+    fn assert_integrity(
+        &self,
+        expected_txs: &[ArcPoolTx],
+    ) -> Vec<(Self::StorageIndex, bool)> {
+        use std::ops::Deref;
+
+        let mut txs_map: HashMap<TxId, ArcPoolTx> = expected_txs
+            .iter()
+            .map(|tx| (tx.id(), tx.clone()))
+            .collect();
+        let mut tx_id_node_id = HashMap::new();
+        let mut txs_info = Vec::new();
+
+        for node_id in self.graph.node_indices() {
+            let node = self
+                .graph
+                .node_weight(node_id)
+                .expect("A node not expected exists in storage");
+            let has_dependencies = Storage::has_dependencies(self, &node_id);
+            let tx_id = node.transaction.id();
+            let tx = txs_map
+                .remove(&tx_id)
+                .expect("A transaction not expected exists in storage");
+            assert_eq!(tx.deref(), node.transaction.deref());
+            tx_id_node_id.insert(tx_id, node_id);
+            txs_info.push((node_id, has_dependencies));
+        }
+        assert!(
+            txs_map.is_empty(),
+            "Some transactions are missing in storage"
+        );
+
+        let mut coins_creators = HashMap::new();
+        let mut contracts_creators = HashMap::new();
+        for expected_tx in expected_txs {
+            for (i, output) in expected_tx.outputs().iter().enumerate() {
+                match output {
+                    Output::Coin { .. } => {
+                        let utxo_id =
+                            UtxoId::new(expected_tx.id(), i.try_into().unwrap());
+                        coins_creators.insert(utxo_id, expected_tx.id());
+                    }
+                    Output::ContractCreated { contract_id, .. } => {
+                        contracts_creators.insert(*contract_id, expected_tx.id());
+                    }
+                    _ => {}
+                }
+            }
+        }
+        for (utxo_id, node_id) in &self.coins_creators {
+            let tx_id = coins_creators.remove(utxo_id).expect(
+                "A coin creator is present in the storage that shouldn't be there",
+            );
+            let expected_node_id = tx_id_node_id
+                .get(&tx_id)
+                .expect("A node id is missing for a transaction");
+            assert_eq!(
+                expected_node_id, node_id,
+                "The node id is different from the expected one"
+            );
+        }
+        assert!(
+            coins_creators.is_empty(),
+            "Some contract creators are missing in storage"
+        );
+
+        for (contract_id, node_id) in &self.contracts_creators {
+            let tx_id = contracts_creators.remove(contract_id).expect(
+                "A contract creator is present in the storage that shouldn't be there",
+            );
+            let expected_node_id = tx_id_node_id
+                .get(&tx_id)
+                .expect("A node id is missing for a transaction");
+            assert_eq!(
+                expected_node_id, node_id,
+                "The node id is different from the expected one"
+            );
+        }
+        assert!(
+            contracts_creators.is_empty(),
+            "Some contract creators are missing in storage"
+        );
+
+        txs_info
+    }
 }
 
 impl RatioTipGasSelectionAlgorithmStorage for GraphStorage {

crates/services/txpool_v2/src/storage/mod.rs

@@ -102,4 +102,12 @@ pub trait Storage {
 
     /// Remove a transaction from the storage.
     fn remove_transaction(&mut self, index: Self::StorageIndex) -> Option<StorageData>;
+
+    #[cfg(test)]
+    /// Asserts the integrity of the storage.
+    /// Returns the list of the storage indexes and a boolean value indicating whether the transaction has dependencies or not.
+    fn assert_integrity(
+        &self,
+        expected_txs: &[ArcPoolTx],
+    ) -> Vec<(Self::StorageIndex, bool)>;
 }

crates/services/txpool_v2/src/tests/mod.rs

@@ -1,3 +1,5 @@
+#![allow(non_snake_case)]
+
 mod mocks;
 mod stability_test;
 mod tests_e2e;

crates/services/txpool_v2/src/tests/stability_test.rs

@@ -3,7 +3,6 @@
 //! correct(not in the unexpected state).
 //! It relies on the `debug_assert` which are present in the code.
 
-#![allow(non_snake_case)]
 #![allow(clippy::cast_possible_truncation)]
 #![allow(clippy::arithmetic_side_effects)]