From 36b8ece0f965d657bae6cbbe6229e4c4a5b9e551 Mon Sep 17 00:00:00 2001 From: Marek Date: Sat, 18 Jan 2025 00:00:32 +0100 Subject: [PATCH 01/37] Add `has_foo` fns to `Transaction` --- zebra-chain/src/transaction.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index c04f4155b4f..c87b336ef92 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -258,6 +258,16 @@ impl Transaction { !self.inputs().is_empty() } + /// Does this transaction have transparent outputs? + pub fn has_transparent_outputs(&self) -> bool { + !self.outputs().is_empty() + } + + /// Does this transaction have transparent inputs or outputs? + pub fn has_transparent_inputs_or_outputs(&self) -> bool { + self.has_transparent_inputs() || self.has_transparent_outputs() + } + /// Does this transaction have transparent or shielded inputs? pub fn has_transparent_or_shielded_inputs(&self) -> bool { self.has_transparent_inputs() || self.has_shielded_inputs() @@ -276,11 +286,6 @@ impl Transaction { .contains(orchard::Flags::ENABLE_SPENDS)) } - /// Does this transaction have transparent or shielded outputs? - pub fn has_transparent_or_shielded_outputs(&self) -> bool { - !self.outputs().is_empty() || self.has_shielded_outputs() - } - /// Does this transaction have shielded outputs? /// /// See [`Self::has_transparent_or_shielded_outputs`] for details. @@ -294,6 +299,11 @@ impl Transaction { .contains(orchard::Flags::ENABLE_OUTPUTS)) } + /// Does this transaction have transparent or shielded outputs? + pub fn has_transparent_or_shielded_outputs(&self) -> bool { + self.has_transparent_outputs() || self.has_shielded_outputs() + } + /// Does this transaction has at least one flag when we have at least one orchard action? pub fn has_enough_orchard_flags(&self) -> bool { if self.version() < 5 || self.orchard_actions().count() == 0 { From d40c83af050f2da0f3d06874cfc9d62388176022 Mon Sep 17 00:00:00 2001 From: Marek Date: Sat, 18 Jan 2025 01:10:01 +0100 Subject: [PATCH 02/37] Add V5 SIGHASH test based on consensus branch ID --- zebra-chain/src/tests/vectors.rs | 1 + zebra-chain/src/transaction/unmined.rs | 18 +- zebra-consensus/src/block.rs | 2 + zebra-consensus/src/transaction.rs | 55 ++++- zebra-consensus/src/transaction/tests.rs | 191 +++++++++++++++++- zebra-consensus/src/transaction/tests/prop.rs | 1 + .../components/inbound/tests/fake_peer_set.rs | 4 + zebrad/src/components/mempool/downloads.rs | 1 + .../components/mempool/storage/tests/prop.rs | 4 + .../mempool/storage/tests/vectors.rs | 1 + zebrad/src/components/mempool/tests/vector.rs | 2 + 11 files changed, 270 insertions(+), 10 deletions(-) diff --git a/zebra-chain/src/tests/vectors.rs b/zebra-chain/src/tests/vectors.rs index 69e4955f6a0..1a5c4223f54 100644 --- a/zebra-chain/src/tests/vectors.rs +++ b/zebra-chain/src/tests/vectors.rs @@ -65,6 +65,7 @@ impl Network { transaction, Amount::try_from(1_000_000).expect("invalid value"), 0, + false, ) .ok() }) diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs index da716573e8b..b0c08307c7e 100644 --- a/zebra-chain/src/transaction/unmined.rs +++ b/zebra-chain/src/transaction/unmined.rs @@ -385,21 +385,31 @@ impl VerifiedUnminedTx { transaction: UnminedTx, miner_fee: Amount, legacy_sigop_count: u64, + // Allows skipping ZIP 317 checks, only in tests. + #[cfg(feature = "proptest-impl")] skip_checks: bool, ) -> Result { let fee_weight_ratio = zip317::conventional_fee_weight_ratio(&transaction, miner_fee); let conventional_actions = zip317::conventional_actions(&transaction.transaction); let unpaid_actions = zip317::unpaid_actions(&transaction, miner_fee); + let tx_size = transaction.size; - zip317::mempool_checks(unpaid_actions, miner_fee, transaction.size)?; - - Ok(Self { + let verified_unmined_tx = Self { transaction, miner_fee, legacy_sigop_count, fee_weight_ratio, conventional_actions, unpaid_actions, - }) + }; + + #[cfg(feature = "proptest-impl")] + if skip_checks { + return Ok(verified_unmined_tx); + } + + zip317::mempool_checks(unpaid_actions, miner_fee, tx_size)?; + + Ok(verified_unmined_tx) } /// Returns `true` if the transaction pays at least the [ZIP-317] conventional fee. diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index 1c959dd284e..093c1891fff 100644 --- a/zebra-consensus/src/block.rs +++ b/zebra-consensus/src/block.rs @@ -251,6 +251,8 @@ where known_utxos: known_utxos.clone(), height, time: block.header.time, + #[cfg(any(test, feature = "proptest-impl"))] + skip_checks: None, }); async_checks.push(rsp); } diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 044a9569f9a..723e2835e4e 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -158,6 +158,9 @@ pub enum Request { height: block::Height, /// The time that the block was mined. time: DateTime, + /// Verification checks to skip, only in tests. + #[cfg(any(test, feature = "proptest-impl"))] + skip_checks: Option>, }, /// Verify the supplied transaction as part of the mempool. /// @@ -172,6 +175,9 @@ pub enum Request { /// The next block is the first block that could possibly contain a /// mempool transaction. height: block::Height, + /// Verification checks to skip, only in tests. + #[cfg(any(test, feature = "proptest-impl"))] + skip_checks: Option>, }, } @@ -318,6 +324,15 @@ impl Request { pub fn is_mempool(&self) -> bool { matches!(self, Request::Mempool { .. }) } + + /// Returns the verification checks that should be skipped, only in tests. + #[cfg(any(test, feature = "proptest-impl"))] + pub fn skip_checks(&self) -> Option> { + match self { + Request::Block { skip_checks, .. } => skip_checks.clone(), + Request::Mempool { skip_checks, .. } => skip_checks.clone(), + } + } } impl Response { @@ -415,7 +430,12 @@ where // Do quick checks first check::has_inputs_and_outputs(&tx)?; check::has_enough_orchard_flags(&tx)?; + #[cfg(not(test))] check::consensus_branch_id(&tx, req.height(), &network)?; + #[cfg(test)] + if req.skip_checks().is_none_or(|c| !c.contains(&SkipCheck::ConsensusBranchId)) { + check::consensus_branch_id(&tx, req.height(), &network)?; + } // Validate the coinbase input consensus rules if req.is_mempool() && tx.is_coinbase() { @@ -432,7 +452,12 @@ where if tx.is_coinbase() { check::coinbase_expiry_height(&req.height(), &tx, &network)?; } else { + #[cfg(not(test))] check::non_coinbase_expiry_height(&req.height(), &tx)?; + #[cfg(test)] + if req.skip_checks().is_none_or(|c| !c.contains(&SkipCheck::ExpiryHeight)) { + check::non_coinbase_expiry_height(&req.height(), &tx)?; + } } // Consensus rule: @@ -575,12 +600,20 @@ where miner_fee, legacy_sigop_count, }, - Request::Mempool { transaction, .. } => { + Request::Mempool { transaction: ref tx, .. } => { + // #[cfg(test)] + #[cfg(any(test, feature = "proptest-impl"))] let transaction = VerifiedUnminedTx::new( - transaction, - miner_fee.expect( - "unexpected mempool coinbase transaction: should have already rejected", - ), + tx.clone(), + miner_fee.expect("fee should have been checked earlier"), + legacy_sigop_count, + req.skip_checks().is_some_and(|checks| checks.contains(&SkipCheck::Zip317)) + )?; + + #[cfg(not(any(test, feature = "proptest-impl")))] + let transaction = VerifiedUnminedTx::new( + tx.clone(), + miner_fee.expect("fee should have been checked earlier"), legacy_sigop_count, )?; @@ -1388,3 +1421,15 @@ where AsyncChecks(iterator.into_iter().map(FutureExt::boxed).collect()) } } + +/// Checks the tx verifier should skip, only in tests. +#[cfg(any(test, feature = "proptest-impl"))] +#[derive(Eq, Hash, PartialEq, Debug, Clone)] +pub enum SkipCheck { + /// Skip checks related to the transaction consensus branch ID. + ConsensusBranchId, + /// Skip mempool checks defined in ZIP 317. + Zip317, + /// Skip the expiry height check. + ExpiryHeight, +} diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 044c8b01842..8575848ce01 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -36,7 +36,10 @@ use zebra_node_services::mempool; use zebra_state::ValidateContextError; use zebra_test::mock_service::MockService; -use crate::{error::TransactionError, transaction::POLL_MEMPOOL_DELAY}; +use crate::{ + error::TransactionError, + transaction::{SkipCheck, POLL_MEMPOOL_DELAY}, +}; use super::{check, Request, Verifier}; @@ -230,6 +233,7 @@ async fn mempool_request_with_missing_input_is_rejected() { let verifier_req = verifier.oneshot(Request::Mempool { transaction: tx.into(), height, + skip_checks: None, }); let (rsp, _) = futures::join!(verifier_req, state_req); @@ -296,6 +300,7 @@ async fn mempool_request_with_present_input_is_accepted() { .oneshot(Request::Mempool { transaction: tx.into(), height, + skip_checks: None, }) .await; @@ -373,6 +378,7 @@ async fn mempool_request_with_invalid_lock_time_is_rejected() { .oneshot(Request::Mempool { transaction: tx.into(), height, + skip_checks: None, }) .await; @@ -442,6 +448,7 @@ async fn mempool_request_with_unlocked_lock_time_is_accepted() { .oneshot(Request::Mempool { transaction: tx.into(), height, + skip_checks: None, }) .await; @@ -512,6 +519,7 @@ async fn mempool_request_with_lock_time_max_sequence_number_is_accepted() { .oneshot(Request::Mempool { transaction: tx.into(), height, + skip_checks: None, }) .await; @@ -587,6 +595,7 @@ async fn mempool_request_with_past_lock_time_is_accepted() { .oneshot(Request::Mempool { transaction: tx.into(), height, + skip_checks: None, }) .await; @@ -680,6 +689,7 @@ async fn mempool_request_with_unmined_output_spends_is_accepted() { .oneshot(Request::Mempool { transaction: tx.into(), height, + skip_checks: None, }) .await; @@ -801,6 +811,7 @@ async fn skips_verification_of_block_transactions_in_mempool() { .oneshot(Request::Mempool { transaction: tx.clone().into(), height, + skip_checks: None, }) .await; @@ -844,6 +855,7 @@ async fn skips_verification_of_block_transactions_in_mempool() { known_utxos: Arc::new(HashMap::new()), height, time: Utc::now(), + skip_checks: None, }; let crate::transaction::Response::Block { .. } = verifier @@ -973,6 +985,7 @@ async fn mempool_request_with_immature_spend_is_rejected() { .oneshot(Request::Mempool { transaction: tx.into(), height, + skip_checks: None, }) .await .expect_err("verification of transaction with immature spend should fail"); @@ -1072,6 +1085,7 @@ async fn mempool_request_with_transparent_coinbase_spend_is_accepted_on_regtest( .oneshot(Request::Mempool { transaction: tx.into(), height, + skip_checks: None, }) .await .expect("verification of transaction with mature spend to transparent outputs should pass"); @@ -1144,6 +1158,7 @@ async fn state_error_converted_correctly() { .oneshot(Request::Mempool { transaction: tx.into(), height, + skip_checks: None, }) .await; @@ -1235,6 +1250,7 @@ async fn v5_transaction_is_rejected_before_nu5_activation() { known_outpoint_hashes: Arc::new(HashSet::new()), height: sapling.activation_height(&net).expect("height"), time: DateTime::::MAX_UTC, + skip_checks: None, }) .await, Err(TransactionError::UnsupportedByNetworkUpgrade(5, sapling)) @@ -1262,6 +1278,7 @@ async fn v5_transaction_is_accepted_after_nu5_activation() { known_outpoint_hashes: Arc::new(HashSet::new()), height: tx_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1316,6 +1333,7 @@ async fn v4_transaction_with_transparent_transfer_is_accepted() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1362,6 +1380,7 @@ async fn v4_transaction_with_last_valid_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1409,6 +1428,7 @@ async fn v4_coinbase_transaction_with_low_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1458,6 +1478,7 @@ async fn v4_transaction_with_too_low_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1510,6 +1531,7 @@ async fn v4_transaction_with_exceeding_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1565,6 +1587,7 @@ async fn v4_coinbase_transaction_with_exceeding_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1618,6 +1641,7 @@ async fn v4_coinbase_transaction_is_accepted() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1675,6 +1699,7 @@ async fn v4_transaction_with_transparent_transfer_is_rejected_by_the_script() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1732,6 +1757,7 @@ async fn v4_transaction_with_conflicting_transparent_spend_is_rejected() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1805,6 +1831,7 @@ fn v4_transaction_with_conflicting_sprout_nullifier_inside_joinsplit_is_rejected known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1883,6 +1910,7 @@ fn v4_transaction_with_conflicting_sprout_nullifier_across_joinsplits_is_rejecte known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1944,6 +1972,7 @@ async fn v5_transaction_with_transparent_transfer_is_accepted() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -1992,6 +2021,7 @@ async fn v5_transaction_with_last_valid_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -2039,6 +2069,7 @@ async fn v5_coinbase_transaction_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -2062,6 +2093,7 @@ async fn v5_coinbase_transaction_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await .map_err(|err| { @@ -2093,6 +2125,7 @@ async fn v5_coinbase_transaction_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await .map_err(|err| { @@ -2132,6 +2165,7 @@ async fn v5_coinbase_transaction_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: new_expiry_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -2185,6 +2219,7 @@ async fn v5_transaction_with_too_low_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -2236,6 +2271,7 @@ async fn v5_transaction_with_exceeding_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: height_max, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -2292,6 +2328,7 @@ async fn v5_coinbase_transaction_is_accepted() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -2351,6 +2388,7 @@ async fn v5_transaction_with_transparent_transfer_is_rejected_by_the_script() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -2401,6 +2439,7 @@ async fn v5_transaction_with_conflicting_transparent_spend_is_rejected() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -2446,6 +2485,7 @@ fn v4_with_signed_sprout_transfer_is_accepted() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -2537,6 +2577,7 @@ async fn v4_with_joinsplit_is_rejected_for_modification( known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await .map_err(|err| { @@ -2585,6 +2626,7 @@ fn v4_with_sapling_spends() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -2629,6 +2671,7 @@ fn v4_with_duplicate_sapling_spends() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -2675,6 +2718,7 @@ fn v4_with_sapling_outputs_and_no_spends() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await; @@ -2717,6 +2761,7 @@ async fn v5_with_sapling_spends() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await .expect("unexpected error response") @@ -2756,6 +2801,7 @@ async fn v5_with_duplicate_sapling_spends() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await, Err(TransactionError::DuplicateSaplingNullifier( @@ -2811,6 +2857,7 @@ async fn v5_with_duplicate_orchard_action() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .await, Err(TransactionError::DuplicateOrchardNullifier( @@ -2869,6 +2916,7 @@ async fn v5_consensus_branch_ids() { // The consensus branch ID of the tx is outdated for this height. height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .map_err(|err| *err.downcast().expect("`TransactionError` type")); @@ -2878,6 +2926,7 @@ async fn v5_consensus_branch_ids() { transaction: tx.clone().into(), // The consensus branch ID of the tx is outdated for this height. height, + skip_checks: None, }) .map_err(|err| *err.downcast().expect("`TransactionError` type")); @@ -2899,6 +2948,7 @@ async fn v5_consensus_branch_ids() { // The consensus branch ID of the tx is supported by this height. height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .map_ok(|rsp| rsp.tx_id()) .map_err(|e| format!("{e}")); @@ -2909,6 +2959,7 @@ async fn v5_consensus_branch_ids() { transaction: tx.clone().into(), // The consensus branch ID of the tx is supported by this height. height, + skip_checks: None, }) .map_ok(|rsp| rsp.tx_id()) .map_err(|e| format!("{e}")); @@ -2958,6 +3009,7 @@ async fn v5_consensus_branch_ids() { // The consensus branch ID of the tx is not supported by this height. height, time: DateTime::::MAX_UTC, + skip_checks: None, }) .map_err(|err| *err.downcast().expect("`TransactionError` type")); @@ -2967,6 +3019,7 @@ async fn v5_consensus_branch_ids() { transaction: tx.clone().into(), // The consensus branch ID of the tx is not supported by this height. height, + skip_checks: None, }) .map_err(|err| *err.downcast().expect("`TransactionError` type")); @@ -2981,6 +3034,140 @@ async fn v5_consensus_branch_ids() { } } +/// Checks that the tx verifier rejects V5 txs with an invalid consensus branch ID in their SIGHASH. +#[tokio::test] +async fn v5_consensus_branch_ids_sighash() { + let mut state = MockService::build().for_unit_tests(); + + for net in Network::iter() { + let verifier = Buffer::new(Verifier::new_for_tests(&net, state.clone()), 10); + + let tx = v5_transactions(net.block_iter()) + .find(|tx| { + !tx.has_transparent_inputs() + && tx.has_sapling_shielded_data() + && tx.has_orchard_shielded_data() + && tx + .network_upgrade() + .is_some_and(|nu| nu == NetworkUpgrade::Nu5) + }) + .expect("NU5 V5 tx with only Orchard & Sapling shielded data"); + + let tx_id = tx.unmined_id(); + + let block_req_with = |nu: NetworkUpgrade, skip_checks| Request::Block { + transaction_hash: tx.hash(), + transaction: tx.clone().into(), + known_outpoint_hashes: Arc::new(HashSet::new()), + known_utxos: Arc::new(HashMap::new()), + height: nu.activation_height(&net).expect("activation height"), + time: DateTime::::MAX_UTC, + skip_checks, + }; + + let mempool_req_with = |nu: NetworkUpgrade, skip_checks| Request::Mempool { + transaction: tx.clone().into(), + height: nu.activation_height(&net).expect("activation height"), + skip_checks, + }; + + // The verification of the tx under NU5 should succeed since we picked an NU5 one. + + let block_verification_result = verifier + .clone() + .oneshot(block_req_with(NetworkUpgrade::Nu5, None)) + .map_ok(|rsp| rsp.tx_id()) + .map_err(|e| format!("{e}")) + .await; + + assert_eq!(block_verification_result, Ok(tx_id)); + + let verifier_req = verifier + .clone() + .oneshot(mempool_req_with( + NetworkUpgrade::Nu5, + // We need to skip ZIP 317 mempool checks because we don't have a Testnet V5 tx in + // our test vectors without unpaid actions. + // + // TODO: Add at least one V5 tx with no unpaid actions to Testnet test vectors. + Some(HashSet::from_iter([SkipCheck::Zip317])), + )) + .map_ok(|rsp| rsp.tx_id()) + .map_err(|e| format!("{e}")); + + let state_req = async { + state + .expect_request_that(|req| { + matches!( + req, + zebra_state::Request::CheckBestChainTipNullifiersAndAnchors(_) + ) + }) + .map(|r| r.respond(zebra_state::Response::ValidBestChainTipNullifiersAndAnchors)) + .await; + }; + + let (_, mempool_verification_result) = futures::join!(state_req, verifier_req); + + assert_eq!(mempool_verification_result, Ok(tx_id)); + + // The verification of the same tx under NU6 should not succeed due to an invalid binding + // sig since the tx's consensus branch ID field is part of the SIGHASH computation and is + // invalid under NU6. To test this, we need to disable the explicit check of the consensus + // branch ID field, which the verifier performs before checking the binding sig. We also + // need to disable some additional checks to let the verifier reach the binding sig + // verification. + + let block_verification_result = verifier + .clone() + .oneshot(block_req_with( + NetworkUpgrade::Nu6, + Some(HashSet::from_iter([ + SkipCheck::ConsensusBranchId, + SkipCheck::ExpiryHeight, + ])), + )) + .map_ok(|rsp| rsp.tx_id()) + .map_err(|e| format!("{e}")) + .await; + + assert_eq!(block_verification_result, Ok(tx_id)); + + let verifier_req = verifier + .clone() + .oneshot(mempool_req_with( + NetworkUpgrade::Nu6, + Some(HashSet::from_iter([ + SkipCheck::ConsensusBranchId, + SkipCheck::ExpiryHeight, + // We need to skip ZIP 317 mempool checks because we don't have a Testnet V5 tx + // in our test vectors without unpaid actions. + // + // TODO: Add at least one V5 tx with no unpaid actions to Testnet test vectors. + SkipCheck::Zip317, + ])), + )) + .map_ok(|rsp| rsp.tx_id()) + .map_err(|e| format!("{e}")); + + let state_req = async { + state + .expect_request_that(|req| { + matches!( + req, + zebra_state::Request::CheckBestChainTipNullifiersAndAnchors(_) + ) + }) + .map(|r| r.respond(zebra_state::Response::ValidBestChainTipNullifiersAndAnchors)) + .await; + }; + + let (_, mempool_verification_result) = futures::join!(state_req, verifier_req); + + assert_eq!(mempool_verification_result, Ok(tx_id)); + } +} + // Utility functions /// Create a mock transparent transfer to be included in a transaction. @@ -3498,6 +3685,7 @@ async fn mempool_zip317_error() { .oneshot(Request::Mempool { transaction: tx.into(), height, + skip_checks: None, }) .await; @@ -3570,6 +3758,7 @@ async fn mempool_zip317_ok() { .oneshot(Request::Mempool { transaction: tx.into(), height, + skip_checks: None, }) .await; diff --git a/zebra-consensus/src/transaction/tests/prop.rs b/zebra-consensus/src/transaction/tests/prop.rs index 8fea9cf3433..f5f79156b96 100644 --- a/zebra-consensus/src/transaction/tests/prop.rs +++ b/zebra-consensus/src/transaction/tests/prop.rs @@ -467,6 +467,7 @@ fn validate( known_outpoint_hashes: Arc::new(HashSet::new()), height, time: block_time, + skip_checks: None, }) .await .map_err(|err| { diff --git a/zebrad/src/components/inbound/tests/fake_peer_set.rs b/zebrad/src/components/inbound/tests/fake_peer_set.rs index 176ec8c1c57..fb91cdf243e 100644 --- a/zebrad/src/components/inbound/tests/fake_peer_set.rs +++ b/zebrad/src/components/inbound/tests/fake_peer_set.rs @@ -168,6 +168,7 @@ async fn mempool_push_transaction() -> Result<(), crate::BoxError> { transaction, Amount::try_from(1_000_000).expect("invalid value"), 0, + false, ) .expect("verification should pass"), )); @@ -273,6 +274,7 @@ async fn mempool_advertise_transaction_ids() -> Result<(), crate::BoxError> { transaction, Amount::try_from(1_000_000).expect("invalid value"), 0, + false, ) .expect("verification should pass"), )); @@ -375,6 +377,7 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> { transaction, Amount::try_from(1_000_000).expect("invalid value"), 0, + false, ) .expect("verification should pass"), )); @@ -514,6 +517,7 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> { transaction, Amount::try_from(1_000_000).expect("invalid value"), 0, + false, ) .expect("verification should pass"), )); diff --git a/zebrad/src/components/mempool/downloads.rs b/zebrad/src/components/mempool/downloads.rs index 68d29aadcaf..4e3e6457858 100644 --- a/zebrad/src/components/mempool/downloads.rs +++ b/zebrad/src/components/mempool/downloads.rs @@ -373,6 +373,7 @@ where .oneshot(tx::Request::Mempool { transaction: tx.clone(), height: next_height, + skip_checks: None }) .map_ok(|rsp| { let tx::Response::Mempool { transaction, spent_mempool_outpoints } = rsp else { diff --git a/zebrad/src/components/mempool/storage/tests/prop.rs b/zebrad/src/components/mempool/storage/tests/prop.rs index 398ba0925f9..01b0f3f7f17 100644 --- a/zebrad/src/components/mempool/storage/tests/prop.rs +++ b/zebrad/src/components/mempool/storage/tests/prop.rs @@ -480,6 +480,7 @@ impl SpendConflictTestInput { // make sure miner fee is big enough for all cases Amount::try_from(1_000_000).expect("invalid value"), 0, + false, ) .expect("verification should pass"), VerifiedUnminedTx::new( @@ -487,6 +488,7 @@ impl SpendConflictTestInput { // make sure miner fee is big enough for all cases Amount::try_from(1_000_000).expect("invalid value"), 0, + false, ) .expect("verification should pass"), ) @@ -510,6 +512,7 @@ impl SpendConflictTestInput { // make sure miner fee is big enough for all cases Amount::try_from(1_000_000).expect("invalid value"), 0, + false, ) .expect("verification should pass"), VerifiedUnminedTx::new( @@ -517,6 +520,7 @@ impl SpendConflictTestInput { // make sure miner fee is big enough for all cases Amount::try_from(1_000_000).expect("invalid value"), 0, + false, ) .expect("verification should pass"), ) diff --git a/zebrad/src/components/mempool/storage/tests/vectors.rs b/zebrad/src/components/mempool/storage/tests/vectors.rs index 30ce35bb832..8656a87c77f 100644 --- a/zebrad/src/components/mempool/storage/tests/vectors.rs +++ b/zebrad/src/components/mempool/storage/tests/vectors.rs @@ -269,6 +269,7 @@ fn mempool_expired_basic_for_network(network: Network) -> Result<()> { tx.into(), Amount::try_from(1_000_000).expect("invalid value"), 0, + false, ) .expect("verification should pass"), Vec::new(), diff --git a/zebrad/src/components/mempool/tests/vector.rs b/zebrad/src/components/mempool/tests/vector.rs index 9dd3557de9c..457b95edc38 100644 --- a/zebrad/src/components/mempool/tests/vector.rs +++ b/zebrad/src/components/mempool/tests/vector.rs @@ -825,6 +825,7 @@ async fn mempool_reverifies_after_tip_change() -> Result<(), Report> { transaction, Amount::try_from(1_000_000).expect("invalid value"), 0, + false, ) .expect("verification should pass"), )); @@ -885,6 +886,7 @@ async fn mempool_reverifies_after_tip_change() -> Result<(), Report> { transaction, Amount::try_from(1_000_000).expect("invalid value"), 0, + false, ) .expect("verification should pass"), )); From a2f3a458191af624880720e68a97e8a518ec3d90 Mon Sep 17 00:00:00 2001 From: Marek Date: Sat, 18 Jan 2025 01:33:13 +0100 Subject: [PATCH 03/37] Guard `skip_checks` by test features --- zebrad/src/components/mempool/downloads.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zebrad/src/components/mempool/downloads.rs b/zebrad/src/components/mempool/downloads.rs index 4e3e6457858..388600004d3 100644 --- a/zebrad/src/components/mempool/downloads.rs +++ b/zebrad/src/components/mempool/downloads.rs @@ -373,6 +373,7 @@ where .oneshot(tx::Request::Mempool { transaction: tx.clone(), height: next_height, + #[cfg(any(test, feature = "proptest-impl", feature = "proptest-derive"))] skip_checks: None }) .map_ok(|rsp| { From 9597792567982ae19643eba0d2537d3a81fe5956 Mon Sep 17 00:00:00 2001 From: Marek Date: Sat, 18 Jan 2025 17:28:18 +0100 Subject: [PATCH 04/37] Enable `proptest-impl` for `zebrad` in tests --- Cargo.lock | 1 + zebrad/Cargo.toml | 5 +++++ zebrad/src/components/mempool/downloads.rs | 4 ++-- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 53b5a13ba46..fa4d347975a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6176,6 +6176,7 @@ dependencies = [ "zebra-state", "zebra-test", "zebra-utils", + "zebrad", ] [[package]] diff --git a/zebrad/Cargo.toml b/zebrad/Cargo.toml index ce5fbde2a21..ece5e927df5 100644 --- a/zebrad/Cargo.toml +++ b/zebrad/Cargo.toml @@ -298,5 +298,10 @@ zebra-grpc = { path = "../zebra-grpc", version = "0.1.0-alpha.11" } # zebra-utils { path = "../zebra-utils", artifact = "bin:zebra-checkpoints" } zebra-utils = { path = "../zebra-utils", version = "1.0.0-beta.44" } +# Enable the "proptest-impl" feature for "self" in tests. +# +# +zebrad = { path = ".", features = ["proptest-impl"], default-features = false } + [lints.rust] unexpected_cfgs = { level = "warn", check-cfg = ['cfg(tokio_unstable)'] } diff --git a/zebrad/src/components/mempool/downloads.rs b/zebrad/src/components/mempool/downloads.rs index 388600004d3..c0cf911fe02 100644 --- a/zebrad/src/components/mempool/downloads.rs +++ b/zebrad/src/components/mempool/downloads.rs @@ -373,8 +373,8 @@ where .oneshot(tx::Request::Mempool { transaction: tx.clone(), height: next_height, - #[cfg(any(test, feature = "proptest-impl", feature = "proptest-derive"))] - skip_checks: None + #[cfg(feature = "proptest-impl")] + skip_checks: None, }) .map_ok(|rsp| { let tx::Response::Mempool { transaction, spent_mempool_outpoints } = rsp else { From 61f8735540c75308d2904e6f10c9d4dfc0c7254e Mon Sep 17 00:00:00 2001 From: Marek Date: Sat, 18 Jan 2025 19:30:03 +0100 Subject: [PATCH 05/37] Simplify conditional compilation --- zebra-consensus/src/transaction.rs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 723e2835e4e..1aeed0bcf88 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -601,22 +601,14 @@ where legacy_sigop_count, }, Request::Mempool { transaction: ref tx, .. } => { - // #[cfg(test)] - #[cfg(any(test, feature = "proptest-impl"))] let transaction = VerifiedUnminedTx::new( tx.clone(), miner_fee.expect("fee should have been checked earlier"), legacy_sigop_count, + #[cfg(any(test, feature = "proptest-impl"))] req.skip_checks().is_some_and(|checks| checks.contains(&SkipCheck::Zip317)) )?; - #[cfg(not(any(test, feature = "proptest-impl")))] - let transaction = VerifiedUnminedTx::new( - tx.clone(), - miner_fee.expect("fee should have been checked earlier"), - legacy_sigop_count, - )?; - if let Some(mut mempool) = mempool { tokio::spawn(async move { // Best-effort poll of the mempool to provide a timely response to From e15d98a7debc960848c07a4e20ab83ac036aa77d Mon Sep 17 00:00:00 2001 From: Marek Date: Sat, 18 Jan 2025 19:36:48 +0100 Subject: [PATCH 06/37] Enable `proptest-impl` in scanner's dev deps --- Cargo.lock | 1 + zebra-scan/Cargo.toml | 2 ++ 2 files changed, 3 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index fa4d347975a..b405192911a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5982,6 +5982,7 @@ dependencies = [ "zcash_note_encryption", "zcash_primitives", "zebra-chain", + "zebra-consensus", "zebra-grpc", "zebra-node-services", "zebra-rpc", diff --git a/zebra-scan/Cargo.toml b/zebra-scan/Cargo.toml index 42cf878a8e6..3475b76250a 100644 --- a/zebra-scan/Cargo.toml +++ b/zebra-scan/Cargo.toml @@ -126,5 +126,7 @@ toml = "0.8.19" tonic = "0.12.3" zebra-state = { path = "../zebra-state", version = "1.0.0-beta.44", features = ["proptest-impl"] } +zebra-consensus = { path = "../zebra-consensus", version = "1.0.0-beta.44", features = ["proptest-impl"] } +zebrad = { path = "../zebrad", version = "2.1.1", features = ["proptest-impl"] } zebra-test = { path = "../zebra-test", version = "1.0.0-beta.44" } From cc8067466120adcd4d3684e7d7525056d51b4387 Mon Sep 17 00:00:00 2001 From: Marek Date: Sat, 18 Jan 2025 20:31:37 +0100 Subject: [PATCH 07/37] Fix conditional compilation in `zebra-chain` tests --- zebra-chain/src/tests/vectors.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zebra-chain/src/tests/vectors.rs b/zebra-chain/src/tests/vectors.rs index 1a5c4223f54..1d9dd9a7db6 100644 --- a/zebra-chain/src/tests/vectors.rs +++ b/zebra-chain/src/tests/vectors.rs @@ -65,6 +65,7 @@ impl Network { transaction, Amount::try_from(1_000_000).expect("invalid value"), 0, + #[cfg(feature = "proptest-impl")] false, ) .ok() From af6de1023cbe130b8485171b0301fb7593643347 Mon Sep 17 00:00:00 2001 From: Marek Date: Wed, 22 Jan 2025 16:08:39 +0100 Subject: [PATCH 08/37] Add error types for `zebra-chain` --- zebra-chain/src/error.rs | 20 ++++++++++++++++++++ zebra-chain/src/lib.rs | 2 ++ 2 files changed, 22 insertions(+) diff --git a/zebra-chain/src/error.rs b/zebra-chain/src/error.rs index a3182a21feb..543d081131b 100644 --- a/zebra-chain/src/error.rs +++ b/zebra-chain/src/error.rs @@ -1,6 +1,10 @@ //! Errors that can occur inside any `zebra-chain` submodule. + +use std::io; use thiserror::Error; +// TODO: Move all these enums into a common enum at the bottom. + /// Errors related to random bytes generation. #[derive(Error, Copy, Clone, Debug, PartialEq, Eq)] pub enum RandError { @@ -51,3 +55,19 @@ pub enum AddressError { #[error("Randomness did not hash into the Jubjub group for producing a new diversifier")] DiversifierGenerationFailure, } + +/// `zebra-chain`'s errors +#[derive(Error, Debug)] +pub enum Error { + /// Invalid consensus branch ID. + #[error("invalid consensus branch id")] + InvalidConsensusBranchId, + + /// Zebra's type could not be converted to its librustzcash equivalent. + #[error("Zebra's type could not be converted to its librustzcash equivalent: ")] + Convertion(#[from] io::Error), + + /// The transaction is missing a network upgrade. + #[error("the transaction is missing a network upgrade")] + MissingNetworkUpgrade, +} diff --git a/zebra-chain/src/lib.rs b/zebra-chain/src/lib.rs index 460d3a850f0..0dd4a57c2d7 100644 --- a/zebra-chain/src/lib.rs +++ b/zebra-chain/src/lib.rs @@ -41,6 +41,8 @@ pub mod transparent; pub mod value_balance; pub mod work; +pub use error::Error; + #[cfg(any(test, feature = "proptest-impl"))] pub use block::LedgerState; From bc72acb427349f562e8bb1c80af86a88a0baccdd Mon Sep 17 00:00:00 2001 From: Marek Date: Wed, 22 Jan 2025 16:10:12 +0100 Subject: [PATCH 09/37] `impl TryFrom for NetworkUpgrade` --- zebra-chain/src/parameters/network_upgrade.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/zebra-chain/src/parameters/network_upgrade.rs b/zebra-chain/src/parameters/network_upgrade.rs index b08cfec520d..041d6f908b8 100644 --- a/zebra-chain/src/parameters/network_upgrade.rs +++ b/zebra-chain/src/parameters/network_upgrade.rs @@ -63,6 +63,18 @@ pub enum NetworkUpgrade { Nu6, } +impl TryFrom for NetworkUpgrade { + type Error = crate::Error; + + fn try_from(branch_id: u32) -> Result { + CONSENSUS_BRANCH_IDS + .iter() + .find(|id| id.1 == ConsensusBranchId(branch_id)) + .map(|nu| nu.0) + .ok_or(Self::Error::InvalidConsensusBranchId) + } +} + impl fmt::Display for NetworkUpgrade { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { // Same as the debug representation for now From 1b433298c6d94f4f912c1031f43da56b4f655fa7 Mon Sep 17 00:00:00 2001 From: Marek Date: Wed, 22 Jan 2025 16:10:51 +0100 Subject: [PATCH 10/37] `impl TryFrom for BranchId` --- zebra-chain/src/parameters/network_upgrade.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/zebra-chain/src/parameters/network_upgrade.rs b/zebra-chain/src/parameters/network_upgrade.rs index 041d6f908b8..36783ea06b7 100644 --- a/zebra-chain/src/parameters/network_upgrade.rs +++ b/zebra-chain/src/parameters/network_upgrade.rs @@ -216,6 +216,15 @@ impl fmt::Display for ConsensusBranchId { } } +impl TryFrom for zcash_primitives::consensus::BranchId { + type Error = crate::Error; + + fn try_from(id: ConsensusBranchId) -> Result { + zcash_primitives::consensus::BranchId::try_from(u32::from(id)) + .map_err(|_| Self::Error::InvalidConsensusBranchId) + } +} + /// Network Upgrade Consensus Branch Ids. /// /// Branch ids are the same for mainnet and testnet. If there is a testnet From 3d831fbbffda62f76089ac9b01ed9d31e8f189dd Mon Sep 17 00:00:00 2001 From: Marek Date: Wed, 22 Jan 2025 16:11:35 +0100 Subject: [PATCH 11/37] Rm `fn from_branch_id() -> Option` --- zebra-chain/src/parameters/network_upgrade.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/zebra-chain/src/parameters/network_upgrade.rs b/zebra-chain/src/parameters/network_upgrade.rs index 36783ea06b7..2485423d417 100644 --- a/zebra-chain/src/parameters/network_upgrade.rs +++ b/zebra-chain/src/parameters/network_upgrade.rs @@ -538,14 +538,6 @@ impl NetworkUpgrade { ) -> Duration { NetworkUpgrade::current(network, height).averaging_window_timespan() } - - /// Returns the NetworkUpgrade given an u32 as ConsensusBranchId - pub fn from_branch_id(branch_id: u32) -> Option { - CONSENSUS_BRANCH_IDS - .iter() - .find(|id| id.1 == ConsensusBranchId(branch_id)) - .map(|nu| nu.0) - } } impl From for NetworkUpgrade { From 1cfda828be03d62c395d93e4c27e587a2a833601 Mon Sep 17 00:00:00 2001 From: Marek Date: Wed, 22 Jan 2025 16:43:32 +0100 Subject: [PATCH 12/37] Check consensus branch ID in SIGHASH computation --- .../src/primitives/zcash_note_encryption.rs | 27 +++------ .../src/primitives/zcash_primitives.rs | 59 ++++++++----------- zebra-chain/src/transaction.rs | 34 +++++++++-- zebra-chain/src/transaction/sighash.rs | 7 +-- zebra-chain/src/transaction/txid.rs | 22 +++---- zebra-consensus/src/script.rs | 6 +- zebra-consensus/src/transaction.rs | 16 ++--- zebra-script/src/lib.rs | 57 +++++++----------- 8 files changed, 100 insertions(+), 128 deletions(-) diff --git a/zebra-chain/src/primitives/zcash_note_encryption.rs b/zebra-chain/src/primitives/zcash_note_encryption.rs index cbc19afc5d2..ae802beb0f7 100644 --- a/zebra-chain/src/primitives/zcash_note_encryption.rs +++ b/zebra-chain/src/primitives/zcash_note_encryption.rs @@ -4,38 +4,29 @@ use crate::{ block::Height, parameters::{Network, NetworkUpgrade}, - primitives::zcash_primitives::convert_tx_to_librustzcash, transaction::Transaction, }; /// Returns true if all Sapling or Orchard outputs, if any, decrypt successfully with /// an all-zeroes outgoing viewing key. -/// -/// # Panics -/// -/// If passed a network/height without matching consensus branch ID (pre-Overwinter), -/// since `librustzcash` won't be able to parse it. -pub fn decrypts_successfully(transaction: &Transaction, network: &Network, height: Height) -> bool { - let network_upgrade = NetworkUpgrade::current(network, height); - let alt_tx = convert_tx_to_librustzcash( - transaction, - network_upgrade - .branch_id() - .expect("should have a branch ID"), - ) - .expect("zcash_primitives and Zebra transaction formats must be compatible"); +pub fn decrypts_successfully(tx: &Transaction, network: &Network, height: Height) -> bool { + let nu = NetworkUpgrade::current(network, height); + + let Ok(tx) = tx.to_librustzcash(nu) else { + return false; + }; let null_sapling_ovk = sapling_crypto::keys::OutgoingViewingKey([0u8; 32]); // Note that, since this function is used to validate coinbase transactions, we can ignore // the "grace period" mentioned in ZIP-212. - let zip_212_enforcement = if network_upgrade >= NetworkUpgrade::Canopy { + let zip_212_enforcement = if nu >= NetworkUpgrade::Canopy { sapling_crypto::note_encryption::Zip212Enforcement::On } else { sapling_crypto::note_encryption::Zip212Enforcement::Off }; - if let Some(bundle) = alt_tx.sapling_bundle() { + if let Some(bundle) = tx.sapling_bundle() { for output in bundle.shielded_outputs().iter() { let recovery = sapling_crypto::note_encryption::try_sapling_output_recovery( &null_sapling_ovk, @@ -48,7 +39,7 @@ pub fn decrypts_successfully(transaction: &Transaction, network: &Network, heigh } } - if let Some(bundle) = alt_tx.orchard_bundle() { + if let Some(bundle) = tx.orchard_bundle() { for act in bundle.actions() { if zcash_note_encryption::try_output_recovery_with_ovk( &orchard::note_encryption::OrchardDomain::for_action(act), diff --git a/zebra-chain/src/primitives/zcash_primitives.rs b/zebra-chain/src/primitives/zcash_primitives.rs index 7ab2f32d751..1c70ea33576 100644 --- a/zebra-chain/src/primitives/zcash_primitives.rs +++ b/zebra-chain/src/primitives/zcash_primitives.rs @@ -8,7 +8,7 @@ use zcash_protocol::value::BalanceError; use crate::{ amount::{Amount, NonNegative}, - parameters::{ConsensusBranchId, Network}, + parameters::{Network, NetworkUpgrade}, serialization::ZcashSerialize, transaction::{AuthDigest, HashType, SigHash, Transaction}, transparent::{self, Script}, @@ -178,20 +178,6 @@ impl TryFrom<&Transaction> for zp_tx::Transaction { } } -pub(crate) fn convert_tx_to_librustzcash( - trans: &Transaction, - branch_id: ConsensusBranchId, -) -> Result { - let serialized_tx = trans.zcash_serialize_to_vec()?; - let branch_id: u32 = branch_id.into(); - // We've already parsed this transaction, so its network upgrade must be valid. - let branch_id: zcash_primitives::consensus::BranchId = branch_id - .try_into() - .expect("zcash_primitives and Zebra have the same branch ids"); - let alt_tx = zp_tx::Transaction::read(&serialized_tx[..], branch_id)?; - Ok(alt_tx) -} - /// Convert a Zebra transparent::Output into a librustzcash one. impl TryFrom<&transparent::Output> for zp_tx::components::TxOut { type Error = io::Error; @@ -261,21 +247,24 @@ impl<'a> PrecomputedTxData<'a> { /// transparent input in the transaction. pub(crate) fn new( tx: &'a Transaction, - branch_id: ConsensusBranchId, + nu: NetworkUpgrade, all_previous_outputs: &'a [transparent::Output], ) -> PrecomputedTxData<'a> { - let alt_tx = convert_tx_to_librustzcash(tx, branch_id) - .expect("zcash_primitives and Zebra transaction formats must be compatible"); - let txid_parts = alt_tx.deref().digest(zp_tx::txid::TxIdDigester); + let tx = tx + .to_librustzcash(nu) + .expect("`zcash_primitives` and Zebra tx formats must be compatible"); + + let txid_parts = tx.deref().digest(zp_tx::txid::TxIdDigester); let f_transparent = MapTransparent { auth: TransparentAuth { all_prev_outputs: all_previous_outputs, }, }; - let tx_data: zp_tx::TransactionData = alt_tx - .into_data() - .map_authorization(f_transparent, IdentityMap, IdentityMap); + + let tx_data: zp_tx::TransactionData = + tx.into_data() + .map_authorization(f_transparent, IdentityMap, IdentityMap); PrecomputedTxData { tx_data, @@ -331,26 +320,24 @@ pub(crate) fn sighash( ) } -/// Compute the authorizing data commitment of this transaction as specified -/// in [ZIP-244]. +/// Compute the authorizing data commitment of this transaction as specified in [ZIP-244]. /// /// # Panics /// /// If passed a pre-v5 transaction. /// /// [ZIP-244]: https://zips.z.cash/zip-0244 -pub(crate) fn auth_digest(trans: &Transaction) -> AuthDigest { - let alt_tx: zp_tx::Transaction = trans - .try_into() - .expect("zcash_primitives and Zebra transaction formats must be compatible"); - - let digest_bytes: [u8; 32] = alt_tx - .auth_commitment() - .as_ref() - .try_into() - .expect("digest has the correct size"); - - AuthDigest(digest_bytes) +pub(crate) fn auth_digest(tx: &Transaction) -> AuthDigest { + let nu = tx.network_upgrade().expect("V5 tx has a network upgrade"); + + AuthDigest( + tx.to_librustzcash(nu) + .expect("V5 tx is convertible to its `zcash_params` equivalent") + .auth_commitment() + .as_ref() + .try_into() + .expect("digest has the correct size"), + ) } /// Return the destination address from a transparent output. diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index c87b336ef92..007e2a312f9 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -37,11 +37,12 @@ pub use sighash::{HashType, SigHash, SigHasher}; pub use unmined::{ zip317, UnminedTx, UnminedTxId, VerifiedUnminedTx, MEMPOOL_TRANSACTION_COST_THRESHOLD, }; +use zcash_protocol::consensus; use crate::{ amount::{Amount, Error as AmountError, NegativeAllowed, NonNegative}, block, orchard, - parameters::{ConsensusBranchId, Network, NetworkUpgrade}, + parameters::{Network, NetworkUpgrade}, primitives::{ed25519, Bctv14Proof, Groth16Proof}, sapling, serialization::ZcashSerialize, @@ -217,22 +218,22 @@ impl Transaction { /// - if the input index is out of bounds for self.inputs() pub fn sighash( &self, - branch_id: ConsensusBranchId, + nu: NetworkUpgrade, hash_type: sighash::HashType, all_previous_outputs: &[transparent::Output], input_index_script_code: Option<(usize, Vec)>, ) -> SigHash { - sighash::SigHasher::new(self, branch_id, all_previous_outputs) + sighash::SigHasher::new(self, nu, all_previous_outputs) .sighash(hash_type, input_index_script_code) } /// Return a [`SigHasher`] for this transaction. pub fn sighasher<'a>( &'a self, - branch_id: ConsensusBranchId, + nu: NetworkUpgrade, all_previous_outputs: &'a [transparent::Output], ) -> sighash::SigHasher<'a> { - sighash::SigHasher::new(self, branch_id, all_previous_outputs) + sighash::SigHasher::new(self, nu, all_previous_outputs) } /// Compute the authorizing data commitment of this transaction as specified @@ -1227,6 +1228,29 @@ impl Transaction { ) -> Result, ValueBalanceError> { self.value_balance_from_outputs(&outputs_from_utxos(utxos.clone())) } + + /// Converts [`Transaction`] to [`zcash_primitives::transaction::Transaction`]. + pub(crate) fn to_librustzcash( + &self, + nu: NetworkUpgrade, + ) -> Result { + if self.network_upgrade().is_some_and(|tx_nu| tx_nu != nu) { + return Err(crate::Error::InvalidConsensusBranchId); + } + + let Some(branch_id) = nu.branch_id() else { + return Err(crate::Error::InvalidConsensusBranchId); + }; + + let Ok(branch_id) = consensus::BranchId::try_from(branch_id) else { + return Err(crate::Error::InvalidConsensusBranchId); + }; + + Ok(zcash_primitives::transaction::Transaction::read( + &self.zcash_serialize_to_vec()?[..], + branch_id, + )?) + } } #[cfg(any(test, feature = "proptest-impl"))] diff --git a/zebra-chain/src/transaction/sighash.rs b/zebra-chain/src/transaction/sighash.rs index 24d58a77b38..a45c79547a7 100644 --- a/zebra-chain/src/transaction/sighash.rs +++ b/zebra-chain/src/transaction/sighash.rs @@ -2,7 +2,7 @@ use super::Transaction; -use crate::parameters::ConsensusBranchId; +use crate::parameters::NetworkUpgrade; use crate::transparent; use crate::primitives::zcash_primitives::{sighash, PrecomputedTxData}; @@ -49,12 +49,11 @@ impl<'a> SigHasher<'a> { /// Create a new SigHasher for the given transaction. pub fn new( trans: &'a Transaction, - branch_id: ConsensusBranchId, + nu: NetworkUpgrade, all_previous_outputs: &'a [transparent::Output], ) -> Self { - let precomputed_tx_data = PrecomputedTxData::new(trans, branch_id, all_previous_outputs); SigHasher { - precomputed_tx_data, + precomputed_tx_data: PrecomputedTxData::new(trans, nu, all_previous_outputs), } } diff --git a/zebra-chain/src/transaction/txid.rs b/zebra-chain/src/transaction/txid.rs index f67f6dee58d..abaffdd4d45 100644 --- a/zebra-chain/src/transaction/txid.rs +++ b/zebra-chain/src/transaction/txid.rs @@ -1,6 +1,5 @@ //! Transaction ID computation. Contains code for generating the Transaction ID //! from the transaction. -use std::io; use super::{Hash, Transaction}; use crate::serialization::{sha256d, ZcashSerialize}; @@ -22,7 +21,7 @@ impl<'a> TxIdBuilder<'a> { } /// Compute the Transaction ID for the previously specified transaction. - pub(super) fn txid(self) -> Result { + pub(super) fn txid(self) -> Option { match self.trans { Transaction::V1 { .. } | Transaction::V2 { .. } @@ -34,22 +33,19 @@ impl<'a> TxIdBuilder<'a> { /// Compute the Transaction ID for transactions V1 to V4. /// In these cases it's simply the hash of the serialized transaction. - #[allow(clippy::unwrap_in_result)] - fn txid_v1_to_v4(self) -> Result { + fn txid_v1_to_v4(self) -> Option { let mut hash_writer = sha256d::Writer::default(); - self.trans - .zcash_serialize(&mut hash_writer) - .expect("Transactions must serialize into the hash."); - Ok(Hash(hash_writer.finish())) + self.trans.zcash_serialize(&mut hash_writer).ok()?; + Some(Hash(hash_writer.finish())) } /// Compute the Transaction ID for a V5 transaction in the given network upgrade. /// In this case it's the hash of a tree of hashes of specific parts of the /// transaction, as specified in ZIP-244 and ZIP-225. - fn txid_v5(self) -> Result { - // The v5 txid (from ZIP-244) is computed using librustzcash. Convert the zebra - // transaction to a librustzcash transaction. - let alt_tx: zcash_primitives::transaction::Transaction = self.trans.try_into()?; - Ok(Hash(*alt_tx.txid().as_ref())) + fn txid_v5(self) -> Option { + let nu = self.trans.network_upgrade()?; + + // We compute v5 txid (from ZIP-244) using librustzcash. + Some(Hash(*self.trans.to_librustzcash(nu).ok()?.txid().as_ref())) } } diff --git a/zebra-consensus/src/script.rs b/zebra-consensus/src/script.rs index 5adbc18c105..4432575e211 100644 --- a/zebra-consensus/src/script.rs +++ b/zebra-consensus/src/script.rs @@ -59,10 +59,6 @@ impl tower::Service for Verifier { upgrade, } = req; let input = &cached_ffi_transaction.inputs()[input_index]; - let branch_id = upgrade - .branch_id() - .expect("post-Sapling NUs have a consensus branch ID"); - match input { transparent::Input::PrevOut { outpoint, .. } => { let outpoint = *outpoint; @@ -71,7 +67,7 @@ impl tower::Service for Verifier { let span = tracing::trace_span!("script", ?outpoint); async move { - cached_ffi_transaction.is_valid(branch_id, input_index)?; + cached_ffi_transaction.is_valid(upgrade, input_index)?; tracing::trace!("script verification succeeded"); Ok(()) diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 1aeed0bcf88..5d74e6e83f6 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -873,14 +873,12 @@ where sapling_shielded_data: &Option>, ) -> Result { let tx = request.transaction(); - let upgrade = request.upgrade(network); + let nu = request.upgrade(network); - Self::verify_v4_transaction_network_upgrade(&tx, upgrade)?; + Self::verify_v4_transaction_network_upgrade(&tx, nu)?; let shielded_sighash = tx.sighash( - upgrade - .branch_id() - .expect("Overwinter-onwards must have branch ID, and we checkpoint on Canopy"), + nu, HashType::ALL, cached_ffi_transaction.all_previous_outputs(), None, @@ -969,14 +967,12 @@ where orchard_shielded_data: &Option, ) -> Result { let transaction = request.transaction(); - let upgrade = request.upgrade(network); + let nu = request.upgrade(network); - Self::verify_v5_transaction_network_upgrade(&transaction, upgrade)?; + Self::verify_v5_transaction_network_upgrade(&transaction, nu)?; let shielded_sighash = transaction.sighash( - upgrade - .branch_id() - .expect("Overwinter-onwards must have branch ID, and we checkpoint on Canopy"), + nu, HashType::ALL, cached_ffi_transaction.all_previous_outputs(), None, diff --git a/zebra-script/src/lib.rs b/zebra-script/src/lib.rs index 5b6e5f2a846..1a51fe3455b 100644 --- a/zebra-script/src/lib.rs +++ b/zebra-script/src/lib.rs @@ -21,7 +21,7 @@ use zcash_script::{ }; use zebra_chain::{ - parameters::ConsensusBranchId, + parameters::NetworkUpgrade, transaction::{HashType, SigHasher, Transaction}, transparent, }; @@ -156,7 +156,7 @@ impl CachedFfiTransaction { /// spends the matching [`transparent::Output`] it refers to, with the [`ConsensusBranchId`] /// of the block containing the transaction. #[allow(clippy::unwrap_in_result)] - pub fn is_valid(&self, branch_id: ConsensusBranchId, input_index: usize) -> Result<(), Error> { + pub fn is_valid(&self, nu: NetworkUpgrade, input_index: usize) -> Result<(), Error> { let previous_output = self .all_previous_outputs .get(input_index) @@ -200,7 +200,7 @@ impl CachedFfiTransaction { let ctx = Box::new(SigHashContext { input_index: n_in, - sighasher: SigHasher::new(&self.transaction, branch_id, &self.all_previous_outputs), + sighasher: SigHasher::new(&self.transaction, nu, &self.all_previous_outputs), }); // SAFETY: The `script_*` fields are created from a valid Rust `slice`. let ret = unsafe { @@ -271,7 +271,7 @@ mod tests { use hex::FromHex; use std::sync::Arc; use zebra_chain::{ - parameters::{ConsensusBranchId, NetworkUpgrade::*}, + parameters::NetworkUpgrade, serialization::{ZcashDeserialize, ZcashDeserializeInto}, transaction::Transaction, transparent::{self, Output}, @@ -286,7 +286,7 @@ mod tests { } fn verify_valid_script( - branch_id: ConsensusBranchId, + nu: NetworkUpgrade, tx: &[u8], amount: u64, pubkey: &[u8], @@ -301,7 +301,7 @@ mod tests { let previous_output = vec![output]; let verifier = super::CachedFfiTransaction::new(transaction, previous_output); - verifier.is_valid(branch_id, input_index)?; + verifier.is_valid(nu, input_index)?; Ok(()) } @@ -311,7 +311,7 @@ mod tests { let _init_guard = zebra_test::init(); verify_valid_script( - Blossom.branch_id().unwrap(), + NetworkUpgrade::Blossom, &SCRIPT_TX, 212 * u64::pow(10, 8), &SCRIPT_PUBKEY, @@ -344,12 +344,8 @@ mod tests { lock_script: transparent::Script::new(&SCRIPT_PUBKEY.clone()[..]), }; let input_index = 0; - let branch_id = Blossom - .branch_id() - .expect("Blossom has a ConsensusBranchId"); - let verifier = super::CachedFfiTransaction::new(transaction, vec![output]); - verifier.is_valid(branch_id, input_index).unwrap_err(); + verifier.is_valid(NetworkUpgrade::Blossom, input_index)?; Ok(()) } @@ -370,13 +366,9 @@ mod tests { let verifier = super::CachedFfiTransaction::new(transaction, vec![output]); let input_index = 0; - let branch_id = Blossom - .branch_id() - .expect("Blossom has a ConsensusBranchId"); - - verifier.is_valid(branch_id, input_index)?; - verifier.is_valid(branch_id, input_index)?; + verifier.is_valid(NetworkUpgrade::Blossom, input_index)?; + verifier.is_valid(NetworkUpgrade::Blossom, input_index)?; Ok(()) } @@ -397,13 +389,9 @@ mod tests { let verifier = super::CachedFfiTransaction::new(transaction, vec![output]); let input_index = 0; - let branch_id = Blossom - .branch_id() - .expect("Blossom has a ConsensusBranchId"); - verifier.is_valid(branch_id, input_index)?; - - verifier.is_valid(branch_id, input_index + 1).unwrap_err(); + verifier.is_valid(NetworkUpgrade::Blossom, input_index)?; + verifier.is_valid(NetworkUpgrade::Blossom, input_index + 1)?; Ok(()) } @@ -424,13 +412,9 @@ mod tests { let verifier = super::CachedFfiTransaction::new(transaction, vec![output]); let input_index = 0; - let branch_id = Blossom - .branch_id() - .expect("Blossom has a ConsensusBranchId"); - - verifier.is_valid(branch_id, input_index + 1).unwrap_err(); - verifier.is_valid(branch_id, input_index)?; + verifier.is_valid(NetworkUpgrade::Blossom, input_index + 1)?; + verifier.is_valid(NetworkUpgrade::Blossom, input_index)?; Ok(()) } @@ -451,13 +435,9 @@ mod tests { let verifier = super::CachedFfiTransaction::new(transaction, vec![output]); let input_index = 0; - let branch_id = Blossom - .branch_id() - .expect("Blossom has a ConsensusBranchId"); - verifier.is_valid(branch_id, input_index + 1).unwrap_err(); - - verifier.is_valid(branch_id, input_index + 1).unwrap_err(); + verifier.is_valid(NetworkUpgrade::Blossom, input_index + 1)?; + verifier.is_valid(NetworkUpgrade::Blossom, input_index + 1)?; Ok(()) } @@ -471,12 +451,15 @@ mod tests { let serialized_output = "4065675c0000000017a914c117756dcbe144a12a7c33a77cfa81aa5aeeb38187"; let tx = Transaction::zcash_deserialize(&hex::decode(serialized_tx).unwrap().to_vec()[..]) .unwrap(); + let previous_output = Output::zcash_deserialize(&hex::decode(serialized_output).unwrap().to_vec()[..]) .unwrap(); let verifier = super::CachedFfiTransaction::new(Arc::new(tx), vec![previous_output]); - verifier.is_valid(Nu5.branch_id().unwrap(), 0)?; + + verifier.is_valid(NetworkUpgrade::Nu5, 0)?; + Ok(()) } } From 1a1c5479c1599e762011ebd58a80b2397b49c478 Mon Sep 17 00:00:00 2001 From: Marek Date: Wed, 22 Jan 2025 16:44:09 +0100 Subject: [PATCH 13/37] Simplify tx deserialization --- zebra-chain/src/serialization/error.rs | 10 ++++++++++ zebra-chain/src/transaction/serialize.rs | 7 +------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/zebra-chain/src/serialization/error.rs b/zebra-chain/src/serialization/error.rs index 17566548d1a..da6683d2a75 100644 --- a/zebra-chain/src/serialization/error.rs +++ b/zebra-chain/src/serialization/error.rs @@ -51,3 +51,13 @@ pub enum SerializationError { #[error("transaction balance is non-zero but doesn't have Sapling shielded spends or outputs")] BadTransactionBalance, } + +impl From for SerializationError { + fn from(value: crate::Error) -> Self { + match value { + crate::Error::InvalidConsensusBranchId => Self::Parse("invalid consensus branch id"), + crate::Error::Convertion(e) => Self::Io(e), + crate::Error::MissingNetworkUpgrade => Self::Parse("missing network upgrade"), + } + } +} diff --git a/zebra-chain/src/transaction/serialize.rs b/zebra-chain/src/transaction/serialize.rs index 47d1a4e4ad8..a8a6b8ed8e5 100644 --- a/zebra-chain/src/transaction/serialize.rs +++ b/zebra-chain/src/transaction/serialize.rs @@ -888,12 +888,7 @@ impl ZcashDeserialize for Transaction { // Denoted as `nConsensusBranchId` in the spec. // Convert it to a NetworkUpgrade let network_upgrade = - NetworkUpgrade::from_branch_id(limited_reader.read_u32::()?) - .ok_or({ - SerializationError::Parse( - "expected a valid network upgrade from the consensus branch id", - ) - })?; + NetworkUpgrade::try_from(limited_reader.read_u32::()?)?; // Denoted as `lock_time` in the spec. let lock_time = LockTime::zcash_deserialize(&mut limited_reader)?; From af43be036e9a11570ef6bd40e52f557a0b13f5a9 Mon Sep 17 00:00:00 2001 From: Marek Date: Wed, 22 Jan 2025 16:44:32 +0100 Subject: [PATCH 14/37] Rm `impl TryFrom<&Trans> for zp_tx::Trans` --- .../src/primitives/zcash_primitives.rs | 28 ------------------- 1 file changed, 28 deletions(-) diff --git a/zebra-chain/src/primitives/zcash_primitives.rs b/zebra-chain/src/primitives/zcash_primitives.rs index 1c70ea33576..cea4fbd9309 100644 --- a/zebra-chain/src/primitives/zcash_primitives.rs +++ b/zebra-chain/src/primitives/zcash_primitives.rs @@ -150,34 +150,6 @@ impl<'a> zp_tx::Authorization for PrecomputedAuth<'a> { // End of (mostly) copied code -impl TryFrom<&Transaction> for zp_tx::Transaction { - type Error = io::Error; - - /// Convert a Zebra transaction into a librustzcash one. - /// - /// # Panics - /// - /// If the transaction is not V5. (Currently there is no need for this - /// conversion for other versions.) - #[allow(clippy::unwrap_in_result)] - fn try_from(trans: &Transaction) -> Result { - let network_upgrade = match trans { - Transaction::V5 { - network_upgrade, .. - } => network_upgrade, - Transaction::V1 { .. } - | Transaction::V2 { .. } - | Transaction::V3 { .. } - | Transaction::V4 { .. } => panic!("Zebra only uses librustzcash for V5 transactions"), - }; - - convert_tx_to_librustzcash( - trans, - network_upgrade.branch_id().expect("V5 txs have branch IDs"), - ) - } -} - /// Convert a Zebra transparent::Output into a librustzcash one. impl TryFrom<&transparent::Output> for zp_tx::components::TxOut { type Error = io::Error; From 54d4eedfca1194bc7300e30e7b1885734cd6f93b Mon Sep 17 00:00:00 2001 From: Marek Date: Wed, 22 Jan 2025 16:45:28 +0100 Subject: [PATCH 15/37] Update tests --- zebra-chain/src/transaction/tests/vectors.rs | 92 ++++++++------------ zebra-consensus/src/transaction/tests.rs | 18 +--- 2 files changed, 38 insertions(+), 72 deletions(-) diff --git a/zebra-chain/src/transaction/tests/vectors.rs b/zebra-chain/src/transaction/tests/vectors.rs index 7daff649f3a..d6b607f25b5 100644 --- a/zebra-chain/src/transaction/tests/vectors.rs +++ b/zebra-chain/src/transaction/tests/vectors.rs @@ -320,7 +320,9 @@ fn empty_v5_librustzcash_round_trip() { let _init_guard = zebra_test::init(); let tx: &Transaction = &EMPTY_V5_TX; - let _alt_tx: zcash_primitives::transaction::Transaction = tx.try_into().expect( + let nu = tx.network_upgrade().expect("network upgrade"); + + tx.to_librustzcash(nu).expect( "librustzcash deserialization might work for empty zebra serialized transactions. \ Hint: if empty transactions fail, but other transactions work, delete this test", ); @@ -417,9 +419,10 @@ fn fake_v5_librustzcash_round_trip_for_network(network: Network) { "v1-v4 transaction data must change when converted to fake v5" ); - let _alt_tx: zcash_primitives::transaction::Transaction = fake_tx - .as_ref() - .try_into() + let nu = fake_tx.network_upgrade().expect("network upgrade"); + + fake_tx + .to_librustzcash(nu) .expect("librustzcash deserialization must work for zebra serialized transactions"); } } @@ -430,14 +433,14 @@ fn zip244_round_trip() -> Result<()> { let _init_guard = zebra_test::init(); for test in zip0244::TEST_VECTORS.iter() { - let transaction = test.tx.zcash_deserialize_into::()?; - let reencoded = transaction.zcash_serialize_to_vec()?; + let tx = test.tx.zcash_deserialize_into::()?; + let reencoded = tx.zcash_serialize_to_vec()?; + assert_eq!(test.tx, reencoded); - // The borrow is actually needed to call the correct trait impl - #[allow(clippy::needless_borrow)] - let _alt_tx: zcash_primitives::transaction::Transaction = (&transaction) - .try_into() + let nu = tx.network_upgrade().expect("network upgrade"); + + tx.to_librustzcash(nu) .expect("librustzcash deserialization must work for zebra serialized transactions"); } @@ -449,9 +452,10 @@ fn zip244_txid() -> Result<()> { let _init_guard = zebra_test::init(); for test in zip0244::TEST_VECTORS.iter() { - let transaction = test.tx.zcash_deserialize_into::()?; - let hasher = TxIdBuilder::new(&transaction); - let txid = hasher.txid()?; + let txid = TxIdBuilder::new(&test.tx.zcash_deserialize_into::()?) + .txid() + .expect("txid"); + assert_eq!(txid.0, test.txid); } @@ -482,11 +486,7 @@ fn test_vec143_1() -> Result<()> { let transaction = ZIP143_1.zcash_deserialize_into::()?; - let hasher = SigHasher::new( - &transaction, - NetworkUpgrade::Overwinter.branch_id().unwrap(), - &[], - ); + let hasher = SigHasher::new(&transaction, NetworkUpgrade::Overwinter, &[]); let hash = hasher.sighash(HashType::ALL, None); let expected = "a1f1a4e5cd9bd522322d661edd2af1bf2a7019cfab94ece18f4ba935b0a19073"; @@ -520,7 +520,7 @@ fn test_vec143_2() -> Result<()> { let hasher = SigHasher::new( &transaction, - NetworkUpgrade::Overwinter.branch_id().unwrap(), + NetworkUpgrade::Overwinter, &all_previous_outputs, ); @@ -549,11 +549,7 @@ fn test_vec243_1() -> Result<()> { let transaction = ZIP243_1.zcash_deserialize_into::()?; - let hasher = SigHasher::new( - &transaction, - NetworkUpgrade::Sapling.branch_id().unwrap(), - &[], - ); + let hasher = SigHasher::new(&transaction, NetworkUpgrade::Sapling, &[]); let hash = hasher.sighash(HashType::ALL, None); let expected = "63d18534de5f2d1c9e169b73f9c783718adbef5c8a7d55b5e7a37affa1dd3ff3"; @@ -567,11 +563,7 @@ fn test_vec243_1() -> Result<()> { let _guard = span.enter(); assert_eq!(expected, result); - let precomputed_tx_data = PrecomputedTxData::new( - &transaction, - NetworkUpgrade::Sapling.branch_id().unwrap(), - &[], - ); + let precomputed_tx_data = PrecomputedTxData::new(&transaction, NetworkUpgrade::Sapling, &[]); let alt_sighash = crate::primitives::zcash_primitives::sighash(&precomputed_tx_data, HashType::ALL, None); let result = hex::encode(alt_sighash); @@ -595,11 +587,7 @@ fn test_vec243_2() -> Result<()> { }; let all_previous_outputs = mock_pre_v5_output_list(output, input_ind); - let hasher = SigHasher::new( - &transaction, - NetworkUpgrade::Sapling.branch_id().unwrap(), - &all_previous_outputs, - ); + let hasher = SigHasher::new(&transaction, NetworkUpgrade::Sapling, &all_previous_outputs); let hash = hasher.sighash( HashType::NONE, @@ -624,11 +612,8 @@ fn test_vec243_2() -> Result<()> { let index = input_ind; let all_previous_outputs = mock_pre_v5_output_list(prevout, input_ind); - let precomputed_tx_data = PrecomputedTxData::new( - &transaction, - NetworkUpgrade::Sapling.branch_id().unwrap(), - &all_previous_outputs, - ); + let precomputed_tx_data = + PrecomputedTxData::new(&transaction, NetworkUpgrade::Sapling, &all_previous_outputs); let alt_sighash = crate::primitives::zcash_primitives::sighash( &precomputed_tx_data, HashType::NONE, @@ -656,11 +641,7 @@ fn test_vec243_3() -> Result<()> { lock_script: lock_script.clone(), }]; - let hasher = SigHasher::new( - &transaction, - NetworkUpgrade::Sapling.branch_id().unwrap(), - &all_previous_outputs, - ); + let hasher = SigHasher::new(&transaction, NetworkUpgrade::Sapling, &all_previous_outputs); let hash = hasher.sighash( HashType::ALL, @@ -687,11 +668,8 @@ fn test_vec243_3() -> Result<()> { let index = input_ind; let all_previous_outputs = &[prevout]; - let precomputed_tx_data = PrecomputedTxData::new( - &transaction, - NetworkUpgrade::Sapling.branch_id().unwrap(), - all_previous_outputs, - ); + let precomputed_tx_data = + PrecomputedTxData::new(&transaction, NetworkUpgrade::Sapling, all_previous_outputs); let alt_sighash = crate::primitives::zcash_primitives::sighash( &precomputed_tx_data, HashType::ALL, @@ -724,7 +702,7 @@ fn zip143_sighash() -> Result<()> { None => vec![], }; let result = hex::encode(transaction.sighash( - ConsensusBranchId(test.consensus_branch_id), + NetworkUpgrade::try_from(test.consensus_branch_id).expect("network upgrade"), HashType::from_bits(test.hash_type).expect("must be a valid HashType"), &all_previous_outputs, input_index.map(|input_index| { @@ -762,7 +740,7 @@ fn zip243_sighash() -> Result<()> { None => vec![], }; let result = hex::encode(transaction.sighash( - ConsensusBranchId(test.consensus_branch_id), + NetworkUpgrade::try_from(test.consensus_branch_id).expect("network upgrade"), HashType::from_bits(test.hash_type).expect("must be a valid HashType"), &all_previous_outputs, input_index.map(|input_index| { @@ -797,7 +775,7 @@ fn zip244_sighash() -> Result<()> { .collect(); let result = hex::encode(transaction.sighash( - NetworkUpgrade::Nu5.branch_id().unwrap(), + NetworkUpgrade::Nu5, HashType::ALL, &all_previous_outputs, None, @@ -808,7 +786,7 @@ fn zip244_sighash() -> Result<()> { if let Some(sighash_all) = test.sighash_all { let result = hex::encode( transaction.sighash( - NetworkUpgrade::Nu5.branch_id().unwrap(), + NetworkUpgrade::Nu5, HashType::ALL, &all_previous_outputs, test.transparent_input @@ -840,9 +818,7 @@ fn binding_signatures() { .block_iter() .skip_while(|(height, _)| **height < sapling_activation_height) { - let branch_id = NetworkUpgrade::current(&net, Height(*height)) - .branch_id() - .expect("consensus branch ID"); + let nu = NetworkUpgrade::current(&net, Height(*height)); for tx in block .zcash_deserialize_into::() @@ -856,7 +832,7 @@ fn binding_signatures() { .. } => { if let Some(sapling_shielded_data) = sapling_shielded_data { - let sighash = tx.sighash(branch_id, HashType::ALL, &[], None); + let sighash = tx.sighash(nu, HashType::ALL, &[], None); let bvk = redjubjub::VerificationKey::try_from( sapling_shielded_data.binding_verification_key(), @@ -885,7 +861,7 @@ fn binding_signatures() { continue; } - let sighash = tx.sighash(branch_id, HashType::ALL, &[], None); + let sighash = tx.sighash(nu, HashType::ALL, &[], None); let bvk = redjubjub::VerificationKey::try_from( sapling_shielded_data.binding_verification_key(), diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index d0249975042..550c8a97426 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -1777,7 +1777,7 @@ fn v4_transaction_with_conflicting_sprout_nullifier_inside_joinsplit_is_rejected let _init_guard = zebra_test::init(); zebra_test::MULTI_THREADED_RUNTIME.block_on(async { let network = Network::Mainnet; - let network_upgrade = NetworkUpgrade::Canopy; + let nu = NetworkUpgrade::Canopy; let canopy_activation_height = NetworkUpgrade::Canopy .activation_height(&network) @@ -1804,12 +1804,7 @@ fn v4_transaction_with_conflicting_sprout_nullifier_inside_joinsplit_is_rejected }; // Sign the transaction - let sighash = transaction.sighash( - network_upgrade.branch_id().expect("must have branch ID"), - HashType::ALL, - &[], - None, - ); + let sighash = transaction.sighash(nu, HashType::ALL, &[], None); match &mut transaction { Transaction::V4 { @@ -1850,7 +1845,7 @@ fn v4_transaction_with_conflicting_sprout_nullifier_across_joinsplits_is_rejecte let _init_guard = zebra_test::init(); zebra_test::MULTI_THREADED_RUNTIME.block_on(async { let network = Network::Mainnet; - let network_upgrade = NetworkUpgrade::Canopy; + let nu = NetworkUpgrade::Canopy; let canopy_activation_height = NetworkUpgrade::Canopy .activation_height(&network) @@ -1883,12 +1878,7 @@ fn v4_transaction_with_conflicting_sprout_nullifier_across_joinsplits_is_rejecte }; // Sign the transaction - let sighash = transaction.sighash( - network_upgrade.branch_id().expect("must have branch ID"), - HashType::ALL, - &[], - None, - ); + let sighash = transaction.sighash(nu, HashType::ALL, &[], None); match &mut transaction { Transaction::V4 { From b5699743e0b69911c06df3ba458480934e806ffb Mon Sep 17 00:00:00 2001 From: Marek Date: Wed, 22 Jan 2025 17:55:45 +0100 Subject: [PATCH 16/37] Update tests --- zebra-consensus/src/transaction/tests.rs | 73 +++++++++--------------- 1 file changed, 28 insertions(+), 45 deletions(-) diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 550c8a97426..41abce82c42 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -3108,53 +3108,36 @@ async fn v5_consensus_branch_ids_sighash() { // need to disable some additional checks to let the verifier reach the binding sig // verification. - let block_verification_result = verifier - .clone() - .oneshot(block_req_with( - NetworkUpgrade::Nu6, - Some(HashSet::from_iter([ - SkipCheck::ConsensusBranchId, - SkipCheck::ExpiryHeight, - ])), - )) - .map_ok(|rsp| rsp.tx_id()) - .map_err(|e| format!("{e}")) - .await; - - assert_eq!(block_verification_result, Ok(tx_id)); - - let verifier_req = verifier - .clone() - .oneshot(mempool_req_with( - NetworkUpgrade::Nu6, - Some(HashSet::from_iter([ - SkipCheck::ConsensusBranchId, - SkipCheck::ExpiryHeight, - // We need to skip ZIP 317 mempool checks because we don't have a Testnet V5 tx - // in our test vectors without unpaid actions. - // - // TODO: Add at least one V5 tx with no unpaid actions to Testnet test vectors. - SkipCheck::Zip317, - ])), - )) - .map_ok(|rsp| rsp.tx_id()) - .map_err(|e| format!("{e}")); - - let state_req = async { - state - .expect_request_that(|req| { - matches!( - req, - zebra_state::Request::CheckBestChainTipNullifiersAndAnchors(_) - ) - }) - .map(|r| r.respond(zebra_state::Response::ValidBestChainTipNullifiersAndAnchors)) - .await; - }; + // Try a block request under NU6. + + std::panic::AssertUnwindSafe(verifier.clone().oneshot(block_req_with( + NetworkUpgrade::Nu6, + Some(HashSet::from_iter([ + SkipCheck::ConsensusBranchId, + SkipCheck::ExpiryHeight, + ])), + ))) + .catch_unwind() + .await + .expect_err("SIGHASH computation should panic due to an invalid consenus branch id in tx"); - let (_, mempool_verification_result) = futures::join!(state_req, verifier_req); + // Try a mempool request under NU6. - assert_eq!(mempool_verification_result, Ok(tx_id)); + std::panic::AssertUnwindSafe(verifier.clone().oneshot(mempool_req_with( + NetworkUpgrade::Nu6, + Some(HashSet::from_iter([ + SkipCheck::ConsensusBranchId, + SkipCheck::ExpiryHeight, + // We need to skip ZIP 317 mempool checks because we don't have a Testnet V5 tx + // in our test vectors without unpaid actions. + // + // TODO: Add at least one V5 tx with no unpaid actions to Testnet test vectors. + SkipCheck::Zip317, + ])), + ))) + .catch_unwind() + .await + .expect_err("SIGHASH computation should panic due to an invalid consenus branch id tx"); } } From 034528eb2ccbbc12d7c1a01d7a8e2e95127294e9 Mon Sep 17 00:00:00 2001 From: Marek Date: Thu, 23 Jan 2025 00:53:43 +0100 Subject: [PATCH 17/37] Add docs for `to_librustzcash` --- zebra-chain/src/transaction.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index 007e2a312f9..aca3bb3c6c1 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -1230,6 +1230,10 @@ impl Transaction { } /// Converts [`Transaction`] to [`zcash_primitives::transaction::Transaction`]. + /// + /// If the tx contains a network upgrade, this network upgrade must match the passed `nu`. The + /// passed `nu` must also contain a consensus branch id convertible to its `librustzcash` + /// equivalent. pub(crate) fn to_librustzcash( &self, nu: NetworkUpgrade, From 1ea4fb0f14633fb77fd8815dcdb881fb38ad8cce Mon Sep 17 00:00:00 2001 From: Marek Date: Thu, 23 Jan 2025 00:57:03 +0100 Subject: [PATCH 18/37] Update docs for `PrecomputedTxData::new` --- zebra-chain/src/primitives/zcash_primitives.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/zebra-chain/src/primitives/zcash_primitives.rs b/zebra-chain/src/primitives/zcash_primitives.rs index cea4fbd9309..f106f0a268b 100644 --- a/zebra-chain/src/primitives/zcash_primitives.rs +++ b/zebra-chain/src/primitives/zcash_primitives.rs @@ -209,14 +209,17 @@ pub(crate) struct PrecomputedTxData<'a> { } impl<'a> PrecomputedTxData<'a> { - /// Compute data used for sighash or txid computation. + /// Computes the data used for sighash or txid computation. /// /// # Inputs /// - /// - `tx`: the relevant transaction - /// - `branch_id`: the branch ID of the transaction - /// - `all_previous_outputs` the transparent Output matching each - /// transparent input in the transaction. + /// - `tx`: the relevant transaction. + /// - `nu`: the network upgrade to which the transaction belongs. + /// - `all_previous_outputs`: the transparent Output matching each transparent input in `tx`. + /// + /// # Panics + /// + /// - If `tx` can't be converted to its `librustzcash` equivalent. pub(crate) fn new( tx: &'a Transaction, nu: NetworkUpgrade, From f8cebad79411793c7b8c8a9f2f5771fc0d5c4a49 Mon Sep 17 00:00:00 2001 From: Marek Date: Thu, 23 Jan 2025 01:28:21 +0100 Subject: [PATCH 19/37] Document the SIGHASH consensus rules we missed --- .../src/primitives/zcash_primitives.rs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/zebra-chain/src/primitives/zcash_primitives.rs b/zebra-chain/src/primitives/zcash_primitives.rs index f106f0a268b..21ffeead609 100644 --- a/zebra-chain/src/primitives/zcash_primitives.rs +++ b/zebra-chain/src/primitives/zcash_primitives.rs @@ -220,6 +220,26 @@ impl<'a> PrecomputedTxData<'a> { /// # Panics /// /// - If `tx` can't be converted to its `librustzcash` equivalent. + /// + /// # Consensus + /// + /// > [NU5 only, pre-NU6] All transactions MUST use the NU5 consensus branch ID `0xF919A198` as + /// > defined in [ZIP-252]. + /// + /// > [NU6 only] All transactions MUST use the NU6 consensus branch ID `0xC8E71055` as defined + /// > in [ZIP-253]. + /// + /// # Notes + /// + /// The check that ensures compliance with the two consensus rules stated above takes place in + /// the [`Transaction::to_librustzcash`] method. If the check fails, the tx can't be converted + /// to its `librustzcash` equivalent, which leads to a panic. The check relies on the passed + /// `nu` parameter, which uniquely represents a consensus branch id and can, therefore, be used + /// as an equivalent to a consensus branch id. The desired `nu` is set either by the script or + /// tx verifier in `zebra-consensus`. + /// + /// [ZIP-252]: + /// [ZIP-253]: pub(crate) fn new( tx: &'a Transaction, nu: NetworkUpgrade, From 645d6665528c295cc55f3e19dd190c10b9f0993a Mon Sep 17 00:00:00 2001 From: Marek Date: Thu, 23 Jan 2025 01:34:38 +0100 Subject: [PATCH 20/37] Update docs for script validation --- zebra-script/src/lib.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/zebra-script/src/lib.rs b/zebra-script/src/lib.rs index 1a51fe3455b..404f2470c20 100644 --- a/zebra-script/src/lib.rs +++ b/zebra-script/src/lib.rs @@ -152,9 +152,8 @@ impl CachedFfiTransaction { &self.all_previous_outputs } - /// Verify if the script in the input at `input_index` of a transaction correctly - /// spends the matching [`transparent::Output`] it refers to, with the [`ConsensusBranchId`] - /// of the block containing the transaction. + /// Verify if the script in the input at `input_index` of a transaction correctly spends the + /// matching [`transparent::Output`] it refers to. #[allow(clippy::unwrap_in_result)] pub fn is_valid(&self, nu: NetworkUpgrade, input_index: usize) -> Result<(), Error> { let previous_output = self From 05a9b6171ccbc06bcc888470e4be8a60b1d574ef Mon Sep 17 00:00:00 2001 From: Marek Date: Fri, 24 Jan 2025 13:28:53 +0100 Subject: [PATCH 21/37] Fix script verification tests In a previous commit, I erroneously edited the tests so that they'd expect `Ok`s instead of `Err`s. This commit fixes that. --- zebra-script/src/lib.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/zebra-script/src/lib.rs b/zebra-script/src/lib.rs index 404f2470c20..a223f279e92 100644 --- a/zebra-script/src/lib.rs +++ b/zebra-script/src/lib.rs @@ -344,7 +344,9 @@ mod tests { }; let input_index = 0; let verifier = super::CachedFfiTransaction::new(transaction, vec![output]); - verifier.is_valid(NetworkUpgrade::Blossom, input_index)?; + verifier + .is_valid(NetworkUpgrade::Blossom, input_index) + .expect_err("verification should fail"); Ok(()) } @@ -390,7 +392,9 @@ mod tests { let input_index = 0; verifier.is_valid(NetworkUpgrade::Blossom, input_index)?; - verifier.is_valid(NetworkUpgrade::Blossom, input_index + 1)?; + verifier + .is_valid(NetworkUpgrade::Blossom, input_index + 1) + .expect_err("verification should fail"); Ok(()) } @@ -412,7 +416,9 @@ mod tests { let input_index = 0; - verifier.is_valid(NetworkUpgrade::Blossom, input_index + 1)?; + verifier + .is_valid(NetworkUpgrade::Blossom, input_index + 1) + .expect_err("verification should fail"); verifier.is_valid(NetworkUpgrade::Blossom, input_index)?; Ok(()) @@ -435,8 +441,12 @@ mod tests { let input_index = 0; - verifier.is_valid(NetworkUpgrade::Blossom, input_index + 1)?; - verifier.is_valid(NetworkUpgrade::Blossom, input_index + 1)?; + verifier + .is_valid(NetworkUpgrade::Blossom, input_index + 1) + .expect_err("verification should fail"); + verifier + .is_valid(NetworkUpgrade::Blossom, input_index + 1) + .expect_err("verification should fail"); Ok(()) } From 1836375cc351ab002ed783f508ad3d2cbc6388f1 Mon Sep 17 00:00:00 2001 From: Marek Date: Fri, 24 Jan 2025 15:06:43 +0100 Subject: [PATCH 22/37] Fix spelling --- book/src/dev/rfcs/0003-inventory-tracking.md | 2 +- book/src/dev/rfcs/0006-contextual-difficulty.md | 4 ++-- book/src/dev/rfcs/drafts/0005-treestate.md | 2 +- zebra-chain/src/error.rs | 2 +- zebra-chain/src/serialization/error.rs | 2 +- zebra-network/src/peer_set/initialize/tests/vectors.rs | 2 +- zebra-test/src/net.rs | 4 ++-- zebrad/src/commands/copy_state.rs | 2 +- zebrad/src/components/sync.rs | 2 +- zebrad/tests/common/test_type.rs | 2 +- 10 files changed, 12 insertions(+), 12 deletions(-) diff --git a/book/src/dev/rfcs/0003-inventory-tracking.md b/book/src/dev/rfcs/0003-inventory-tracking.md index ff7bac4d18a..a562843070f 100644 --- a/book/src/dev/rfcs/0003-inventory-tracking.md +++ b/book/src/dev/rfcs/0003-inventory-tracking.md @@ -192,7 +192,7 @@ specific inventory request is ready, because until we get the request, we can't determine which peers might be required to process it. We could attempt to ensure that the peer set would be ready to process a -specific inventory request would be to pre-emptively "reserve" a peer as soon +specific inventory request would be to preemptively "reserve" a peer as soon as it advertises an inventory item. But this doesn't actually work to ensure readiness, because a peer could advertise two inventory items, and only be able to service one request at a time. It also potentially locks the peer diff --git a/book/src/dev/rfcs/0006-contextual-difficulty.md b/book/src/dev/rfcs/0006-contextual-difficulty.md index accc8f00332..b23fbab8c24 100644 --- a/book/src/dev/rfcs/0006-contextual-difficulty.md +++ b/book/src/dev/rfcs/0006-contextual-difficulty.md @@ -753,10 +753,10 @@ would be a security issue. # Future possibilities [future-possibilities]: #future-possibilities -## Re-using the relevant chain API in other contextual checks +## Reusing the relevant chain API in other contextual checks [relevant-chain-api-reuse]: #relevant-chain-api-reuse -The relevant chain iterator can be re-used to implement other contextual +The relevant chain iterator can be reused to implement other contextual validation checks. For example, responding to peer requests for block locators, which means diff --git a/book/src/dev/rfcs/drafts/0005-treestate.md b/book/src/dev/rfcs/drafts/0005-treestate.md index 67ad62262e8..d18654375dc 100644 --- a/book/src/dev/rfcs/drafts/0005-treestate.md +++ b/book/src/dev/rfcs/drafts/0005-treestate.md @@ -126,7 +126,7 @@ finished validating everything that can be validated without the context of their anchor's finalization state. So for each transaction, for both `Spend` descriptions and `JoinSplit`s, we can -pre-emptively try to do our consensus check by looking up the anchors in our +preemptively try to do our consensus check by looking up the anchors in our finalized set first. For `Spend`s, we then trigger the remaining validation and when that finishes we are full done with those. For `JoinSplit`s, the anchor state check may pass early if it's a previous block Sprout `NoteCommitment` tree diff --git a/zebra-chain/src/error.rs b/zebra-chain/src/error.rs index 543d081131b..755e508e402 100644 --- a/zebra-chain/src/error.rs +++ b/zebra-chain/src/error.rs @@ -65,7 +65,7 @@ pub enum Error { /// Zebra's type could not be converted to its librustzcash equivalent. #[error("Zebra's type could not be converted to its librustzcash equivalent: ")] - Convertion(#[from] io::Error), + Conversion(#[from] io::Error), /// The transaction is missing a network upgrade. #[error("the transaction is missing a network upgrade")] diff --git a/zebra-chain/src/serialization/error.rs b/zebra-chain/src/serialization/error.rs index da6683d2a75..411e2649537 100644 --- a/zebra-chain/src/serialization/error.rs +++ b/zebra-chain/src/serialization/error.rs @@ -56,7 +56,7 @@ impl From for SerializationError { fn from(value: crate::Error) -> Self { match value { crate::Error::InvalidConsensusBranchId => Self::Parse("invalid consensus branch id"), - crate::Error::Convertion(e) => Self::Io(e), + crate::Error::Conversion(e) => Self::Io(e), crate::Error::MissingNetworkUpgrade => Self::Parse("missing network upgrade"), } } diff --git a/zebra-network/src/peer_set/initialize/tests/vectors.rs b/zebra-network/src/peer_set/initialize/tests/vectors.rs index 8568c7ea13a..6a53b2ed0ab 100644 --- a/zebra-network/src/peer_set/initialize/tests/vectors.rs +++ b/zebra-network/src/peer_set/initialize/tests/vectors.rs @@ -1575,7 +1575,7 @@ where let nil_peer_set = service_fn(move |req| async move { let rsp = match req { // Return the correct response variant for Peers requests, - // re-using one of the peers we already provided. + // reusing one of the peers we already provided. Request::Peers => Response::Peers(vec![fake_peer.unwrap()]), _ => unreachable!("unexpected request: {:?}", req), }; diff --git a/zebra-test/src/net.rs b/zebra-test/src/net.rs index 161a5ce80aa..1b66ff466c4 100644 --- a/zebra-test/src/net.rs +++ b/zebra-test/src/net.rs @@ -58,7 +58,7 @@ pub fn zebra_skip_ipv6_tests() -> bool { /// to - it has a small risk of port conflicts. /// /// Use this function when you need to use the same random port multiple -/// times. For example: setting up both ends of a connection, or re-using +/// times. For example: setting up both ends of a connection, or reusing /// the same port multiple times. pub fn random_known_port() -> u16 { use rand::Rng; @@ -99,7 +99,7 @@ pub fn random_known_port() -> u16 { /// between this fn call and binding the tcp listener. /// /// Use this function when you need to use the same random port multiple -/// times. For example: setting up both ends of a connection, or re-using +/// times. For example: setting up both ends of a connection, or reusing /// the same port multiple times. /// /// ## Panics diff --git a/zebrad/src/commands/copy_state.rs b/zebrad/src/commands/copy_state.rs index 56e7eea6263..a285209b62d 100644 --- a/zebrad/src/commands/copy_state.rs +++ b/zebrad/src/commands/copy_state.rs @@ -280,7 +280,7 @@ impl CopyStateCmd { // then deserializes bytes into new `Block` structs when reading. // So these checks are sufficient to detect block data corruption. // - // If Zebra starts re-using cached `Block` structs after writing them, + // If Zebra starts reusing cached `Block` structs after writing them, // we'll also need to check `Block` structs created from the actual database bytes. if source_block_hash != target_block_commit_hash || source_block_hash != target_block_data_hash diff --git a/zebrad/src/components/sync.rs b/zebrad/src/components/sync.rs index 06ab0b70cef..bc35656dcc3 100644 --- a/zebrad/src/components/sync.rs +++ b/zebrad/src/components/sync.rs @@ -463,7 +463,7 @@ where // The Hedge middleware is the outermost layer, hedging requests // between two retry-wrapped networks. The innermost timeout // layer is relatively unimportant, because slow requests will - // probably be pre-emptively hedged. + // probably be preemptively hedged. // // The Hedge goes outside the Retry, because the Retry layer // abstracts away spurious failures from individual peers diff --git a/zebrad/tests/common/test_type.rs b/zebrad/tests/common/test_type.rs index bc0cd8ef417..5621b12b690 100644 --- a/zebrad/tests/common/test_type.rs +++ b/zebrad/tests/common/test_type.rs @@ -200,7 +200,7 @@ impl TestType { if !use_internet_connection { config.network.initial_mainnet_peers = IndexSet::new(); config.network.initial_testnet_peers = IndexSet::new(); - // Avoid re-using cached peers from disk when we're supposed to be a disconnected instance + // Avoid reusing cached peers from disk when we're supposed to be a disconnected instance config.network.cache_dir = CacheDir::disabled(); // Activate the mempool immediately by default From 054e2c61bc62ee7e4d3dab8e1baf77d7b2c589bc Mon Sep 17 00:00:00 2001 From: Marek Date: Mon, 27 Jan 2025 14:12:11 +0100 Subject: [PATCH 23/37] Impl `NetworkUpgrade::iter()` --- zebra-chain/src/parameters/network_upgrade.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/zebra-chain/src/parameters/network_upgrade.rs b/zebra-chain/src/parameters/network_upgrade.rs index 2485423d417..0ce085e8729 100644 --- a/zebra-chain/src/parameters/network_upgrade.rs +++ b/zebra-chain/src/parameters/network_upgrade.rs @@ -538,6 +538,11 @@ impl NetworkUpgrade { ) -> Duration { NetworkUpgrade::current(network, height).averaging_window_timespan() } + + /// Returns an iterator over [`NetworkUpgrade`] variants. + pub fn iter() -> impl Iterator { + NETWORK_UPGRADES_IN_ORDER.into_iter() + } } impl From for NetworkUpgrade { From 9ec61fb95289e3cb379bd841b658586455b2e97b Mon Sep 17 00:00:00 2001 From: Marek Date: Mon, 27 Jan 2025 14:14:38 +0100 Subject: [PATCH 24/37] Refactor `Network_upgrade::next_upgrade` --- zebra-chain/src/parameters/network_upgrade.rs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/zebra-chain/src/parameters/network_upgrade.rs b/zebra-chain/src/parameters/network_upgrade.rs index 0ce085e8729..db921a10ce7 100644 --- a/zebra-chain/src/parameters/network_upgrade.rs +++ b/zebra-chain/src/parameters/network_upgrade.rs @@ -350,17 +350,10 @@ impl NetworkUpgrade { /// Returns the next expected network upgrade after this network upgrade pub fn next_upgrade(self) -> Option { - match self { - Genesis => Some(BeforeOverwinter), - BeforeOverwinter => Some(Overwinter), - Overwinter => Some(Sapling), - Sapling => Some(Blossom), - Blossom => Some(Heartwood), - Heartwood => Some(Canopy), - Canopy => Some(Nu5), - Nu5 => Some(Nu6), - Nu6 => None, - } + Self::iter() + .position(|nu| self == nu) + .filter(|&p| (p + 1) < NETWORK_UPGRADES_IN_ORDER.len()) + .map(|p| NETWORK_UPGRADES_IN_ORDER[p + 1]) } /// Returns the next network upgrade for `network` and `height`. From e518fee850d135486890c23de2f4456054ac3d01 Mon Sep 17 00:00:00 2001 From: Marek Date: Mon, 27 Jan 2025 14:33:26 +0100 Subject: [PATCH 25/37] Impl `NetworkUpgrade::previous_upgrade` --- zebra-chain/src/parameters/network_upgrade.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/zebra-chain/src/parameters/network_upgrade.rs b/zebra-chain/src/parameters/network_upgrade.rs index db921a10ce7..690f9296e8d 100644 --- a/zebra-chain/src/parameters/network_upgrade.rs +++ b/zebra-chain/src/parameters/network_upgrade.rs @@ -356,6 +356,14 @@ impl NetworkUpgrade { .map(|p| NETWORK_UPGRADES_IN_ORDER[p + 1]) } + /// Returns the previous network before after this network upgrade + pub fn previous_upgrade(self) -> Option { + Self::iter() + .position(|nu| self == nu) + .filter(|&p| p > 0) + .map(|p| NETWORK_UPGRADES_IN_ORDER[p - 1]) + } + /// Returns the next network upgrade for `network` and `height`. /// /// Returns None if the next upgrade has not been implemented in Zebra From a338b5fa69c69651b28fc9d1bf83b73bdf5a80f6 Mon Sep 17 00:00:00 2001 From: Marek Date: Mon, 27 Jan 2025 14:34:57 +0100 Subject: [PATCH 26/37] Impl `Transaction::hash_shielded_data` --- 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 aca3bb3c6c1..f7cc72045fc 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -1255,6 +1255,13 @@ impl Transaction { branch_id, )?) } + + // Common Sapling & Orchard Properties + + /// Does this transaction have shielded inputs or outputs? + pub fn has_shielded_data(&self) -> bool { + self.has_shielded_inputs() || self.has_shielded_outputs() + } } #[cfg(any(test, feature = "proptest-impl"))] From 9ba7045f493cddde74c868e905e0c66ce0f35dfe Mon Sep 17 00:00:00 2001 From: Marek Date: Mon, 27 Jan 2025 14:36:12 +0100 Subject: [PATCH 27/37] Don't make `NETWORK_UPGRADES_IN_ORDER` `pub` --- zebra-chain/src/parameters/network/testnet.rs | 6 +++--- zebra-chain/src/parameters/network/tests/vectors.rs | 11 +++++------ zebra-chain/src/parameters/network_upgrade.rs | 2 +- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/zebra-chain/src/parameters/network/testnet.rs b/zebra-chain/src/parameters/network/testnet.rs index 1f77e95e750..6045a7e2581 100644 --- a/zebra-chain/src/parameters/network/testnet.rs +++ b/zebra-chain/src/parameters/network/testnet.rs @@ -7,7 +7,7 @@ use crate::{ constants::{magics, SLOW_START_INTERVAL, SLOW_START_SHIFT}, network_upgrade::TESTNET_ACTIVATION_HEIGHTS, subsidy::{funding_stream_address_period, FUNDING_STREAM_RECEIVER_DENOMINATOR}, - Network, NetworkKind, NetworkUpgrade, NETWORK_UPGRADES_IN_ORDER, + Network, NetworkKind, NetworkUpgrade, }, work::difficulty::{ExpandedDifficulty, U256}, }; @@ -369,7 +369,7 @@ impl ParametersBuilder { // Check that the provided network upgrade activation heights are in the same order by height as the default testnet activation heights let mut activation_heights_iter = activation_heights.iter(); - for expected_network_upgrade in NETWORK_UPGRADES_IN_ORDER { + for expected_network_upgrade in NetworkUpgrade::iter() { if !network_upgrades.contains(&expected_network_upgrade) { continue; } else if let Some((&height, &network_upgrade)) = activation_heights_iter.next() { @@ -381,7 +381,7 @@ impl ParametersBuilder { assert!( network_upgrade == expected_network_upgrade, - "network upgrades must be activated in order, the correct order is {NETWORK_UPGRADES_IN_ORDER:?}" + "network upgrades must be activated in order specified by the protocol" ); } } diff --git a/zebra-chain/src/parameters/network/tests/vectors.rs b/zebra-chain/src/parameters/network/tests/vectors.rs index 4282c86844f..8845090b50c 100644 --- a/zebra-chain/src/parameters/network/tests/vectors.rs +++ b/zebra-chain/src/parameters/network/tests/vectors.rs @@ -15,8 +15,7 @@ use crate::{ self, ConfiguredActivationHeights, ConfiguredFundingStreamRecipient, ConfiguredFundingStreams, MAX_NETWORK_NAME_LENGTH, RESERVED_NETWORK_NAMES, }, - Network, NetworkUpgrade, MAINNET_ACTIVATION_HEIGHTS, NETWORK_UPGRADES_IN_ORDER, - TESTNET_ACTIVATION_HEIGHTS, + Network, NetworkUpgrade, MAINNET_ACTIVATION_HEIGHTS, TESTNET_ACTIVATION_HEIGHTS, }, }; @@ -124,7 +123,7 @@ fn activates_network_upgrades_correctly() { "activation height for all networks after Genesis and BeforeOverwinter should match NU5 activation height" ); - for nu in NETWORK_UPGRADES_IN_ORDER.into_iter().skip(1) { + for nu in NetworkUpgrade::iter().skip(1) { let activation_height = nu .activation_height(&network) .expect("must return an activation height"); @@ -286,8 +285,8 @@ fn check_full_activation_list() { }) .to_network(); - // We expect the first 8 network upgrades to be included, up to NU5 - let expected_network_upgrades = &NETWORK_UPGRADES_IN_ORDER[..8]; + // We expect the first 8 network upgrades to be included, up to and including NU5 + let expected_network_upgrades = NetworkUpgrade::iter().take(8); let full_activation_list_network_upgrades: Vec<_> = network .full_activation_list() .into_iter() @@ -296,7 +295,7 @@ fn check_full_activation_list() { for expected_network_upgrade in expected_network_upgrades { assert!( - full_activation_list_network_upgrades.contains(expected_network_upgrade), + full_activation_list_network_upgrades.contains(&expected_network_upgrade), "full activation list should contain expected network upgrade" ); } diff --git a/zebra-chain/src/parameters/network_upgrade.rs b/zebra-chain/src/parameters/network_upgrade.rs index 690f9296e8d..e0d52e65963 100644 --- a/zebra-chain/src/parameters/network_upgrade.rs +++ b/zebra-chain/src/parameters/network_upgrade.rs @@ -15,7 +15,7 @@ use hex::{FromHex, ToHex}; use proptest_derive::Arbitrary; /// A list of network upgrades in the order that they must be activated. -pub const NETWORK_UPGRADES_IN_ORDER: [NetworkUpgrade; 9] = [ +const NETWORK_UPGRADES_IN_ORDER: [NetworkUpgrade; 9] = [ Genesis, BeforeOverwinter, Overwinter, From efd290779a6b9dc2f255101cc71f7ae0eb16e77e Mon Sep 17 00:00:00 2001 From: Marek Date: Mon, 27 Jan 2025 14:36:56 +0100 Subject: [PATCH 28/37] Test `Transaction::sighash` with cons branch ids --- zebra-chain/src/transaction/tests/vectors.rs | 27 ++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/zebra-chain/src/transaction/tests/vectors.rs b/zebra-chain/src/transaction/tests/vectors.rs index d6b607f25b5..6315b686183 100644 --- a/zebra-chain/src/transaction/tests/vectors.rs +++ b/zebra-chain/src/transaction/tests/vectors.rs @@ -1,8 +1,10 @@ //! Fixed test vectors for transactions. +use arbitrary::v5_transactions; use chrono::DateTime; use color_eyre::eyre::Result; use lazy_static::lazy_static; +use rand::{seq::IteratorRandom, thread_rng}; use crate::{ block::{Block, Height, MAX_BLOCK_BYTES}, @@ -801,6 +803,31 @@ fn zip244_sighash() -> Result<()> { Ok(()) } +#[test] +fn consensus_branch_id_sighash() { + for net in Network::iter() { + for tx in v5_transactions(net.block_iter()).filter(|tx| { + !tx.has_transparent_inputs() && tx.has_shielded_data() && tx.network_upgrade().is_some() + }) { + let tx_nu = tx + .network_upgrade() + .expect("this test shouldn't use txs without a network upgrade"); + + let any_other_nu = NetworkUpgrade::iter() + .filter(|&nu| nu != tx_nu) + .choose(&mut thread_rng()) + .expect("there must be a network upgrade other than the tx one"); + + // The sighash computation should succeed under the correct nu. + tx.sighash(tx_nu, HashType::ALL, &[], None); + + // The sighash computation should panic under a wrong nu. + std::panic::catch_unwind(|| tx.sighash(any_other_nu, HashType::ALL, &[], None)) + .expect_err("the sighash computation should panic under a wrong network upgrade"); + } + } +} + #[test] fn binding_signatures() { let _init_guard = zebra_test::init(); From 62e7f0a24b657a247cd52e38b3f7b7b1aeae0028 Mon Sep 17 00:00:00 2001 From: Marek Date: Mon, 27 Jan 2025 15:40:43 +0100 Subject: [PATCH 29/37] Extend the `consensus_branch_id` test --- zebra-chain/src/transaction/tests/vectors.rs | 23 ++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/zebra-chain/src/transaction/tests/vectors.rs b/zebra-chain/src/transaction/tests/vectors.rs index 6315b686183..d8dd2e54097 100644 --- a/zebra-chain/src/transaction/tests/vectors.rs +++ b/zebra-chain/src/transaction/tests/vectors.rs @@ -804,7 +804,7 @@ fn zip244_sighash() -> Result<()> { } #[test] -fn consensus_branch_id_sighash() { +fn consensus_branch_id() { for net in Network::iter() { for tx in v5_transactions(net.block_iter()).filter(|tx| { !tx.has_transparent_inputs() && tx.has_shielded_data() && tx.network_upgrade().is_some() @@ -818,12 +818,27 @@ fn consensus_branch_id_sighash() { .choose(&mut thread_rng()) .expect("there must be a network upgrade other than the tx one"); - // The sighash computation should succeed under the correct nu. + // All computations should succeed under the tx nu. + + tx.to_librustzcash(tx_nu) + .expect("tx is convertible under tx nu"); + PrecomputedTxData::new(&tx, tx_nu, &[]); + sighash::SigHasher::new(&tx, tx_nu, &[]); tx.sighash(tx_nu, HashType::ALL, &[], None); - // The sighash computation should panic under a wrong nu. + // All computations should fail under an nu other than the tx one. + + tx.to_librustzcash(any_other_nu) + .expect_err("tx is not convertible under nu other than the tx one"); + + std::panic::catch_unwind(|| PrecomputedTxData::new(&tx, any_other_nu, &[])) + .expect_err("precomputing tx sighash data panics under nu other than the tx one"); + + std::panic::catch_unwind(|| sighash::SigHasher::new(&tx, any_other_nu, &[])) + .expect_err("creating the sighasher panics under nu other than the tx one"); + std::panic::catch_unwind(|| tx.sighash(any_other_nu, HashType::ALL, &[], None)) - .expect_err("the sighash computation should panic under a wrong network upgrade"); + .expect_err("the sighash computation panics under nu other than the tx one"); } } } From eff23110d66fb1c80697eed5aad0191416a5b9e5 Mon Sep 17 00:00:00 2001 From: Marek Date: Mon, 27 Jan 2025 17:52:43 +0100 Subject: [PATCH 30/37] Derive `Debug` for `SigHasher` --- zebra-chain/src/transaction/sighash.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zebra-chain/src/transaction/sighash.rs b/zebra-chain/src/transaction/sighash.rs index a45c79547a7..4738a77bc06 100644 --- a/zebra-chain/src/transaction/sighash.rs +++ b/zebra-chain/src/transaction/sighash.rs @@ -41,6 +41,7 @@ impl AsRef<[u8]> for SigHash { /// A SigHasher context which stores precomputed data that is reused /// between sighash computations for the same transaction. +#[derive(Debug)] pub struct SigHasher<'a> { precomputed_tx_data: PrecomputedTxData<'a>, } From 659afd2bdeac3009fbcafed35600adf061a42237 Mon Sep 17 00:00:00 2001 From: Marek Date: Mon, 27 Jan 2025 18:01:37 +0100 Subject: [PATCH 31/37] Remove the beautiful test for tx verifier --- zebra-consensus/src/transaction/tests.rs | 117 ----------------------- 1 file changed, 117 deletions(-) diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 41abce82c42..4039d052c47 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -3024,123 +3024,6 @@ async fn v5_consensus_branch_ids() { } } -/// Checks that the tx verifier rejects V5 txs with an invalid consensus branch ID in their SIGHASH. -#[tokio::test] -async fn v5_consensus_branch_ids_sighash() { - let mut state = MockService::build().for_unit_tests(); - - for net in Network::iter() { - let verifier = Buffer::new(Verifier::new_for_tests(&net, state.clone()), 10); - - let tx = v5_transactions(net.block_iter()) - .find(|tx| { - !tx.has_transparent_inputs() - && tx.has_sapling_shielded_data() - && tx.has_orchard_shielded_data() - && tx - .network_upgrade() - .is_some_and(|nu| nu == NetworkUpgrade::Nu5) - }) - .expect("NU5 V5 tx with only Orchard & Sapling shielded data"); - - let tx_id = tx.unmined_id(); - - let block_req_with = |nu: NetworkUpgrade, skip_checks| Request::Block { - transaction_hash: tx.hash(), - transaction: tx.clone().into(), - known_outpoint_hashes: Arc::new(HashSet::new()), - known_utxos: Arc::new(HashMap::new()), - height: nu.activation_height(&net).expect("activation height"), - time: DateTime::::MAX_UTC, - skip_checks, - }; - - let mempool_req_with = |nu: NetworkUpgrade, skip_checks| Request::Mempool { - transaction: tx.clone().into(), - height: nu.activation_height(&net).expect("activation height"), - skip_checks, - }; - - // The verification of the tx under NU5 should succeed since we picked an NU5 one. - - let block_verification_result = verifier - .clone() - .oneshot(block_req_with(NetworkUpgrade::Nu5, None)) - .map_ok(|rsp| rsp.tx_id()) - .map_err(|e| format!("{e}")) - .await; - - assert_eq!(block_verification_result, Ok(tx_id)); - - let verifier_req = verifier - .clone() - .oneshot(mempool_req_with( - NetworkUpgrade::Nu5, - // We need to skip ZIP 317 mempool checks because we don't have a Testnet V5 tx in - // our test vectors without unpaid actions. - // - // TODO: Add at least one V5 tx with no unpaid actions to Testnet test vectors. - Some(HashSet::from_iter([SkipCheck::Zip317])), - )) - .map_ok(|rsp| rsp.tx_id()) - .map_err(|e| format!("{e}")); - - let state_req = async { - state - .expect_request_that(|req| { - matches!( - req, - zebra_state::Request::CheckBestChainTipNullifiersAndAnchors(_) - ) - }) - .map(|r| r.respond(zebra_state::Response::ValidBestChainTipNullifiersAndAnchors)) - .await; - }; - - let (_, mempool_verification_result) = futures::join!(state_req, verifier_req); - - assert_eq!(mempool_verification_result, Ok(tx_id)); - - // The verification of the same tx under NU6 should not succeed due to an invalid binding - // sig since the tx's consensus branch ID field is part of the SIGHASH computation and is - // invalid under NU6. To test this, we need to disable the explicit check of the consensus - // branch ID field, which the verifier performs before checking the binding sig. We also - // need to disable some additional checks to let the verifier reach the binding sig - // verification. - - // Try a block request under NU6. - - std::panic::AssertUnwindSafe(verifier.clone().oneshot(block_req_with( - NetworkUpgrade::Nu6, - Some(HashSet::from_iter([ - SkipCheck::ConsensusBranchId, - SkipCheck::ExpiryHeight, - ])), - ))) - .catch_unwind() - .await - .expect_err("SIGHASH computation should panic due to an invalid consenus branch id in tx"); - - // Try a mempool request under NU6. - - std::panic::AssertUnwindSafe(verifier.clone().oneshot(mempool_req_with( - NetworkUpgrade::Nu6, - Some(HashSet::from_iter([ - SkipCheck::ConsensusBranchId, - SkipCheck::ExpiryHeight, - // We need to skip ZIP 317 mempool checks because we don't have a Testnet V5 tx - // in our test vectors without unpaid actions. - // - // TODO: Add at least one V5 tx with no unpaid actions to Testnet test vectors. - SkipCheck::Zip317, - ])), - ))) - .catch_unwind() - .await - .expect_err("SIGHASH computation should panic due to an invalid consenus branch id tx"); - } -} - // Utility functions /// Create a mock transparent transfer to be included in a transaction. From d5972d4b1c19f519c78dd0a707bdf95d3e416bb4 Mon Sep 17 00:00:00 2001 From: Marek Date: Mon, 27 Jan 2025 18:05:34 +0100 Subject: [PATCH 32/37] Remove the "skip check" functionality --- zebra-chain/src/tests/vectors.rs | 4 +- zebra-chain/src/transaction/unmined.rs | 18 ++---- zebra-consensus/src/block.rs | 2 - zebra-consensus/src/transaction.rs | 39 ------------- zebra-consensus/src/transaction/tests.rs | 57 +------------------ zebra-consensus/src/transaction/tests/prop.rs | 1 - .../components/inbound/tests/fake_peer_set.rs | 12 ++-- zebrad/src/components/mempool/downloads.rs | 2 - .../components/mempool/storage/tests/prop.rs | 12 ++-- .../mempool/storage/tests/vectors.rs | 3 +- zebrad/src/components/mempool/tests/vector.rs | 2 - 11 files changed, 15 insertions(+), 137 deletions(-) diff --git a/zebra-chain/src/tests/vectors.rs b/zebra-chain/src/tests/vectors.rs index 1d9dd9a7db6..44fe6644fbe 100644 --- a/zebra-chain/src/tests/vectors.rs +++ b/zebra-chain/src/tests/vectors.rs @@ -63,10 +63,8 @@ impl Network { .filter_map(|transaction| { VerifiedUnminedTx::new( transaction, - Amount::try_from(1_000_000).expect("invalid value"), + Amount::try_from(1_000_000).expect("valid amount"), 0, - #[cfg(feature = "proptest-impl")] - false, ) .ok() }) diff --git a/zebra-chain/src/transaction/unmined.rs b/zebra-chain/src/transaction/unmined.rs index b0c08307c7e..da716573e8b 100644 --- a/zebra-chain/src/transaction/unmined.rs +++ b/zebra-chain/src/transaction/unmined.rs @@ -385,31 +385,21 @@ impl VerifiedUnminedTx { transaction: UnminedTx, miner_fee: Amount, legacy_sigop_count: u64, - // Allows skipping ZIP 317 checks, only in tests. - #[cfg(feature = "proptest-impl")] skip_checks: bool, ) -> Result { let fee_weight_ratio = zip317::conventional_fee_weight_ratio(&transaction, miner_fee); let conventional_actions = zip317::conventional_actions(&transaction.transaction); let unpaid_actions = zip317::unpaid_actions(&transaction, miner_fee); - let tx_size = transaction.size; - let verified_unmined_tx = Self { + zip317::mempool_checks(unpaid_actions, miner_fee, transaction.size)?; + + Ok(Self { transaction, miner_fee, legacy_sigop_count, fee_weight_ratio, conventional_actions, unpaid_actions, - }; - - #[cfg(feature = "proptest-impl")] - if skip_checks { - return Ok(verified_unmined_tx); - } - - zip317::mempool_checks(unpaid_actions, miner_fee, tx_size)?; - - Ok(verified_unmined_tx) + }) } /// Returns `true` if the transaction pays at least the [ZIP-317] conventional fee. diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index 093c1891fff..1c959dd284e 100644 --- a/zebra-consensus/src/block.rs +++ b/zebra-consensus/src/block.rs @@ -251,8 +251,6 @@ where known_utxos: known_utxos.clone(), height, time: block.header.time, - #[cfg(any(test, feature = "proptest-impl"))] - skip_checks: None, }); async_checks.push(rsp); } diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 5d74e6e83f6..feed4b0608c 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -158,9 +158,6 @@ pub enum Request { height: block::Height, /// The time that the block was mined. time: DateTime, - /// Verification checks to skip, only in tests. - #[cfg(any(test, feature = "proptest-impl"))] - skip_checks: Option>, }, /// Verify the supplied transaction as part of the mempool. /// @@ -175,9 +172,6 @@ pub enum Request { /// The next block is the first block that could possibly contain a /// mempool transaction. height: block::Height, - /// Verification checks to skip, only in tests. - #[cfg(any(test, feature = "proptest-impl"))] - skip_checks: Option>, }, } @@ -324,15 +318,6 @@ impl Request { pub fn is_mempool(&self) -> bool { matches!(self, Request::Mempool { .. }) } - - /// Returns the verification checks that should be skipped, only in tests. - #[cfg(any(test, feature = "proptest-impl"))] - pub fn skip_checks(&self) -> Option> { - match self { - Request::Block { skip_checks, .. } => skip_checks.clone(), - Request::Mempool { skip_checks, .. } => skip_checks.clone(), - } - } } impl Response { @@ -430,12 +415,7 @@ where // Do quick checks first check::has_inputs_and_outputs(&tx)?; check::has_enough_orchard_flags(&tx)?; - #[cfg(not(test))] check::consensus_branch_id(&tx, req.height(), &network)?; - #[cfg(test)] - if req.skip_checks().is_none_or(|c| !c.contains(&SkipCheck::ConsensusBranchId)) { - check::consensus_branch_id(&tx, req.height(), &network)?; - } // Validate the coinbase input consensus rules if req.is_mempool() && tx.is_coinbase() { @@ -452,12 +432,7 @@ where if tx.is_coinbase() { check::coinbase_expiry_height(&req.height(), &tx, &network)?; } else { - #[cfg(not(test))] check::non_coinbase_expiry_height(&req.height(), &tx)?; - #[cfg(test)] - if req.skip_checks().is_none_or(|c| !c.contains(&SkipCheck::ExpiryHeight)) { - check::non_coinbase_expiry_height(&req.height(), &tx)?; - } } // Consensus rule: @@ -605,8 +580,6 @@ where tx.clone(), miner_fee.expect("fee should have been checked earlier"), legacy_sigop_count, - #[cfg(any(test, feature = "proptest-impl"))] - req.skip_checks().is_some_and(|checks| checks.contains(&SkipCheck::Zip317)) )?; if let Some(mut mempool) = mempool { @@ -1409,15 +1382,3 @@ where AsyncChecks(iterator.into_iter().map(FutureExt::boxed).collect()) } } - -/// Checks the tx verifier should skip, only in tests. -#[cfg(any(test, feature = "proptest-impl"))] -#[derive(Eq, Hash, PartialEq, Debug, Clone)] -pub enum SkipCheck { - /// Skip checks related to the transaction consensus branch ID. - ConsensusBranchId, - /// Skip mempool checks defined in ZIP 317. - Zip317, - /// Skip the expiry height check. - ExpiryHeight, -} diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 4039d052c47..5d5ff2f644f 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -36,10 +36,7 @@ use zebra_node_services::mempool; use zebra_state::ValidateContextError; use zebra_test::mock_service::MockService; -use crate::{ - error::TransactionError, - transaction::{SkipCheck, POLL_MEMPOOL_DELAY}, -}; +use crate::{error::TransactionError, transaction::POLL_MEMPOOL_DELAY}; use super::{check, Request, Verifier}; @@ -233,7 +230,6 @@ async fn mempool_request_with_missing_input_is_rejected() { let verifier_req = verifier.oneshot(Request::Mempool { transaction: tx.into(), height, - skip_checks: None, }); let (rsp, _) = futures::join!(verifier_req, state_req); @@ -300,7 +296,6 @@ async fn mempool_request_with_present_input_is_accepted() { .oneshot(Request::Mempool { transaction: tx.into(), height, - skip_checks: None, }) .await; @@ -378,7 +373,6 @@ async fn mempool_request_with_invalid_lock_time_is_rejected() { .oneshot(Request::Mempool { transaction: tx.into(), height, - skip_checks: None, }) .await; @@ -448,7 +442,6 @@ async fn mempool_request_with_unlocked_lock_time_is_accepted() { .oneshot(Request::Mempool { transaction: tx.into(), height, - skip_checks: None, }) .await; @@ -519,7 +512,6 @@ async fn mempool_request_with_lock_time_max_sequence_number_is_accepted() { .oneshot(Request::Mempool { transaction: tx.into(), height, - skip_checks: None, }) .await; @@ -595,7 +587,6 @@ async fn mempool_request_with_past_lock_time_is_accepted() { .oneshot(Request::Mempool { transaction: tx.into(), height, - skip_checks: None, }) .await; @@ -689,7 +680,6 @@ async fn mempool_request_with_unmined_output_spends_is_accepted() { .oneshot(Request::Mempool { transaction: tx.into(), height, - skip_checks: None, }) .await; @@ -811,7 +801,6 @@ async fn skips_verification_of_block_transactions_in_mempool() { .oneshot(Request::Mempool { transaction: tx.clone().into(), height, - skip_checks: None, }) .await; @@ -855,7 +844,6 @@ async fn skips_verification_of_block_transactions_in_mempool() { known_utxos: Arc::new(HashMap::new()), height, time: Utc::now(), - skip_checks: None, }; let crate::transaction::Response::Block { .. } = verifier @@ -985,7 +973,6 @@ async fn mempool_request_with_immature_spend_is_rejected() { .oneshot(Request::Mempool { transaction: tx.into(), height, - skip_checks: None, }) .await .expect_err("verification of transaction with immature spend should fail"); @@ -1085,7 +1072,6 @@ async fn mempool_request_with_transparent_coinbase_spend_is_accepted_on_regtest( .oneshot(Request::Mempool { transaction: tx.into(), height, - skip_checks: None, }) .await .expect("verification of transaction with mature spend to transparent outputs should pass"); @@ -1158,7 +1144,6 @@ async fn state_error_converted_correctly() { .oneshot(Request::Mempool { transaction: tx.into(), height, - skip_checks: None, }) .await; @@ -1250,7 +1235,6 @@ async fn v5_transaction_is_rejected_before_nu5_activation() { known_outpoint_hashes: Arc::new(HashSet::new()), height: sapling.activation_height(&net).expect("height"), time: DateTime::::MAX_UTC, - skip_checks: None, }) .await, Err(TransactionError::UnsupportedByNetworkUpgrade(5, sapling)) @@ -1278,7 +1262,6 @@ async fn v5_transaction_is_accepted_after_nu5_activation() { known_outpoint_hashes: Arc::new(HashSet::new()), height: tx_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -1333,7 +1316,6 @@ async fn v4_transaction_with_transparent_transfer_is_accepted() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -1380,7 +1362,6 @@ async fn v4_transaction_with_last_valid_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -1428,7 +1409,6 @@ async fn v4_coinbase_transaction_with_low_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -1478,7 +1458,6 @@ async fn v4_transaction_with_too_low_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -1531,7 +1510,6 @@ async fn v4_transaction_with_exceeding_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -1587,7 +1565,6 @@ async fn v4_coinbase_transaction_with_exceeding_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -1641,7 +1618,6 @@ async fn v4_coinbase_transaction_is_accepted() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -1699,7 +1675,6 @@ async fn v4_transaction_with_transparent_transfer_is_rejected_by_the_script() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -1757,7 +1732,6 @@ async fn v4_transaction_with_conflicting_transparent_spend_is_rejected() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -1826,7 +1800,6 @@ fn v4_transaction_with_conflicting_sprout_nullifier_inside_joinsplit_is_rejected known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -1900,7 +1873,6 @@ fn v4_transaction_with_conflicting_sprout_nullifier_across_joinsplits_is_rejecte known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -1962,7 +1934,6 @@ async fn v5_transaction_with_transparent_transfer_is_accepted() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2011,7 +1982,6 @@ async fn v5_transaction_with_last_valid_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2059,7 +2029,6 @@ async fn v5_coinbase_transaction_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2083,7 +2052,6 @@ async fn v5_coinbase_transaction_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await .map_err(|err| { @@ -2115,7 +2083,6 @@ async fn v5_coinbase_transaction_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await .map_err(|err| { @@ -2155,7 +2122,6 @@ async fn v5_coinbase_transaction_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: new_expiry_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2209,7 +2175,6 @@ async fn v5_transaction_with_too_low_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2261,7 +2226,6 @@ async fn v5_transaction_with_exceeding_expiry_height() { known_outpoint_hashes: Arc::new(HashSet::new()), height: height_max, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2318,7 +2282,6 @@ async fn v5_coinbase_transaction_is_accepted() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2378,7 +2341,6 @@ async fn v5_transaction_with_transparent_transfer_is_rejected_by_the_script() { known_outpoint_hashes: Arc::new(HashSet::new()), height: transaction_block_height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2429,7 +2391,6 @@ async fn v5_transaction_with_conflicting_transparent_spend_is_rejected() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2475,7 +2436,6 @@ fn v4_with_signed_sprout_transfer_is_accepted() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2567,7 +2527,6 @@ async fn v4_with_joinsplit_is_rejected_for_modification( known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await .map_err(|err| { @@ -2616,7 +2575,6 @@ fn v4_with_sapling_spends() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2661,7 +2619,6 @@ fn v4_with_duplicate_sapling_spends() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2708,7 +2665,6 @@ fn v4_with_sapling_outputs_and_no_spends() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await; @@ -2751,7 +2707,6 @@ async fn v5_with_sapling_spends() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await .expect("unexpected error response") @@ -2791,7 +2746,6 @@ async fn v5_with_duplicate_sapling_spends() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await, Err(TransactionError::DuplicateSaplingNullifier( @@ -2847,7 +2801,6 @@ async fn v5_with_duplicate_orchard_action() { known_outpoint_hashes: Arc::new(HashSet::new()), height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .await, Err(TransactionError::DuplicateOrchardNullifier( @@ -2906,7 +2859,6 @@ async fn v5_consensus_branch_ids() { // The consensus branch ID of the tx is outdated for this height. height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .map_err(|err| *err.downcast().expect("`TransactionError` type")); @@ -2916,7 +2868,6 @@ async fn v5_consensus_branch_ids() { transaction: tx.clone().into(), // The consensus branch ID of the tx is outdated for this height. height, - skip_checks: None, }) .map_err(|err| *err.downcast().expect("`TransactionError` type")); @@ -2938,7 +2889,6 @@ async fn v5_consensus_branch_ids() { // The consensus branch ID of the tx is supported by this height. height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .map_ok(|rsp| rsp.tx_id()) .map_err(|e| format!("{e}")); @@ -2949,7 +2899,6 @@ async fn v5_consensus_branch_ids() { transaction: tx.clone().into(), // The consensus branch ID of the tx is supported by this height. height, - skip_checks: None, }) .map_ok(|rsp| rsp.tx_id()) .map_err(|e| format!("{e}")); @@ -2999,7 +2948,6 @@ async fn v5_consensus_branch_ids() { // The consensus branch ID of the tx is not supported by this height. height, time: DateTime::::MAX_UTC, - skip_checks: None, }) .map_err(|err| *err.downcast().expect("`TransactionError` type")); @@ -3009,7 +2957,6 @@ async fn v5_consensus_branch_ids() { transaction: tx.clone().into(), // The consensus branch ID of the tx is not supported by this height. height, - skip_checks: None, }) .map_err(|err| *err.downcast().expect("`TransactionError` type")); @@ -3572,7 +3519,6 @@ async fn mempool_zip317_error() { .oneshot(Request::Mempool { transaction: tx.into(), height, - skip_checks: None, }) .await; @@ -3645,7 +3591,6 @@ async fn mempool_zip317_ok() { .oneshot(Request::Mempool { transaction: tx.into(), height, - skip_checks: None, }) .await; diff --git a/zebra-consensus/src/transaction/tests/prop.rs b/zebra-consensus/src/transaction/tests/prop.rs index f5f79156b96..8fea9cf3433 100644 --- a/zebra-consensus/src/transaction/tests/prop.rs +++ b/zebra-consensus/src/transaction/tests/prop.rs @@ -467,7 +467,6 @@ fn validate( known_outpoint_hashes: Arc::new(HashSet::new()), height, time: block_time, - skip_checks: None, }) .await .map_err(|err| { diff --git a/zebrad/src/components/inbound/tests/fake_peer_set.rs b/zebrad/src/components/inbound/tests/fake_peer_set.rs index fb91cdf243e..07402dafb50 100644 --- a/zebrad/src/components/inbound/tests/fake_peer_set.rs +++ b/zebrad/src/components/inbound/tests/fake_peer_set.rs @@ -166,9 +166,8 @@ async fn mempool_push_transaction() -> Result<(), crate::BoxError> { responder.respond(transaction::Response::from( VerifiedUnminedTx::new( transaction, - Amount::try_from(1_000_000).expect("invalid value"), + Amount::try_from(1_000_000).expect("valid amount"), 0, - false, ) .expect("verification should pass"), )); @@ -272,9 +271,8 @@ async fn mempool_advertise_transaction_ids() -> Result<(), crate::BoxError> { responder.respond(transaction::Response::from( VerifiedUnminedTx::new( transaction, - Amount::try_from(1_000_000).expect("invalid value"), + Amount::try_from(1_000_000).expect("valid amount"), 0, - false, ) .expect("verification should pass"), )); @@ -375,9 +373,8 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> { responder.respond(transaction::Response::from( VerifiedUnminedTx::new( transaction, - Amount::try_from(1_000_000).expect("invalid value"), + Amount::try_from(1_000_000).expect("valid amount"), 0, - false, ) .expect("verification should pass"), )); @@ -515,9 +512,8 @@ async fn mempool_transaction_expiration() -> Result<(), crate::BoxError> { responder.respond(transaction::Response::from( VerifiedUnminedTx::new( transaction, - Amount::try_from(1_000_000).expect("invalid value"), + Amount::try_from(1_000_000).expect("valid amount"), 0, - false, ) .expect("verification should pass"), )); diff --git a/zebrad/src/components/mempool/downloads.rs b/zebrad/src/components/mempool/downloads.rs index c0cf911fe02..68d29aadcaf 100644 --- a/zebrad/src/components/mempool/downloads.rs +++ b/zebrad/src/components/mempool/downloads.rs @@ -373,8 +373,6 @@ where .oneshot(tx::Request::Mempool { transaction: tx.clone(), height: next_height, - #[cfg(feature = "proptest-impl")] - skip_checks: None, }) .map_ok(|rsp| { let tx::Response::Mempool { transaction, spent_mempool_outpoints } = rsp else { diff --git a/zebrad/src/components/mempool/storage/tests/prop.rs b/zebrad/src/components/mempool/storage/tests/prop.rs index 01b0f3f7f17..bac143349a5 100644 --- a/zebrad/src/components/mempool/storage/tests/prop.rs +++ b/zebrad/src/components/mempool/storage/tests/prop.rs @@ -478,17 +478,15 @@ impl SpendConflictTestInput { VerifiedUnminedTx::new( first.0.into(), // make sure miner fee is big enough for all cases - Amount::try_from(1_000_000).expect("invalid value"), + Amount::try_from(1_000_000).expect("valid amount"), 0, - false, ) .expect("verification should pass"), VerifiedUnminedTx::new( second.0.into(), // make sure miner fee is big enough for all cases - Amount::try_from(1_000_000).expect("invalid value"), + Amount::try_from(1_000_000).expect("valid amount"), 0, - false, ) .expect("verification should pass"), ) @@ -510,17 +508,15 @@ impl SpendConflictTestInput { VerifiedUnminedTx::new( first.0.into(), // make sure miner fee is big enough for all cases - Amount::try_from(1_000_000).expect("invalid value"), + Amount::try_from(1_000_000).expect("valid amount"), 0, - false, ) .expect("verification should pass"), VerifiedUnminedTx::new( second.0.into(), // make sure miner fee is big enough for all cases - Amount::try_from(1_000_000).expect("invalid value"), + Amount::try_from(1_000_000).expect("valid amount"), 0, - false, ) .expect("verification should pass"), ) diff --git a/zebrad/src/components/mempool/storage/tests/vectors.rs b/zebrad/src/components/mempool/storage/tests/vectors.rs index 8656a87c77f..e4c1cd471fc 100644 --- a/zebrad/src/components/mempool/storage/tests/vectors.rs +++ b/zebrad/src/components/mempool/storage/tests/vectors.rs @@ -267,9 +267,8 @@ fn mempool_expired_basic_for_network(network: Network) -> Result<()> { storage.insert( VerifiedUnminedTx::new( tx.into(), - Amount::try_from(1_000_000).expect("invalid value"), + Amount::try_from(1_000_000).expect("valid amount"), 0, - false, ) .expect("verification should pass"), Vec::new(), diff --git a/zebrad/src/components/mempool/tests/vector.rs b/zebrad/src/components/mempool/tests/vector.rs index 457b95edc38..9dd3557de9c 100644 --- a/zebrad/src/components/mempool/tests/vector.rs +++ b/zebrad/src/components/mempool/tests/vector.rs @@ -825,7 +825,6 @@ async fn mempool_reverifies_after_tip_change() -> Result<(), Report> { transaction, Amount::try_from(1_000_000).expect("invalid value"), 0, - false, ) .expect("verification should pass"), )); @@ -886,7 +885,6 @@ async fn mempool_reverifies_after_tip_change() -> Result<(), Report> { transaction, Amount::try_from(1_000_000).expect("invalid value"), 0, - false, ) .expect("verification should pass"), )); From 45a7d5a8e9cea1ddc18f7f647f736acd6c3be7b2 Mon Sep 17 00:00:00 2001 From: Marek Date: Mon, 27 Jan 2025 18:12:18 +0100 Subject: [PATCH 33/37] Revert the compilation adjustments --- Cargo.lock | 2 -- zebra-scan/Cargo.toml | 3 --- zebrad/Cargo.toml | 5 ----- 3 files changed, 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a952065597a..c7a22a6093f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5967,7 +5967,6 @@ dependencies = [ "zcash_note_encryption", "zcash_primitives", "zebra-chain", - "zebra-consensus", "zebra-grpc", "zebra-node-services", "zebra-rpc", @@ -6162,7 +6161,6 @@ dependencies = [ "zebra-state", "zebra-test", "zebra-utils", - "zebrad", ] [[package]] diff --git a/zebra-scan/Cargo.toml b/zebra-scan/Cargo.toml index 3475b76250a..3ec3d918d51 100644 --- a/zebra-scan/Cargo.toml +++ b/zebra-scan/Cargo.toml @@ -126,7 +126,4 @@ toml = "0.8.19" tonic = "0.12.3" zebra-state = { path = "../zebra-state", version = "1.0.0-beta.44", features = ["proptest-impl"] } -zebra-consensus = { path = "../zebra-consensus", version = "1.0.0-beta.44", features = ["proptest-impl"] } -zebrad = { path = "../zebrad", version = "2.1.1", features = ["proptest-impl"] } zebra-test = { path = "../zebra-test", version = "1.0.0-beta.44" } - diff --git a/zebrad/Cargo.toml b/zebrad/Cargo.toml index ece5e927df5..ce5fbde2a21 100644 --- a/zebrad/Cargo.toml +++ b/zebrad/Cargo.toml @@ -298,10 +298,5 @@ zebra-grpc = { path = "../zebra-grpc", version = "0.1.0-alpha.11" } # zebra-utils { path = "../zebra-utils", artifact = "bin:zebra-checkpoints" } zebra-utils = { path = "../zebra-utils", version = "1.0.0-beta.44" } -# Enable the "proptest-impl" feature for "self" in tests. -# -# -zebrad = { path = ".", features = ["proptest-impl"], default-features = false } - [lints.rust] unexpected_cfgs = { level = "warn", check-cfg = ['cfg(tokio_unstable)'] } From 83fd675097487d8734b28ba0086bae7cc5bff5ab Mon Sep 17 00:00:00 2001 From: Marek Date: Tue, 28 Jan 2025 01:20:40 +0100 Subject: [PATCH 34/37] Apply suggestions from code review Co-authored-by: Arya --- zebra-chain/src/parameters/network_upgrade.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/zebra-chain/src/parameters/network_upgrade.rs b/zebra-chain/src/parameters/network_upgrade.rs index e0d52e65963..3bbd23be223 100644 --- a/zebra-chain/src/parameters/network_upgrade.rs +++ b/zebra-chain/src/parameters/network_upgrade.rs @@ -350,18 +350,12 @@ impl NetworkUpgrade { /// Returns the next expected network upgrade after this network upgrade pub fn next_upgrade(self) -> Option { - Self::iter() - .position(|nu| self == nu) - .filter(|&p| (p + 1) < NETWORK_UPGRADES_IN_ORDER.len()) - .map(|p| NETWORK_UPGRADES_IN_ORDER[p + 1]) + Self::iter().skip_while(|&nu| self != nu).nth(1) } /// Returns the previous network before after this network upgrade pub fn previous_upgrade(self) -> Option { - Self::iter() - .position(|nu| self == nu) - .filter(|&p| p > 0) - .map(|p| NETWORK_UPGRADES_IN_ORDER[p - 1]) + Self::iter().rev().skip_while(|&nu| self != nu).nth(1) } /// Returns the next network upgrade for `network` and `height`. @@ -541,7 +535,7 @@ impl NetworkUpgrade { } /// Returns an iterator over [`NetworkUpgrade`] variants. - pub fn iter() -> impl Iterator { + pub fn iter() -> impl DoubleEndedIterator { NETWORK_UPGRADES_IN_ORDER.into_iter() } } From 2e32fd688c53e6956d0a7447bf750f37a7df0a99 Mon Sep 17 00:00:00 2001 From: Marek Date: Tue, 28 Jan 2025 01:26:29 +0100 Subject: [PATCH 35/37] Fix docs --- zebra-chain/src/parameters/network_upgrade.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zebra-chain/src/parameters/network_upgrade.rs b/zebra-chain/src/parameters/network_upgrade.rs index 3bbd23be223..74bc59cf162 100644 --- a/zebra-chain/src/parameters/network_upgrade.rs +++ b/zebra-chain/src/parameters/network_upgrade.rs @@ -348,12 +348,12 @@ impl NetworkUpgrade { .expect("every height has a current network upgrade") } - /// Returns the next expected network upgrade after this network upgrade + /// Returns the next expected network upgrade after this network upgrade. pub fn next_upgrade(self) -> Option { Self::iter().skip_while(|&nu| self != nu).nth(1) } - /// Returns the previous network before after this network upgrade + /// Returns the previous network upgrade before this network upgrade. pub fn previous_upgrade(self) -> Option { Self::iter().rev().skip_while(|&nu| self != nu).nth(1) } From 3b25023db93e708837da4e6d625810e932e81869 Mon Sep 17 00:00:00 2001 From: Marek Date: Thu, 30 Jan 2025 13:02:35 +0100 Subject: [PATCH 36/37] Clarify panic conditions in docs --- zebra-chain/src/primitives/zcash_primitives.rs | 2 ++ zebra-chain/src/transaction.rs | 4 ++++ zebra-chain/src/transaction/sighash.rs | 8 ++++++++ 3 files changed, 14 insertions(+) diff --git a/zebra-chain/src/primitives/zcash_primitives.rs b/zebra-chain/src/primitives/zcash_primitives.rs index 21ffeead609..149ca423cd4 100644 --- a/zebra-chain/src/primitives/zcash_primitives.rs +++ b/zebra-chain/src/primitives/zcash_primitives.rs @@ -220,6 +220,8 @@ impl<'a> PrecomputedTxData<'a> { /// # Panics /// /// - If `tx` can't be converted to its `librustzcash` equivalent. + /// - If `nu` doesn't contain a consensus branch id convertible to its `librustzcash` + /// equivalent. /// /// # Consensus /// diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index f7cc72045fc..025bd22881d 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -216,6 +216,10 @@ impl Transaction { /// - if called on a v1 or v2 transaction /// - if the input index points to a transparent::Input::CoinBase /// - if the input index is out of bounds for self.inputs() + /// - if the tx contains `nConsensusBranchId` field and `nu` doesn't match it + /// - if the tx is not convertible to its `librustzcash` equivalent + /// - if `nu` doesn't contain a consensus branch id convertible to its `librustzcash` + /// equivalent pub fn sighash( &self, nu: NetworkUpgrade, diff --git a/zebra-chain/src/transaction/sighash.rs b/zebra-chain/src/transaction/sighash.rs index 4738a77bc06..c2460a7101c 100644 --- a/zebra-chain/src/transaction/sighash.rs +++ b/zebra-chain/src/transaction/sighash.rs @@ -48,6 +48,14 @@ pub struct SigHasher<'a> { impl<'a> SigHasher<'a> { /// Create a new SigHasher for the given transaction. + /// + /// # Panics + /// + /// - If `trans` can't be converted to its `librustzcash` equivalent. This could happen, for + /// example, if `trans` contains the `nConsensusBranchId` field, and `nu` doesn't match it. + /// More details in [`PrecomputedTxData::new`]. + /// - If `nu` doesn't contain a consensus branch id convertible to its `librustzcash` + /// equivalent. pub fn new( trans: &'a Transaction, nu: NetworkUpgrade, From ed9d2d3f043986b554456ecf1a8d9d98cbc21cbf Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Sat, 1 Feb 2025 07:20:16 -0300 Subject: [PATCH 37/37] remove duplicated verification Co-authored-by: Arya --- zebra-script/src/lib.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/zebra-script/src/lib.rs b/zebra-script/src/lib.rs index a223f279e92..3747b84c46e 100644 --- a/zebra-script/src/lib.rs +++ b/zebra-script/src/lib.rs @@ -441,9 +441,6 @@ mod tests { let input_index = 0; - verifier - .is_valid(NetworkUpgrade::Blossom, input_index + 1) - .expect_err("verification should fail"); verifier .is_valid(NetworkUpgrade::Blossom, input_index + 1) .expect_err("verification should fail");