Applies suggestions from code review

Avoids unnecessarily rejecting dependent transactions of randomly evicted mempool transactions.

Updates `TransactionDependencies::remove_all()` to omit provided transaction id from the list of removed transaction ids.

zebra-node-services/src/mempool/transaction_dependencies.rs

@@ -75,11 +75,11 @@ impl TransactionDependencies {
     /// that are tracked as being directly or indirectly dependent on that transaction from
     /// this [`TransactionDependencies`].
     ///
-    /// Returns a list of transaction hashes that have been removed if they were previously
-    /// in this [`TransactionDependencies`].
+    /// Returns a list of transaction hashes that were being tracked as dependents of the
+    /// provided transaction hash.
     pub fn remove_all(&mut self, &tx_hash: &transaction::Hash) -> HashSet<transaction::Hash> {
+        let mut all_dependents = HashSet::new();
         let mut current_level_dependents: HashSet<_> = [tx_hash].into();
-        let mut all_dependents = current_level_dependents.clone();
 
         while !current_level_dependents.is_empty() {
             current_level_dependents = current_level_dependents

zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template.rs

@@ -264,7 +264,7 @@ impl GetBlockTemplate {
                 .into_iter()
                 .map(|(min_tx_index, tx)| (min_tx_index, (&tx).into(), tx))
                 .collect();
-            #[cfg(test)]
+
             if like_zcashd {
                 // Sort in serialized data order, excluding the length byte.
                 // `zcashd` sometimes seems to do this, but other times the order is arbitrary.

zebrad/src/components/mempool/storage.rs

@@ -270,25 +270,21 @@ impl Storage {
             // > EvictTransaction MUST do the following:
             // > Select a random transaction to evict, with probability in direct proportion to
             // > eviction weight. (...) Remove it from the mempool.
-            let victim_txs = self.verified.evict_one();
+            let victim_tx = self
+                .verified
+                .evict_one()
+                .expect("mempool is empty, but was expected to be full");
 
-            assert!(
-                !victim_txs.is_empty(),
-                "mempool is empty, but was expected to be full"
+            // > Add the txid and the current time to RecentlyEvicted, dropping the oldest entry in
+            // > RecentlyEvicted if necessary to keep it to at most `eviction_memory_entries entries`.
+            self.reject(
+                victim_tx.transaction.id,
+                SameEffectsChainRejectionError::RandomlyEvicted.into(),
             );
 
-            for victim_tx in victim_txs {
-                // > Add the txid and the current time to RecentlyEvicted, dropping the oldest entry in
-                // > RecentlyEvicted if necessary to keep it to at most `eviction_memory_entries entries`.
-                self.reject(
-                    victim_tx.transaction.id,
-                    SameEffectsChainRejectionError::RandomlyEvicted.into(),
-                );
-
-                // If this transaction gets evicted, set its result to the same error
-                if victim_tx.transaction.id == unmined_tx_id {
-                    result = Err(SameEffectsChainRejectionError::RandomlyEvicted.into());
-                }
+            // If this transaction gets evicted, set its result to the same error
+            if victim_tx.transaction.id == unmined_tx_id {
+                result = Err(SameEffectsChainRejectionError::RandomlyEvicted.into());
             }
         }
 

zebrad/src/components/mempool/storage/verified_set.rs

@@ -178,7 +178,8 @@ impl VerifiedSet {
         Ok(())
     }
 
-    /// Evict one transaction from the set, returns the victim transaction.
+    /// Evict one transaction and any transactions that directly or indirectly depend on
+    /// its outputs from the set, returns the victim transaction and any dependent transactions.
     ///
     /// Removes a transaction with probability in direct proportion to the
     /// eviction weight, as per [ZIP-401].
@@ -196,30 +197,31 @@ impl VerifiedSet {
     /// to 20,000 (mempooltxcostlimit/min(cost)), so the actual cost shouldn't
     /// be too bad.
     ///
+    /// This function is equivalent to `EvictTransaction` in [ZIP-401].
+    ///
     /// [ZIP-401]: https://zips.z.cash/zip-0401
     #[allow(clippy::unwrap_in_result)]
-    pub fn evict_one(&mut self) -> Vec<VerifiedUnminedTx> {
-        if self.transactions.is_empty() {
-            Vec::new()
-        } else {
-            use rand::distributions::{Distribution, WeightedIndex};
-            use rand::prelude::thread_rng;
-
-            let (keys, weights): (Vec<transaction::Hash>, Vec<u64>) = self
-                .transactions
-                .iter()
-                .map(|(&tx_id, tx)| (tx_id, tx.eviction_weight()))
-                .unzip();
-
-            let dist = WeightedIndex::new(weights)
-                .expect("there is at least one weight, all weights are non-negative, and the total is positive");
-
-            let key_to_remove = keys
-                .get(dist.sample(&mut thread_rng()))
-                .expect("should have a key at every index in the distribution");
-
-            self.remove(key_to_remove)
-        }
+    pub fn evict_one(&mut self) -> Option<VerifiedUnminedTx> {
+        use rand::distributions::{Distribution, WeightedIndex};
+        use rand::prelude::thread_rng;
+
+        let (keys, weights): (Vec<transaction::Hash>, Vec<u64>) = self
+            .transactions
+            .iter()
+            .map(|(&tx_id, tx)| (tx_id, tx.eviction_weight()))
+            .unzip();
+
+        let dist = WeightedIndex::new(weights).expect(
+            "there is at least one weight, all weights are non-negative, and the total is positive",
+        );
+
+        let key_to_remove = keys
+            .get(dist.sample(&mut thread_rng()))
+            .expect("should have a key at every index in the distribution");
+
+        // Removes the randomly selected transaction and all of its dependents from the set,
+        // then returns just the randomly selected transaction
+        self.remove(key_to_remove).pop()
     }
 
     /// Clears a list of mined transaction ids from the lists of dependencies for
@@ -248,14 +250,21 @@ impl VerifiedSet {
         removed_count
     }
 
-    /// Removes a transaction from the set.
+    /// Accepts a transaction id for a transaction to remove from the verified set.
+    ///
+    /// Removes the transaction and any transactions that directly or indirectly
+    /// depend on it from the set.
+    ///
+    /// Returns a list of transactions that have been removed with the target transaction
+    /// as the last item.
     ///
-    /// Also removes its outputs from the internal caches.
+    /// Also removes the outputs of any removed transactions from the internal caches.
     fn remove(&mut self, key_to_remove: &transaction::Hash) -> Vec<VerifiedUnminedTx> {
         let removed_transactions: Vec<_> = self
             .transaction_dependencies
             .remove_all(key_to_remove)
             .iter()
+            .chain(std::iter::once(key_to_remove))
             .map(|key_to_remove| {
                 let removed_tx = self
                     .transactions