Skip to content

Commit

Permalink
merges remove_same_effects in with reject_invalidated_transactions, m…
Browse files Browse the repository at this point in the history 
…oves nullifier retrieval to the new Storage method, rejects mined_ids, and updates tests
  • Loading branch information
@arya2
arya2 committed Oct 21, 2022
1 parent 9680fb4 commit b5cffbf
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 80 deletions.
30 changes: 1 addition & 29 deletions zebrad/src/components/mempool.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -365,39 +365,11 @@ impl Service<Request> for Mempool {
if let Some(TipAction::Grow { block }) = tip_action {
tracing::trace!(block_height = ?block.height, "handling blocks added to tip");

let spent_outpoints = block
.transactions
.iter()
.flat_map(|tx| tx.spent_outpoints())
.collect();

let sprout_nullifiers = block
.transactions
.iter()
.flat_map(|transaction| transaction.sprout_nullifiers())
.collect();
let sapling_nullifiers = block
.transactions
.iter()
.flat_map(|transaction| transaction.sapling_nullifiers())
.collect();
let orchard_nullifiers = block
.transactions
.iter()
.flat_map(|transaction| transaction.orchard_nullifiers())
.collect();

// Cancel downloads/verifications/storage of transactions
// 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_invalidated_transactions(
spent_outpoints,
sprout_nullifiers,
sapling_nullifiers,
orchard_nullifiers,
);
storage.reject_and_remove_same_effects(&mined_ids, block.transactions);
storage.clear_tip_rejections();
}

Expand Down
100 changes: 63 additions & 37 deletions zebrad/src/components/mempool/storage.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,14 @@
use std::{
collections::{HashMap, HashSet},
mem::size_of,
sync::Arc,
time::Duration,
};

use thiserror::Error;

use zebra_chain::{
orchard, sapling, sprout,
transaction::{self, Hash, UnminedTx, UnminedTxId, VerifiedUnminedTx},
transparent::OutPoint,
use zebra_chain::transaction::{
self, Hash, Transaction, UnminedTx, UnminedTxId, VerifiedUnminedTx,
};

use self::{eviction_list::EvictionList, verified_set::VerifiedSet};
Expand Down Expand Up @@ -82,7 +81,13 @@ pub enum SameEffectsChainRejectionError {
#[error("best chain tip has reached transaction expiry height")]
Expired,

#[error("transaction outpoints or nullifiers were committed to the best chain")]
#[error(
"transaction rejected because another transaction in the chain or mempool \
has already spent some of its inputs"
)]
DuplicateSpend,

#[error("transaction was committed to the best chain")]
Mined,

/// Otherwise valid transaction removed from mempool due to [ZIP-401] random
Expand Down Expand Up @@ -283,36 +288,46 @@ impl Storage {
.remove_all_that(|tx| exact_wtxids.contains(&tx.transaction.id))
}

/// 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.
/// 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.
///
/// Does not add or remove from the 'rejected' tracking set.
pub fn remove_same_effects(&mut self, mined_ids: &HashSet<transaction::Hash>) -> usize {
self.verified
.remove_all_that(|tx| mined_ids.contains(&tx.transaction.id.mined_id()))
}

/// Removes and rejects transactions from the mempool that contain any outpoints or nullifiers in
/// 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 that were removed and rejected.
pub fn reject_invalidated_transactions(
/// Returns the number of transactions that were removed.
pub fn reject_and_remove_same_effects(
&mut self,
spent_outpoints: HashSet<OutPoint>,
sprout_nullifiers: HashSet<&sprout::Nullifier>,
sapling_nullifiers: HashSet<&sapling::Nullifier>,
orchard_nullifiers: HashSet<&orchard::Nullifier>,
mined_ids: &HashSet<transaction::Hash>,
transactions: Vec<Arc<Transaction>>,
) -> usize {
let mined_ids: HashSet<_> = self
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| {
Expand All @@ -331,19 +346,29 @@ impl Storage {
.transaction
.orchard_nullifiers()
.any(|nullifier| orchard_nullifiers.contains(nullifier)))
.then(|| tx.id)
.then_some(tx.id)
})
.collect();

let num_removals = self
let num_removed_duplicate_spend = self
.verified
.remove_all_that(|tx| mined_ids.contains(&tx.transaction.id));
.remove_all_that(|tx| duplicate_spend_ids.contains(&tx.transaction.id));

for mined_id in mined_ids {
self.reject(mined_id, SameEffectsChainRejectionError::Mined.into());
for &mined_id in mined_ids {
self.reject(
UnminedTxId::Legacy(mined_id),
SameEffectsChainRejectionError::Mined.into(),
);
}

for duplicate_spend_id in duplicate_spend_ids {
self.reject(
duplicate_spend_id,
SameEffectsChainRejectionError::DuplicateSpend.into(),
);
}

num_removals
num_removed_mined + num_removed_duplicate_spend
}

/// Clears the whole mempool storage.
Expand Down Expand Up @@ -572,7 +597,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() {
Expand Down
18 changes: 12 additions & 6 deletions zebrad/src/components/mempool/storage/tests/prop.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,8 @@ proptest! {
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 {
Expand All @@ -377,14 +378,19 @@ 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, .. } =>
storage.reject_and_remove_same_effects(mined_ids_to_remove, vec![]),
};

// Check that the removed transactions are not in the storage.
let removed_transactions = input.removed_transaction_ids();

for removed_transaction_id in &removed_transactions {
prop_assert!(!storage.contains_transaction_exact(removed_transaction_id));
prop_assert_eq!(
storage.rejection_error(removed_transaction_id),
Some(SameEffectsChainRejectionError::Mined.into())
);
}

// Check that the remaining transactions are still in the storage.
Expand Down Expand Up @@ -911,7 +917,7 @@ pub enum MultipleTransactionRemovalTestInput {
wtx_ids_to_remove: SummaryDebug<HashSet<UnminedTxId>>,
},

RemoveSameEffects {
RejectAndRemoveSameEffects {
transactions: SummaryDebug<Vec<VerifiedUnminedTx>>,
mined_ids_to_remove: SummaryDebug<HashSet<transaction::Hash>>,
},
Expand Down Expand Up @@ -947,7 +953,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(),
}),
Expand All @@ -967,7 +973,7 @@ impl MultipleTransactionRemovalTestInput {
/// Iterate over all transactions generated as input.
pub fn transactions(&self) -> impl Iterator<Item = &VerifiedUnminedTx> + '_ {
match self {
RemoveExact { transactions, .. } | RemoveSameEffects { transactions, .. } => {
RemoveExact { transactions, .. } | RejectAndRemoveSameEffects { transactions, .. } => {
transactions.iter()
}
}
Expand All @@ -980,7 +986,7 @@ impl MultipleTransactionRemovalTestInput {
wtx_ids_to_remove, ..
} => wtx_ids_to_remove.0.clone(),

RemoveSameEffects {
RejectAndRemoveSameEffects {
transactions,
mined_ids_to_remove,
} => transactions
Expand Down
62 changes: 54 additions & 8 deletions zebrad/src/components/mempool/storage/tests/vectors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,23 +162,69 @@ 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.clone()),
Err(SameEffectsChainRejectionError::Mined.into())
);

// Get a different unmined transaction
let unmined_tx_2 = unmined_transactions_in_blocks(1.., network)
.next()
.expect("at least one unmined transaction");

// 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(
&iter::once(unmined_tx_1.transaction.id.mined_id()).collect(),
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::Mined.into())
);
}

#[test]
Expand Down

0 comments on commit b5cffbf

Please sign in to comment.