From 2f0926a8e49d26f846b9a3b21f1f6cf3d5828d78 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 12 Oct 2021 10:35:50 +1000 Subject: [PATCH] Stop ignoring the mempool conflicting transaction reject list size limit (#2855) * Limit the size of rejection lists when there is a spend conflict Previously, `insert` would return early with an error, and skip limiting the rejection list sizes. * Use prop_assert macros in proptests, rather than assert --- zebra-chain/src/transaction/auth_digest.rs | 2 +- zebra-chain/src/transaction/unmined.rs | 25 ++- zebrad/src/components/mempool/storage.rs | 41 ++-- .../components/mempool/storage/tests/prop.rs | 192 ++++++++++++++++-- 4 files changed, 228 insertions(+), 32 deletions(-) diff --git a/zebra-chain/src/transaction/auth_digest.rs b/zebra-chain/src/transaction/auth_digest.rs index d42605ca4..968e584ed 100644 --- a/zebra-chain/src/transaction/auth_digest.rs +++ b/zebra-chain/src/transaction/auth_digest.rs @@ -20,7 +20,7 @@ use super::Transaction; /// [ZIP-244]: https://zips.z.cash/zip-0244 #[derive(Copy, Clone, Eq, PartialEq, Hash)] #[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))] -pub struct AuthDigest(pub(crate) [u8; 32]); +pub struct AuthDigest(pub [u8; 32]); impl From for AuthDigest { /// Computes the authorizing data commitment for a transaction. diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs index 89432a097..930991ed8 100644 --- a/zebra-chain/src/transaction/unmined.rs +++ b/zebra-chain/src/transaction/unmined.rs @@ -111,7 +111,6 @@ impl UnminedTxId { /// /// This method must only be used for v1-v4 transaction IDs. /// [`Hash`] does not uniquely identify unmined v5 transactions. - #[allow(dead_code)] pub fn from_legacy_id(legacy_tx_id: Hash) -> UnminedTxId { Legacy(legacy_tx_id) } @@ -125,7 +124,6 @@ impl UnminedTxId { /// if its authorizing data changes (signatures, proofs, and scripts). /// /// But for v5 transactions, this ID uniquely identifies the transaction's effects. - #[allow(dead_code)] pub fn mined_id(&self) -> Hash { match self { Legacy(legacy_id) => *legacy_id, @@ -133,15 +131,36 @@ impl UnminedTxId { } } + /// Returns a mutable reference to the unique ID + /// that will be used if this transaction gets mined into a block. + /// + /// See [mined_id] for details. + #[cfg(any(test, feature = "proptest-impl"))] + pub fn mined_id_mut(&mut self) -> &mut Hash { + match self { + Legacy(legacy_id) => legacy_id, + Witnessed(wtx_id) => &mut wtx_id.id, + } + } + /// Return the digest of this transaction's authorizing data, /// (signatures, proofs, and scripts), if it is a v5 transaction. - #[allow(dead_code)] pub fn auth_digest(&self) -> Option { match self { Legacy(_) => None, Witnessed(wtx_id) => Some(wtx_id.auth_digest), } } + + /// Returns a mutable reference to the digest of this transaction's authorizing data, + /// (signatures, proofs, and scripts), if it is a v5 transaction. + #[cfg(any(test, feature = "proptest-impl"))] + pub fn auth_digest_mut(&mut self) -> Option<&mut AuthDigest> { + match self { + Legacy(_) => None, + Witnessed(wtx_id) => Some(&mut wtx_id.auth_digest), + } + } } /// An unmined transaction, and its pre-calculated unique identifying ID. diff --git a/zebrad/src/components/mempool/storage.rs b/zebrad/src/components/mempool/storage.rs index 7958303ee..320eaeaec 100644 --- a/zebrad/src/components/mempool/storage.rs +++ b/zebrad/src/components/mempool/storage.rs @@ -26,7 +26,7 @@ const MEMPOOL_SIZE: usize = 4; /// https://zips.z.cash/zip-0401#specification /// /// We use the specified value for all lists for consistency. -const MAX_EVICTION_MEMORY_ENTRIES: usize = 40_000; +pub(crate) const MAX_EVICTION_MEMORY_ENTRIES: usize = 40_000; /// Transactions rejected based on transaction authorizing data (scripts, proofs, signatures), /// These rejections are only valid for the current tip. @@ -79,6 +79,7 @@ pub enum SameEffectsChainRejectionError { /// Storage error that combines all other specific error types. #[derive(Error, Clone, Debug, PartialEq, Eq)] +#[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))] #[allow(dead_code)] pub enum RejectionError { #[error(transparent)] @@ -116,11 +117,18 @@ pub struct Storage { } impl Storage { - /// Insert a [`UnminedTx`] into the mempool. + /// Insert a [`UnminedTx`] into the mempool, caching any rejections. /// - /// If its insertion results in evicting other transactions, they will be tracked + /// Returns an error if the mempool's verified transactions or rejection caches + /// prevent this transaction from being inserted. + /// These errors should not be propagated to peers, because the transactions are valid. + /// + /// If inserting this transaction evicts other transactions, they will be tracked /// as [`StorageRejectionError::RandomlyEvicted`]. pub fn insert(&mut self, tx: UnminedTx) -> Result { + // # Security + // + // This method must call `reject`, rather than modifying the rejection lists directly. let tx_id = tx.id; // First, check if we have a cached rejection for this transaction. @@ -137,10 +145,11 @@ impl Storage { } // Then, we try to insert into the pool. If this fails the transaction is rejected. + let mut result = Ok(tx_id); if let Err(rejection_error) = self.verified.insert(tx) { - self.tip_rejected_same_effects - .insert(tx_id.mined_id(), rejection_error.clone()); - return Err(rejection_error.into()); + // We could return here, but we still want to check the mempool size + self.reject(tx_id, rejection_error.clone().into()); + result = Err(rejection_error.into()); } // Once inserted, we evict transactions over the pool size limit. @@ -150,19 +159,21 @@ impl Storage { .evict_one() .expect("mempool is empty, but was expected to be full"); - let _ = self - .chain_rejected_same_effects - .entry(SameEffectsChainRejectionError::RandomlyEvicted) - .or_default() - .insert(evicted_tx.id.mined_id()); + self.reject( + evicted_tx.id, + SameEffectsChainRejectionError::RandomlyEvicted.into(), + ); + + // If this transaction gets evicted, set its result to the same error + // (we could return here, but we still want to check the mempool size) + if evicted_tx.id == tx_id { + result = Err(SameEffectsChainRejectionError::RandomlyEvicted.into()); + } } 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) + result } /// Remove [`UnminedTx`]es from the mempool via exact [`UnminedTxId`]. diff --git a/zebrad/src/components/mempool/storage/tests/prop.rs b/zebrad/src/components/mempool/storage/tests/prop.rs index ff6e623f1..180880926 100644 --- a/zebrad/src/components/mempool/storage/tests/prop.rs +++ b/zebrad/src/components/mempool/storage/tests/prop.rs @@ -1,4 +1,4 @@ -use std::{collections::HashSet, convert::TryFrom, fmt::Debug}; +use std::{collections::HashSet, convert::TryFrom, env, fmt::Debug}; use proptest::{collection::vec, prelude::*}; use proptest_derive::Arbitrary; @@ -15,10 +15,176 @@ use zebra_chain::{ transparent, LedgerState, }; -use crate::components::mempool::storage::SameEffectsTipRejectionError; +use crate::components::mempool::{ + storage::{ + MempoolError, RejectionError, SameEffectsTipRejectionError, Storage, + MAX_EVICTION_MEMORY_ENTRIES, MEMPOOL_SIZE, + }, + SameEffectsChainRejectionError, +}; use self::MultipleTransactionRemovalTestInput::*; -use super::super::{MempoolError, Storage, MEMPOOL_SIZE}; + +/// The mempool list limit tests can run for a long time. +/// +/// We reduce the number of cases for those tests, +/// so individual tests take less than 10 seconds on most machines. +const DEFAULT_MEMPOOL_LIST_PROPTEST_CASES: u32 = 64; + +proptest! { + #![proptest_config( + proptest::test_runner::Config::with_cases(env::var("PROPTEST_CASES") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(DEFAULT_MEMPOOL_LIST_PROPTEST_CASES)) + )] + + /// Test that the reject list length limits are applied when inserting conflicting transactions. + #[test] + fn reject_lists_are_limited_insert_conflict( + input in any::(), + mut rejection_template in any::() + ) { + let mut storage = Storage::default(); + + let (first_transaction, second_transaction) = input.conflicting_transactions(); + let input_permutations = vec![ + (first_transaction.clone(), second_transaction.clone()), + (second_transaction, first_transaction), + ]; + + for (transaction_to_accept, transaction_to_reject) in input_permutations { + let id_to_accept = transaction_to_accept.id; + + prop_assert_eq!(storage.insert(transaction_to_accept), Ok(id_to_accept)); + + // Make unique IDs by converting the index to bytes, and writing it to each ID + let unique_ids = (0..MAX_EVICTION_MEMORY_ENTRIES as 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 + }); + + for rejection in unique_ids { + storage.reject(rejection, SameEffectsTipRejectionError::SpendConflict.into()); + } + + // Make sure there were no duplicates + prop_assert_eq!(storage.rejected_transaction_count(), MAX_EVICTION_MEMORY_ENTRIES); + + // The transaction_to_reject will conflict with at least one of: + // - transaction_to_accept, or + // - a rejection from rejections + prop_assert_eq!( + storage.insert(transaction_to_reject), + Err(MempoolError::StorageEffectsTip(SameEffectsTipRejectionError::SpendConflict)) + ); + + // Since we inserted more than MAX_EVICTION_MEMORY_ENTRIES, + // the storage should have cleared the reject list + prop_assert_eq!(storage.rejected_transaction_count(), 0); + + storage.clear(); + } + } + + /// Test that the reject list length limits are applied when evicting transactions. + #[test] + fn reject_lists_are_limited_insert_eviction( + transactions in vec(any::(), MEMPOOL_SIZE + 1).prop_map(SummaryDebug), + mut rejection_template in any::() + ) { + let mut storage = Storage::default(); + + // Make unique IDs by converting the index to bytes, and writing it to each ID + let unique_ids = (0..MAX_EVICTION_MEMORY_ENTRIES as 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 + }); + + for rejection in unique_ids { + storage.reject(rejection, SameEffectsChainRejectionError::RandomlyEvicted.into()); + } + + // Make sure there were no duplicates + prop_assert_eq!(storage.rejected_transaction_count(), MAX_EVICTION_MEMORY_ENTRIES); + + for transaction in transactions { + let tx_id = transaction.id; + + if storage.transaction_count() < MEMPOOL_SIZE { + // The initial transactions should be successful + prop_assert_eq!( + storage.insert(transaction), + 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); + + if result.is_ok() { + prop_assert_eq!( + result, + Ok(tx_id) + ); + } else { + prop_assert_eq!( + result, + Err(MempoolError::StorageEffectsChain(SameEffectsChainRejectionError::RandomlyEvicted)) + ); + } + } + } + + // Since we inserted more than MAX_EVICTION_MEMORY_ENTRIES, + // the storage should have cleared the reject list + prop_assert_eq!(storage.rejected_transaction_count(), 0); + } + + /// Test that the reject list length limits are applied when directly rejecting transactions. + #[test] + fn reject_lists_are_limited_reject( + rejection_error in any::(), + mut rejection_template in any::() + ) { + let mut storage = Storage::default(); + + // Make unique IDs by converting the index to bytes, and writing it to each ID + let unique_ids = (0..(MAX_EVICTION_MEMORY_ENTRIES + 1) as 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 + }); + + for (index, rejection) in unique_ids.enumerate() { + storage.reject(rejection, rejection_error.clone()); + + if index == MAX_EVICTION_MEMORY_ENTRIES - 1 { + // Make sure there were no duplicates + prop_assert_eq!(storage.rejected_transaction_count(), MAX_EVICTION_MEMORY_ENTRIES); + } 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 + prop_assert_eq!(storage.rejected_transaction_count(), 0); + } + } + } +} proptest! { /// Test if a transaction that has a spend conflict with a transaction already in the mempool @@ -40,14 +206,14 @@ proptest! { let id_to_accept = transaction_to_accept.id; let id_to_reject = transaction_to_reject.id; - assert_eq!(storage.insert(transaction_to_accept), Ok(id_to_accept)); + prop_assert_eq!(storage.insert(transaction_to_accept), Ok(id_to_accept)); - assert_eq!( + prop_assert_eq!( storage.insert(transaction_to_reject), Err(MempoolError::StorageEffectsTip(SameEffectsTipRejectionError::SpendConflict)) ); - assert!(storage.contains_rejected(&id_to_reject)); + prop_assert!(storage.contains_rejected(&id_to_reject)); storage.clear(); } @@ -86,19 +252,19 @@ proptest! { let second_id_to_accept = second_transaction_to_accept.id; let id_to_reject = transaction_to_reject.id; - assert_eq!( + prop_assert_eq!( storage.insert(first_transaction_to_accept), Ok(first_id_to_accept) ); - assert_eq!( + prop_assert_eq!( storage.insert(transaction_to_reject), Err(MempoolError::StorageEffectsTip(SameEffectsTipRejectionError::SpendConflict)) ); - assert!(storage.contains_rejected(&id_to_reject)); + prop_assert!(storage.contains_rejected(&id_to_reject)); - assert_eq!( + prop_assert_eq!( storage.insert(second_transaction_to_accept), Ok(second_id_to_accept) ); @@ -124,7 +290,7 @@ proptest! { // Check that the inserted transactions are still there. for transaction_id in &inserted_transactions { - assert!(storage.contains_transaction_exact(transaction_id)); + prop_assert!(storage.contains_transaction_exact(transaction_id)); } // Remove some transactions. @@ -137,14 +303,14 @@ proptest! { let removed_transactions = input.removed_transaction_ids(); for removed_transaction_id in &removed_transactions { - assert!(!storage.contains_transaction_exact(removed_transaction_id)); + prop_assert!(!storage.contains_transaction_exact(removed_transaction_id)); } // Check that the remaining transactions are still in the storage. let remaining_transactions = inserted_transactions.difference(&removed_transactions); for remaining_transaction_id in remaining_transactions { - assert!(storage.contains_transaction_exact(remaining_transaction_id)); + prop_assert!(storage.contains_transaction_exact(remaining_transaction_id)); } } }