Split storage errors into same effects and exact matches (#2833)

Co-authored-by: Conrado Gouvea <conrado@zfnd.org>

zebrad/src/components/mempool.rs

@@ -33,7 +33,7 @@ mod tests;
 
 pub use self::crawler::Crawler;
 pub use self::error::MempoolError;
-pub use self::storage::StorageRejectionError;
+pub use self::storage::{ExactRejectionError, SameEffectsRejectionError};
 
 #[cfg(test)]
 pub use self::storage::tests::unmined_transactions_in_blocks;
@@ -185,7 +185,7 @@ impl Mempool {
             return Err(MempoolError::InMempool);
         }
         if let Some(error) = storage.rejection_error(&txid) {
-            return Err(error.into());
+            return Err(error);
         }
         Ok(())
     }

zebrad/src/components/mempool/error.rs

@@ -5,14 +5,17 @@ use thiserror::Error;
 #[cfg(any(test, feature = "proptest-impl"))]
 use proptest_derive::Arbitrary;
 
-use super::storage::StorageRejectionError;
+use super::storage::{ExactRejectionError, SameEffectsRejectionError};
 
 #[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 transaction")]
-    Storage(#[from] StorageRejectionError),
+    #[error("mempool storage has a cached rejection for this exact transaction")]
+    StorageExact(#[from] ExactRejectionError),
+
+    #[error("mempool storage has a cached rejection for any transaction with the same effects")]
+    StorageEffects(#[from] SameEffectsRejectionError),
 
     #[error("transaction already exists in mempool")]
     InMempool,

zebrad/src/components/mempool/storage.rs

@@ -17,10 +17,21 @@ pub mod tests;
 
 const MEMPOOL_SIZE: usize = 2;
 
+/// Transactions rejected based on transaction authorizing data (scripts, proofs, signatures),
+/// or for other reasons.
 #[derive(Error, Clone, Debug, PartialEq, Eq)]
 #[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
 #[allow(dead_code)]
-pub enum StorageRejectionError {
+pub enum ExactRejectionError {
+    #[error("transaction did not pass consensus validation")]
+    FailedVerification(#[from] zebra_consensus::error::TransactionError),
+}
+
+/// Transactions rejected based only on their effects (spends, outputs, transaction header).
+#[derive(Error, Clone, Debug, PartialEq, Eq)]
+#[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
+#[allow(dead_code)]
+pub enum SameEffectsRejectionError {
     #[error(
         "transaction rejected because another transaction in the mempool has already spent some of \
         its inputs"
@@ -38,9 +49,6 @@ pub enum StorageRejectionError {
     /// https://zips.z.cash/zip-0401#specification
     #[error("transaction evicted from the mempool due to ZIP-401 denial of service limits")]
     RandomlyEvicted,
-
-    #[error("transaction did not pass consensus validation")]
-    FailedVerification(#[from] zebra_consensus::error::TransactionError),
 }
 
 #[derive(Default)]
@@ -48,8 +56,17 @@ pub struct Storage {
     /// The set of verified transactions in the mempool. This is a
     /// cache of size [`MEMPOOL_SIZE`].
     verified: VecDeque<UnminedTx>,
-    /// The set of rejected transactions by id, and their rejection reasons.
-    rejected: HashMap<UnminedTxId, StorageRejectionError>,
+
+    /// The set of transactions rejected due to bad authorizations, or for other reasons,
+    /// and their rejection reasons.
+    ///
+    /// Only transactions with the exact `UnminedTxId` are invalid.
+    rejected_exact: HashMap<UnminedTxId, ExactRejectionError>,
+
+    /// The set of transactions rejected for their effects, and their rejection reasons.
+    ///
+    /// Any transaction with the same `transaction::Hash` is invalid.
+    rejected_same_effects: HashMap<transaction::Hash, SameEffectsRejectionError>,
 }
 
 impl Storage {
@@ -62,7 +79,7 @@ impl Storage {
 
         // First, check if we have a cached rejection for this transaction.
         if let Some(error) = self.rejection_error(&tx.id) {
-            return Err(error.into());
+            return Err(error);
         }
 
         // If `tx` is already in the mempool, we don't change anything.
@@ -77,9 +94,9 @@ impl Storage {
         // nullifier already revealed by another transaction in the mempool, reject that
         // transaction.
         if self.check_spend_conflicts(&tx) {
-            self.rejected
-                .insert(tx.id, StorageRejectionError::SpendConflict);
-            return Err(StorageRejectionError::SpendConflict.into());
+            self.rejected_same_effects
+                .insert(tx.id.mined_id(), SameEffectsRejectionError::SpendConflict);
+            return Err(SameEffectsRejectionError::SpendConflict.into());
         }
 
         // Then, we insert into the pool.
@@ -91,9 +108,10 @@ impl Storage {
         // 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
-                    .insert(evicted_tx.id, StorageRejectionError::RandomlyEvicted);
+                let _ = self.rejected_same_effects.insert(
+                    evicted_tx.id.mined_id(),
+                    SameEffectsRejectionError::RandomlyEvicted,
+                );
             }
 
             assert_eq!(self.verified.len(), MEMPOOL_SIZE);
@@ -172,31 +190,46 @@ impl Storage {
         self.verified.iter().cloned().collect()
     }
 
-    /// Returns `true` if a [`UnminedTx`] matching an [`UnminedTxId`] is in
+    /// Returns `true` if a [`UnminedTx`] matching the supplied [`UnminedTxId`] is in
     /// the mempool rejected list.
-    #[allow(dead_code)]
     pub fn contains_rejected(&self, txid: &UnminedTxId) -> bool {
-        self.rejected.contains_key(txid)
+        self.rejected_exact.contains_key(txid)
+            || self.rejected_same_effects.contains_key(&txid.mined_id())
     }
 
     /// Returns `true` if a [`UnminedTx`] matching an [`UnminedTxId`] is in
     /// the mempool rejected list.
-    pub fn rejection_error(&self, txid: &UnminedTxId) -> Option<StorageRejectionError> {
-        self.rejected.get(txid).cloned()
+    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(effects_error) = self.rejected_same_effects.get(&txid.mined_id()) {
+            return Some(effects_error.clone().into());
+        }
+
+        None
     }
 
     /// Returns the set of [`UnminedTxId`]s matching ids in the rejected list.
     pub fn rejected_transactions(&self, tx_ids: HashSet<UnminedTxId>) -> Vec<UnminedTxId> {
         tx_ids
             .into_iter()
-            .filter(|tx| self.rejected.contains_key(tx))
+            .filter(|txid| self.contains_rejected(txid))
             .collect()
     }
 
+    /// 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()
+    }
+
     /// Clears the whole mempool storage.
     pub fn clear(&mut self) {
         self.verified.clear();
-        self.rejected.clear();
+        self.rejected_exact.clear();
+        self.rejected_same_effects.clear();
     }
 
     /// 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::StorageRejectionError;
+use crate::components::mempool::storage::SameEffectsRejectionError;
 
 use super::super::{MempoolError, Storage};
 
@@ -42,7 +42,7 @@ proptest! {
 
             assert_eq!(
                 storage.insert(transaction_to_reject),
-                Err(MempoolError::Storage(StorageRejectionError::SpendConflict))
+                Err(MempoolError::StorageEffects(SameEffectsRejectionError::SpendConflict))
             );
 
             assert!(storage.contains_rejected(&id_to_reject));

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

@@ -96,7 +96,10 @@ fn mempool_storage_basic_for_network(network: Network) -> Result<()> {
     assert_eq!(storage.verified.len(), MEMPOOL_SIZE);
 
     // The rest of the transactions will be in rejected
-    assert_eq!(storage.rejected.len(), rejected_transaction_count);
+    assert_eq!(
+        storage.rejected_transaction_count(),
+        rejected_transaction_count
+    );
 
     // Make sure the last MEMPOOL_SIZE transactions we sent are in the verified
     for tx in expected_in_mempool {

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::Storage(
-            StorageRejectionError::RandomlyEvicted
+        Err(MempoolError::StorageEffects(
+            SameEffectsRejectionError::RandomlyEvicted
         ))
     );