Improve TxPool tests and documentation

CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - [2378](https://github.com/FuelLabs/fuel-core/pull/2378): Use cached hash of the topic instead of calculating it on each publishing gossip message.
 
 ### 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.
 - [2321](https://github.com/FuelLabs/fuel-core/pull/2321): New metrics for the txpool: "The size of transactions in the txpool" (`txpool_tx_size`), "The time spent by a transaction in the txpool in seconds" (`txpool_tx_time_in_txpool_seconds`), The number of transactions in the txpool (`txpool_number_of_transactions`), "The number of transactions pending verification before entering the txpool" (`txpool_number_of_transactions_pending_verification`), "The number of executable transactions in the txpool" (`txpool_number_of_executable_transactions`), "The time it took to select transactions for inclusion in a block in nanoseconds" (`txpool_select_transaction_time_nanoseconds`), The time it took to insert a transaction in the txpool in milliseconds (`txpool_insert_transaction_time_milliseconds`).
 - [2347](https://github.com/FuelLabs/fuel-core/pull/2364): Add activity concept in order to protect against infinitely increasing DA gas price scenarios
 - [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.
@@ -47,7 +48,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>,
@@ -236,4 +239,79 @@ where
             };
         }
     }
+
+    #[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.
+    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() {
+                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 senders 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
+        );
+    }
 }

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,45 @@
+//! 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.
+
 #![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

@@ -526,6 +526,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.
@@ -45,4 +48,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;
 
@@ -253,6 +259,27 @@ where
         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).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<_>>()
+        );
+    }
+
     fn number_of_executable_transactions(&self) -> usize {
         self.executable_transactions_sorted_tip_gas_ratio.len()
     }

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

@@ -623,6 +623,92 @@ impl Storage for GraphStorage {
     fn tx_count(&self) -> usize {
         self.graph.node_count()
     }
+
+    #[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 {:?}",
+            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());
+                    }
+                    _ => {}
+                }
+            }
+        }
+        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 RatioTipGasSelectionAlgorithmStorage for GraphStorage {

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

@@ -105,4 +105,12 @@ pub trait Storage {
 
     /// Returns the number of transactions in the storage.
     fn tx_count(&self) -> usize;
+
+    #[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)]