feat: better usage of by_id methods in transaction pools (#5791)

crates/transaction-pool/src/pool/blob.rs

@@ -47,11 +47,7 @@ impl<T: PoolTransaction> BlobTransactions<T> {
     pub(crate) fn add_transaction(&mut self, tx: Arc<ValidPoolTransaction<T>>) {
         assert!(tx.is_eip4844(), "transaction is not a blob tx");
         let id = *tx.id();
-        assert!(
-            !self.by_id.contains_key(&id),
-            "transaction already included {:?}",
-            self.by_id.contains_key(&id)
-        );
+        assert!(!self.contains(&id), "transaction already included {:?}", self.contains(&id));
         let submission_id = self.next_id();
 
         // keep track of size
@@ -204,12 +200,15 @@ impl<T: PoolTransaction> BlobTransactions<T> {
     }
 
     /// Returns `true` if the transaction with the given id is already included in this pool.
-    #[cfg(test)]
-    #[allow(unused)]
     pub(crate) fn contains(&self, id: &TransactionId) -> bool {
         self.by_id.contains_key(id)
     }
 
+    /// Retrieves a transaction with the given ID from the pool, if it exists.
+    fn get(&self, id: &TransactionId) -> Option<&BlobTransaction<T>> {
+        self.by_id.get(id)
+    }
+
     /// Asserts that the bijection between `by_id` and `all` is valid.
     #[cfg(any(test, feature = "test-utils"))]
     pub(crate) fn assert_invariants(&self) {

crates/transaction-pool/src/pool/parked.rs

@@ -49,9 +49,9 @@ impl<T: ParkedOrd> ParkedPool<T> {
     pub fn add_transaction(&mut self, tx: Arc<ValidPoolTransaction<T::Transaction>>) {
         let id = *tx.id();
         assert!(
-            !self.by_id.contains_key(&id),
+            !self.contains(&id),
             "transaction already included {:?}",
-            self.by_id.contains_key(&id)
+            self.get(&id).unwrap().transaction.hash()
         );
         let submission_id = self.next_id();
 
@@ -213,11 +213,15 @@ impl<T: ParkedOrd> ParkedPool<T> {
     }
 
     /// Returns `true` if the transaction with the given id is already included in this pool.
-    #[cfg(test)]
     pub(crate) fn contains(&self, id: &TransactionId) -> bool {
         self.by_id.contains_key(id)
     }
 
+    /// Retrieves a transaction with the given ID from the pool, if it exists.
+    fn get(&self, id: &TransactionId) -> Option<&ParkedPoolTransaction<T>> {
+        self.by_id.get(id)
+    }
+
     /// Asserts that the bijection between `by_id` and `best` is valid.
     #[cfg(any(test, feature = "test-utils"))]
     pub(crate) fn assert_invariants(&self) {
@@ -236,7 +240,7 @@ impl<T: PoolTransaction> ParkedPool<BasefeeOrd<T>> {
         let ids = self.satisfy_base_fee_ids(basefee);
         let mut txs = Vec::with_capacity(ids.len());
         for id in ids {
-            txs.push(self.by_id.get(&id).expect("transaction exists").transaction.clone().into());
+            txs.push(self.get(&id).expect("transaction exists").transaction.clone().into());
         }
         txs
     }
@@ -476,7 +480,7 @@ mod tests {
         let tx = f.validated_arc(MockTransaction::eip1559().inc_price());
         pool.add_transaction(tx.clone());
 
-        assert!(pool.by_id.contains_key(tx.id()));
+        assert!(pool.contains(tx.id()));
         assert_eq!(pool.len(), 1);
 
         let removed = pool.enforce_basefee(u64::MAX);
@@ -498,8 +502,8 @@ mod tests {
         let descendant_tx = f.validated_arc(t.inc_nonce().decr_price());
         pool.add_transaction(descendant_tx.clone());
 
-        assert!(pool.by_id.contains_key(root_tx.id()));
-        assert!(pool.by_id.contains_key(descendant_tx.id()));
+        assert!(pool.contains(root_tx.id()));
+        assert!(pool.contains(descendant_tx.id()));
         assert_eq!(pool.len(), 2);
 
         let removed = pool.enforce_basefee(u64::MAX);
@@ -514,8 +518,8 @@ mod tests {
             assert_eq!(removed.len(), 1);
             assert_eq!(pool2.len(), 1);
             // root got popped - descendant should be skipped
-            assert!(!pool2.by_id.contains_key(root_tx.id()));
-            assert!(pool2.by_id.contains_key(descendant_tx.id()));
+            assert!(!pool2.contains(root_tx.id()));
+            assert!(pool2.contains(descendant_tx.id()));
         }
 
         // remove root transaction via descendant tx fee

crates/transaction-pool/src/pool/pending.rs

@@ -273,7 +273,7 @@ impl<T: TransactionOrdering> PendingPool<T> {
     /// Note: for a transaction with nonce higher than the current on chain nonce this will always
     /// return an ancestor since all transaction in this pool are gapless.
     fn ancestor(&self, id: &TransactionId) -> Option<&PendingTransaction<T>> {
-        self.by_id.get(&id.unchecked_ancestor()?)
+        self.get(&id.unchecked_ancestor()?)
     }
 
     /// Adds a new transactions to the pending queue.
@@ -287,9 +287,9 @@ impl<T: TransactionOrdering> PendingPool<T> {
         base_fee: u64,
     ) {
         assert!(
-            !self.by_id.contains_key(tx.id()),
+            !self.contains(tx.id()),
             "transaction already included {:?}",
-            self.by_id.contains_key(tx.id())
+            self.get(tx.id()).unwrap().transaction.hash()
         );
 
         // keep track of size
@@ -320,7 +320,7 @@ impl<T: TransactionOrdering> PendingPool<T> {
         id: &TransactionId,
     ) -> Option<Arc<ValidPoolTransaction<T::Transaction>>> {
         // mark the next as independent if it exists
-        if let Some(unlocked) = self.by_id.get(&id.descendant()) {
+        if let Some(unlocked) = self.get(&id.descendant()) {
             self.independent_transactions.insert(unlocked.clone());
         };
         self.remove_transaction(id)
@@ -486,11 +486,15 @@ impl<T: TransactionOrdering> PendingPool<T> {
     }
 
     /// Returns `true` if the transaction with the given id is already included in this pool.
-    #[cfg(test)]
     pub(crate) fn contains(&self, id: &TransactionId) -> bool {
         self.by_id.contains_key(id)
     }
 
+    /// Retrieves a transaction with the given ID from the pool, if it exists.
+    fn get(&self, id: &TransactionId) -> Option<&PendingTransaction<T>> {
+        self.by_id.get(id)
+    }
+
     /// Asserts that the bijection between `by_id` and `all` is valid.
     #[cfg(any(test, feature = "test-utils"))]
     pub(crate) fn assert_invariants(&self) {
@@ -579,7 +583,7 @@ mod tests {
         let tx = f.validated_arc(MockTransaction::eip1559().inc_price());
         pool.add_transaction(tx.clone(), 0);
 
-        assert!(pool.by_id.contains_key(tx.id()));
+        assert!(pool.contains(tx.id()));
         assert_eq!(pool.len(), 1);
 
         let removed = pool.update_base_fee(0);
@@ -601,8 +605,8 @@ mod tests {
         let descendant_tx = f.validated_arc(t.inc_nonce().decr_price());
         pool.add_transaction(descendant_tx.clone(), 0);
 
-        assert!(pool.by_id.contains_key(root_tx.id()));
-        assert!(pool.by_id.contains_key(descendant_tx.id()));
+        assert!(pool.contains(root_tx.id()));
+        assert!(pool.contains(descendant_tx.id()));
         assert_eq!(pool.len(), 2);
 
         assert_eq!(pool.independent_transactions.len(), 1);
@@ -619,8 +623,8 @@ mod tests {
             assert_eq!(removed.len(), 1);
             assert_eq!(pool2.len(), 1);
             // descendant got popped
-            assert!(pool2.by_id.contains_key(root_tx.id()));
-            assert!(!pool2.by_id.contains_key(descendant_tx.id()));
+            assert!(pool2.contains(root_tx.id()));
+            assert!(!pool2.contains(descendant_tx.id()));
         }
 
         // remove root transaction via fee