From ebb241456db1ce496fab848eaf461431df527761 Mon Sep 17 00:00:00 2001 From: Arya Date: Tue, 19 Nov 2024 01:08:07 -0500 Subject: [PATCH 1/6] skips re-verifying transactions in blocks that are present in the mempool. --- zebra-consensus/src/block.rs | 13 ++- zebra-consensus/src/transaction.rs | 85 ++++++++++++++++++- zebra-consensus/src/transaction/tests.rs | 69 ++++++++++++++- zebra-consensus/src/transaction/tests/prop.rs | 8 +- zebra-node-services/src/mempool.rs | 11 +++ zebrad/src/components/mempool.rs | 20 ++++- zebrad/src/components/mempool/storage.rs | 17 ++++ 7 files changed, 215 insertions(+), 8 deletions(-) diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index 611aea2ceba..fe8499abfba 100644 --- a/zebra-consensus/src/block.rs +++ b/zebra-consensus/src/block.rs @@ -8,6 +8,7 @@ //! verification, where it may be accepted or rejected. use std::{ + collections::HashSet, future::Future, pin::Pin, sync::Arc, @@ -25,7 +26,7 @@ use zebra_chain::{ amount::Amount, block, parameters::{subsidy::FundingStreamReceiver, Network}, - transparent, + transaction, transparent, work::equihash, }; use zebra_state as zs; @@ -232,13 +233,21 @@ where &block, &transaction_hashes, )); - for transaction in &block.transactions { + + let known_outpoint_hashes: Arc> = + Arc::new(known_utxos.keys().map(|outpoint| outpoint.hash).collect()); + + for (&transaction_hash, transaction) in + transaction_hashes.iter().zip(block.transactions.iter()) + { let rsp = transaction_verifier .ready() .await .expect("transaction verifier is always ready") .call(tx::Request::Block { + transaction_hash, transaction: transaction.clone(), + known_outpoint_hashes: known_outpoint_hashes.clone(), known_utxos: known_utxos.clone(), height, time: block.header.time, diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index aac77a055d6..e25b3b8d28e 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -1,7 +1,7 @@ //! Asynchronous verification of transactions. use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, future::Future, pin::Pin, sync::Arc, @@ -146,8 +146,12 @@ where pub enum Request { /// Verify the supplied transaction as part of a block. Block { + /// The transaction hash. + transaction_hash: transaction::Hash, /// The transaction itself. transaction: Arc, + /// Set of transaction hashes that create new transparent outputs. + known_outpoint_hashes: Arc>, /// Additional UTXOs which are known at the time of verification. known_utxos: Arc>, /// The height of the block containing this transaction. @@ -259,6 +263,16 @@ impl Request { } } + /// The mined transaction ID for the transaction in this request. + pub fn tx_mined_id(&self) -> transaction::Hash { + match self { + Request::Block { + transaction_hash, .. + } => *transaction_hash, + Request::Mempool { transaction, .. } => transaction.id.mined_id(), + } + } + /// The set of additional known unspent transaction outputs that's in this request. pub fn known_utxos(&self) -> Arc> { match self { @@ -267,6 +281,17 @@ impl Request { } } + /// The set of additional known [`transparent::OutPoint`]s of unspent transaction outputs that's in this request. + pub fn known_outpoint_hashes(&self) -> Arc> { + match self { + Request::Block { + known_outpoint_hashes, + .. + } => known_outpoint_hashes.clone(), + Request::Mempool { .. } => HashSet::new().into(), + } + } + /// The height used to select the consensus rules for verifying this transaction. pub fn height(&self) -> block::Height { match self { @@ -377,6 +402,16 @@ where async move { tracing::trace!(?tx_id, ?req, "got tx verify request"); + if let Some(result) = Self::try_find_verified_unmined_tx(&req, mempool.clone()).await { + let verified_tx = result?; + + return Ok(Response::Block { + tx_id: verified_tx.transaction.id, + miner_fee: Some(verified_tx.miner_fee), + legacy_sigop_count: verified_tx.legacy_sigop_count + }); + } + // Do quick checks first check::has_inputs_and_outputs(&tx)?; check::has_enough_orchard_flags(&tx)?; @@ -608,8 +643,52 @@ where } } - /// Waits for the UTXOs that are being spent by the given transaction to arrive in - /// the state for [`Block`](Request::Block) requests. + /// Attempts to find a transaction in the mempool by its transaction hash and checks + /// that all of its dependencies are available in the block. + /// + /// Returns [`Some(Ok(VerifiedUnminedTx))`](VerifiedUnminedTx) if successful, + /// None if the transaction id was not found in the mempool, + /// or `Some(Err(TransparentInputNotFound))` if the transaction was found, but some of its + /// dependencies are missing in the block. + async fn try_find_verified_unmined_tx( + req: &Request, + mempool: Option>, + ) -> Option> { + if req.is_mempool() || req.transaction().is_coinbase() { + return None; + } + + let mempool = mempool?; + let known_outpoint_hashes = req.known_outpoint_hashes(); + let tx_id = req.tx_mined_id(); + + let mempool::Response::TransactionWithDeps { + transaction, + dependencies, + } = mempool + .oneshot(mempool::Request::TransactionWithDepsByMinedId(tx_id)) + .await + .ok()? + else { + panic!("unexpected response to TransactionWithDepsByMinedId request"); + }; + + // Note: This does not verify that the spends are in order, this should be + // done during contextual validation in zebra-state. + let has_all_tx_deps = dependencies + .into_iter() + .all(|dependency_id| known_outpoint_hashes.contains(&dependency_id)); + + let result = if has_all_tx_deps { + Ok(transaction) + } else { + Err(TransactionError::TransparentInputNotFound) + }; + + Some(result) + } + + /// Wait for the UTXOs that are being spent by the given transaction. /// /// Looks up UTXOs that are being spent by the given transaction in the state or waits /// for them to be added to the mempool for [`Mempool`](Request::Mempool) requests. diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index d42bbb8594c..b323cd7f555 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -2,7 +2,10 @@ // // TODO: split fixed test vectors into a `vectors` module? -use std::{collections::HashMap, sync::Arc}; +use std::{ + collections::{HashMap, HashSet}, + sync::Arc, +}; use chrono::{DateTime, TimeZone, Utc}; use color_eyre::eyre::Report; @@ -982,8 +985,10 @@ async fn v5_transaction_is_rejected_before_nu5_activation() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height: canopy .activation_height(&network) .expect("Canopy activation height is specified"), @@ -1042,8 +1047,10 @@ fn v5_transaction_is_accepted_after_nu5_activation_for_network(network: Network) let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height: expiry_height, time: DateTime::::MAX_UTC, }) @@ -1097,8 +1104,10 @@ async fn v4_transaction_with_transparent_transfer_is_accepted() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, }) @@ -1141,8 +1150,10 @@ async fn v4_transaction_with_last_valid_expiry_height() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction.clone()), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, }) @@ -1186,8 +1197,10 @@ async fn v4_coinbase_transaction_with_low_expiry_height() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction.clone()), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, }) @@ -1233,8 +1246,10 @@ async fn v4_transaction_with_too_low_expiry_height() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction.clone()), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, }) @@ -1283,8 +1298,10 @@ async fn v4_transaction_with_exceeding_expiry_height() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction.clone()), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, }) @@ -1336,8 +1353,10 @@ async fn v4_coinbase_transaction_with_exceeding_expiry_height() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction.clone()), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, }) @@ -1387,8 +1406,10 @@ async fn v4_coinbase_transaction_is_accepted() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, }) @@ -1442,8 +1463,10 @@ async fn v4_transaction_with_transparent_transfer_is_rejected_by_the_script() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, }) @@ -1497,8 +1520,10 @@ async fn v4_transaction_with_conflicting_transparent_spend_is_rejected() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, }) @@ -1568,8 +1593,10 @@ fn v4_transaction_with_conflicting_sprout_nullifier_inside_joinsplit_is_rejected let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, }) @@ -1644,8 +1671,10 @@ fn v4_transaction_with_conflicting_sprout_nullifier_across_joinsplits_is_rejecte let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, }) @@ -1703,8 +1732,10 @@ async fn v5_transaction_with_transparent_transfer_is_accepted() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, }) @@ -1749,8 +1780,10 @@ async fn v5_transaction_with_last_valid_expiry_height() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction.clone()), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, }) @@ -1794,8 +1827,10 @@ async fn v5_coinbase_transaction_expiry_height() { let result = verifier .clone() .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction.clone()), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, }) @@ -1815,8 +1850,10 @@ async fn v5_coinbase_transaction_expiry_height() { let result = verifier .clone() .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(new_transaction.clone()), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, }) @@ -1844,8 +1881,10 @@ async fn v5_coinbase_transaction_expiry_height() { let result = verifier .clone() .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(new_transaction.clone()), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, }) @@ -1875,8 +1914,10 @@ async fn v5_coinbase_transaction_expiry_height() { let result = verifier .clone() .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(new_transaction.clone()), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height: new_expiry_height, time: DateTime::::MAX_UTC, }) @@ -1924,8 +1965,10 @@ async fn v5_transaction_with_too_low_expiry_height() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction.clone()), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, }) @@ -1975,8 +2018,10 @@ async fn v5_transaction_with_exceeding_expiry_height() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction.clone()), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, }) @@ -2029,8 +2074,10 @@ async fn v5_coinbase_transaction_is_accepted() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, }) @@ -2086,8 +2133,10 @@ async fn v5_transaction_with_transparent_transfer_is_rejected_by_the_script() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, }) @@ -2143,8 +2192,10 @@ async fn v5_transaction_with_conflicting_transparent_spend_is_rejected() { let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, }) @@ -2187,8 +2238,10 @@ fn v4_with_signed_sprout_transfer_is_accepted() { // Test the transaction verifier let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction, known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, }) @@ -2276,8 +2329,10 @@ async fn v4_with_joinsplit_is_rejected_for_modification( let result = verifier .clone() .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: transaction.clone(), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, }) @@ -2322,8 +2377,10 @@ fn v4_with_sapling_spends() { // Test the transaction verifier let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction, known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, }) @@ -2364,8 +2421,10 @@ fn v4_with_duplicate_sapling_spends() { // Test the transaction verifier let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction, known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, }) @@ -2408,8 +2467,10 @@ fn v4_with_sapling_outputs_and_no_spends() { // Test the transaction verifier let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction, known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, }) @@ -2456,8 +2517,10 @@ fn v5_with_sapling_spends() { // Test the transaction verifier let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, }) @@ -2499,8 +2562,10 @@ fn v5_with_duplicate_sapling_spends() { // Test the transaction verifier let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, }) @@ -2561,8 +2626,10 @@ fn v5_with_duplicate_orchard_action() { // Test the transaction verifier let result = verifier .oneshot(Request::Block { + transaction_hash: transaction.hash(), transaction: Arc::new(transaction), known_utxos: Arc::new(HashMap::new()), + known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, }) diff --git a/zebra-consensus/src/transaction/tests/prop.rs b/zebra-consensus/src/transaction/tests/prop.rs index 856742e5d74..8fea9cf3433 100644 --- a/zebra-consensus/src/transaction/tests/prop.rs +++ b/zebra-consensus/src/transaction/tests/prop.rs @@ -1,6 +1,9 @@ //! Randomised property tests for transaction verification. -use std::{collections::HashMap, sync::Arc}; +use std::{ + collections::{HashMap, HashSet}, + sync::Arc, +}; use chrono::{DateTime, Duration, Utc}; use proptest::{collection::vec, prelude::*}; @@ -452,13 +455,16 @@ fn validate( tower::service_fn(|_| async { unreachable!("State service should not be called") }); let verifier = transaction::Verifier::new_for_tests(&network, state_service); let verifier = Buffer::new(verifier, 10); + let transaction_hash = transaction.hash(); // Test the transaction verifier verifier .clone() .oneshot(transaction::Request::Block { + transaction_hash, transaction: Arc::new(transaction), known_utxos: Arc::new(known_utxos), + known_outpoint_hashes: Arc::new(HashSet::new()), height, time: block_time, }) diff --git a/zebra-node-services/src/mempool.rs b/zebra-node-services/src/mempool.rs index 10f51cf4a30..793e5b1fe3b 100644 --- a/zebra-node-services/src/mempool.rs +++ b/zebra-node-services/src/mempool.rs @@ -61,6 +61,9 @@ pub enum Request { /// Outdated requests are pruned on a regular basis. AwaitOutput(transparent::OutPoint), + /// Request a [`VerifiedUnminedTx`] and its dependencies by its mined id. + TransactionWithDepsByMinedId(transaction::Hash), + /// Get all the [`VerifiedUnminedTx`] in the mempool. /// /// Equivalent to `TransactionsById(TransactionIds)`, @@ -124,6 +127,14 @@ pub enum Response { /// Response to [`Request::AwaitOutput`] with the transparent output UnspentOutput(transparent::Output), + /// Response to [`Request::TransactionWithDepsByMinedId`]. + TransactionWithDeps { + /// The queried transaction + transaction: VerifiedUnminedTx, + /// A list of dependencies of the queried transaction. + dependencies: HashSet, + }, + /// Returns all [`VerifiedUnminedTx`] in the mempool. // // TODO: make the Transactions response return VerifiedUnminedTx, diff --git a/zebrad/src/components/mempool.rs b/zebrad/src/components/mempool.rs index b94ad0b09b8..bc5796df615 100644 --- a/zebrad/src/components/mempool.rs +++ b/zebrad/src/components/mempool.rs @@ -733,6 +733,24 @@ impl Service for Mempool { async move { Ok(Response::Transactions(res)) }.boxed() } + Request::TransactionWithDepsByMinedId(tx_id) => { + trace!(?req, "got mempool request"); + + let res = if let Some((transaction, dependencies)) = + storage.transaction_with_deps(tx_id) + { + Ok(Response::TransactionWithDeps { + transaction, + dependencies, + }) + } else { + Err("transaction not found in mempool".into()) + }; + + trace!(?req, ?res, "answered mempool request"); + + async move { res }.boxed() + } Request::AwaitOutput(outpoint) => { trace!(?req, "got mempool request"); @@ -828,7 +846,7 @@ impl Service for Mempool { Request::TransactionsById(_) => Response::Transactions(Default::default()), Request::TransactionsByMinedId(_) => Response::Transactions(Default::default()), - Request::AwaitOutput(_) => { + Request::TransactionWithDepsByMinedId(_) | Request::AwaitOutput(_) => { return async move { Err("mempool is not active: wait for Zebra to sync to the tip".into()) } diff --git a/zebrad/src/components/mempool/storage.rs b/zebrad/src/components/mempool/storage.rs index ce6f09cf1d6..ef3733b128e 100644 --- a/zebrad/src/components/mempool/storage.rs +++ b/zebrad/src/components/mempool/storage.rs @@ -513,6 +513,23 @@ impl Storage { .map(|(_, tx)| &tx.transaction) } + /// Returns a transaction and the transaction ids of its dependencies, if it is in the verified set. + pub fn transaction_with_deps( + &self, + tx_id: transaction::Hash, + ) -> Option<(VerifiedUnminedTx, HashSet)> { + let tx = self.verified.transactions().get(&tx_id).cloned()?; + let deps = self + .verified + .transaction_dependencies() + .dependencies() + .get(&tx_id) + .cloned() + .unwrap_or_default(); + + Some((tx, deps)) + } + /// Returns `true` if a transaction exactly matching an [`UnminedTxId`] is in /// the mempool. /// From 2c933d27ba30333cbbed92716eb87b50cc7d5db9 Mon Sep 17 00:00:00 2001 From: Arya Date: Tue, 19 Nov 2024 01:09:23 -0500 Subject: [PATCH 2/6] clippy fix --- .../src/methods/tests/snapshot/get_block_template_rpcs.rs | 3 ++- zebra-rpc/src/methods/tests/vectors.rs | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/zebra-rpc/src/methods/tests/snapshot/get_block_template_rpcs.rs b/zebra-rpc/src/methods/tests/snapshot/get_block_template_rpcs.rs index b2e012c7bcd..4949b419c43 100644 --- a/zebra-rpc/src/methods/tests/snapshot/get_block_template_rpcs.rs +++ b/zebra-rpc/src/methods/tests/snapshot/get_block_template_rpcs.rs @@ -105,7 +105,8 @@ pub async fn test_responses( extra_coinbase_data: None, debug_like_zcashd: true, // TODO: Use default field values when optional features are enabled in tests #8183 - ..Default::default() + #[cfg(feature = "internal-miner")] + internal_miner: true, }; // nu5 block height diff --git a/zebra-rpc/src/methods/tests/vectors.rs b/zebra-rpc/src/methods/tests/vectors.rs index 007a89ee893..9c86bb58046 100644 --- a/zebra-rpc/src/methods/tests/vectors.rs +++ b/zebra-rpc/src/methods/tests/vectors.rs @@ -1552,7 +1552,8 @@ async fn rpc_getblocktemplate_mining_address(use_p2pkh: bool) { extra_coinbase_data: None, debug_like_zcashd: true, // TODO: Use default field values when optional features are enabled in tests #8183 - ..Default::default() + #[cfg(feature = "internal-miner")] + internal_miner: true, }; // nu5 block height @@ -2006,7 +2007,8 @@ async fn rpc_getdifficulty() { extra_coinbase_data: None, debug_like_zcashd: true, // TODO: Use default field values when optional features are enabled in tests #8183 - ..Default::default() + #[cfg(feature = "internal-miner")] + internal_miner: true, }; // nu5 block height From 1fd4f4e7091097f33a4c6924349da62af1c8ef05 Mon Sep 17 00:00:00 2001 From: Arya Date: Thu, 21 Nov 2024 18:54:23 -0500 Subject: [PATCH 3/6] adds a test --- zebra-consensus/src/transaction.rs | 6 +- zebra-consensus/src/transaction/tests.rs | 174 ++++++++++++++++++++++- 2 files changed, 173 insertions(+), 7 deletions(-) diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index e25b3b8d28e..02f97fb7778 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -406,7 +406,7 @@ where let verified_tx = result?; return Ok(Response::Block { - tx_id: verified_tx.transaction.id, + tx_id, miner_fee: Some(verified_tx.miner_fee), legacy_sigop_count: verified_tx.legacy_sigop_count }); @@ -673,8 +673,8 @@ where panic!("unexpected response to TransactionWithDepsByMinedId request"); }; - // Note: This does not verify that the spends are in order, this should be - // done during contextual validation in zebra-state. + // Note: This does not verify that the spends are in order, the spend order + // should be verified during contextual validation in zebra-state. let has_all_tx_deps = dependencies .into_iter() .all(|dependency_id| known_outpoint_hashes.contains(&dependency_id)); diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index b323cd7f555..ab4318b159b 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -696,13 +696,179 @@ async fn mempool_request_with_unmined_output_spends_is_accepted() { ); tokio::time::sleep(POLL_MEMPOOL_DELAY * 2).await; + // polled before AwaitOutput request and after a mempool transaction with transparent outputs + // is successfully verified assert_eq!( mempool.poll_count(), 2, - "the mempool service should have been polled twice, \ - first before being called with an AwaitOutput request, \ - then again shortly after a mempool transaction with transparent outputs \ - is successfully verified" + "the mempool service should have been polled twice" + ); +} + +#[tokio::test] +async fn skips_verification_of_block_transactions_in_mempool() { + let mut state: MockService<_, _, _, _> = MockService::build().for_prop_tests(); + let mempool: MockService<_, _, _, _> = MockService::build().for_prop_tests(); + let (mempool_setup_tx, mempool_setup_rx) = tokio::sync::oneshot::channel(); + let verifier = Verifier::new(&Network::Mainnet, state.clone(), mempool_setup_rx); + let verifier = Buffer::new(verifier, 1); + + mempool_setup_tx + .send(mempool.clone()) + .ok() + .expect("send should succeed"); + + let height = NetworkUpgrade::Canopy + .activation_height(&Network::Mainnet) + .expect("Canopy activation height is specified"); + let fund_height = (height - 1).expect("fake source fund block height is too small"); + let (input, output, known_utxos) = mock_transparent_transfer( + fund_height, + true, + 0, + Amount::try_from(10001).expect("invalid value"), + ); + + // Create a non-coinbase V4 tx with the last valid expiry height. + let tx = Transaction::V4 { + inputs: vec![input], + outputs: vec![output], + lock_time: LockTime::min_lock_time_timestamp(), + expiry_height: height, + joinsplit_data: None, + sapling_shielded_data: None, + }; + + let tx_hash = tx.hash(); + let input_outpoint = match tx.inputs()[0] { + transparent::Input::PrevOut { outpoint, .. } => outpoint, + transparent::Input::Coinbase { .. } => panic!("requires a non-coinbase transaction"), + }; + + tokio::spawn(async move { + state + .expect_request(zebra_state::Request::BestChainNextMedianTimePast) + .await + .expect("verifier should call mock state service with correct request") + .respond(zebra_state::Response::BestChainNextMedianTimePast( + DateTime32::MAX, + )); + + state + .expect_request(zebra_state::Request::UnspentBestChainUtxo(input_outpoint)) + .await + .expect("verifier should call mock state service with correct request") + .respond(zebra_state::Response::UnspentBestChainUtxo(None)); + + state + .expect_request_that(|req| { + matches!( + req, + zebra_state::Request::CheckBestChainTipNullifiersAndAnchors(_) + ) + }) + .await + .expect("verifier should call mock state service with correct request") + .respond(zebra_state::Response::ValidBestChainTipNullifiersAndAnchors); + }); + + let mut mempool_clone = mempool.clone(); + tokio::spawn(async move { + mempool_clone + .expect_request(mempool::Request::AwaitOutput(input_outpoint)) + .await + .expect("verifier should call mock state service with correct request") + .respond(mempool::Response::UnspentOutput( + known_utxos + .get(&input_outpoint) + .expect("input outpoint should exist in known_utxos") + .utxo + .output + .clone(), + )); + }); + + let verifier_response = verifier + .clone() + .oneshot(Request::Mempool { + transaction: tx.clone().into(), + height, + }) + .await; + + assert!( + verifier_response.is_ok(), + "expected successful verification, got: {verifier_response:?}" + ); + + let crate::transaction::Response::Mempool { + transaction, + spent_mempool_outpoints, + } = verifier_response.expect("already checked that response is ok") + else { + panic!("unexpected response variant from transaction verifier for Mempool request") + }; + + assert_eq!( + spent_mempool_outpoints, + vec![input_outpoint], + "spent_mempool_outpoints in tx verifier response should match input_outpoint" + ); + + let mut mempool_clone = mempool.clone(); + tokio::spawn(async move { + for _ in 0..2 { + mempool_clone + .expect_request(mempool::Request::TransactionWithDepsByMinedId(tx_hash)) + .await + .expect("verifier should call mock state service with correct request") + .respond(mempool::Response::TransactionWithDeps { + transaction: transaction.clone(), + dependencies: [input_outpoint.hash].into(), + }); + } + }); + + let make_request = |known_outpoint_hashes| Request::Block { + transaction_hash: tx_hash, + transaction: Arc::new(tx), + known_outpoint_hashes, + known_utxos: Arc::new(HashMap::new()), + height, + time: Utc::now(), + }; + + let crate::transaction::Response::Block { .. } = verifier + .clone() + .oneshot(make_request.clone()(Arc::new([input_outpoint.hash].into()))) + .await + .expect("should return Ok without calling state service") + else { + panic!("unexpected response variant from transaction verifier for Block request") + }; + + let verifier_response_err = *verifier + .clone() + .oneshot(make_request(Arc::new(HashSet::new()))) + .await + .expect_err("should return Err without calling state service") + .downcast::() + .expect("tx verifier error type should be TransactionError"); + + assert_eq!( + verifier_response_err, + TransactionError::TransparentInputNotFound, + "should be a transparent input not found error" + ); + + tokio::time::sleep(POLL_MEMPOOL_DELAY * 2).await; + // polled before AwaitOutput request, after a mempool transaction with transparent outputs, + // is successfully verified, and twice more when checking if a transaction in a block is + // already the mempool. + assert_eq!( + mempool.poll_count(), + 4, + "the mempool service should have been polled 4 times" ); } From 4d16805f3142554fe354fc046d4ac23447af1c6c Mon Sep 17 00:00:00 2001 From: Arya Date: Thu, 21 Nov 2024 20:32:24 -0500 Subject: [PATCH 4/6] fixes clippy lint --- zebra-node-services/src/mempool.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/zebra-node-services/src/mempool.rs b/zebra-node-services/src/mempool.rs index 793e5b1fe3b..784c3ba607e 100644 --- a/zebra-node-services/src/mempool.rs +++ b/zebra-node-services/src/mempool.rs @@ -6,13 +6,10 @@ use std::collections::HashSet; use tokio::sync::oneshot; use zebra_chain::{ - transaction::{self, UnminedTx, UnminedTxId}, + transaction::{self, UnminedTx, UnminedTxId, VerifiedUnminedTx}, transparent, }; -#[cfg(feature = "getblocktemplate-rpcs")] -use zebra_chain::transaction::VerifiedUnminedTx; - use crate::BoxError; mod gossip; From af9001dd91fdec765ec4137882bd8831bf0402db Mon Sep 17 00:00:00 2001 From: ar Date: Tue, 3 Dec 2024 12:51:53 -0500 Subject: [PATCH 5/6] Use NU6 & V5 tx in new test --- zebra-consensus/src/transaction/tests.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index ab4318b159b..56e27c259f8 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -718,7 +718,7 @@ async fn skips_verification_of_block_transactions_in_mempool() { .ok() .expect("send should succeed"); - let height = NetworkUpgrade::Canopy + let height = NetworkUpgrade::Nu6 .activation_height(&Network::Mainnet) .expect("Canopy activation height is specified"); let fund_height = (height - 1).expect("fake source fund block height is too small"); @@ -730,13 +730,14 @@ async fn skips_verification_of_block_transactions_in_mempool() { ); // Create a non-coinbase V4 tx with the last valid expiry height. - let tx = Transaction::V4 { + let tx = Transaction::V5 { + network_upgrade: NetworkUpgrade::Nu5, inputs: vec![input], outputs: vec![output], lock_time: LockTime::min_lock_time_timestamp(), expiry_height: height, - joinsplit_data: None, sapling_shielded_data: None, + orchard_shielded_data: None, }; let tx_hash = tx.hash(); From 064d0eed219a4c67f3183e7e0a5f2df758ec53a2 Mon Sep 17 00:00:00 2001 From: Arya Date: Thu, 12 Dec 2024 20:47:26 -0500 Subject: [PATCH 6/6] Uses correct consensus branch id in test --- zebra-consensus/src/transaction/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 63035ca5845..417298cc7f3 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -732,7 +732,7 @@ async fn skips_verification_of_block_transactions_in_mempool() { // Create a non-coinbase V4 tx with the last valid expiry height. let tx = Transaction::V5 { - network_upgrade: NetworkUpgrade::Nu5, + network_upgrade: NetworkUpgrade::Nu6, inputs: vec![input], outputs: vec![output], lock_time: LockTime::min_lock_time_timestamp(),