Limit the size and age of the ZIP-401 rejected transaction ID list (#2932)

* Limit the size and age of the ZIP-401 rejected transaction ID list

* Apply suggestions from code review

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

* Fix bug in EvictionList; improve documentation

* Separate public and non-public parts of the documentation

* Fix tests

* Apply suggestions from code review

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

* Fix bug in EvictionList::len()

* Make EvictionList::len() mutable; prune the list inside it

* Limit the size of EvictedList::ordered_entries

* Increase eviction_list_time_mixed time constants to try to make it pass on MacOS

* Simplify logic by assuming refreshes will never happen

* Apply suggestions from code review

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

* Compiling fixes

* Remove MEMPOOL_SIZE and just rely on the ZIP-401 cost limit

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

zebrad/proptest-regressions/components/mempool/storage/tests/prop.txt

@@ -5,3 +5,4 @@
 # It is recommended to check this file in to source control so that
 # everyone who runs the test benefits from these saved cases.
 cc b258d507c0b2aef6c2793bdb3fc29e6367e62fba9df378ea6797e9bc97fd2780 # shrinks to input = RemoveSameEffects { transactions: alloc::vec::Vec<zebra_chain::transaction::unmined::UnminedTx><zebra_chain::transaction::unmined::UnminedTx>, len=2, mined_ids_to_remove: std::collections::hash::set::HashSet<zebra_chain::transaction::hash::Hash><zebra_chain::transaction::hash::Hash>, len=2 }
+cc 4616d813ba54e7b7475a1adb880905dfb05a63b59a18dc079893bf963ae92097 # shrinks to rejection_error = SameEffectsChain(Expired), mut rejection_template = Witnessed(WtxId { id: transaction::Hash("0000000000000000000000000000000000000000000000000000000000000000"), auth_digest: AuthDigest("353b92c552d72e06b512c81f909991b291ca0fec5251d6696755091100000000") })

zebrad/src/components/mempool/storage.rs

@@ -10,13 +10,14 @@
 use std::{
     collections::{HashMap, HashSet},
     mem::size_of,
+    time::Duration,
 };
 
 use thiserror::Error;
 
 use zebra_chain::transaction::{self, UnminedTx, UnminedTxId, VerifiedUnminedTx};
 
-use self::verified_set::VerifiedSet;
+use self::{eviction_list::EvictionList, verified_set::VerifiedSet};
 use super::{config, downloads::TransactionDownloadVerifyError, MempoolError};
 
 #[cfg(any(test, feature = "proptest-impl"))]
@@ -25,11 +26,9 @@ use proptest_derive::Arbitrary;
 #[cfg(test)]
 pub mod tests;
 
+mod eviction_list;
 mod verified_set;
 
-/// The maximum number of verified transactions to store in the mempool.
-const MEMPOOL_SIZE: usize = 4;
-
 /// The size limit for mempool transaction rejection lists.
 ///
 /// > The size of RecentlyEvicted SHOULD never exceed `eviction_memory_entries` entries,
@@ -103,7 +102,6 @@ pub enum RejectionError {
 }
 
 /// Hold mempool verified and rejected mempool transactions.
-#[derive(Default)]
 pub struct Storage {
     /// The set of verified transactions in the mempool. This is a
     /// cache of size [`MEMPOOL_SIZE`].
@@ -125,8 +123,18 @@ pub struct Storage {
     /// These rejections apply until a rollback or network upgrade.
     ///
     /// Any transaction with the same `transaction::Hash` is invalid.
-    chain_rejected_same_effects:
+    ///
-        HashMap<SameEffectsChainRejectionError, HashSet<transaction::Hash>>,
+    /// An [`EvictionList`] is used for both randomly evicted and expired transactions,
+    /// even if it is only needed for the evicted ones. This was done just to simplify
+    /// the existing code; there is no harm in having a timeout for expired transactions
+    /// too since re-checking expired transactions is cheap.
+    // If this code is ever refactored and the lists are split in different fields,
+    // then we can use an `EvictionList` just for the evicted list.
+    chain_rejected_same_effects: HashMap<SameEffectsChainRejectionError, EvictionList>,
+
+    /// The mempool transaction eviction age limit.
+    /// Same as [`Config::eviction_memory_time`].
+    eviction_memory_time: Duration,
 
     /// Max total cost of the verified mempool set, beyond which transactions
     /// are evicted to make room.
@@ -142,9 +150,14 @@ impl Drop for Storage {
 impl Storage {
     #[allow(clippy::field_reassign_with_default)]
     pub(crate) fn new(config: &config::Config) -> Self {
-        let mut default: Storage = Default::default();
+        Self {
-        default.tx_cost_limit = config.tx_cost_limit;
+            tx_cost_limit: config.tx_cost_limit,
-        default
+            eviction_memory_time: config.eviction_memory_time,
+            verified: Default::default(),
+            tip_rejected_exact: Default::default(),
+            tip_rejected_same_effects: Default::default(),
+            chain_rejected_same_effects: Default::default(),
+        }
     }
 
     /// Insert a [`VerifiedUnminedTx`] into the mempool, caching any rejections.
@@ -182,15 +195,26 @@ impl Storage {
             result = Err(rejection_error.into());
         }
 
-        // Once inserted, we evict transactions over the pool size limit.
+        // Once inserted, we evict transactions over the pool size limit per [ZIP-401];
-        while self.verified.transaction_count() > MEMPOOL_SIZE
+        //
-            || self.verified.total_cost() > self.tx_cost_limit
+        // > On receiving a transaction: (...)
-        {
+        // > Calculate its cost. If the total cost of transactions in the mempool including this
+        // > one would `exceed mempooltxcostlimit`, then the node MUST repeatedly call
+        // > EvictTransaction (with the new transaction included as a candidate to evict) until the
+        // > total cost does not exceed `mempooltxcostlimit`.
+        //
+        // [ZIP-401]: https://zips.z.cash/zip-0401
+        while self.verified.total_cost() > self.tx_cost_limit {
+            // > EvictTransaction MUST do the following:
+            // > Select a random transaction to evict, with probability in direct proportion to
+            // > eviction weight. (...) Remove it from the mempool.
             let victim_tx = self
                 .verified
                 .evict_one()
                 .expect("mempool is empty, but was expected to be full");
 
+            // > Add the txid and the current time to RecentlyEvicted, dropping the oldest entry in
+            // > RecentlyEvicted if necessary to keep it to at most `eviction_memory_entries entries`.
             self.reject(
                 victim_tx.transaction.id,
                 SameEffectsChainRejectionError::RandomlyEvicted.into(),
@@ -203,8 +227,6 @@ impl Storage {
             }
         }
 
-        assert!(self.verified.transaction_count() <= MEMPOOL_SIZE);
-
         result
     }
 
@@ -277,11 +299,7 @@ impl Storage {
         if self.tip_rejected_same_effects.len() > MAX_EVICTION_MEMORY_ENTRIES {
             self.tip_rejected_same_effects.clear();
         }
-        for (_, map) in self.chain_rejected_same_effects.iter_mut() {
+        // `chain_rejected_same_effects` limits its size by itself
-            if map.len() > MAX_EVICTION_MEMORY_ENTRIES {
-                map.clear();
-            }
-        }
         self.update_rejected_metrics();
     }
 
@@ -326,12 +344,12 @@ impl Storage {
     ///
     /// Transactions on multiple rejected lists are counted multiple times.
     #[allow(dead_code)]
-    pub fn rejected_transaction_count(&self) -> usize {
+    pub fn rejected_transaction_count(&mut self) -> usize {
         self.tip_rejected_exact.len()
             + self.tip_rejected_same_effects.len()
             + self
                 .chain_rejected_same_effects
-                .iter()
+                .iter_mut()
                 .map(|(_, map)| map.len())
                 .sum::<usize>()
     }
@@ -346,12 +364,12 @@ impl Storage {
                 self.tip_rejected_same_effects.insert(txid.mined_id(), e);
             }
             RejectionError::SameEffectsChain(e) => {
-                // TODO: track evicted victims times, removing those older than
+                let eviction_memory_time = self.eviction_memory_time;
-                // config.eviction_memory_time, as well as FIFO more than
-                // MAX_EVICTION_MEMORY_ENTRIES
                 self.chain_rejected_same_effects
                     .entry(e)
-                    .or_default()
+                    .or_insert_with(|| {
+                        EvictionList::new(MAX_EVICTION_MEMORY_ENTRIES, eviction_memory_time)
+                    })
                     .insert(txid.mined_id());
             }
         }
@@ -374,7 +392,7 @@ impl Storage {
         }
 
         for (error, set) in self.chain_rejected_same_effects.iter() {
-            if set.contains(&txid.mined_id()) {
+            if set.contains_key(&txid.mined_id()) {
                 return Some(error.clone().into());
             }
         }
@@ -486,7 +504,7 @@ impl Storage {
     /// Update metrics related to the rejected lists.
     ///
     /// Must be called every time the rejected lists change.
-    fn update_rejected_metrics(&self) {
+    fn update_rejected_metrics(&mut self) {
         metrics::gauge!(
             "mempool.rejected.transaction.ids",
             self.rejected_transaction_count() as _

zebrad/src/components/mempool/storage/eviction_list.rs

@@ -0,0 +1,144 @@
+//! [`EvictionList`] represents the transaction eviction list with
+//! efficient operations.
+use std::{
+    collections::{HashMap, VecDeque},
+    time::{Duration, Instant},
+};
+
+use zebra_chain::transaction;
+
+/// An eviction list that allows Zebra to efficiently add entries, get entries,
+/// and remove older entries in the order they were inserted.
+pub struct EvictionList {
+    // Maps each TXID in the list to the most recent instant they were added.
+    unique_entries: HashMap<transaction::Hash, Instant>,
+    // The same as `unique_entries` but in the order they were inserted.
+    ordered_entries: VecDeque<transaction::Hash>,
+    // The maximum size of `unique_entries`.
+    max_size: usize,
+    /// The mempool transaction eviction age limit.
+    /// Same as [`Config::eviction_memory_time`].
+    eviction_memory_time: Duration,
+}
+
+impl EvictionList {
+    /// Create a new [`EvictionList`] with the given maximum size and
+    /// eviction time.
+    pub fn new(max_size: usize, eviction_memory_time: Duration) -> Self {
+        Self {
+            unique_entries: Default::default(),
+            ordered_entries: Default::default(),
+            max_size,
+            eviction_memory_time,
+        }
+    }
+
+    /// Inserts a TXID in the list, keeping track of the time it was inserted.
+    ///
+    /// All entries older than [`EvictionList::eviction_memory_time`] will be removed.
+    ///
+    /// # Panics
+    ///
+    /// If the TXID is already in the list.
+    ///
+    pub fn insert(&mut self, key: transaction::Hash) {
+        // From https://zips.z.cash/zip-0401#specification:
+        // > Nodes SHOULD remove transactions from RecentlyEvicted that were evicted more than
+        // > mempoolevictionmemoryminutes minutes ago. This MAY be done periodically,
+        // > and/or just before RecentlyEvicted is accessed when receiving a transaction.
+        self.prune_old();
+        // > Add the txid and the current time to RecentlyEvicted, dropping the oldest entry
+        // > in RecentlyEvicted if necessary to keep it to at most eviction_memory_entries entries.
+        if self.len() >= self.max_size {
+            self.pop_front();
+        }
+        let value = Instant::now();
+        let old_value = self.unique_entries.insert(key, value);
+        // It should be impossible for an already-evicted transaction to be evicted
+        // again since transactions are not added to the mempool if they are evicted,
+        // and the mempool doesn't allow inserting two transactions with the same
+        // hash (they would conflict).
+        assert_eq!(
+            old_value, None,
+            "an already-evicted transaction should not be evicted again"
+        );
+        self.ordered_entries.push_back(key)
+    }
+
+    /// Checks if the given TXID is in the list.
+    pub fn contains_key(&self, txid: &transaction::Hash) -> bool {
+        if let Some(evicted_at) = self.unique_entries.get(txid) {
+            // Since the list is pruned only in mutable functions, make sure
+            // we take expired items into account.
+            if !self.has_expired(evicted_at) {
+                return true;
+            }
+        }
+        false
+    }
+
+    /// Get the size of the list.
+    //
+    // Note: if this method being mutable becomes an issue, it's possible
+    // to compute the number of expired transactions and subtract,
+    // at the cost of `O(len + expired)` performance each time the method is called.
+    //
+    // Currently the performance is `O(expired)` for the first call, then `O(1)` until the next expiry.
+    pub fn len(&mut self) -> usize {
+        self.prune_old();
+        self.unique_entries.len()
+    }
+
+    /// Clear the list.
+    #[allow(dead_code)]
+    pub fn clear(&mut self) {
+        self.unique_entries.clear();
+        self.ordered_entries.clear();
+    }
+
+    /// Prune TXIDs that are older than `eviction_time` ago.
+    ///
+    // This method is public because ZIP-401 states about pruning:
+    // > This MAY be done periodically,
+    pub fn prune_old(&mut self) {
+        while let Some(txid) = self.front() {
+            let evicted_at = self
+                .unique_entries
+                .get(txid)
+                .unwrap_or_else(|| panic!("all entries should exist in both ordered_entries and unique_entries, missing {:?} in unique_entries", txid));
+            if self.has_expired(evicted_at) {
+                self.pop_front();
+            } else {
+                break;
+            }
+        }
+    }
+
+    /// Get the oldest TXID in the list.
+    fn front(&self) -> Option<&transaction::Hash> {
+        self.ordered_entries.front()
+    }
+
+    /// Removes the first element and returns it, or `None` if the `EvictionList`
+    /// is empty.
+    fn pop_front(&mut self) -> Option<transaction::Hash> {
+        if let Some(key) = self.ordered_entries.pop_front() {
+            let removed = self.unique_entries.remove(&key);
+            assert!(
+                removed.is_some(),
+                "all entries should exist in both ordered_entries and unique_entries, missing {:?} in unique_entries",
+                key
+            );
+            Some(key)
+        } else {
+            None
+        }
+    }
+
+    /// Returns if `evicted_at` is considered expired considering the current
+    /// time and the configured eviction time.
+    fn has_expired(&self, evicted_at: &Instant) -> bool {
+        let now = Instant::now();
+        (now - *evicted_at) > self.eviction_memory_time
+    }
+}

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

@@ -1,4 +1,4 @@
-use std::{collections::HashSet, convert::TryFrom, env, fmt::Debug};
+use std::{collections::HashSet, convert::TryFrom, env, fmt::Debug, thread, time::Duration};
 
 use proptest::{collection::vec, prelude::*};
 use proptest_derive::Arbitrary;
@@ -19,8 +19,8 @@ use zebra_chain::{
 use crate::components::mempool::{
     config::Config,
     storage::{
-        MempoolError, RejectionError, SameEffectsTipRejectionError, Storage,
+        eviction_list::EvictionList, MempoolError, RejectionError, SameEffectsTipRejectionError,
-        MAX_EVICTION_MEMORY_ENTRIES, MEMPOOL_SIZE,
+        Storage, MAX_EVICTION_MEMORY_ENTRIES,
     },
     SameEffectsChainRejectionError,
 };
@@ -33,6 +33,13 @@ use MultipleTransactionRemovalTestInput::*;
 /// so individual tests take less than 10 seconds on most machines.
 const DEFAULT_MEMPOOL_LIST_PROPTEST_CASES: u32 = 64;
 
+/// Eviction memory time used for tests. Most tests won't care about this
+/// so we use a large enough value that will never be reached in the tests.
+const EVICTION_MEMORY_TIME: Duration = Duration::from_secs(60 * 60);
+
+/// Transaction count used in some tests to derive the mempool test size.
+const MEMPOOL_TX_COUNT: usize = 4;
+
 proptest! {
     #![proptest_config(
         proptest::test_runner::Config::with_cases(env::var("PROPTEST_CASES")
@@ -50,6 +57,7 @@ proptest! {
         let mut storage = Storage::new(
             &Config {
                 tx_cost_limit: 160_000_000,
+                eviction_memory_time: EVICTION_MEMORY_TIME,
                 ..Default::default()
             });
 
@@ -101,11 +109,15 @@ proptest! {
     /// Test that the reject list length limits are applied when evicting transactions.
     #[test]
     fn reject_lists_are_limited_insert_eviction(
-        transactions in vec(any::<VerifiedUnminedTx>(), MEMPOOL_SIZE + 1).prop_map(SummaryDebug),
+        transactions in vec(any::<VerifiedUnminedTx>(), MEMPOOL_TX_COUNT + 1).prop_map(SummaryDebug),
         mut rejection_template in any::<UnminedTxId>()
     ) {
+        // Use as cost limit the costs of all transactions except one
+        let cost_limit = transactions.iter().take(MEMPOOL_TX_COUNT).map(|tx| tx.cost()).sum();
+
         let mut storage: Storage = Storage::new(&Config {
-            tx_cost_limit: 160_000_000,
+            tx_cost_limit: cost_limit,
+            eviction_memory_time: EVICTION_MEMORY_TIME,
             ..Default::default()
         });
 
@@ -127,19 +139,19 @@ proptest! {
         // Make sure there were no duplicates
         prop_assert_eq!(storage.rejected_transaction_count(), MAX_EVICTION_MEMORY_ENTRIES);
 
-        for transaction in transactions {
+        for (i, transaction) in transactions.iter().enumerate() {
             let tx_id = transaction.transaction.id;
 
-            if storage.transaction_count() < MEMPOOL_SIZE {
+            if i < transactions.len() - 1 {
                 // The initial transactions should be successful
                 prop_assert_eq!(
-                    storage.insert(transaction),
+                    storage.insert(transaction.clone()),
                     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);
+                let result = storage.insert(transaction.clone());
 
                 if result.is_ok() {
                     prop_assert_eq!(
@@ -155,9 +167,13 @@ proptest! {
             }
         }
 
+        // Check if at least one transaction was evicted.
+        // (More than one an be evicted to meet the limit.)
+        prop_assert!(storage.transaction_count() <= MEMPOOL_TX_COUNT);
+
         // Since we inserted more than MAX_EVICTION_MEMORY_ENTRIES,
-        // the storage should have cleared the reject list
+        // the storage should have removed the older entries and kept its size
-        prop_assert_eq!(storage.rejected_transaction_count(), 0);
+        prop_assert_eq!(storage.rejected_transaction_count(), MAX_EVICTION_MEMORY_ENTRIES);
     }
 
     /// Test that the reject list length limits are applied when directly rejecting transactions.
@@ -168,6 +184,7 @@ proptest! {
     ) {
         let mut storage: Storage = Storage::new(&Config {
             tx_cost_limit: 160_000_000,
+            eviction_memory_time: EVICTION_MEMORY_TIME,
             ..Default::default()
         });
 
@@ -191,11 +208,51 @@ proptest! {
             } 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
+                // the storage should have either cleared the reject list
-                prop_assert_eq!(storage.rejected_transaction_count(), 0);
+                // or removed the oldest ones, depending on the structure
+                // used.
+                match rejection_error {
+                    RejectionError::ExactTip(_) |
+                    RejectionError::SameEffectsTip(_) => {
+                        prop_assert_eq!(storage.rejected_transaction_count(), 0);
+                    },
+                    RejectionError::SameEffectsChain(_) => {
+                        prop_assert_eq!(storage.rejected_transaction_count(), MAX_EVICTION_MEMORY_ENTRIES);
+                    },
+                }
             }
         }
     }
+
+    /// Test that the reject list length time limits are applied
+    /// when directly rejecting transactions.
+    #[test]
+    fn reject_lists_are_time_pruned(
+        mut rejection_template in any::<UnminedTxId>()
+    ) {
+        let mut storage = Storage::new(&Config {
+            tx_cost_limit: 160_000_000,
+            eviction_memory_time: Duration::from_millis(10),
+            ..Default::default()
+        });
+
+        // Make unique IDs by converting the index to bytes, and writing it to each ID
+        let unique_ids: Vec<UnminedTxId> = (0..2_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
+        }).collect();
+
+        storage.reject(unique_ids[0], SameEffectsChainRejectionError::RandomlyEvicted.into());
+        thread::sleep(Duration::from_millis(11));
+        storage.reject(unique_ids[1], SameEffectsChainRejectionError::RandomlyEvicted.into());
+
+        prop_assert_eq!(storage.rejected_transaction_count(), 1);
+    }
 }
 
 proptest! {
@@ -208,6 +265,7 @@ proptest! {
     fn conflicting_transactions_are_rejected(input in any::<SpendConflictTestInput>()) {
         let mut storage: Storage = Storage::new(&Config {
             tx_cost_limit: 160_000_000,
+            eviction_memory_time: EVICTION_MEMORY_TIME,
             ..Default::default()
         });
 
@@ -243,6 +301,7 @@ proptest! {
     {
         let mut storage: Storage = Storage::new(&Config {
             tx_cost_limit: 160_000_000,
+            eviction_memory_time: EVICTION_MEMORY_TIME,
             ..Default::default()
         });
 
@@ -299,6 +358,7 @@ proptest! {
     fn removal_of_multiple_transactions(input in any::<MultipleTransactionRemovalTestInput>()) {
         let mut storage: Storage = Storage::new(&Config {
             tx_cost_limit: 160_000_000,
+            eviction_memory_time: EVICTION_MEMORY_TIME,
             ..Default::default()
         });
 
@@ -861,7 +921,7 @@ impl Arbitrary for MultipleTransactionRemovalTestInput {
     type Parameters = ();
 
     fn arbitrary_with(_: Self::Parameters) -> Self::Strategy {
-        vec(any::<VerifiedUnminedTx>(), 1..MEMPOOL_SIZE)
+        vec(any::<VerifiedUnminedTx>(), 1..MEMPOOL_TX_COUNT)
             .prop_flat_map(|transactions| {
                 let indices_to_remove =
                     vec(any::<bool>(), 1..=transactions.len()).prop_map(|removal_markers| {
@@ -931,3 +991,142 @@ impl MultipleTransactionRemovalTestInput {
         }
     }
 }
+
+proptest! {
+
+    // Some tests need to sleep which makes tests slow, so use a single
+    // case. We also don't need multiple cases since proptest is just used
+    // to generate random TXIDs.
+    #![proptest_config(
+        proptest::test_runner::Config::with_cases(1)
+    )]
+
+    /// Check if EvictionList limits the number of entries.
+    #[test]
+    fn eviction_list_size(
+        mut rejection_template in any::<UnminedTxId>()
+    ) {
+        // Make unique IDs by converting the index to bytes, and writing it to each ID
+        let txids: Vec<UnminedTxId> = (0..4_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
+        }).collect();
+
+        let mut e = EvictionList::new(2, EVICTION_MEMORY_TIME);
+        for txid in txids.iter() {
+            e.insert(txid.mined_id());
+        }
+        prop_assert!(!e.contains_key(&txids[0].mined_id()));
+        prop_assert!(!e.contains_key(&txids[1].mined_id()));
+        prop_assert!(e.contains_key(&txids[2].mined_id()));
+        prop_assert!(e.contains_key(&txids[3].mined_id()));
+        prop_assert_eq!(e.len(), 2);
+    }
+
+    /// Check if EvictionList removes old entries.
+    #[test]
+    fn eviction_list_time(
+        mut rejection_template in any::<UnminedTxId>()
+    ) {
+        // Make unique IDs by converting the index to bytes, and writing it to each ID
+        let txids: Vec<UnminedTxId> = (0..2_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
+        }).collect();
+
+        let mut e = EvictionList::new(2, Duration::from_millis(10));
+        e.insert(txids[0].mined_id());
+        thread::sleep(Duration::from_millis(11));
+
+        // First txid has expired, but list wasn't pruned yet.
+        // Make sure len() and contains_key() take that into account.
+        prop_assert!(!e.contains_key(&txids[0].mined_id()));
+        prop_assert_eq!(e.len(), 0);
+
+        e.insert(txids[1].mined_id());
+        prop_assert!(!e.contains_key(&txids[0].mined_id()));
+        prop_assert!(e.contains_key(&txids[1].mined_id()));
+        prop_assert_eq!(e.len(), 1);
+    }
+
+    /// Check if EvictionList removes old entries and computes length correctly
+    /// when the list becomes mixed with expired and non-expired entries.
+    #[test]
+    fn eviction_list_time_mixed(
+        mut rejection_template in any::<UnminedTxId>()
+    ) {
+        // Eviction time (in ms) used in this test. If the value is too low it may cause
+        // this test to fail in slower systems.
+        const EVICTION_TIME: u64 = 500;
+        // Time to wait (in ms) before adding transactions that should not expire
+        // after EVICTION_TIME.
+        // If should be a bit smaller than EVICTION_TIME, but with enough time to
+        // add 10 transactions to the list and still have time to spare before EVICTION_TIME.
+        const BEFORE_EVICTION_TIME: u64 = 250;
+
+        // Make unique IDs by converting the index to bytes, and writing it to each ID
+        let txids: Vec<UnminedTxId> = (0..20_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
+        }).collect();
+
+        let mut e = EvictionList::new(20, Duration::from_millis(EVICTION_TIME));
+        for txid in txids.iter().take(10) {
+            e.insert(txid.mined_id());
+        }
+        thread::sleep(Duration::from_millis(BEFORE_EVICTION_TIME));
+        // Add the next 10 before the eviction time to avoid a prune from
+        // happening.
+        for txid in txids.iter().skip(10) {
+            e.insert(txid.mined_id());
+        }
+        thread::sleep(Duration::from_millis(EVICTION_TIME - BEFORE_EVICTION_TIME + 1));
+
+        // At this point, the first 10 entries should be expired
+        // and the next 10 should not, and the list hasn't been pruned yet.
+        // Make sure len() and contains_key() take that into account.
+        // Note: if one of these fails, you may need to adjust EVICTION_TIME and/or
+        // BEFORE_EVICTION_TIME, see above.
+        for txid in txids.iter().take(10) {
+            prop_assert!(!e.contains_key(&txid.mined_id()));
+        }
+        for txid in txids.iter().skip(10) {
+            prop_assert!(e.contains_key(&txid.mined_id()));
+        }
+        prop_assert_eq!(e.len(), 10);
+
+        // Make sure all of them are expired
+        thread::sleep(Duration::from_millis(EVICTION_TIME + 1));
+
+        for txid in txids.iter() {
+            prop_assert!(!e.contains_key(&txid.mined_id()));
+        }
+        prop_assert_eq!(e.len(), 0);
+    }
+
+    /// Check if EvictionList panics if entries are added multiple times.
+    #[test]
+    #[should_panic]
+    fn eviction_list_refresh(
+        txid in any::<UnminedTxId>()
+    ) {
+        let mut e = EvictionList::new(2, EVICTION_MEMORY_TIME);
+        e.insert(txid.mined_id());
+        e.insert(txid.mined_id());
+    }
+}

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

@@ -14,6 +14,10 @@ use crate::components::mempool::{
     config, storage::tests::unmined_transactions_in_blocks, storage::*, Mempool,
 };
 
+/// Eviction memory time used for tests. Most tests won't care about this
+/// so we use a large enough value that will never be reached in the tests.
+const EVICTION_MEMORY_TIME: Duration = Duration::from_secs(60 * 60);
+
 #[test]
 fn mempool_storage_crud_exact_mainnet() {
     zebra_test::init();
@@ -23,6 +27,7 @@ fn mempool_storage_crud_exact_mainnet() {
     // Create an empty storage instance
     let mut storage: Storage = Storage::new(&config::Config {
         tx_cost_limit: u64::MAX,
+        eviction_memory_time: EVICTION_MEMORY_TIME,
         ..Default::default()
     });
 
@@ -54,6 +59,7 @@ fn mempool_storage_crud_same_effects_mainnet() {
     // Create an empty storage instance
     let mut storage: Storage = Storage::new(&config::Config {
         tx_cost_limit: 160_000_000,
+        eviction_memory_time: EVICTION_MEMORY_TIME,
         ..Default::default()
     });
 
@@ -91,6 +97,7 @@ fn mempool_expired_basic_for_network(network: Network) -> Result<()> {
     // Create an empty storage
     let mut storage: Storage = Storage::new(&config::Config {
         tx_cost_limit: 160_000_000,
+        eviction_memory_time: EVICTION_MEMORY_TIME,
         ..Default::default()
     });