diff --git a/zebrad/src/components/inbound/tests.rs b/zebrad/src/components/inbound/tests.rs index 97fcebea..ec937a2f 100644 --- a/zebrad/src/components/inbound/tests.rs +++ b/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)`" ), }; + // 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(); diff --git a/zebrad/src/components/mempool.rs b/zebrad/src/components/mempool.rs index cc2c0614..4d4b12ba 100644 --- a/zebrad/src/components/mempool.rs +++ b/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 diff --git a/zebrad/src/components/mempool/storage.rs b/zebrad/src/components/mempool/storage.rs index 630e98cb..7958303e 100644 --- a/zebrad/src/components/mempool/storage.rs +++ b/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, - /// 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, + chain_rejected_same_effects: + HashMap>, } 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( - evicted_tx.id.mined_id(), - SameEffectsChainRejectionError::RandomlyEvicted, - ); + let _ = self + .chain_rejected_same_effects + .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 { - self.chain_rejected_same_effects.clear(); + for (_, map) in self.chain_rejected_same_effects.iter_mut() { + 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::() } /// 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 - /// any mempool rejected list. + /// Returns the rejection error if a [`UnminedTx`] matching an [`UnminedTxId`] + /// 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 { 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()) { - return Some(error.clone().into()); + for (error, set) in self.chain_rejected_same_effects.iter() { + 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 - .tip_rejected_same_effects - .contains_key(&txid.mined_id()) - || self - .chain_rejected_same_effects - .contains_key(&txid.mined_id()) + self.rejection_error(txid).is_some() } }