Add expired transactions to the mempool rejected list (#2852)

* Add expired transactions to the mempool rejected list

* Apply suggestions from code review

Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>

* Apply suggestions from code review

Co-authored-by: teor <teor@riseup.net>

* Refactor contains_rejected() by just calling rejection_error()

* Improve rejection_error() documentation

* Improve rejected_transaction_count() documentation

Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>
Co-authored-by: teor <teor@riseup.net>

zebrad/src/components/inbound/tests.rs

@@ -273,7 +273,7 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> {
     // Get services
     let (
         inbound_service,
-        _mempool_guard,
+        mempool,
         _committed_blocks,
         _added_transactions,
         mut tx_verifier,
@@ -402,6 +402,25 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> {
             "`MempoolTransactionIds` requests should always respond `Ok(Vec<UnminedTxId>)`"
         ),
     };
+    // Check if tx1 was added to the rejected list as well
+    let response = mempool
+        .clone()
+        .oneshot(mempool::Request::Queue(vec![tx1_id.into()]))
+        .await
+        .unwrap();
+
+    let queued_responses = match response {
+        mempool::Response::Queued(queue_responses) => queue_responses,
+        _ => unreachable!("will never happen in this test"),
+    };
+
+    assert_eq!(queued_responses.len(), 1);
+    assert_eq!(
+        queued_responses[0],
+        Err(mempool::MempoolError::StorageEffectsChain(
+            mempool::SameEffectsChainRejectionError::Expired
+        ))
+    );
 
     // Test transaction 2 is gossiped
     let mut hs = HashSet::new();

zebrad/src/components/mempool.rs

@@ -365,17 +365,26 @@ fn remove_expired_transactions(
     tip_height: zebra_chain::block::Height,
 ) {
     let mut txid_set = HashSet::new();
+    // we need a separate set, since reject() takes the original unmined ID,
+    // then extracts the mined ID out of it
+    let mut unmined_id_set = HashSet::new();
 
     for t in storage.transactions() {
         if let Some(expiry_height) = t.transaction.expiry_height() {
             if tip_height >= expiry_height {
                 txid_set.insert(t.id.mined_id());
+                unmined_id_set.insert(t.id);
             }
         }
     }
 
     // expiry height is effecting data, so we match by non-malleable TXID
     storage.remove_same_effects(&txid_set);
+
+    // also reject it
+    for id in unmined_id_set {
+        storage.reject(id, SameEffectsChainRejectionError::Expired.into());
+    }
 }
 
 /// Add a transaction that failed download and verification to the rejected list

zebrad/src/components/mempool/storage.rs

@@ -60,7 +60,7 @@ pub enum SameEffectsTipRejectionError {
 ///
 /// 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)]
+#[derive(Error, Clone, Debug, PartialEq, Eq, Hash)]
 #[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
 #[allow(dead_code)]
 pub enum SameEffectsChainRejectionError {
@@ -107,11 +107,12 @@ pub struct Storage {
     /// Any transaction with the same `transaction::Hash` is invalid.
     tip_rejected_same_effects: HashMap<transaction::Hash, SameEffectsTipRejectionError>,
 
-    /// The set of transactions rejected for their effects, and their rejection reasons.
+    /// Sets of transactions rejected for their effects, keyed by rejection reason.
     /// 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>,
+    chain_rejected_same_effects:
+        HashMap<SameEffectsChainRejectionError, HashSet<transaction::Hash>>,
 }
 
 impl Storage {
@@ -142,8 +143,6 @@ impl Storage {
             return Err(rejection_error.into());
         }
 
-        // Security: stop the transaction or rejection lists using too much memory
-
         // Once inserted, we evict transactions over the pool size limit.
         while self.verified.transaction_count() > MEMPOOL_SIZE {
             let evicted_tx = self
@@ -151,14 +150,16 @@ impl Storage {
                 .evict_one()
                 .expect("mempool is empty, but was expected to be full");
 
-            let _ = self.chain_rejected_same_effects.insert(
+            let _ = self
-                evicted_tx.id.mined_id(),
+                .chain_rejected_same_effects
-                SameEffectsChainRejectionError::RandomlyEvicted,
+                .entry(SameEffectsChainRejectionError::RandomlyEvicted)
-            );
+                .or_default()
+                .insert(evicted_tx.id.mined_id());
         }
 
         assert!(self.verified.transaction_count() <= MEMPOOL_SIZE);
 
+        // Security: stop the transaction or rejection lists using too much memory
         self.limit_rejection_list_memory();
 
         Ok(tx_id)
@@ -230,8 +231,10 @@ impl Storage {
         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 {
+        for (_, map) in self.chain_rejected_same_effects.iter_mut() {
-            self.chain_rejected_same_effects.clear();
+            if map.len() > MAX_EVICTION_MEMORY_ENTRIES {
+                map.clear();
+            }
         }
     }
 
@@ -273,11 +276,17 @@ impl Storage {
     }
 
     /// Returns the number of rejected [`UnminedTxId`]s or [`transaction::Hash`]es.
+    ///
+    /// Transactions on multiple rejected lists are counted multiple times.
     #[allow(dead_code)]
     pub fn rejected_transaction_count(&self) -> usize {
         self.tip_rejected_exact.len()
             + self.tip_rejected_same_effects.len()
-            + self.chain_rejected_same_effects.len()
+            + self
+                .chain_rejected_same_effects
+                .iter()
+                .map(|(_, map)| map.len())
+                .sum::<usize>()
     }
 
     /// Add a transaction to the rejected list for the given reason.
@@ -290,16 +299,21 @@ impl Storage {
                 self.tip_rejected_same_effects.insert(txid.mined_id(), e);
             }
             RejectionError::SameEffectsChain(e) => {
-                self.chain_rejected_same_effects.insert(txid.mined_id(), e);
+                self.chain_rejected_same_effects
+                    .entry(e)
+                    .or_default()
+                    .insert(txid.mined_id());
             }
         }
         self.limit_rejection_list_memory();
     }
 
-    /// Returns `true` if a [`UnminedTx`] matching an [`UnminedTxId`] is in
+    /// Returns the rejection error if a [`UnminedTx`] matching an [`UnminedTxId`]
-    /// any mempool rejected list.
+    /// is in any mempool rejected list.
     ///
     /// This matches transactions based on each rejection list's matching rule.
+    ///
+    /// Returns an arbitrary error if the transaction is in multiple lists.
     pub fn rejection_error(&self, txid: &UnminedTxId) -> Option<MempoolError> {
         if let Some(error) = self.tip_rejected_exact.get(txid) {
             return Some(error.clone().into());
@@ -309,8 +323,10 @@ impl Storage {
             return Some(error.clone().into());
         }
 
-        if let Some(error) = self.chain_rejected_same_effects.get(&txid.mined_id()) {
+        for (error, set) in self.chain_rejected_same_effects.iter() {
-            return Some(error.clone().into());
+            if set.contains(&txid.mined_id()) {
+                return Some(error.clone().into());
+            }
         }
 
         None
@@ -333,12 +349,6 @@ impl Storage {
     ///
     /// This matches transactions based on each rejection list's matching rule.
     pub fn contains_rejected(&self, txid: &UnminedTxId) -> bool {
-        self.tip_rejected_exact.contains_key(txid)
+        self.rejection_error(txid).is_some()
-            || self
-                .tip_rejected_same_effects
-                .contains_key(&txid.mined_id())
-            || self
-                .chain_rejected_same_effects
-                .contains_key(&txid.mined_id())
     }
 }