Improve TxPool tests and documentation

CHANGELOG.md

@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - [2362](https://github.com/FuelLabs/fuel-core/pull/2362): Added a new request_response protocol version `/fuel/req_res/0.0.2`. In comparison with `/fuel/req/0.0.1`, which returns an empty response when a request cannot be fulfilled, this version returns more meaningful error codes. Nodes still support the version `0.0.1` of the protocol to guarantee backward compatibility with fuel-core nodes. Empty responses received from nodes using the old protocol `/fuel/req/0.0.1` are automatically converted into an error `ProtocolV1EmptyResponse` with error code 0, which is also the only error code implemented. More specific error codes will be added in the future.
 - [2386](https://github.com/FuelLabs/fuel-core/pull/2386): Add a flag to define the maximum number of file descriptors that RocksDB can use. By default it's half of the OS limit.
 - [2376](https://github.com/FuelLabs/fuel-core/pull/2376): Add a way to fetch transactions in P2P without specifying a peer.
+- [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
 - [2366](https://github.com/FuelLabs/fuel-core/pull/2366): The `importer_gas_price_for_block` metric is properly collected.
@@ -56,7 +57,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - [2324](https://github.com/FuelLabs/fuel-core/pull/2324): Added metrics for sync, async processor and for all GraphQL queries.
 - [2320](https://github.com/FuelLabs/fuel-core/pull/2320): Added new CLI flag `graphql-max-resolver-recursive-depth` to limit recursion within resolver. The default value it "1".
 
-
 ## Fixed
 - [2320](https://github.com/FuelLabs/fuel-core/issues/2320): Prevent `/health` and `/v1/health` from being throttled by the concurrency limiter.
 - [2322](https://github.com/FuelLabs/fuel-core/issues/2322): Set the salt of genesis contracts to zero on execution.

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>,
@@ -75,6 +78,81 @@ impl<StorageIndex> BasicCollisionManager<StorageIndex> {
             && self.contracts_creators.is_empty()
             && self.blobs_users.is_empty()
     }
+
+    #[cfg(test)]
+    /// Expected transactions are the transactions that must be populate all elements present in the collision manager.
+    /// This function will check if all elements present in the collision manager are present in the expected transactions and vice versa.
+    pub(crate) 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());
+                    }
+                    Input::Contract { .. } => {}
+                }
+            }
+            for output in tx.outputs() {
+                if let Output::ContractCreated { contract_id, .. } = output {
+                    contracts_creators.insert(*contract_id, tx.id());
+                }
+            }
+        }
+        for nonce in self.messages_spenders.keys() {
+            message_spenders.remove(nonce).unwrap_or_else(|| panic!(
+                "A message ({}) spender is present on the collision manager that shouldn't be there.",
+                nonce
+            ));
+        }
+        assert!(
+            message_spenders.is_empty(),
+            "Some message spenders are missing from the collision manager: {:?}",
+            message_spenders
+        );
+        for utxo_id in self.coins_spenders.keys() {
+            coins_spenders.remove(utxo_id).unwrap_or_else(|| panic!(
+                "A coin ({}) spender is present on the collision manager that shouldn't be there.",
+                utxo_id
+            ));
+        }
+        assert!(
+            coins_spenders.is_empty(),
+            "Some coin senders are missing from the collision manager: {:?}",
+            coins_spenders
+        );
+        for contract_id in self.contracts_creators.keys() {
+            contracts_creators.remove(contract_id).unwrap_or_else(|| panic!(
+                "A contract ({}) creator is present on the collision manager that shouldn't be there.",
+                contract_id
+            ));
+        }
+        assert!(
+            contracts_creators.is_empty(),
+            "Some contract creators are missing from the collision manager: {:?}",
+            contracts_creators
+        );
+    }
 }
 
 impl<StorageIndex> Default for BasicCollisionManager<StorageIndex> {

crates/services/txpool_v2/src/lib.rs

@@ -1,10 +1,46 @@
+//! This crate manage the verification, storage, organisation and selection of the transactions for the network.
+//! A transaction in Fuel has inputs and outputs. 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 number 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 colliding 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 cumulative profitability of the colliding transactions, and all
+//!         the transactions that depend on it.
+//! - 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.
+
+// TODO: Rename the folder from `txpool_v2` to `txpool` after the migration is complete.
 #![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

@@ -46,6 +46,9 @@ use crate::{
     },
 };
 
+#[cfg(test)]
+use std::collections::HashSet;
+
 /// The pool is the main component of the txpool service. It is responsible for storing transactions
 /// and allowing the selection of transactions for inclusion in a block.
 pub struct Pool<S, SI, CM, SA> {
@@ -561,6 +564,22 @@ where
         }
         self.register_transaction_counts();
     }
+
+    #[cfg(test)]
+    pub fn assert_integrity(&self, mut expected_txs: HashSet<TxId>) {
+        for tx in &self.tx_id_to_storage_id {
+            if !expected_txs.remove(tx.0) {
+                panic!(
+                    "Transaction with id {:?} is not in the expected transactions",
+                    tx.0
+                );
+            }
+        }
+        assert!(
+            expected_txs.is_empty(),
+            "Some transactions are not found in the pool"
+        );
+    }
 }
 
 pub struct NotEnoughSpace {

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;
 
@@ -116,6 +122,27 @@ where
         self.executable_transactions_sorted_tip_gas_ratio
             .remove(&Reverse(key));
     }
+
+    #[cfg(test)]
+    pub(crate) 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).unwrap_or_else(|| {
+                panic!(
+                    "Transaction with id {:?} is not in the expected transactions.",
+                    key.0.tx_id
+                )
+            });
+        }
+        assert!(
+            expected_txs.is_empty(),
+            "Some transactions are missing from the selection algorithm: {:?}",
+            expected_txs.keys().collect::<Vec<_>>()
+        );
+    }
 }
 
 impl<S> SelectionAlgorithm for RatioTipGasSelection<S>

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

@@ -368,6 +368,94 @@ impl GraphStorage {
     fn has_dependent(&self, index: NodeIndex) -> bool {
         self.get_direct_dependents(index).next().is_some()
     }
+
+    #[cfg(test)]
+    pub(crate) fn assert_integrity(
+        &self,
+        expected_txs: &[ArcPoolTx],
+    ) -> Vec<(NodeIndex, 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 {:?}",
+            txs_map.keys()
+        );
+
+        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());
+                    }
+                    Output::Contract(_)
+                    | Output::Change { .. }
+                    | Output::Variable { .. } => {}
+                }
+            }
+        }
+        for (utxo_id, node_id) in &self.coins_creators {
+            let tx_id = coins_creators.remove(utxo_id).unwrap_or_else(|| panic!("A coin creator (coin: {}) is present in the storage that shouldn't be there", utxo_id));
+            let expected_node_id = tx_id_node_id.get(&tx_id).unwrap_or_else(|| {
+                panic!("A node id is missing for a transaction (tx_id: {})", tx_id)
+            });
+            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: {:?}",
+            coins_creators
+        );
+
+        for (contract_id, node_id) in &self.contracts_creators {
+            let tx_id = contracts_creators.remove(contract_id).unwrap_or_else(|| panic!("A contract creator (contract: {}) is present in the storage that shouldn't be there", contract_id));
+            let expected_node_id = tx_id_node_id.get(&tx_id).unwrap_or_else(|| {
+                panic!("A node id is missing for a transaction (tx_id: {})", tx_id)
+            });
+            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: {:?}",
+            contracts_creators
+        );
+
+        txs_info
+    }
 }
 
 impl Storage for GraphStorage {

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)]