From 964c819c80723bfddb4def9314413e72f272d92b Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 8 Oct 2021 01:04:23 +1000 Subject: [PATCH] Split storage errors into same effects and exact matches (#2833) Co-authored-by: Conrado Gouvea --- zebrad/src/components/mempool.rs | 4 +- zebrad/src/components/mempool/error.rs | 9 ++- zebrad/src/components/mempool/storage.rs | 73 ++++++++++++++----- .../components/mempool/storage/tests/prop.rs | 4 +- .../mempool/storage/tests/vectors.rs | 5 +- zebrad/src/components/mempool/tests/vector.rs | 4 +- 6 files changed, 69 insertions(+), 30 deletions(-) diff --git a/zebrad/src/components/mempool.rs b/zebrad/src/components/mempool.rs index 27c262d2..fbbeaab3 100644 --- a/zebrad/src/components/mempool.rs +++ b/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(()) } diff --git a/zebrad/src/components/mempool/error.rs b/zebrad/src/components/mempool/error.rs index 1690bf1f..763156f3 100644 --- a/zebrad/src/components/mempool/error.rs +++ b/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, diff --git a/zebrad/src/components/mempool/storage.rs b/zebrad/src/components/mempool/storage.rs index 45a5fd85..d3115071 100644 --- a/zebrad/src/components/mempool/storage.rs +++ b/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, - /// The set of rejected transactions by id, and their rejection reasons. - rejected: HashMap, + + /// 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, + + /// 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, } 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 { - self.rejected.get(txid).cloned() + pub fn rejection_error(&self, txid: &UnminedTxId) -> Option { + 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) -> Vec { 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. diff --git a/zebrad/src/components/mempool/storage/tests/prop.rs b/zebrad/src/components/mempool/storage/tests/prop.rs index 135f640e..bcaf3f17 100644 --- a/zebrad/src/components/mempool/storage/tests/prop.rs +++ b/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)); diff --git a/zebrad/src/components/mempool/storage/tests/vectors.rs b/zebrad/src/components/mempool/storage/tests/vectors.rs index 42cd6914..dd17e6f1 100644 --- a/zebrad/src/components/mempool/storage/tests/vectors.rs +++ b/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 { diff --git a/zebrad/src/components/mempool/tests/vector.rs b/zebrad/src/components/mempool/tests/vector.rs index 5d972148..06711c6f 100644 --- a/zebrad/src/components/mempool/tests/vector.rs +++ b/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 )) );