From b614d42109cac39c8818f05758eb11ec7577e729 Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Tue, 21 Sep 2021 11:35:51 +0000 Subject: [PATCH 01/12] Add `Transaction::spent_outpoints` getter method Returns an iterator over the UTXO `OutPoint`s spent by the transaction. --- zebra-chain/src/transaction.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index 315e7ce5bf2..67c1d0ff3b9 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -349,6 +349,13 @@ impl Transaction { } } + /// Access the [`transparent::OutPoint`]s spent by this transaction. + pub fn spent_outpoints(&self) -> impl Iterator + '_ { + self.inputs() + .iter() + .filter_map(transparent::Input::outpoint) + } + /// Access the transparent outputs of this transaction, regardless of version. pub fn outputs(&self) -> &[transparent::Output] { match self { From 49856a61be230e8942446319702b464549f6b6af Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Thu, 16 Sep 2021 13:28:55 +0000 Subject: [PATCH 02/12] Add `mempool::Error::Conflict` variant An error representing that a transaction was rejected because it conflicts with another transaction that's already in the mempool. --- zebrad/src/components/mempool/error.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/zebrad/src/components/mempool/error.rs b/zebrad/src/components/mempool/error.rs index 9d5eb94990d..59b55a75e19 100644 --- a/zebrad/src/components/mempool/error.rs +++ b/zebrad/src/components/mempool/error.rs @@ -43,4 +43,8 @@ pub enum MempoolError { /// The queue's capacity is [`super::downloads::MAX_INBOUND_CONCURRENCY`]. #[error("transaction dropped because the queue is full")] FullQueue, + + /// The transaction conflicts with another transaction already in the mempool. + #[error("transaction rejected because it conflicts with another transaction in the mempool")] + Conflict, } From 2e4dd3b27f3908fcebd87b459d096ec34208630a Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Thu, 16 Sep 2021 13:36:47 +0000 Subject: [PATCH 03/12] Reject conflicting mempool transactions Reject including a transaction in the mempool if it spends outputs already spent by, or reveals nullifiers already revealed by another transaction in the mempool. --- zebrad/src/components/mempool/storage.rs | 53 +++++++++++++++++++++++- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/zebrad/src/components/mempool/storage.rs b/zebrad/src/components/mempool/storage.rs index 6f706c857f3..94a392a7196 100644 --- a/zebrad/src/components/mempool/storage.rs +++ b/zebrad/src/components/mempool/storage.rs @@ -1,8 +1,11 @@ -use std::collections::{HashMap, HashSet, VecDeque}; +use std::{ + collections::{HashMap, HashSet, VecDeque}, + hash::Hash, +}; use zebra_chain::{ block, - transaction::{UnminedTx, UnminedTxId}, + transaction::{Transaction, UnminedTx, UnminedTxId}, }; use zebra_consensus::error::TransactionError; @@ -21,6 +24,8 @@ pub enum State { /// An otherwise valid mempool transaction was mined into a block, therefore /// no longer belongs in the mempool. Confirmed(block::Hash), + /// Rejected because it conflicted with another transaction already in the mempool. + Conflict, /// Stayed in mempool for too long without being mined. // TODO(2021-09-09): Implement ZIP-203: Validate Transaction Expiry Height. // TODO(2021-09-09): https://github.com/ZcashFoundation/zebra/issues/2387 @@ -58,6 +63,7 @@ impl Storage { State::Confirmed(block_hash) => MempoolError::InBlock(*block_hash), State::Excess => MempoolError::Excess, State::LowFee => MempoolError::LowFee, + State::Conflict => MempoolError::Conflict, }); } @@ -69,6 +75,16 @@ impl Storage { return Err(MempoolError::InMempool); } + // If `tx` spends an UTXO already spent by another transaction in the mempool or reveals a + // nullifier already revealed by another transaction in the mempool, reject that + // transaction. + // + // TODO: Consider replacing the transaction in the mempool if the fee is higher (#2781). + if self.check_spend_conflicts(&tx) { + self.rejected.insert(tx.id, State::Conflict); + return Err(MempoolError::Rejected); + } + // Then, we insert into the pool. self.verified.push_front(tx); @@ -146,4 +162,37 @@ impl Storage { self.verified.clear(); self.rejected.clear(); } + + /// Checks if the `tx` transaction conflicts with another transaction in the mempool. + /// + /// Two transactions conflict if they spent the same UTXO or if they reveal the same nullifier. + fn check_spend_conflicts(&self, tx: &UnminedTx) -> bool { + self.has_conflicts(tx, Transaction::spent_outpoints) + || self.has_conflicts(tx, Transaction::sprout_nullifiers) + || self.has_conflicts(tx, Transaction::sapling_nullifiers) + || self.has_conflicts(tx, Transaction::orchard_nullifiers) + } + + /// Checks if the `tx` transaction has any conflicts with the transactions in the mempool for + /// the provided output type obtrained through the `extractor`. + fn has_conflicts<'slf, 'tx, Extractor, Outputs>( + &'slf self, + tx: &'tx UnminedTx, + extractor: Extractor, + ) -> bool + where + 'slf: 'tx, + Extractor: Fn(&'tx Transaction) -> Outputs, + Outputs: IntoIterator, + Outputs::Item: Eq + Hash + 'tx, + { + // TODO: This algorithm should be improved to avoid a performance impact when the mempool + // size is increased (#2784). + let new_outputs: HashSet<_> = extractor(&tx.transaction).into_iter().collect(); + + self.verified + .iter() + .flat_map(|tx| extractor(&tx.transaction)) + .any(|output| new_outputs.contains(&output)) + } } From 3778015ca4be91ce61e1833acda7e7a4cf1efabb Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Mon, 20 Sep 2021 14:41:59 -0300 Subject: [PATCH 04/12] Fix typo in documentation Remove the `r` that was incorrectly added. Co-authored-by: teor --- zebrad/src/components/mempool/storage.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebrad/src/components/mempool/storage.rs b/zebrad/src/components/mempool/storage.rs index 94a392a7196..40caaf926e0 100644 --- a/zebrad/src/components/mempool/storage.rs +++ b/zebrad/src/components/mempool/storage.rs @@ -174,7 +174,7 @@ impl Storage { } /// Checks if the `tx` transaction has any conflicts with the transactions in the mempool for - /// the provided output type obtrained through the `extractor`. + /// the provided output type obtained through the `extractor`. fn has_conflicts<'slf, 'tx, Extractor, Outputs>( &'slf self, tx: &'tx UnminedTx, From fcf87cd29869e9126f95d35f2ab56d637b831135 Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Mon, 20 Sep 2021 14:43:15 -0300 Subject: [PATCH 05/12] Specify that the conflict is a spend conflict Make the situation clearer, because there are other types of conflict. Co-authored-by: teor --- zebrad/src/components/mempool/error.rs | 9 +++++--- zebrad/src/components/mempool/storage.rs | 27 ++++++++++++------------ 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/zebrad/src/components/mempool/error.rs b/zebrad/src/components/mempool/error.rs index 59b55a75e19..c700ced0fbd 100644 --- a/zebrad/src/components/mempool/error.rs +++ b/zebrad/src/components/mempool/error.rs @@ -44,7 +44,10 @@ pub enum MempoolError { #[error("transaction dropped because the queue is full")] FullQueue, - /// The transaction conflicts with another transaction already in the mempool. - #[error("transaction rejected because it conflicts with another transaction in the mempool")] - Conflict, + /// The transaction has a spend conflict with another transaction already in the mempool. + #[error( + "transaction rejected because another transaction in the mempool has already spent some of \ + its inputs" + )] + SpendConflict, } diff --git a/zebrad/src/components/mempool/storage.rs b/zebrad/src/components/mempool/storage.rs index 40caaf926e0..b665a88efd7 100644 --- a/zebrad/src/components/mempool/storage.rs +++ b/zebrad/src/components/mempool/storage.rs @@ -24,8 +24,8 @@ pub enum State { /// An otherwise valid mempool transaction was mined into a block, therefore /// no longer belongs in the mempool. Confirmed(block::Hash), - /// Rejected because it conflicted with another transaction already in the mempool. - Conflict, + /// Rejected because it has a spend conflict with another transaction already in the mempool. + SpendConflict, /// Stayed in mempool for too long without being mined. // TODO(2021-09-09): Implement ZIP-203: Validate Transaction Expiry Height. // TODO(2021-09-09): https://github.com/ZcashFoundation/zebra/issues/2387 @@ -63,7 +63,7 @@ impl Storage { State::Confirmed(block_hash) => MempoolError::InBlock(*block_hash), State::Excess => MempoolError::Excess, State::LowFee => MempoolError::LowFee, - State::Conflict => MempoolError::Conflict, + State::SpendConflict => MempoolError::SpendConflict, }); } @@ -81,7 +81,7 @@ impl Storage { // // TODO: Consider replacing the transaction in the mempool if the fee is higher (#2781). if self.check_spend_conflicts(&tx) { - self.rejected.insert(tx.id, State::Conflict); + self.rejected.insert(tx.id, State::SpendConflict); return Err(MempoolError::Rejected); } @@ -163,19 +163,20 @@ impl Storage { self.rejected.clear(); } - /// Checks if the `tx` transaction conflicts with another transaction in the mempool. + /// Checks if the `tx` transaction has spend conflicts with another transaction in the mempool. /// - /// Two transactions conflict if they spent the same UTXO or if they reveal the same nullifier. + /// Two transactions have a spend conflict if they spent the same UTXO or if they reveal the + /// same nullifier. fn check_spend_conflicts(&self, tx: &UnminedTx) -> bool { - self.has_conflicts(tx, Transaction::spent_outpoints) - || self.has_conflicts(tx, Transaction::sprout_nullifiers) - || self.has_conflicts(tx, Transaction::sapling_nullifiers) - || self.has_conflicts(tx, Transaction::orchard_nullifiers) + self.has_spend_conflicts(tx, Transaction::spent_outpoints) + || self.has_spend_conflicts(tx, Transaction::sprout_nullifiers) + || self.has_spend_conflicts(tx, Transaction::sapling_nullifiers) + || self.has_spend_conflicts(tx, Transaction::orchard_nullifiers) } - /// Checks if the `tx` transaction has any conflicts with the transactions in the mempool for - /// the provided output type obtained through the `extractor`. - fn has_conflicts<'slf, 'tx, Extractor, Outputs>( + /// Checks if the `tx` transaction has any spend conflicts with the transactions in the mempool + /// for the provided output type obtained through the `extractor`. + fn has_spend_conflicts<'slf, 'tx, Extractor, Outputs>( &'slf self, tx: &'tx UnminedTx, extractor: Extractor, From bc46fcf4541a77eb87764d93f4c2f46968762179 Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Wed, 22 Sep 2021 19:49:44 -0300 Subject: [PATCH 06/12] Clarify that the outpoints are from inputs Because otherwise it could lead to confusion because it could also mean the outputs of the transaction represented as `OutPoint` references. Co-authored-by: teor --- zebra-chain/src/transaction.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index 67c1d0ff3b9..2fc9a2afe53 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -349,7 +349,7 @@ impl Transaction { } } - /// Access the [`transparent::OutPoint`]s spent by this transaction. + /// Access the [`transparent::OutPoint`]s spent by this transaction's [`transparent::Input`]s. pub fn spent_outpoints(&self) -> impl Iterator + '_ { self.inputs() .iter() From 9003da84929a8eb60728618220a27c9cbb989a8f Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Thu, 23 Sep 2021 14:39:03 +0000 Subject: [PATCH 07/12] Create `storage::tests::vectors` module Refactor to follow the convention used for other tests. --- .../src/components/mempool/storage/tests.rs | 95 +----------------- .../mempool/storage/tests/vectors.rs | 96 +++++++++++++++++++ 2 files changed, 97 insertions(+), 94 deletions(-) create mode 100644 zebrad/src/components/mempool/storage/tests/vectors.rs diff --git a/zebrad/src/components/mempool/storage/tests.rs b/zebrad/src/components/mempool/storage/tests.rs index ce1a7b301d8..e6166dd7f07 100644 --- a/zebrad/src/components/mempool/storage/tests.rs +++ b/zebrad/src/components/mempool/storage/tests.rs @@ -1,103 +1,10 @@ use std::ops::RangeBounds; -use super::*; - use zebra_chain::{ block::Block, parameters::Network, serialization::ZcashDeserializeInto, transaction::UnminedTx, }; -use color_eyre::eyre::Result; - -#[test] -fn mempool_storage_crud_mainnet() { - zebra_test::init(); - - let network = Network::Mainnet; - - // Create an empty storage instance - let mut storage: Storage = Default::default(); - - // Get one (1) unmined transaction - let unmined_tx = unmined_transactions_in_blocks(.., network) - .next() - .expect("at least one unmined transaction"); - - // Insert unmined tx into the mempool. - let _ = storage.insert(unmined_tx.clone()); - - // Check that it is in the mempool, and not rejected. - assert!(storage.contains(&unmined_tx.id)); - - // Remove tx - let _ = storage.remove(&unmined_tx.id); - - // Check that it is /not/ in the mempool. - assert!(!storage.contains(&unmined_tx.id)); -} - -#[test] -fn mempool_storage_basic() -> Result<()> { - zebra_test::init(); - - mempool_storage_basic_for_network(Network::Mainnet)?; - mempool_storage_basic_for_network(Network::Testnet)?; - - Ok(()) -} - -fn mempool_storage_basic_for_network(network: Network) -> Result<()> { - // Create an empty storage - let mut storage: Storage = Default::default(); - - // Get transactions from the first 10 blocks of the Zcash blockchain - let unmined_transactions: Vec<_> = unmined_transactions_in_blocks(..=10, network).collect(); - let total_transactions = unmined_transactions.len(); - - // Insert them all to the storage - for unmined_transaction in unmined_transactions.clone() { - storage.insert(unmined_transaction)?; - } - - // Separate transactions into the ones expected to be in the mempool and those expected to be - // rejected. - let rejected_transaction_count = total_transactions - MEMPOOL_SIZE; - let expected_to_be_rejected = &unmined_transactions[..rejected_transaction_count]; - let expected_in_mempool = &unmined_transactions[rejected_transaction_count..]; - - // Only MEMPOOL_SIZE should land in verified - assert_eq!(storage.verified.len(), MEMPOOL_SIZE); - - // The rest of the transactions will be in rejected - assert_eq!(storage.rejected.len(), rejected_transaction_count); - - // Make sure the last MEMPOOL_SIZE transactions we sent are in the verified - for tx in expected_in_mempool { - assert!(storage.contains(&tx.id)); - } - - // Anything greater should not be in the verified - for tx in expected_to_be_rejected { - assert!(!storage.contains(&tx.id)); - } - - // Query all the ids we have for rejected, get back `total - MEMPOOL_SIZE` - let all_ids: HashSet = unmined_transactions.iter().map(|tx| tx.id).collect(); - - // Convert response to a `HashSet` as we need a fixed order to compare. - let rejected_response: HashSet = - storage.rejected_transactions(all_ids).into_iter().collect(); - - let rejected_ids = expected_to_be_rejected.iter().map(|tx| tx.id).collect(); - - assert_eq!(rejected_response, rejected_ids); - - // Use `contains_rejected` to make sure the first id stored is now rejected - assert!(storage.contains_rejected(&expected_to_be_rejected[0].id)); - // Use `contains_rejected` to make sure the last id stored is not rejected - assert!(!storage.contains_rejected(&expected_in_mempool[0].id)); - - Ok(()) -} +mod vectors; pub fn unmined_transactions_in_blocks( block_height_range: impl RangeBounds, diff --git a/zebrad/src/components/mempool/storage/tests/vectors.rs b/zebrad/src/components/mempool/storage/tests/vectors.rs new file mode 100644 index 00000000000..e1f5130e6a8 --- /dev/null +++ b/zebrad/src/components/mempool/storage/tests/vectors.rs @@ -0,0 +1,96 @@ +use super::{super::*, unmined_transactions_in_blocks}; + +use zebra_chain::parameters::Network; + +use color_eyre::eyre::Result; + +#[test] +fn mempool_storage_crud_mainnet() { + zebra_test::init(); + + let network = Network::Mainnet; + + // Create an empty storage instance + let mut storage: Storage = Default::default(); + + // Get one (1) unmined transaction + let unmined_tx = unmined_transactions_in_blocks(.., network) + .next() + .expect("at least one unmined transaction"); + + // Insert unmined tx into the mempool. + let _ = storage.insert(unmined_tx.clone()); + + // Check that it is in the mempool, and not rejected. + assert!(storage.contains(&unmined_tx.id)); + + // Remove tx + let _ = storage.remove(&unmined_tx.id); + + // Check that it is /not/ in the mempool. + assert!(!storage.contains(&unmined_tx.id)); +} + +#[test] +fn mempool_storage_basic() -> Result<()> { + zebra_test::init(); + + mempool_storage_basic_for_network(Network::Mainnet)?; + mempool_storage_basic_for_network(Network::Testnet)?; + + Ok(()) +} + +fn mempool_storage_basic_for_network(network: Network) -> Result<()> { + // Create an empty storage + let mut storage: Storage = Default::default(); + + // Get transactions from the first 10 blocks of the Zcash blockchain + let unmined_transactions: Vec<_> = unmined_transactions_in_blocks(..=10, network).collect(); + let total_transactions = unmined_transactions.len(); + + // Insert them all to the storage + for unmined_transaction in unmined_transactions.clone() { + storage.insert(unmined_transaction)?; + } + + // Separate transactions into the ones expected to be in the mempool and those expected to be + // rejected. + let rejected_transaction_count = total_transactions - MEMPOOL_SIZE; + let expected_to_be_rejected = &unmined_transactions[..rejected_transaction_count]; + let expected_in_mempool = &unmined_transactions[rejected_transaction_count..]; + + // Only MEMPOOL_SIZE should land in verified + assert_eq!(storage.verified.len(), MEMPOOL_SIZE); + + // The rest of the transactions will be in rejected + assert_eq!(storage.rejected.len(), rejected_transaction_count); + + // Make sure the last MEMPOOL_SIZE transactions we sent are in the verified + for tx in expected_in_mempool { + assert!(storage.contains(&tx.id)); + } + + // Anything greater should not be in the verified + for tx in expected_to_be_rejected { + assert!(!storage.contains(&tx.id)); + } + + // Query all the ids we have for rejected, get back `total - MEMPOOL_SIZE` + let all_ids: HashSet = unmined_transactions.iter().map(|tx| tx.id).collect(); + + // Convert response to a `HashSet` as we need a fixed order to compare. + let rejected_response: HashSet = + storage.rejected_transactions(all_ids).into_iter().collect(); + + let rejected_ids = expected_to_be_rejected.iter().map(|tx| tx.id).collect(); + + assert_eq!(rejected_response, rejected_ids); + + // Use `contains_rejected` to make sure the first id stored is now rejected + assert!(storage.contains_rejected(&expected_to_be_rejected[0].id)); + // Use `contains_rejected` to make sure the last id stored is not rejected + assert!(!storage.contains_rejected(&expected_in_mempool[0].id)); + + Ok(()) +} From 610b040d10b99b500f684d47c340d00f85d4434a Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Fri, 24 Sep 2021 01:11:04 +0000 Subject: [PATCH 08/12] Add an `AtLeastOne::first_mut` method A getter to allow changing the first element. --- zebra-chain/src/serialization/constraint.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/zebra-chain/src/serialization/constraint.rs b/zebra-chain/src/serialization/constraint.rs index 1aac711cac0..3fe3a0e32c2 100644 --- a/zebra-chain/src/serialization/constraint.rs +++ b/zebra-chain/src/serialization/constraint.rs @@ -185,6 +185,13 @@ impl AtLeastOne { &self.inner[0] } + /// Returns a mutable reference to the first element. + /// + /// Unlike `Vec` or slice, `AtLeastOne` always has a first element. + pub fn first_mut(&mut self) -> &mut T { + &mut self.inner[0] + } + /// Returns the first and all the rest of the elements of the vector. /// /// Unlike `Vec` or slice, `AtLeastOne` always has a first element. From fe8836c5339509088a1584ea691b580491c7d8fa Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Fri, 24 Sep 2021 01:11:55 +0000 Subject: [PATCH 09/12] Add an `AtLeastOne::push` method Allow appending elements to the collection. --- zebra-chain/src/serialization/constraint.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/zebra-chain/src/serialization/constraint.rs b/zebra-chain/src/serialization/constraint.rs index 3fe3a0e32c2..3c9effa6fcd 100644 --- a/zebra-chain/src/serialization/constraint.rs +++ b/zebra-chain/src/serialization/constraint.rs @@ -192,6 +192,11 @@ impl AtLeastOne { &mut self.inner[0] } + /// Appends an element to the back of the collection. + pub fn push(&mut self, element: T) { + self.inner.push(element); + } + /// Returns the first and all the rest of the elements of the vector. /// /// Unlike `Vec` or slice, `AtLeastOne` always has a first element. From 4f05b32bed6175abbc84269a2e987b77bf0f09b8 Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Fri, 24 Sep 2021 01:16:50 +0000 Subject: [PATCH 10/12] Derive `Arbitrary` for `FieldNotPresent` This is just to make the code that generates arbitrary anchors a bit simpler. --- zebra-chain/src/sapling/shielded_data.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/zebra-chain/src/sapling/shielded_data.rs b/zebra-chain/src/sapling/shielded_data.rs index 4a468cf49e5..d923c3af3f0 100644 --- a/zebra-chain/src/sapling/shielded_data.rs +++ b/zebra-chain/src/sapling/shielded_data.rs @@ -4,6 +4,8 @@ //! The `value_balance` change is handled using the default zero value. //! The anchor change is handled using the `AnchorVariant` type trait. +#[cfg(any(test, feature = "proptest-impl"))] +use proptest_derive::Arbitrary; use serde::{de::DeserializeOwned, Serialize}; use crate::{ @@ -35,6 +37,7 @@ pub struct SharedAnchor {} /// This field is not present in this transaction version. #[derive(Copy, Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] +#[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))] pub struct FieldNotPresent; impl AnchorVariant for PerSpendAnchor { From c87b0673fe37cc3087bce39d67cec4ce0a89b17b Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Fri, 24 Sep 2021 01:17:36 +0000 Subject: [PATCH 11/12] Test if conflicting transactions are rejected Generate two transactions (either V4 or V5) and insert a conflicting spend, which can be either a transparent UTXO, or a nullifier for one of the shielded pools. Check that any attempt to insert both transactions causes one to be accepted and the other to be rejected. --- zebrad/Cargo.toml | 1 + .../src/components/mempool/storage/tests.rs | 1 + .../components/mempool/storage/tests/prop.rs | 305 ++++++++++++++++++ 3 files changed, 307 insertions(+) create mode 100644 zebrad/src/components/mempool/storage/tests/prop.rs diff --git a/zebrad/Cargo.toml b/zebrad/Cargo.toml index 286a992b90c..8cb6991b04c 100644 --- a/zebrad/Cargo.toml +++ b/zebrad/Cargo.toml @@ -60,6 +60,7 @@ tokio = { version = "0.3.6", features = ["full", "test-util"] } proptest = "0.10" proptest-derive = "0.3" +zebra-chain = { path = "../zebra-chain", features = ["proptest-impl"] } zebra-test = { path = "../zebra-test" } [features] diff --git a/zebrad/src/components/mempool/storage/tests.rs b/zebrad/src/components/mempool/storage/tests.rs index e6166dd7f07..fe21f06078b 100644 --- a/zebrad/src/components/mempool/storage/tests.rs +++ b/zebrad/src/components/mempool/storage/tests.rs @@ -4,6 +4,7 @@ use zebra_chain::{ block::Block, parameters::Network, serialization::ZcashDeserializeInto, transaction::UnminedTx, }; +mod prop; mod vectors; pub fn unmined_transactions_in_blocks( diff --git a/zebrad/src/components/mempool/storage/tests/prop.rs b/zebrad/src/components/mempool/storage/tests/prop.rs new file mode 100644 index 00000000000..bbb99f87f96 --- /dev/null +++ b/zebrad/src/components/mempool/storage/tests/prop.rs @@ -0,0 +1,305 @@ +use std::fmt::Debug; + +use proptest::prelude::*; +use proptest_derive::Arbitrary; + +use zebra_chain::{ + at_least_one, orchard, + primitives::Groth16Proof, + sapling, + transaction::{self, Transaction, UnminedTx}, + transparent, LedgerState, +}; + +use super::super::{MempoolError, Storage}; + +proptest! { + /// Test if a transaction that has a spend conflict with a transaction already in the mempool + /// is rejected. + /// + /// A spend conflict in this case is when two transactions spend the same UTXO or reveal the + /// same nullifier. + #[test] + fn conflicting_transactions_are_rejected(input 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; + let id_to_reject = transaction_to_reject.id; + + assert_eq!( + storage.insert(transaction_to_accept), + Ok(id_to_accept) + ); + + assert_eq!( + storage.insert(transaction_to_reject), + Err(MempoolError::Rejected) + ); + + assert!(storage.contains_rejected(&id_to_reject)); + + storage.clear(); + } + } +} + +/// Test input consisting of two transactions and a conflict to be applied to them. +/// +/// When the conflict is applied, both transactions will have a shared spend (either a UTXO used as +/// an input, or a nullifier revealed by both transactions). +#[derive(Arbitrary, Debug)] +enum SpendConflictTestInput { + /// Test V4 transactions to include Sprout nullifier conflicts. + V4 { + #[proptest(strategy = "Transaction::v4_strategy(LedgerState::default())")] + first: Transaction, + + #[proptest(strategy = "Transaction::v4_strategy(LedgerState::default())")] + second: Transaction, + + conflict: SpendConflictForTransactionV4, + }, + + /// Test V5 transactions to include Orchard nullifier conflicts. + V5 { + #[proptest(strategy = "Transaction::v5_strategy(LedgerState::default())")] + first: Transaction, + + #[proptest(strategy = "Transaction::v5_strategy(LedgerState::default())")] + second: Transaction, + + conflict: SpendConflictForTransactionV5, + }, +} + +impl SpendConflictTestInput { + /// Return two transactions that have a spend conflict. + pub fn conflicting_transactions(self) -> (UnminedTx, UnminedTx) { + let (first, second) = match self { + SpendConflictTestInput::V4 { + mut first, + mut second, + conflict, + } => { + conflict.clone().apply_to(&mut first); + conflict.apply_to(&mut second); + + (first, second) + } + SpendConflictTestInput::V5 { + mut first, + mut second, + conflict, + } => { + conflict.clone().apply_to(&mut first); + conflict.apply_to(&mut second); + + (first, second) + } + }; + + (first.into(), second.into()) + } +} + +/// A spend conflict valid for V4 transactions. +#[derive(Arbitrary, Clone, Debug)] +enum SpendConflictForTransactionV4 { + Transparent(Box), + Sprout(Box), + Sapling(Box>), +} + +/// A spend conflict valid for V5 transactions. +#[derive(Arbitrary, Clone, Debug)] +enum SpendConflictForTransactionV5 { + Transparent(Box), + Sapling(Box>), + Orchard(Box), +} + +/// A conflict caused by spending the same UTXO. +#[derive(Arbitrary, Clone, Debug)] +struct TransparentSpendConflict { + new_input: transparent::Input, +} + +/// A conflict caused by revealing the same Sprout nullifier. +#[derive(Arbitrary, Clone, Debug)] +struct SproutSpendConflict { + new_joinsplit_data: transaction::JoinSplitData, +} + +/// A conflict caused by revealing the same Sapling nullifier. +#[derive(Clone, Debug)] +struct SaplingSpendConflict { + new_spend: sapling::Spend, + new_shared_anchor: A::Shared, + fallback_shielded_data: sapling::ShieldedData, +} + +/// A conflict caused by revealing the same Orchard nullifier. +#[derive(Arbitrary, Clone, Debug)] +struct OrchardSpendConflict { + new_shielded_data: orchard::ShieldedData, +} + +impl SpendConflictForTransactionV4 { + /// Apply a spend conflict to a V4 transaction. + /// + /// Changes the `transaction_v4` to include the spend that will result in a conflict. + pub fn apply_to(self, transaction_v4: &mut Transaction) { + let (inputs, joinsplit_data, sapling_shielded_data) = match transaction_v4 { + Transaction::V4 { + inputs, + joinsplit_data, + sapling_shielded_data, + .. + } => (inputs, joinsplit_data, sapling_shielded_data), + _ => unreachable!("incorrect transaction version generated for test"), + }; + + use SpendConflictForTransactionV4::*; + match self { + Transparent(transparent_conflict) => transparent_conflict.apply_to(inputs), + Sprout(sprout_conflict) => sprout_conflict.apply_to(joinsplit_data), + Sapling(sapling_conflict) => sapling_conflict.apply_to(sapling_shielded_data), + } + } +} + +impl SpendConflictForTransactionV5 { + /// Apply a spend conflict to a V5 transaction. + /// + /// Changes the `transaction_v5` to include the spend that will result in a conflict. + pub fn apply_to(self, transaction_v5: &mut Transaction) { + let (inputs, sapling_shielded_data, orchard_shielded_data) = match transaction_v5 { + Transaction::V5 { + inputs, + sapling_shielded_data, + orchard_shielded_data, + .. + } => (inputs, sapling_shielded_data, orchard_shielded_data), + _ => unreachable!("incorrect transaction version generated for test"), + }; + + use SpendConflictForTransactionV5::*; + match self { + Transparent(transparent_conflict) => transparent_conflict.apply_to(inputs), + Sapling(sapling_conflict) => sapling_conflict.apply_to(sapling_shielded_data), + Orchard(orchard_conflict) => orchard_conflict.apply_to(orchard_shielded_data), + } + } +} + +impl TransparentSpendConflict { + /// Apply a transparent spend conflict. + /// + /// Adds a new input to a transaction's list of transparent `inputs`. The transaction will then + /// conflict with any other transaction that also has that same new input. + pub fn apply_to(self, inputs: &mut Vec) { + inputs.push(self.new_input); + } +} + +impl SproutSpendConflict { + /// Apply a Sprout spend conflict. + /// + /// Ensures that a transaction's `joinsplit_data` has a nullifier used to represent a conflict. + /// If the transaction already has Sprout joinsplits, the first nullifier is replaced with the + /// new nullifier. Otherwise, a joinsplit is inserted with that new nullifier in the + /// transaction. + /// + /// The transaction will then conflict with any other transaction with the same new nullifier. + pub fn apply_to(self, joinsplit_data: &mut Option>) { + if let Some(existing_joinsplit_data) = joinsplit_data.as_mut() { + existing_joinsplit_data.first.nullifiers[0] = + self.new_joinsplit_data.first.nullifiers[0]; + } else { + *joinsplit_data = Some(self.new_joinsplit_data); + } + } +} + +/// Generate arbitrary [`SaplingSpendConflict`]s. +/// +/// This had to be implemented manually because of the constraints required as a consequence of the +/// generic type parameter. +impl Arbitrary for SaplingSpendConflict +where + A: sapling::AnchorVariant + Clone + Debug + 'static, + A::Shared: Arbitrary, + sapling::Spend: Arbitrary, + sapling::TransferData: Arbitrary, +{ + type Parameters = (); + + fn arbitrary_with(_: Self::Parameters) -> Self::Strategy { + any::<(sapling::Spend, A::Shared, sapling::ShieldedData)>() + .prop_map(|(new_spend, new_shared_anchor, fallback_shielded_data)| { + SaplingSpendConflict { + new_spend, + new_shared_anchor, + fallback_shielded_data, + } + }) + .boxed() + } + + type Strategy = BoxedStrategy; +} + +impl SaplingSpendConflict { + /// Apply a Sapling spend conflict. + /// + /// Ensures that a transaction's `sapling_shielded_data` has a nullifier used to represent a + /// conflict. If the transaction already has Sapling shielded data, a new spend is added with + /// the new nullifier. Otherwise, a fallback instance of Sapling shielded data is inserted in + /// the transaction, and then the spend is added. + /// + /// The transaction will then conflict with any other transaction with the same new nullifier. + pub fn apply_to(self, sapling_shielded_data: &mut Option>) { + use sapling::TransferData::*; + + let shielded_data = sapling_shielded_data.get_or_insert(self.fallback_shielded_data); + + match &mut shielded_data.transfers { + SpendsAndMaybeOutputs { ref mut spends, .. } => spends.push(self.new_spend), + JustOutputs { ref mut outputs } => { + let new_outputs = outputs.clone(); + + shielded_data.transfers = SpendsAndMaybeOutputs { + shared_anchor: self.new_shared_anchor, + spends: at_least_one![self.new_spend], + maybe_outputs: new_outputs.into_vec(), + }; + } + } + } +} + +impl OrchardSpendConflict { + /// Apply a Orchard spend conflict. + /// + /// Ensures that a transaction's `orchard_shielded_data` has a nullifier used to represent a + /// conflict. If the transaction already has Orchard shielded data, a new action is added with + /// the new nullifier. Otherwise, a fallback instance of Orchard shielded data that contains + /// the new action is inserted in the transaction. + /// + /// The transaction will then conflict with any other transaction with the same new nullifier. + pub fn apply_to(self, orchard_shielded_data: &mut Option) { + if let Some(shielded_data) = orchard_shielded_data.as_mut() { + shielded_data.actions.first_mut().action.nullifier = + self.new_shielded_data.actions.first().action.nullifier; + } else { + *orchard_shielded_data = Some(self.new_shielded_data); + } + } +} From a56ff452cee70326c8cb150df26bc3ca7fd23f53 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 28 Sep 2021 10:24:17 +1000 Subject: [PATCH 12/12] Delete a TODO comment that we decided not to do --- zebrad/src/components/mempool/storage.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/zebrad/src/components/mempool/storage.rs b/zebrad/src/components/mempool/storage.rs index b665a88efd7..e811506bad9 100644 --- a/zebrad/src/components/mempool/storage.rs +++ b/zebrad/src/components/mempool/storage.rs @@ -78,8 +78,6 @@ impl Storage { // If `tx` spends an UTXO already spent by another transaction in the mempool or reveals a // nullifier already revealed by another transaction in the mempool, reject that // transaction. - // - // TODO: Consider replacing the transaction in the mempool if the fee is higher (#2781). if self.check_spend_conflicts(&tx) { self.rejected.insert(tx.id, State::SpendConflict); return Err(MempoolError::Rejected);