From 874372a8bfeabf08ce9aa903e62dfff8b97a566d Mon Sep 17 00:00:00 2001
From: Arya <aryasolhi@gmail.com>
Date: Fri, 8 Nov 2024 19:39:18 -0500
Subject: [PATCH] Applies suggestions from code review.

---
 zebrad/src/components/mempool/storage.rs      | 19 +++++----
 .../mempool/storage/verified_set.rs           | 42 +++++++------------
 2 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/zebrad/src/components/mempool/storage.rs b/zebrad/src/components/mempool/storage.rs
index 2459efc1149..ce6f09cf1d6 100644
--- a/zebrad/src/components/mempool/storage.rs
+++ b/zebrad/src/components/mempool/storage.rs
@@ -646,29 +646,30 @@ impl Storage {
         tip_height: zebra_chain::block::Height,
     ) -> HashSet<transaction::Hash> {
         let mut tx_ids = HashSet::new();
-        // we need a separate set, since reject() takes the original unmined ID,
-        // then extracts the mined ID out of it
-        let mut mined_tx_ids = HashSet::new();
 
-        for (tx_id, tx) in self.transactions() {
+        for (&tx_id, tx) in self.transactions() {
             if let Some(expiry_height) = tx.transaction.transaction.expiry_height() {
                 if tip_height >= expiry_height {
-                    tx_ids.insert(tx.transaction.id);
-                    mined_tx_ids.insert(*tx_id);
+                    tx_ids.insert(tx_id);
                 }
             }
         }
 
         // expiry height is effecting data, so we match by non-malleable TXID
         self.verified
-            .remove_all_that(|tx| tx_ids.contains(&tx.transaction.id));
+            .remove_all_that(|tx| tx_ids.contains(&tx.transaction.id.mined_id()));
 
         // also reject it
         for &id in &tx_ids {
-            self.reject(id, SameEffectsChainRejectionError::Expired.into());
+            self.reject(
+                // It's okay to omit the auth digest here as we know that `reject()` will always
+                // use mined ids for `SameEffectsChainRejectionError`s.
+                UnminedTxId::Legacy(id),
+                SameEffectsChainRejectionError::Expired.into(),
+            );
         }
 
-        mined_tx_ids
+        tx_ids
     }
 
     /// Check if transaction should be downloaded and/or verified.
diff --git a/zebrad/src/components/mempool/storage/verified_set.rs b/zebrad/src/components/mempool/storage/verified_set.rs
index bf6a17925fd..27d5c90f4d9 100644
--- a/zebrad/src/components/mempool/storage/verified_set.rs
+++ b/zebrad/src/components/mempool/storage/verified_set.rs
@@ -8,7 +8,7 @@ use std::{
 
 use zebra_chain::{
     orchard, sapling, sprout,
-    transaction::{self, Transaction, UnminedTx, VerifiedUnminedTx},
+    transaction::{self, UnminedTx, VerifiedUnminedTx},
     transparent,
 };
 use zebra_node_services::mempool::TransactionDependencies;
@@ -41,10 +41,9 @@ pub struct VerifiedSet {
     /// spend or create outputs of other transactions in the mempool.
     transaction_dependencies: TransactionDependencies,
 
-    /// The [`transparent::Utxo`]s created by verified transactions in the mempool
+    /// The [`transparent::Output`]s created by verified transactions in the mempool.
     ///
-    /// Note that these UTXOs may not be unspent.
-    /// Outputs can be spent by later transactions or blocks in the chain.
+    /// These outputs may be spent by other transactions in the mempool.
     created_outputs: HashMap<transparent::OutPoint, transparent::Output>,
 
     /// The total size of the transactions in the mempool if they were
@@ -163,11 +162,17 @@ impl VerifiedSet {
         self.transaction_dependencies
             .add(tx_id, spent_mempool_outpoints);
 
-        self.cache_outputs_and_respond_to_pending_output_requests_from(
-            tx_id,
-            &transaction.transaction.transaction,
-            pending_outputs,
-        );
+        let tx = &transaction.transaction.transaction;
+        for (index, output) in tx.outputs().iter().cloned().enumerate() {
+            let outpoint = transparent::OutPoint::from_usize(tx_id, index);
+            self.created_outputs.insert(outpoint, output.clone());
+            pending_outputs.respond(&outpoint, output)
+        }
+
+        self.spent_outpoints.extend(tx.spent_outpoints());
+        self.sprout_nullifiers.extend(tx.sprout_nullifiers());
+        self.sapling_nullifiers.extend(tx.sapling_nullifiers());
+        self.orchard_nullifiers.extend(tx.orchard_nullifiers());
 
         self.transactions_serialized_size += transaction.transaction.size;
         self.total_cost += transaction.cost();
@@ -297,25 +302,6 @@ impl VerifiedSet {
             || Self::has_conflicts(&self.orchard_nullifiers, tx.orchard_nullifiers().copied())
     }
 
-    /// Inserts the transaction's outputs into the internal caches and responds to pending output requests.
-    fn cache_outputs_and_respond_to_pending_output_requests_from(
-        &mut self,
-        tx_hash: transaction::Hash,
-        tx: &Transaction,
-        pending_outputs: &mut PendingOutputs,
-    ) {
-        for (index, output) in tx.outputs().iter().cloned().enumerate() {
-            let outpoint = transparent::OutPoint::from_usize(tx_hash, index);
-            self.created_outputs.insert(outpoint, output.clone());
-            pending_outputs.respond(&outpoint, output)
-        }
-
-        self.spent_outpoints.extend(tx.spent_outpoints());
-        self.sprout_nullifiers.extend(tx.sprout_nullifiers());
-        self.sapling_nullifiers.extend(tx.sapling_nullifiers());
-        self.orchard_nullifiers.extend(tx.orchard_nullifiers());
-    }
-
     /// Removes the tracked transaction outputs from the mempool.
     fn remove_outputs(&mut self, unmined_tx: &UnminedTx) {
         let tx = &unmined_tx.transaction;