Un-reject mempool transactions if the rejection depends on the current tip (#2844)

* Split mempool storage errors into tip-based and chain-based

* Expire tip rejections every time we get a new block

FailedVerification and SpendConflict rejections only apply to the current tip.
The next tip can provide missing inputs, or evict conflicting transactions.

* Enforce MAX_EVICTION_MEMORY_ENTRIES for mempool reject lists

zebrad/src/components/mempool.rs

@@ -33,7 +33,9 @@ mod tests;
 
 pub use self::crawler::Crawler;
 pub use self::error::MempoolError;
-pub use self::storage::{ExactRejectionError, SameEffectsRejectionError};
+pub use self::storage::{
+    ExactTipRejectionError, SameEffectsChainRejectionError, SameEffectsTipRejectionError,
+};
 
 #[cfg(test)]
 pub use self::storage::tests::unmined_transactions_in_blocks;
@@ -218,6 +220,7 @@ impl Service<Request> for Mempool {
                             let mined_ids = block.transaction_hashes.iter().cloned().collect();
                             tx_downloads.cancel(&mined_ids);
                             storage.remove_same_effects(&mined_ids);
+                            storage.clear_tip_rejections();
                         }
                     }
                 }

zebrad/src/components/mempool/error.rs

@@ -5,17 +5,26 @@ use thiserror::Error;
 #[cfg(any(test, feature = "proptest-impl"))]
 use proptest_derive::Arbitrary;
 
-use super::storage::{ExactRejectionError, SameEffectsRejectionError};
+use super::storage::{
+    ExactTipRejectionError, SameEffectsChainRejectionError, SameEffectsTipRejectionError,
+};
 
 #[derive(Error, Clone, Debug, PartialEq, Eq)]
 #[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
 #[allow(dead_code)]
 pub enum MempoolError {
-    #[error("mempool storage has a cached rejection for this exact transaction")]
-    StorageExact(#[from] ExactRejectionError),
+    #[error("mempool storage has a cached tip rejection for this exact transaction")]
+    StorageExactTip(#[from] ExactTipRejectionError),
 
-    #[error("mempool storage has a cached rejection for any transaction with the same effects")]
-    StorageEffects(#[from] SameEffectsRejectionError),
+    #[error(
+        "mempool storage has a cached tip rejection for any transaction with the same effects"
+    )]
+    StorageEffectsTip(#[from] SameEffectsTipRejectionError),
+
+    #[error(
+        "mempool storage has a cached chain rejection for any transaction with the same effects"
+    )]
+    StorageEffectsChain(#[from] SameEffectsChainRejectionError),
 
     #[error("transaction already exists in mempool")]
     InMempool,

zebrad/src/components/mempool/storage.rs

@@ -17,27 +17,52 @@ pub mod tests;
 
 const MEMPOOL_SIZE: usize = 2;
 
+/// The size limit for mempool transaction rejection lists.
+///
+/// > The size of RecentlyEvicted SHOULD never exceed `eviction_memory_entries` entries,
+/// > which is the constant 40000.
+///
+/// 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;
+
 /// Transactions rejected based on transaction authorizing data (scripts, proofs, signatures),
-/// or for other reasons.
+/// These rejections are only valid for the current tip.
+///
+/// Each committed block clears these rejections, because new blocks can supply missing inputs.
 #[derive(Error, Clone, Debug, PartialEq, Eq)]
 #[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
 #[allow(dead_code)]
-pub enum ExactRejectionError {
+pub enum ExactTipRejectionError {
     #[error("transaction did not pass consensus validation")]
     FailedVerification(#[from] zebra_consensus::error::TransactionError),
 }
 
 /// Transactions rejected based only on their effects (spends, outputs, transaction header).
+/// These rejections are only valid for the current tip.
+///
+/// Each committed block clears these rejections, because new blocks can evict other transactions.
 #[derive(Error, Clone, Debug, PartialEq, Eq)]
 #[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
 #[allow(dead_code)]
-pub enum SameEffectsRejectionError {
+pub enum SameEffectsTipRejectionError {
     #[error(
         "transaction rejected because another transaction in the mempool has already spent some of \
         its inputs"
     )]
     SpendConflict,
+}
 
+/// Transactions rejected based only on their effects (spends, outputs, transaction header).
+/// These rejections are valid while the current chain continues to grow.
+///
+/// Rollbacks and network upgrades clear these rejections, because they can lower the tip height,
+/// or change the consensus rules.
+#[derive(Error, Clone, Debug, PartialEq, Eq)]
+#[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
+#[allow(dead_code)]
+pub enum SameEffectsChainRejectionError {
     #[error("best chain tip has reached transaction expiry height")]
     Expired,
 
@@ -58,15 +83,22 @@ pub struct Storage {
     verified: VecDeque<UnminedTx>,
 
     /// The set of transactions rejected due to bad authorizations, or for other reasons,
-    /// and their rejection reasons.
+    /// and their rejection reasons. These rejections only apply to the current tip.
     ///
     /// Only transactions with the exact `UnminedTxId` are invalid.
-    rejected_exact: HashMap<UnminedTxId, ExactRejectionError>,
+    tip_rejected_exact: HashMap<UnminedTxId, ExactTipRejectionError>,
 
-    /// The set of transactions rejected for their effects, and their rejection reasons.
+    /// A set of transactions rejected for their effects, and their rejection reasons.
+    /// These rejections only apply to the current tip.
     ///
     /// Any transaction with the same `transaction::Hash` is invalid.
-    rejected_same_effects: HashMap<transaction::Hash, SameEffectsRejectionError>,
+    tip_rejected_same_effects: HashMap<transaction::Hash, SameEffectsTipRejectionError>,
+
+    /// The set of transactions rejected for their effects, and their rejection reasons.
+    /// These rejections apply until a rollback or network upgrade.
+    ///
+    /// Any transaction with the same `transaction::Hash` is invalid.
+    chain_rejected_same_effects: HashMap<transaction::Hash, SameEffectsChainRejectionError>,
 }
 
 impl Storage {
@@ -94,29 +126,34 @@ impl Storage {
         // nullifier already revealed by another transaction in the mempool, reject that
         // transaction.
         if self.check_spend_conflicts(&tx) {
-            self.rejected_same_effects
-                .insert(tx.id.mined_id(), SameEffectsRejectionError::SpendConflict);
-            return Err(SameEffectsRejectionError::SpendConflict.into());
+            let error = SameEffectsTipRejectionError::SpendConflict;
+            self.tip_rejected_same_effects
+                .insert(tx.id.mined_id(), error.clone());
+            return Err(error.into());
         }
 
         // Then, we insert into the pool.
         self.verified.push_front(tx);
 
+        // Security: stop the transaction or rejection lists using too much memory
+
         // Once inserted, we evict transactions over the pool size limit in FIFO
         // order.
         //
         // TODO: use random weighted eviction as specified in ZIP-401 (#2780)
         if self.verified.len() > MEMPOOL_SIZE {
             for evicted_tx in self.verified.drain(MEMPOOL_SIZE..) {
-                let _ = self.rejected_same_effects.insert(
+                let _ = self.chain_rejected_same_effects.insert(
                     evicted_tx.id.mined_id(),
-                    SameEffectsRejectionError::RandomlyEvicted,
+                    SameEffectsChainRejectionError::RandomlyEvicted,
                 );
             }
 
             assert_eq!(self.verified.len(), MEMPOOL_SIZE);
         }
 
+        self.limit_rejection_list_memory();
+
         Ok(tx_id)
     }
 
@@ -168,8 +205,34 @@ impl Storage {
     /// Clears the whole mempool storage.
     pub fn clear(&mut self) {
         self.verified.clear();
-        self.rejected_exact.clear();
-        self.rejected_same_effects.clear();
+        self.tip_rejected_exact.clear();
+        self.tip_rejected_same_effects.clear();
+        self.chain_rejected_same_effects.clear();
+    }
+
+    /// Clears rejections that only apply to the current tip.
+    pub fn clear_tip_rejections(&mut self) {
+        self.tip_rejected_exact.clear();
+        self.tip_rejected_same_effects.clear();
+    }
+
+    /// Clears rejections that only apply to the current tip.
+    ///
+    /// # Security
+    ///
+    /// This method must be called at the end of every method that adds rejections.
+    /// Otherwise, peers could make our reject lists use a lot of RAM.
+    fn limit_rejection_list_memory(&mut self) {
+        // These lists are an optimisation - it's ok to totally clear them as needed.
+        if self.tip_rejected_exact.len() > MAX_EVICTION_MEMORY_ENTRIES {
+            self.tip_rejected_exact.clear();
+        }
+        if self.tip_rejected_same_effects.len() > MAX_EVICTION_MEMORY_ENTRIES {
+            self.tip_rejected_same_effects.clear();
+        }
+        if self.chain_rejected_same_effects.len() > MAX_EVICTION_MEMORY_ENTRIES {
+            self.chain_rejected_same_effects.clear();
+        }
     }
 
     /// Returns the set of [`UnminedTxId`]s in the mempool.
@@ -211,7 +274,9 @@ impl Storage {
     /// Returns the number of rejected [`UnminedTxId`]s or [`transaction::Hash`]es.
     #[allow(dead_code)]
     pub fn rejected_transaction_count(&self) -> usize {
-        self.rejected_exact.len() + self.rejected_same_effects.len()
+        self.tip_rejected_exact.len()
+            + self.tip_rejected_same_effects.len()
+            + self.chain_rejected_same_effects.len()
     }
 
     /// Returns `true` if a [`UnminedTx`] matching an [`UnminedTxId`] is in
@@ -219,12 +284,16 @@ impl Storage {
     ///
     /// This matches transactions based on each rejection list's matching rule.
     pub fn rejection_error(&self, txid: &UnminedTxId) -> Option<MempoolError> {
-        if let Some(exact_error) = self.rejected_exact.get(txid) {
-            return Some(exact_error.clone().into());
+        if let Some(error) = self.tip_rejected_exact.get(txid) {
+            return Some(error.clone().into());
         }
 
-        if let Some(effects_error) = self.rejected_same_effects.get(&txid.mined_id()) {
-            return Some(effects_error.clone().into());
+        if let Some(error) = self.tip_rejected_same_effects.get(&txid.mined_id()) {
+            return Some(error.clone().into());
+        }
+
+        if let Some(error) = self.chain_rejected_same_effects.get(&txid.mined_id()) {
+            return Some(error.clone().into());
         }
 
         None
@@ -247,8 +316,13 @@ impl Storage {
     ///
     /// This matches transactions based on each rejection list's matching rule.
     pub fn contains_rejected(&self, txid: &UnminedTxId) -> bool {
-        self.rejected_exact.contains_key(txid)
-            || self.rejected_same_effects.contains_key(&txid.mined_id())
+        self.tip_rejected_exact.contains_key(txid)
+            || self
+                .tip_rejected_same_effects
+                .contains_key(&txid.mined_id())
+            || self
+                .chain_rejected_same_effects
+                .contains_key(&txid.mined_id())
     }
 
     /// Checks if the `tx` transaction has spend conflicts with another transaction in the mempool.

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

@@ -11,7 +11,7 @@ use zebra_chain::{
     transparent, LedgerState,
 };
 
-use crate::components::mempool::storage::SameEffectsRejectionError;
+use crate::components::mempool::storage::SameEffectsTipRejectionError;
 
 use super::super::{MempoolError, Storage};
 
@@ -42,7 +42,7 @@ proptest! {
 
             assert_eq!(
                 storage.insert(transaction_to_reject),
-                Err(MempoolError::StorageEffects(SameEffectsRejectionError::SpendConflict))
+                Err(MempoolError::StorageEffectsTip(SameEffectsTipRejectionError::SpendConflict))
             );
 
             assert!(storage.contains_rejected(&id_to_reject));

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

@@ -231,8 +231,8 @@ async fn mempool_queue() -> Result<(), Report> {
     assert_eq!(queued_responses.len(), 1);
     assert_eq!(
         queued_responses[0],
-        Err(MempoolError::StorageEffects(
-            SameEffectsRejectionError::RandomlyEvicted
+        Err(MempoolError::StorageEffectsChain(
+            SameEffectsChainRejectionError::RandomlyEvicted
         ))
     );