change(mempool) reject transactions with spent outpoints or nullifiers

zebra-state/src/arbitrary.rs

@@ -59,10 +59,12 @@ impl From<PreparedBlock> for ChainTipBlock {
             new_outputs: _,
             transaction_hashes,
         } = prepared;
+
         Self {
             hash,
             height,
             time: block.header.time,
+            transactions: block.transactions.clone(),
             transaction_hashes,
             previous_block_hash: block.header.previous_block_hash,
         }

zebra-state/src/service/chain_tip.rs

@@ -20,7 +20,7 @@ use zebra_chain::{
     block,
     chain_tip::ChainTip,
     parameters::{Network, NetworkUpgrade},
-    transaction,
+    transaction::{self, Transaction},
 };
 
 use crate::{
@@ -56,6 +56,9 @@ pub struct ChainTipBlock {
     )]
     pub time: DateTime<Utc>,
 
+    /// The block transactions.
+    pub transactions: Vec<Arc<Transaction>>,
+
     /// The mined transaction IDs of the transactions in `block`,
     /// in the same order as `block.transactions`.
     pub transaction_hashes: Arc<[transaction::Hash]>,
@@ -84,6 +87,7 @@ impl From<ContextuallyValidBlock> for ChainTipBlock {
             hash,
             height,
             time: block.header.time,
+            transactions: block.transactions.clone(),
             transaction_hashes,
             previous_block_hash: block.header.previous_block_hash,
         }
@@ -99,10 +103,12 @@ impl From<FinalizedBlock> for ChainTipBlock {
             transaction_hashes,
             ..
         } = finalized;
+
         Self {
             hash,
             height,
             time: block.header.time,
+            transactions: block.transactions.clone(),
             transaction_hashes,
             previous_block_hash: block.header.previous_block_hash,
         }

zebrad/src/components/mempool.rs

@@ -369,7 +369,7 @@ impl Service<Request> for Mempool {
                 // with the same mined IDs as recently mined transactions.
                 let mined_ids = block.transaction_hashes.iter().cloned().collect();
                 tx_downloads.cancel(&mined_ids);
-                storage.remove_same_effects(&mined_ids);
+                storage.reject_and_remove_same_effects(&mined_ids, block.transactions);
                 storage.clear_tip_rejections();
             }
 

zebrad/src/components/mempool/storage.rs

@@ -10,12 +10,15 @@
 use std::{
     collections::{HashMap, HashSet},
     mem::size_of,
+    sync::Arc,
     time::Duration,
 };
 
 use thiserror::Error;
 
-use zebra_chain::transaction::{self, Hash, UnminedTx, UnminedTxId, VerifiedUnminedTx};
+use zebra_chain::transaction::{
+    self, Hash, Transaction, UnminedTx, UnminedTxId, VerifiedUnminedTx,
+};
 
 use self::{eviction_list::EvictionList, verified_set::VerifiedSet};
 use super::{config, downloads::TransactionDownloadVerifyError, MempoolError};
@@ -78,6 +81,12 @@ pub enum SameEffectsChainRejectionError {
     #[error("best chain tip has reached transaction expiry height")]
     Expired,
 
+    #[error("transaction inputs were spent, or nullifiers were revealed, in the best chain")]
+    DuplicateSpend,
+
+    #[error("transaction was committed to the best chain")]
+    Mined,
+
     /// Otherwise valid transaction removed from mempool due to [ZIP-401] random
     /// eviction.
     ///
@@ -276,22 +285,89 @@ impl Storage {
             .remove_all_that(|tx| exact_wtxids.contains(&tx.transaction.id))
     }
 
-    /// Remove transactions from the mempool via non-malleable [`transaction::Hash`].
+    /// Reject and remove transactions from the mempool via non-malleable [`transaction::Hash`].
+    /// - For v5 transactions, transactions are matched by TXID,
+    ///   using only the non-malleable transaction ID.
+    ///   This matches any transaction with the same effect on the blockchain state,
+    ///   even if its signatures and proofs are different.
+    /// - Returns the number of transactions which were removed.
+    /// - Removes from the 'verified' set, if present.
+    ///   Maintains the order in which the other unmined transactions have been inserted into the mempool.
     ///
-    /// For v5 transactions, transactions are matched by TXID,
-    /// using only the non-malleable transaction ID.
-    /// This matches any transaction with the same effect on the blockchain state,
-    /// even if its signatures and proofs are different.
+    /// Reject and remove transactions from the mempool that contain any outpoints or nullifiers in
+    /// the `spent_outpoints` or `nullifiers` collections that are passed in.
     ///
-    /// Returns the number of transactions which were removed.
-    ///
-    /// Removes from the 'verified' set, if present.
-    /// Maintains the order in which the other unmined transactions have been inserted into the mempool.
-    ///
-    /// Does not add or remove from the 'rejected' tracking set.
-    pub fn remove_same_effects(&mut self, mined_ids: &HashSet<transaction::Hash>) -> usize {
-        self.verified
-            .remove_all_that(|tx| mined_ids.contains(&tx.transaction.id.mined_id()))
+    /// Returns the number of transactions that were removed.
+    pub fn reject_and_remove_same_effects(
+        &mut self,
+        mined_ids: &HashSet<transaction::Hash>,
+        transactions: Vec<Arc<Transaction>>,
+    ) -> usize {
+        let num_removed_mined = self
+            .verified
+            .remove_all_that(|tx| mined_ids.contains(&tx.transaction.id.mined_id()));
+
+        let spent_outpoints: HashSet<_> = transactions
+            .iter()
+            .flat_map(|tx| tx.spent_outpoints())
+            .collect();
+        let sprout_nullifiers: HashSet<_> = transactions
+            .iter()
+            .flat_map(|transaction| transaction.sprout_nullifiers())
+            .collect();
+        let sapling_nullifiers: HashSet<_> = transactions
+            .iter()
+            .flat_map(|transaction| transaction.sapling_nullifiers())
+            .collect();
+        let orchard_nullifiers: HashSet<_> = transactions
+            .iter()
+            .flat_map(|transaction| transaction.orchard_nullifiers())
+            .collect();
+
+        let duplicate_spend_ids: HashSet<_> = self
+            .verified
+            .transactions()
+            .filter_map(|tx| {
+                (tx.transaction
+                    .spent_outpoints()
+                    .any(|outpoint| spent_outpoints.contains(&outpoint))
+                    || tx
+                        .transaction
+                        .sprout_nullifiers()
+                        .any(|nullifier| sprout_nullifiers.contains(nullifier))
+                    || tx
+                        .transaction
+                        .sapling_nullifiers()
+                        .any(|nullifier| sapling_nullifiers.contains(nullifier))
+                    || tx
+                        .transaction
+                        .orchard_nullifiers()
+                        .any(|nullifier| orchard_nullifiers.contains(nullifier)))
+                .then_some(tx.id)
+            })
+            .collect();
+
+        let num_removed_duplicate_spend = self
+            .verified
+            .remove_all_that(|tx| duplicate_spend_ids.contains(&tx.transaction.id));
+
+        for &mined_id in mined_ids {
+            self.reject(
+                // the reject and rejection_error fns that store and check `SameEffectsChainRejectionError`s
+                // only use the mined id, so using `Legacy` ids will apply to v5 transactions as well.
+                UnminedTxId::Legacy(mined_id),
+                SameEffectsChainRejectionError::Mined.into(),
+            );
+        }
+
+        for duplicate_spend_id in duplicate_spend_ids {
+            self.reject(
+                duplicate_spend_id,
+                SameEffectsChainRejectionError::DuplicateSpend.into(),
+            );
+        }
+
+        num_removed_mined + num_removed_duplicate_spend
     }
 
     /// Clears the whole mempool storage.
@@ -520,7 +596,8 @@ impl Storage {
         }
 
         // expiry height is effecting data, so we match by non-malleable TXID
-        self.remove_same_effects(&txid_set);
+        self.verified
+            .remove_all_that(|tx| txid_set.contains(&tx.transaction.id.mined_id()));
 
         // also reject it
         for id in unmined_id_set.iter() {

zebrad/src/components/mempool/storage/tests/prop.rs

@@ -364,10 +364,14 @@ proptest! {
 
         // Insert all input transactions, and keep track of the IDs of the one that were actually
         // inserted.
-        let inserted_transactions: HashSet<_> = input.transactions().filter_map(|transaction| {
-            let id = transaction.transaction.id;
+        let inserted_transactions: HashSet<_> = input
+            .transactions()
+            .filter_map(|transaction| {
+                let id = transaction.transaction.id;
 
-            storage.insert(transaction.clone()).ok().map(|_| id)}).collect();
+                storage.insert(transaction.clone()).ok().map(|_| id)
+            })
+            .collect();
 
         // Check that the inserted transactions are still there.
         for transaction_id in &inserted_transactions {
@@ -377,7 +381,16 @@ proptest! {
         // Remove some transactions.
         match &input {
             RemoveExact { wtx_ids_to_remove, .. } => storage.remove_exact(wtx_ids_to_remove),
-            RemoveSameEffects { mined_ids_to_remove, .. } => storage.remove_same_effects(mined_ids_to_remove),
+            RejectAndRemoveSameEffects { mined_ids_to_remove, .. } => {
+                let num_removals = storage.reject_and_remove_same_effects(mined_ids_to_remove, vec![]);
+                    for &removed_transaction_id in mined_ids_to_remove.iter() {
+                        prop_assert_eq!(
+                            storage.rejection_error(&UnminedTxId::Legacy(removed_transaction_id)),
+                            Some(SameEffectsChainRejectionError::Mined.into())
+                        );
+                    }
+                num_removals
+            },
         };
 
         // Check that the removed transactions are not in the storage.
@@ -911,7 +924,7 @@ pub enum MultipleTransactionRemovalTestInput {
         wtx_ids_to_remove: SummaryDebug<HashSet<UnminedTxId>>,
     },
 
-    RemoveSameEffects {
+    RejectAndRemoveSameEffects {
         transactions: SummaryDebug<Vec<VerifiedUnminedTx>>,
         mined_ids_to_remove: SummaryDebug<HashSet<transaction::Hash>>,
     },
@@ -947,7 +960,7 @@ impl Arbitrary for MultipleTransactionRemovalTestInput {
                     .collect();
 
                 prop_oneof![
-                    Just(RemoveSameEffects {
+                    Just(RejectAndRemoveSameEffects {
                         transactions: transactions.clone().into(),
                         mined_ids_to_remove: mined_ids_to_remove.into(),
                     }),
@@ -967,7 +980,7 @@ impl MultipleTransactionRemovalTestInput {
     /// Iterate over all transactions generated as input.
     pub fn transactions(&self) -> impl Iterator<Item = &VerifiedUnminedTx> + '_ {
         match self {
-            RemoveExact { transactions, .. } | RemoveSameEffects { transactions, .. } => {
+            RemoveExact { transactions, .. } | RejectAndRemoveSameEffects { transactions, .. } => {
                 transactions.iter()
             }
         }
@@ -980,7 +993,7 @@ impl MultipleTransactionRemovalTestInput {
                 wtx_ids_to_remove, ..
             } => wtx_ids_to_remove.0.clone(),
 
-            RemoveSameEffects {
+            RejectAndRemoveSameEffects {
                 transactions,
                 mined_ids_to_remove,
             } => transactions

zebrad/src/components/mempool/storage/tests/vectors.rs

@@ -162,23 +162,75 @@ fn mempool_storage_crud_same_effects_mainnet() {
     });
 
     // Get one (1) unmined transaction
-    let unmined_tx = unmined_transactions_in_blocks(.., network)
+    let unmined_tx_1 = unmined_transactions_in_blocks(.., network)
         .next()
         .expect("at least one unmined transaction");
 
     // Insert unmined tx into the mempool.
-    let _ = storage.insert(unmined_tx.clone());
+    let _ = storage.insert(unmined_tx_1.clone());
 
     // Check that it is in the mempool, and not rejected.
-    assert!(storage.contains_transaction_exact(&unmined_tx.transaction.id));
+    assert!(storage.contains_transaction_exact(&unmined_tx_1.transaction.id));
 
-    // Remove tx
-    let removal_count =
-        storage.remove_same_effects(&iter::once(unmined_tx.transaction.id.mined_id()).collect());
+    // Reject and remove mined tx
+    let removal_count = storage.reject_and_remove_same_effects(
+        &iter::once(unmined_tx_1.transaction.id.mined_id()).collect(),
+        vec![unmined_tx_1.transaction.transaction.clone()],
+    );
 
-    // Check that it is /not/ in the mempool.
+    // Check that it is /not/ in the mempool as a verified transaction.
     assert_eq!(removal_count, 1);
-    assert!(!storage.contains_transaction_exact(&unmined_tx.transaction.id));
+    assert!(!storage.contains_transaction_exact(&unmined_tx_1.transaction.id));
+
+    // Check that it's rejection is cached in the chain_rejected_same_effects' `Mined` eviction list.
+    assert_eq!(
+        storage.rejection_error(&unmined_tx_1.transaction.id),
+        Some(SameEffectsChainRejectionError::Mined.into())
+    );
+    assert_eq!(
+        storage.insert(unmined_tx_1),
+        Err(SameEffectsChainRejectionError::Mined.into())
+    );
+
+    // Get a different unmined transaction
+    let unmined_tx_2 = unmined_transactions_in_blocks(1.., network)
+        .find(|tx| {
+            tx.transaction
+                .transaction
+                .spent_outpoints()
+                .next()
+                .is_some()
+        })
+        .expect("at least one unmined transaction with at least 1 spent outpoint");
+
+    // Insert unmined tx into the mempool.
+    assert_eq!(
+        storage.insert(unmined_tx_2.clone()),
+        Ok(unmined_tx_2.transaction.id)
+    );
+
+    // Check that it is in the mempool, and not rejected.
+    assert!(storage.contains_transaction_exact(&unmined_tx_2.transaction.id));
+
+    // Reject and remove duplicate spend tx
+    let removal_count = storage.reject_and_remove_same_effects(
+        &HashSet::new(),
+        vec![unmined_tx_2.transaction.transaction.clone()],
+    );
+
+    // Check that it is /not/ in the mempool as a verified transaction.
+    assert_eq!(removal_count, 1);
+    assert!(!storage.contains_transaction_exact(&unmined_tx_2.transaction.id));
+
+    // Check that it's rejection is cached in the chain_rejected_same_effects' `SpendConflict` eviction list.
+    assert_eq!(
+        storage.rejection_error(&unmined_tx_2.transaction.id),
+        Some(SameEffectsChainRejectionError::DuplicateSpend.into())
+    );
+    assert_eq!(
+        storage.insert(unmined_tx_2),
+        Err(SameEffectsChainRejectionError::DuplicateSpend.into())
+    );
 }
 
 #[test]

zebrad/src/components/mempool/tests/prop.rs

@@ -288,6 +288,7 @@ impl FakeChainTip {
                     hash: chain_tip_block.hash,
                     height,
                     time: previous.time + mock_block_time_delta,
+                    transactions: chain_tip_block.transactions.clone(),
                     transaction_hashes: chain_tip_block.transaction_hashes.clone(),
                     previous_block_hash: previous.hash,
                 }