Stop ignoring the mempool conflicting transaction reject list size limit (#2855)

* Limit the size of rejection lists when there is a spend conflict

Previously, `insert` would return early with an error,
and skip limiting the rejection list sizes.

* Use prop_assert macros in proptests, rather than assert

zebra-chain/src/transaction/auth_digest.rs

@@ -20,7 +20,7 @@ use super::Transaction;
 /// [ZIP-244]: https://zips.z.cash/zip-0244
 #[derive(Copy, Clone, Eq, PartialEq, Hash)]
 #[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
-pub struct AuthDigest(pub(crate) [u8; 32]);
+pub struct AuthDigest(pub [u8; 32]);
 
 impl From<Transaction> for AuthDigest {
     /// Computes the authorizing data commitment for a transaction.

zebra-chain/src/transaction/unmined.rs

@@ -111,7 +111,6 @@ impl UnminedTxId {
     ///
     /// This method must only be used for v1-v4 transaction IDs.
     /// [`Hash`] does not uniquely identify unmined v5 transactions.
-    #[allow(dead_code)]
     pub fn from_legacy_id(legacy_tx_id: Hash) -> UnminedTxId {
         Legacy(legacy_tx_id)
     }
@@ -125,7 +124,6 @@ impl UnminedTxId {
     /// if its authorizing data changes (signatures, proofs, and scripts).
     ///
     /// But for v5 transactions, this ID uniquely identifies the transaction's effects.
-    #[allow(dead_code)]
     pub fn mined_id(&self) -> Hash {
         match self {
             Legacy(legacy_id) => *legacy_id,
@@ -133,15 +131,36 @@ impl UnminedTxId {
         }
     }
 
+    /// Returns a mutable reference to the unique ID
+    /// that will be used if this transaction gets mined into a block.
+    ///
+    /// See [mined_id] for details.
+    #[cfg(any(test, feature = "proptest-impl"))]
+    pub fn mined_id_mut(&mut self) -> &mut Hash {
+        match self {
+            Legacy(legacy_id) => legacy_id,
+            Witnessed(wtx_id) => &mut wtx_id.id,
+        }
+    }
+
     /// Return the digest of this transaction's authorizing data,
     /// (signatures, proofs, and scripts), if it is a v5 transaction.
-    #[allow(dead_code)]
     pub fn auth_digest(&self) -> Option<AuthDigest> {
         match self {
             Legacy(_) => None,
             Witnessed(wtx_id) => Some(wtx_id.auth_digest),
         }
     }
+
+    /// Returns a mutable reference to the digest of this transaction's authorizing data,
+    /// (signatures, proofs, and scripts), if it is a v5 transaction.
+    #[cfg(any(test, feature = "proptest-impl"))]
+    pub fn auth_digest_mut(&mut self) -> Option<&mut AuthDigest> {
+        match self {
+            Legacy(_) => None,
+            Witnessed(wtx_id) => Some(&mut wtx_id.auth_digest),
+        }
+    }
 }
 
 /// An unmined transaction, and its pre-calculated unique identifying ID.

zebrad/src/components/mempool/storage.rs

@@ -26,7 +26,7 @@ const MEMPOOL_SIZE: usize = 4;
 /// https://zips.z.cash/zip-0401#specification
 ///
 /// We use the specified value for all lists for consistency.
-const MAX_EVICTION_MEMORY_ENTRIES: usize = 40_000;
+pub(crate) const MAX_EVICTION_MEMORY_ENTRIES: usize = 40_000;
 
 /// Transactions rejected based on transaction authorizing data (scripts, proofs, signatures),
 /// These rejections are only valid for the current tip.
@@ -79,6 +79,7 @@ pub enum SameEffectsChainRejectionError {
 
 /// Storage error that combines all other specific error types.
 #[derive(Error, Clone, Debug, PartialEq, Eq)]
+#[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
 #[allow(dead_code)]
 pub enum RejectionError {
     #[error(transparent)]
@@ -116,11 +117,18 @@ pub struct Storage {
 }
 
 impl Storage {
-    /// Insert a [`UnminedTx`] into the mempool.
+    /// Insert a [`UnminedTx`] into the mempool, caching any rejections.
     ///
-    /// If its insertion results in evicting other transactions, they will be tracked
+    /// Returns an error if the mempool's verified transactions or rejection caches
+    /// prevent this transaction from being inserted.
+    /// These errors should not be propagated to peers, because the transactions are valid.
+    ///
+    /// If inserting this transaction evicts other transactions, they will be tracked
     /// as [`StorageRejectionError::RandomlyEvicted`].
     pub fn insert(&mut self, tx: UnminedTx) -> Result<UnminedTxId, MempoolError> {
+        // # Security
+        //
+        // This method must call `reject`, rather than modifying the rejection lists directly.
         let tx_id = tx.id;
 
         // First, check if we have a cached rejection for this transaction.
@@ -137,10 +145,11 @@ impl Storage {
         }
 
         // Then, we try to insert into the pool. If this fails the transaction is rejected.
+        let mut result = Ok(tx_id);
         if let Err(rejection_error) = self.verified.insert(tx) {
-            self.tip_rejected_same_effects
+            // We could return here, but we still want to check the mempool size
-                .insert(tx_id.mined_id(), rejection_error.clone());
+            self.reject(tx_id, rejection_error.clone().into());
-            return Err(rejection_error.into());
+            result = Err(rejection_error.into());
         }
 
         // Once inserted, we evict transactions over the pool size limit.
@@ -150,19 +159,21 @@ impl Storage {
                 .evict_one()
                 .expect("mempool is empty, but was expected to be full");
 
-            let _ = self
+            self.reject(
-                .chain_rejected_same_effects
+                evicted_tx.id,
-                .entry(SameEffectsChainRejectionError::RandomlyEvicted)
+                SameEffectsChainRejectionError::RandomlyEvicted.into(),
-                .or_default()
+            );
-                .insert(evicted_tx.id.mined_id());
+
+            // If this transaction gets evicted, set its result to the same error
+            // (we could return here, but we still want to check the mempool size)
+            if evicted_tx.id == tx_id {
+                result = Err(SameEffectsChainRejectionError::RandomlyEvicted.into());
+            }
         }
 
         assert!(self.verified.transaction_count() <= MEMPOOL_SIZE);
 
-        // Security: stop the transaction or rejection lists using too much memory
+        result
-        self.limit_rejection_list_memory();
-
-        Ok(tx_id)
     }
 
     /// Remove [`UnminedTx`]es from the mempool via exact [`UnminedTxId`].

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

@@ -1,4 +1,4 @@
-use std::{collections::HashSet, convert::TryFrom, fmt::Debug};
+use std::{collections::HashSet, convert::TryFrom, env, fmt::Debug};
 
 use proptest::{collection::vec, prelude::*};
 use proptest_derive::Arbitrary;
@@ -15,10 +15,176 @@ use zebra_chain::{
     transparent, LedgerState,
 };
 
-use crate::components::mempool::storage::SameEffectsTipRejectionError;
+use crate::components::mempool::{
+    storage::{
+        MempoolError, RejectionError, SameEffectsTipRejectionError, Storage,
+        MAX_EVICTION_MEMORY_ENTRIES, MEMPOOL_SIZE,
+    },
+    SameEffectsChainRejectionError,
+};
 
 use self::MultipleTransactionRemovalTestInput::*;
-use super::super::{MempoolError, Storage, MEMPOOL_SIZE};
+
+/// The mempool list limit tests can run for a long time.
+///
+/// We reduce the number of cases for those tests,
+/// so individual tests take less than 10 seconds on most machines.
+const DEFAULT_MEMPOOL_LIST_PROPTEST_CASES: u32 = 64;
+
+proptest! {
+    #![proptest_config(
+        proptest::test_runner::Config::with_cases(env::var("PROPTEST_CASES")
+            .ok()
+            .and_then(|v| v.parse().ok())
+            .unwrap_or(DEFAULT_MEMPOOL_LIST_PROPTEST_CASES))
+    )]
+
+    /// Test that the reject list length limits are applied when inserting conflicting transactions.
+    #[test]
+    fn reject_lists_are_limited_insert_conflict(
+        input in any::<SpendConflictTestInput>(),
+        mut rejection_template in any::<UnminedTxId>()
+    ) {
+        let mut storage = Storage::default();
+
+        let (first_transaction, second_transaction) = input.conflicting_transactions();
+        let input_permutations = vec![
+            (first_transaction.clone(), second_transaction.clone()),
+            (second_transaction, first_transaction),
+        ];
+
+        for (transaction_to_accept, transaction_to_reject) in input_permutations {
+            let id_to_accept = transaction_to_accept.id;
+
+            prop_assert_eq!(storage.insert(transaction_to_accept), Ok(id_to_accept));
+
+            // Make unique IDs by converting the index to bytes, and writing it to each ID
+            let unique_ids = (0..MAX_EVICTION_MEMORY_ENTRIES as u32).map(move |index| {
+                let index = index.to_le_bytes();
+                rejection_template.mined_id_mut().0[0..4].copy_from_slice(&index);
+                if let Some(auth_digest) = rejection_template.auth_digest_mut() {
+                    auth_digest.0[0..4].copy_from_slice(&index);
+                }
+
+                rejection_template
+            });
+
+            for rejection in unique_ids {
+                storage.reject(rejection, SameEffectsTipRejectionError::SpendConflict.into());
+            }
+
+            // Make sure there were no duplicates
+            prop_assert_eq!(storage.rejected_transaction_count(), MAX_EVICTION_MEMORY_ENTRIES);
+
+            // The transaction_to_reject will conflict with at least one of:
+            // - transaction_to_accept, or
+            // - a rejection from rejections
+            prop_assert_eq!(
+                storage.insert(transaction_to_reject),
+                Err(MempoolError::StorageEffectsTip(SameEffectsTipRejectionError::SpendConflict))
+            );
+
+            // Since we inserted more than MAX_EVICTION_MEMORY_ENTRIES,
+            // the storage should have cleared the reject list
+            prop_assert_eq!(storage.rejected_transaction_count(), 0);
+
+            storage.clear();
+        }
+    }
+
+    /// Test that the reject list length limits are applied when evicting transactions.
+    #[test]
+    fn reject_lists_are_limited_insert_eviction(
+        transactions in vec(any::<UnminedTx>(), MEMPOOL_SIZE + 1).prop_map(SummaryDebug),
+        mut rejection_template in any::<UnminedTxId>()
+    ) {
+        let mut storage = Storage::default();
+
+        // Make unique IDs by converting the index to bytes, and writing it to each ID
+        let unique_ids = (0..MAX_EVICTION_MEMORY_ENTRIES as u32).map(move |index| {
+            let index = index.to_le_bytes();
+            rejection_template.mined_id_mut().0[0..4].copy_from_slice(&index);
+            if let Some(auth_digest) = rejection_template.auth_digest_mut() {
+                auth_digest.0[0..4].copy_from_slice(&index);
+            }
+
+            rejection_template
+        });
+
+        for rejection in unique_ids {
+            storage.reject(rejection, SameEffectsChainRejectionError::RandomlyEvicted.into());
+        }
+
+        // Make sure there were no duplicates
+        prop_assert_eq!(storage.rejected_transaction_count(), MAX_EVICTION_MEMORY_ENTRIES);
+
+        for transaction in transactions {
+            let tx_id = transaction.id;
+
+            if storage.transaction_count() < MEMPOOL_SIZE {
+                // The initial transactions should be successful
+                prop_assert_eq!(
+                    storage.insert(transaction),
+                    Ok(tx_id)
+                );
+            } else {
+                // The final transaction will cause a random eviction,
+                // which might return an error if this transaction is chosen
+                let result = storage.insert(transaction);
+
+                if result.is_ok() {
+                    prop_assert_eq!(
+                        result,
+                        Ok(tx_id)
+                    );
+                } else {
+                    prop_assert_eq!(
+                        result,
+                        Err(MempoolError::StorageEffectsChain(SameEffectsChainRejectionError::RandomlyEvicted))
+                    );
+                }
+            }
+        }
+
+        // Since we inserted more than MAX_EVICTION_MEMORY_ENTRIES,
+        // the storage should have cleared the reject list
+        prop_assert_eq!(storage.rejected_transaction_count(), 0);
+    }
+
+    /// Test that the reject list length limits are applied when directly rejecting transactions.
+    #[test]
+    fn reject_lists_are_limited_reject(
+        rejection_error in any::<RejectionError>(),
+        mut rejection_template in any::<UnminedTxId>()
+    ) {
+        let mut storage = Storage::default();
+
+        // Make unique IDs by converting the index to bytes, and writing it to each ID
+        let unique_ids = (0..(MAX_EVICTION_MEMORY_ENTRIES + 1) as u32).map(move |index| {
+            let index = index.to_le_bytes();
+            rejection_template.mined_id_mut().0[0..4].copy_from_slice(&index);
+            if let Some(auth_digest) = rejection_template.auth_digest_mut() {
+                auth_digest.0[0..4].copy_from_slice(&index);
+            }
+
+            rejection_template
+        });
+
+        for (index, rejection) in unique_ids.enumerate() {
+            storage.reject(rejection, rejection_error.clone());
+
+            if index == MAX_EVICTION_MEMORY_ENTRIES - 1 {
+                // Make sure there were no duplicates
+                prop_assert_eq!(storage.rejected_transaction_count(), MAX_EVICTION_MEMORY_ENTRIES);
+            } else if index == MAX_EVICTION_MEMORY_ENTRIES {
+                // Since we inserted more than MAX_EVICTION_MEMORY_ENTRIES,
+                // all with the same error,
+                // the storage should have cleared the reject list
+                prop_assert_eq!(storage.rejected_transaction_count(), 0);
+            }
+        }
+    }
+}
 
 proptest! {
     /// Test if a transaction that has a spend conflict with a transaction already in the mempool
@@ -40,14 +206,14 @@ proptest! {
             let id_to_accept = transaction_to_accept.id;
             let id_to_reject = transaction_to_reject.id;
 
-            assert_eq!(storage.insert(transaction_to_accept), Ok(id_to_accept));
+            prop_assert_eq!(storage.insert(transaction_to_accept), Ok(id_to_accept));
 
-            assert_eq!(
+            prop_assert_eq!(
                 storage.insert(transaction_to_reject),
                 Err(MempoolError::StorageEffectsTip(SameEffectsTipRejectionError::SpendConflict))
             );
 
-            assert!(storage.contains_rejected(&id_to_reject));
+            prop_assert!(storage.contains_rejected(&id_to_reject));
 
             storage.clear();
         }
@@ -86,19 +252,19 @@ proptest! {
             let second_id_to_accept = second_transaction_to_accept.id;
             let id_to_reject = transaction_to_reject.id;
 
-            assert_eq!(
+            prop_assert_eq!(
                 storage.insert(first_transaction_to_accept),
                 Ok(first_id_to_accept)
             );
 
-            assert_eq!(
+            prop_assert_eq!(
                 storage.insert(transaction_to_reject),
                 Err(MempoolError::StorageEffectsTip(SameEffectsTipRejectionError::SpendConflict))
             );
 
-            assert!(storage.contains_rejected(&id_to_reject));
+            prop_assert!(storage.contains_rejected(&id_to_reject));
 
-            assert_eq!(
+            prop_assert_eq!(
                 storage.insert(second_transaction_to_accept),
                 Ok(second_id_to_accept)
             );
@@ -124,7 +290,7 @@ proptest! {
 
         // Check that the inserted transactions are still there.
         for transaction_id in &inserted_transactions {
-            assert!(storage.contains_transaction_exact(transaction_id));
+            prop_assert!(storage.contains_transaction_exact(transaction_id));
         }
 
         // Remove some transactions.
@@ -137,14 +303,14 @@ proptest! {
         let removed_transactions = input.removed_transaction_ids();
 
         for removed_transaction_id in &removed_transactions {
-            assert!(!storage.contains_transaction_exact(removed_transaction_id));
+            prop_assert!(!storage.contains_transaction_exact(removed_transaction_id));
         }
 
         // Check that the remaining transactions are still in the storage.
         let remaining_transactions = inserted_transactions.difference(&removed_transactions);
 
         for remaining_transaction_id in remaining_transactions {
-            assert!(storage.contains_transaction_exact(remaining_transaction_id));
+            prop_assert!(storage.contains_transaction_exact(remaining_transaction_id));
         }
     }
 }