diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index 073336faebd..6239e5c9ddc 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -57,7 +57,7 @@ jobs: - uses: Swatinem/rust-cache@v1 - name: Setup mdBook - uses: peaceiris/actions-mdbook@v1.1.14 + uses: peaceiris/actions-mdbook@v1.2.0 with: mdbook-version: '0.4.18' diff --git a/zebra-state/src/arbitrary.rs b/zebra-state/src/arbitrary.rs index 653cd5aa0cf..849c047bff3 100644 --- a/zebra-state/src/arbitrary.rs +++ b/zebra-state/src/arbitrary.rs @@ -59,10 +59,12 @@ impl From for ChainTipBlock { new_outputs: _, transaction_hashes, } = prepared; + Self { hash, height, time: block.header.time, + transactions: block.transactions.clone(), transaction_hashes, previous_block_hash: block.header.previous_block_hash, } diff --git a/zebra-state/src/service/chain_tip.rs b/zebra-state/src/service/chain_tip.rs index 3657440c3a8..0b9ed7fba4d 100644 --- a/zebra-state/src/service/chain_tip.rs +++ b/zebra-state/src/service/chain_tip.rs @@ -20,7 +20,7 @@ use zebra_chain::{ block, chain_tip::ChainTip, parameters::{Network, NetworkUpgrade}, - transaction, + transaction::{self, Transaction}, }; use crate::{ @@ -56,6 +56,9 @@ pub struct ChainTipBlock { )] pub time: DateTime, + /// The block transactions. + pub transactions: Vec>, + /// The mined transaction IDs of the transactions in `block`, /// in the same order as `block.transactions`. pub transaction_hashes: Arc<[transaction::Hash]>, @@ -84,6 +87,7 @@ impl From for ChainTipBlock { hash, height, time: block.header.time, + transactions: block.transactions.clone(), transaction_hashes, previous_block_hash: block.header.previous_block_hash, } @@ -99,10 +103,12 @@ impl From for ChainTipBlock { transaction_hashes, .. } = finalized; + Self { hash, height, time: block.header.time, + transactions: block.transactions.clone(), transaction_hashes, previous_block_hash: block.header.previous_block_hash, } diff --git a/zebrad/src/components/mempool.rs b/zebrad/src/components/mempool.rs index 6202aadd532..4d084071504 100644 --- a/zebrad/src/components/mempool.rs +++ b/zebrad/src/components/mempool.rs @@ -369,7 +369,7 @@ impl Service for Mempool { // with the same mined IDs as recently mined transactions. let mined_ids = block.transaction_hashes.iter().cloned().collect(); tx_downloads.cancel(&mined_ids); - storage.remove_same_effects(&mined_ids); + storage.reject_and_remove_same_effects(&mined_ids, block.transactions); storage.clear_tip_rejections(); } diff --git a/zebrad/src/components/mempool/storage.rs b/zebrad/src/components/mempool/storage.rs index d4acc47f33a..c1a3f2141f9 100644 --- a/zebrad/src/components/mempool/storage.rs +++ b/zebrad/src/components/mempool/storage.rs @@ -10,12 +10,15 @@ use std::{ collections::{HashMap, HashSet}, mem::size_of, + sync::Arc, time::Duration, }; use thiserror::Error; -use zebra_chain::transaction::{self, Hash, UnminedTx, UnminedTxId, VerifiedUnminedTx}; +use zebra_chain::transaction::{ + self, Hash, Transaction, UnminedTx, UnminedTxId, VerifiedUnminedTx, +}; use self::{eviction_list::EvictionList, verified_set::VerifiedSet}; use super::{config, downloads::TransactionDownloadVerifyError, MempoolError}; @@ -78,6 +81,12 @@ pub enum SameEffectsChainRejectionError { #[error("best chain tip has reached transaction expiry height")] Expired, + #[error("transaction inputs were spent, or nullifiers were revealed, in the best chain")] + DuplicateSpend, + + #[error("transaction was committed to the best chain")] + Mined, + /// Otherwise valid transaction removed from mempool due to [ZIP-401] random /// eviction. /// @@ -276,22 +285,89 @@ impl Storage { .remove_all_that(|tx| exact_wtxids.contains(&tx.transaction.id)) } - /// Remove transactions from the mempool via non-malleable [`transaction::Hash`]. + /// Reject and remove transactions from the mempool via non-malleable [`transaction::Hash`]. + /// - For v5 transactions, transactions are matched by TXID, + /// using only the non-malleable transaction ID. + /// This matches any transaction with the same effect on the blockchain state, + /// even if its signatures and proofs are different. + /// - Returns the number of transactions which were removed. + /// - Removes from the 'verified' set, if present. + /// Maintains the order in which the other unmined transactions have been inserted into the mempool. /// - /// For v5 transactions, transactions are matched by TXID, - /// using only the non-malleable transaction ID. - /// This matches any transaction with the same effect on the blockchain state, - /// even if its signatures and proofs are different. + /// Reject and remove transactions from the mempool that contain any outpoints or nullifiers in + /// the `spent_outpoints` or `nullifiers` collections that are passed in. /// - /// Returns the number of transactions which were removed. - /// - /// Removes from the 'verified' set, if present. - /// Maintains the order in which the other unmined transactions have been inserted into the mempool. - /// - /// Does not add or remove from the 'rejected' tracking set. - pub fn remove_same_effects(&mut self, mined_ids: &HashSet) -> usize { - self.verified - .remove_all_that(|tx| mined_ids.contains(&tx.transaction.id.mined_id())) + /// Returns the number of transactions that were removed. + pub fn reject_and_remove_same_effects( + &mut self, + mined_ids: &HashSet, + transactions: Vec>, + ) -> usize { + let num_removed_mined = self + .verified + .remove_all_that(|tx| mined_ids.contains(&tx.transaction.id.mined_id())); + + let spent_outpoints: HashSet<_> = transactions + .iter() + .flat_map(|tx| tx.spent_outpoints()) + .collect(); + let sprout_nullifiers: HashSet<_> = transactions + .iter() + .flat_map(|transaction| transaction.sprout_nullifiers()) + .collect(); + let sapling_nullifiers: HashSet<_> = transactions + .iter() + .flat_map(|transaction| transaction.sapling_nullifiers()) + .collect(); + let orchard_nullifiers: HashSet<_> = transactions + .iter() + .flat_map(|transaction| transaction.orchard_nullifiers()) + .collect(); + + let duplicate_spend_ids: HashSet<_> = self + .verified + .transactions() + .filter_map(|tx| { + (tx.transaction + .spent_outpoints() + .any(|outpoint| spent_outpoints.contains(&outpoint)) + || tx + .transaction + .sprout_nullifiers() + .any(|nullifier| sprout_nullifiers.contains(nullifier)) + || tx + .transaction + .sapling_nullifiers() + .any(|nullifier| sapling_nullifiers.contains(nullifier)) + || tx + .transaction + .orchard_nullifiers() + .any(|nullifier| orchard_nullifiers.contains(nullifier))) + .then_some(tx.id) + }) + .collect(); + + let num_removed_duplicate_spend = self + .verified + .remove_all_that(|tx| duplicate_spend_ids.contains(&tx.transaction.id)); + + for &mined_id in mined_ids { + self.reject( + // the reject and rejection_error fns that store and check `SameEffectsChainRejectionError`s + // only use the mined id, so using `Legacy` ids will apply to v5 transactions as well. + UnminedTxId::Legacy(mined_id), + SameEffectsChainRejectionError::Mined.into(), + ); + } + + for duplicate_spend_id in duplicate_spend_ids { + self.reject( + duplicate_spend_id, + SameEffectsChainRejectionError::DuplicateSpend.into(), + ); + } + + num_removed_mined + num_removed_duplicate_spend } /// Clears the whole mempool storage. @@ -520,7 +596,8 @@ impl Storage { } // expiry height is effecting data, so we match by non-malleable TXID - self.remove_same_effects(&txid_set); + self.verified + .remove_all_that(|tx| txid_set.contains(&tx.transaction.id.mined_id())); // also reject it for id in unmined_id_set.iter() { diff --git a/zebrad/src/components/mempool/storage/tests/prop.rs b/zebrad/src/components/mempool/storage/tests/prop.rs index 29d9e0d605b..7db00632142 100644 --- a/zebrad/src/components/mempool/storage/tests/prop.rs +++ b/zebrad/src/components/mempool/storage/tests/prop.rs @@ -364,10 +364,14 @@ proptest! { // Insert all input transactions, and keep track of the IDs of the one that were actually // inserted. - let inserted_transactions: HashSet<_> = input.transactions().filter_map(|transaction| { - let id = transaction.transaction.id; + let inserted_transactions: HashSet<_> = input + .transactions() + .filter_map(|transaction| { + let id = transaction.transaction.id; - storage.insert(transaction.clone()).ok().map(|_| id)}).collect(); + storage.insert(transaction.clone()).ok().map(|_| id) + }) + .collect(); // Check that the inserted transactions are still there. for transaction_id in &inserted_transactions { @@ -377,7 +381,16 @@ proptest! { // Remove some transactions. match &input { RemoveExact { wtx_ids_to_remove, .. } => storage.remove_exact(wtx_ids_to_remove), - RemoveSameEffects { mined_ids_to_remove, .. } => storage.remove_same_effects(mined_ids_to_remove), + RejectAndRemoveSameEffects { mined_ids_to_remove, .. } => { + let num_removals = storage.reject_and_remove_same_effects(mined_ids_to_remove, vec![]); + for &removed_transaction_id in mined_ids_to_remove.iter() { + prop_assert_eq!( + storage.rejection_error(&UnminedTxId::Legacy(removed_transaction_id)), + Some(SameEffectsChainRejectionError::Mined.into()) + ); + } + num_removals + }, }; // Check that the removed transactions are not in the storage. @@ -911,7 +924,7 @@ pub enum MultipleTransactionRemovalTestInput { wtx_ids_to_remove: SummaryDebug>, }, - RemoveSameEffects { + RejectAndRemoveSameEffects { transactions: SummaryDebug>, mined_ids_to_remove: SummaryDebug>, }, @@ -947,7 +960,7 @@ impl Arbitrary for MultipleTransactionRemovalTestInput { .collect(); prop_oneof![ - Just(RemoveSameEffects { + Just(RejectAndRemoveSameEffects { transactions: transactions.clone().into(), mined_ids_to_remove: mined_ids_to_remove.into(), }), @@ -967,7 +980,7 @@ impl MultipleTransactionRemovalTestInput { /// Iterate over all transactions generated as input. pub fn transactions(&self) -> impl Iterator + '_ { match self { - RemoveExact { transactions, .. } | RemoveSameEffects { transactions, .. } => { + RemoveExact { transactions, .. } | RejectAndRemoveSameEffects { transactions, .. } => { transactions.iter() } } @@ -980,7 +993,7 @@ impl MultipleTransactionRemovalTestInput { wtx_ids_to_remove, .. } => wtx_ids_to_remove.0.clone(), - RemoveSameEffects { + RejectAndRemoveSameEffects { transactions, mined_ids_to_remove, } => transactions diff --git a/zebrad/src/components/mempool/storage/tests/vectors.rs b/zebrad/src/components/mempool/storage/tests/vectors.rs index c4bb2acb681..457ed86f5f7 100644 --- a/zebrad/src/components/mempool/storage/tests/vectors.rs +++ b/zebrad/src/components/mempool/storage/tests/vectors.rs @@ -162,23 +162,75 @@ fn mempool_storage_crud_same_effects_mainnet() { }); // Get one (1) unmined transaction - let unmined_tx = unmined_transactions_in_blocks(.., network) + let unmined_tx_1 = unmined_transactions_in_blocks(.., network) .next() .expect("at least one unmined transaction"); // Insert unmined tx into the mempool. - let _ = storage.insert(unmined_tx.clone()); + let _ = storage.insert(unmined_tx_1.clone()); // Check that it is in the mempool, and not rejected. - assert!(storage.contains_transaction_exact(&unmined_tx.transaction.id)); + assert!(storage.contains_transaction_exact(&unmined_tx_1.transaction.id)); - // Remove tx - let removal_count = - storage.remove_same_effects(&iter::once(unmined_tx.transaction.id.mined_id()).collect()); + // Reject and remove mined tx + let removal_count = storage.reject_and_remove_same_effects( + &iter::once(unmined_tx_1.transaction.id.mined_id()).collect(), + vec![unmined_tx_1.transaction.transaction.clone()], + ); - // Check that it is /not/ in the mempool. + // Check that it is /not/ in the mempool as a verified transaction. assert_eq!(removal_count, 1); - assert!(!storage.contains_transaction_exact(&unmined_tx.transaction.id)); + assert!(!storage.contains_transaction_exact(&unmined_tx_1.transaction.id)); + + // Check that it's rejection is cached in the chain_rejected_same_effects' `Mined` eviction list. + assert_eq!( + storage.rejection_error(&unmined_tx_1.transaction.id), + Some(SameEffectsChainRejectionError::Mined.into()) + ); + assert_eq!( + storage.insert(unmined_tx_1), + Err(SameEffectsChainRejectionError::Mined.into()) + ); + + // Get a different unmined transaction + let unmined_tx_2 = unmined_transactions_in_blocks(1.., network) + .find(|tx| { + tx.transaction + .transaction + .spent_outpoints() + .next() + .is_some() + }) + .expect("at least one unmined transaction with at least 1 spent outpoint"); + + // Insert unmined tx into the mempool. + assert_eq!( + storage.insert(unmined_tx_2.clone()), + Ok(unmined_tx_2.transaction.id) + ); + + // Check that it is in the mempool, and not rejected. + assert!(storage.contains_transaction_exact(&unmined_tx_2.transaction.id)); + + // Reject and remove duplicate spend tx + let removal_count = storage.reject_and_remove_same_effects( + &HashSet::new(), + vec![unmined_tx_2.transaction.transaction.clone()], + ); + + // Check that it is /not/ in the mempool as a verified transaction. + assert_eq!(removal_count, 1); + assert!(!storage.contains_transaction_exact(&unmined_tx_2.transaction.id)); + + // Check that it's rejection is cached in the chain_rejected_same_effects' `SpendConflict` eviction list. + assert_eq!( + storage.rejection_error(&unmined_tx_2.transaction.id), + Some(SameEffectsChainRejectionError::DuplicateSpend.into()) + ); + assert_eq!( + storage.insert(unmined_tx_2), + Err(SameEffectsChainRejectionError::DuplicateSpend.into()) + ); } #[test] diff --git a/zebrad/src/components/mempool/tests/prop.rs b/zebrad/src/components/mempool/tests/prop.rs index 3d866f1df73..c2344423d98 100644 --- a/zebrad/src/components/mempool/tests/prop.rs +++ b/zebrad/src/components/mempool/tests/prop.rs @@ -288,6 +288,7 @@ impl FakeChainTip { hash: chain_tip_block.hash, height, time: previous.time + mock_block_time_delta, + transactions: chain_tip_block.transactions.clone(), transaction_hashes: chain_tip_block.transaction_hashes.clone(), previous_block_hash: previous.hash, }