From 26c474fda081af5ee57161a54c681e2187ef35fc Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Mon, 26 Apr 2021 17:08:06 -0300 Subject: [PATCH 1/7] validate sapling v5 tx --- zebra-chain/Cargo.toml | 3 +- zebra-chain/src/transaction.rs | 2 +- zebra-chain/src/transaction/arbitrary.rs | 129 ++++++++++++++++++ zebra-chain/src/transaction/tests/vectors.rs | 132 +------------------ zebra-consensus/Cargo.toml | 1 + zebra-consensus/src/transaction.rs | 2 + zebra-consensus/src/transaction/check.rs | 39 +++++- zebra-consensus/src/transaction/tests.rs | 65 +++++++++ 8 files changed, 232 insertions(+), 141 deletions(-) create mode 100644 zebra-consensus/src/transaction/tests.rs diff --git a/zebra-chain/Cargo.toml b/zebra-chain/Cargo.toml index ea72120beb9..76d842c4381 100644 --- a/zebra-chain/Cargo.toml +++ b/zebra-chain/Cargo.toml @@ -26,6 +26,7 @@ displaydoc = "0.2.1" equihash = "0.1" futures = "0.3" hex = "0.4" +itertools = "0.10.0" jubjub = "0.6.0" lazy_static = "1.4.0" primitive-types = "0.9.0" @@ -55,8 +56,6 @@ criterion = { version = "0.3", features = ["html_reports"] } spandoc = "0.2" tracing = "0.1.25" -itertools = "0.10.0" - proptest = "0.10" proptest-derive = "0.3" diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index 929563fd74a..05e52fa6d6a 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -10,7 +10,7 @@ mod serialize; mod sighash; #[cfg(any(test, feature = "proptest-impl"))] -mod arbitrary; +pub mod arbitrary; #[cfg(test)] mod tests; diff --git a/zebra-chain/src/transaction/arbitrary.rs b/zebra-chain/src/transaction/arbitrary.rs index ab582382f62..09f044eaf81 100644 --- a/zebra-chain/src/transaction/arbitrary.rs +++ b/zebra-chain/src/transaction/arbitrary.rs @@ -11,6 +11,8 @@ use crate::{ sapling, sprout, transparent, LedgerState, }; +use itertools::Itertools; + use super::{FieldNotPresent, JoinSplitData, LockTime, Memo, Transaction}; use sapling::{AnchorVariant, PerSpendAnchor, SharedAnchor}; @@ -330,3 +332,130 @@ impl Arbitrary for Transaction { type Strategy = BoxedStrategy; } + +/// Transaction utility tests functions + +/// Convert `trans` into a fake v5 transaction, +/// converting sapling shielded data from v4 to v5 if possible. +pub fn transaction_to_fake_v5(trans: &Transaction) -> Transaction { + use Transaction::*; + + match trans { + V1 { + inputs, + outputs, + lock_time, + } => V5 { + inputs: inputs.to_vec(), + outputs: outputs.to_vec(), + lock_time: *lock_time, + expiry_height: block::Height(0), + sapling_shielded_data: None, + }, + V2 { + inputs, + outputs, + lock_time, + .. + } => V5 { + inputs: inputs.to_vec(), + outputs: outputs.to_vec(), + lock_time: *lock_time, + expiry_height: block::Height(0), + sapling_shielded_data: None, + }, + V3 { + inputs, + outputs, + lock_time, + expiry_height, + .. + } => V5 { + inputs: inputs.to_vec(), + outputs: outputs.to_vec(), + lock_time: *lock_time, + expiry_height: *expiry_height, + sapling_shielded_data: None, + }, + V4 { + inputs, + outputs, + lock_time, + expiry_height, + sapling_shielded_data, + .. + } => V5 { + inputs: inputs.to_vec(), + outputs: outputs.to_vec(), + lock_time: *lock_time, + expiry_height: *expiry_height, + sapling_shielded_data: sapling_shielded_data + .clone() + .map(sapling_shielded_v4_to_fake_v5) + .flatten(), + }, + v5 @ V5 { .. } => v5.clone(), + } +} + +/// Convert a v4 sapling shielded data into a fake v5 sapling shielded data, +/// if possible. +fn sapling_shielded_v4_to_fake_v5( + v4_shielded: sapling::ShieldedData, +) -> Option> { + use sapling::ShieldedData; + use sapling::TransferData::*; + + let unique_anchors: Vec<_> = v4_shielded + .spends() + .map(|spend| spend.per_spend_anchor) + .unique() + .collect(); + + let fake_spends: Vec<_> = v4_shielded + .spends() + .cloned() + .map(sapling_spend_v4_to_fake_v5) + .collect(); + + let transfers = match v4_shielded.transfers { + SpendsAndMaybeOutputs { maybe_outputs, .. } => { + let shared_anchor = match unique_anchors.as_slice() { + [unique_anchor] => *unique_anchor, + // Multiple different anchors, can't convert to v5 + _ => return None, + }; + + SpendsAndMaybeOutputs { + shared_anchor, + spends: fake_spends.try_into().unwrap(), + maybe_outputs, + } + } + JustOutputs { outputs } => JustOutputs { outputs }, + }; + + let fake_shielded_v5 = ShieldedData:: { + value_balance: v4_shielded.value_balance, + transfers, + binding_sig: v4_shielded.binding_sig, + }; + + Some(fake_shielded_v5) +} + +/// Convert a v4 sapling spend into a fake v5 sapling spend. +fn sapling_spend_v4_to_fake_v5( + v4_spend: sapling::Spend, +) -> sapling::Spend { + use sapling::Spend; + + Spend:: { + cv: v4_spend.cv, + per_spend_anchor: FieldNotPresent, + nullifier: v4_spend.nullifier, + rk: v4_spend.rk, + zkproof: v4_spend.zkproof, + spend_auth_sig: v4_spend.spend_auth_sig, + } +} diff --git a/zebra-chain/src/transaction/tests/vectors.rs b/zebra-chain/src/transaction/tests/vectors.rs index 1dc154a441f..90ade53361d 100644 --- a/zebra-chain/src/transaction/tests/vectors.rs +++ b/zebra-chain/src/transaction/tests/vectors.rs @@ -2,12 +2,9 @@ use super::super::*; use crate::{ block::{Block, MAX_BLOCK_BYTES}, - sapling::{PerSpendAnchor, SharedAnchor}, serialization::{ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize}, }; -use itertools::Itertools; - use std::convert::TryInto; #[test] @@ -186,7 +183,7 @@ fn fake_v5_round_trip() { .transactions .iter() .map(AsRef::as_ref) - .map(transaction_to_fake_v5) + .map(arbitrary::transaction_to_fake_v5) .map(Into::into) .collect(); @@ -264,130 +261,3 @@ fn fake_v5_round_trip() { ); } } - -// Utility functions - -/// Convert `trans` into a fake v5 transaction, -/// converting sapling shielded data from v4 to v5 if possible. -fn transaction_to_fake_v5(trans: &Transaction) -> Transaction { - use Transaction::*; - - match trans { - V1 { - inputs, - outputs, - lock_time, - } => V5 { - inputs: inputs.to_vec(), - outputs: outputs.to_vec(), - lock_time: *lock_time, - expiry_height: block::Height(0), - sapling_shielded_data: None, - }, - V2 { - inputs, - outputs, - lock_time, - .. - } => V5 { - inputs: inputs.to_vec(), - outputs: outputs.to_vec(), - lock_time: *lock_time, - expiry_height: block::Height(0), - sapling_shielded_data: None, - }, - V3 { - inputs, - outputs, - lock_time, - expiry_height, - .. - } => V5 { - inputs: inputs.to_vec(), - outputs: outputs.to_vec(), - lock_time: *lock_time, - expiry_height: *expiry_height, - sapling_shielded_data: None, - }, - V4 { - inputs, - outputs, - lock_time, - expiry_height, - sapling_shielded_data, - .. - } => V5 { - inputs: inputs.to_vec(), - outputs: outputs.to_vec(), - lock_time: *lock_time, - expiry_height: *expiry_height, - sapling_shielded_data: sapling_shielded_data - .clone() - .map(sapling_shielded_v4_to_fake_v5) - .flatten(), - }, - v5 @ V5 { .. } => v5.clone(), - } -} - -/// Convert a v4 sapling shielded data into a fake v5 sapling shielded data, -/// if possible. -fn sapling_shielded_v4_to_fake_v5( - v4_shielded: sapling::ShieldedData, -) -> Option> { - use sapling::ShieldedData; - use sapling::TransferData::*; - - let unique_anchors: Vec<_> = v4_shielded - .spends() - .map(|spend| spend.per_spend_anchor) - .unique() - .collect(); - - let fake_spends: Vec<_> = v4_shielded - .spends() - .cloned() - .map(sapling_spend_v4_to_fake_v5) - .collect(); - - let transfers = match v4_shielded.transfers { - SpendsAndMaybeOutputs { maybe_outputs, .. } => { - let shared_anchor = match unique_anchors.as_slice() { - [unique_anchor] => *unique_anchor, - // Multiple different anchors, can't convert to v5 - _ => return None, - }; - - SpendsAndMaybeOutputs { - shared_anchor, - spends: fake_spends.try_into().unwrap(), - maybe_outputs, - } - } - JustOutputs { outputs } => JustOutputs { outputs }, - }; - - let fake_shielded_v5 = ShieldedData:: { - value_balance: v4_shielded.value_balance, - transfers, - binding_sig: v4_shielded.binding_sig, - }; - - Some(fake_shielded_v5) -} - -/// Convert a v4 sapling spend into a fake v5 sapling spend. -fn sapling_spend_v4_to_fake_v5( - v4_spend: sapling::Spend, -) -> sapling::Spend { - use sapling::Spend; - - Spend:: { - cv: v4_spend.cv, - per_spend_anchor: FieldNotPresent, - nullifier: v4_spend.nullifier, - rk: v4_spend.rk, - zkproof: v4_spend.zkproof, - spend_auth_sig: v4_spend.spend_auth_sig, - } -} diff --git a/zebra-consensus/Cargo.toml b/zebra-consensus/Cargo.toml index e0022b49e34..96878d68f59 100644 --- a/zebra-consensus/Cargo.toml +++ b/zebra-consensus/Cargo.toml @@ -42,4 +42,5 @@ tokio = { version = "0.3.6", features = ["full"] } tracing-error = "0.1.2" tracing-subscriber = "0.2.17" +zebra-chain = { path = "../zebra-chain", features = ["proptest-impl"] } zebra-test = { path = "../zebra-test/" } diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 6cea0280b29..28bd0ad0057 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -26,6 +26,8 @@ use zebra_state as zs; use crate::{error::TransactionError, primitives, script, BoxError}; mod check; +#[cfg(test)] +mod tests; /// Asynchronous transaction verification. #[derive(Debug, Clone)] diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 8f72f443fe5..7981b11b5db 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -55,8 +55,30 @@ pub fn has_inputs_and_outputs(tx: &Transaction) -> Result<(), TransactionError> Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => { unreachable!("tx version is checked first") } - Transaction::V5 { .. } => { - unimplemented!("v5 transaction format as specified in ZIP-225") + Transaction::V5 { + inputs, + outputs, + sapling_shielded_data, + .. + } => { + let tx_in_count = inputs.len(); + let tx_out_count = outputs.len(); + let n_shielded_spend = sapling_shielded_data + .as_ref() + .map(|d| d.spends().count()) + .unwrap_or(0); + let n_shielded_output = sapling_shielded_data + .as_ref() + .map(|d| d.outputs().count()) + .unwrap_or(0); + + if tx_in_count + n_shielded_spend == 0 { + Err(TransactionError::NoInputs) + } else if tx_out_count + n_shielded_output == 0 { + Err(TransactionError::NoOutputs) + } else { + Ok(()) + } } } } @@ -100,15 +122,18 @@ pub fn coinbase_tx_no_joinsplit_or_spend(tx: &Transaction) -> Result<(), Transac Err(TransactionError::CoinbaseHasSpend) } - Transaction::V4 { .. } => Ok(()), + Transaction::V5 { + sapling_shielded_data: Some(sapling_shielded_data), + .. + } if sapling_shielded_data.spends().count() > 0 => { + Err(TransactionError::CoinbaseHasSpend) + } + + Transaction::V4 { .. } | Transaction::V5 { .. } => Ok(()), Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => { unreachable!("tx version is checked first") } - - Transaction::V5 { .. } => { - unimplemented!("v5 coinbase validation as specified in ZIP-225 and the draft spec") - } } } else { Ok(()) diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs new file mode 100644 index 00000000000..1c75b1c671b --- /dev/null +++ b/zebra-consensus/src/transaction/tests.rs @@ -0,0 +1,65 @@ +use zebra_chain::{ + block::Block, + serialization::ZcashDeserializeInto, + transaction::{arbitrary::transaction_to_fake_v5, Transaction}, +}; + +use crate::error::TransactionError::*; +use color_eyre::eyre::Report; + +#[test] +fn v5_fake_transactions() -> Result<(), Report> { + zebra_test::init(); + + // get all the blocks we have available + for original_bytes in zebra_test::vectors::BLOCKS.iter() { + let original_block = original_bytes + .zcash_deserialize_into::() + .expect("block is structurally valid"); + + // convert all transactions from the block to V5 + let transactions: Vec = original_block + .transactions + .iter() + .map(AsRef::as_ref) + .map(transaction_to_fake_v5) + .map(Into::into) + .collect(); + + // after the conversion some transactions end up with no inputs nor outputs. + for transaction in transactions { + match super::check::has_inputs_and_outputs(&transaction) { + Err(e) => { + if e != NoInputs && e != NoOutputs { + panic!("error must be NoInputs or NoOutputs") + } + } + Ok(()) => (), + }; + + // make sure there are no joinsplits nor spends in coinbase + super::check::coinbase_tx_no_joinsplit_or_spend(&transaction)?; + + // validate the sapling shielded data + match transaction { + Transaction::V5 { + sapling_shielded_data, + .. + } => { + if let Some(s) = sapling_shielded_data { + super::check::shielded_balances_match(&s)?; + + for spend in s.spends_per_anchor() { + super::check::spend_cv_rk_not_small_order(&spend)? + } + for output in s.outputs() { + super::check::output_cv_epk_not_small_order(&output)?; + } + } + } + _ => panic!("we should have no tx other than 5"), + } + } + } + Ok(()) +} From c8c3b68ab6043e0d118230a5f6e283699b1c452f Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 27 Apr 2021 09:33:08 +1000 Subject: [PATCH 2/7] Make itertools dependency optional We only need itertools when the `proptest-impl` feature is enabled. --- zebra-chain/Cargo.toml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/zebra-chain/Cargo.toml b/zebra-chain/Cargo.toml index 76d842c4381..c281ada144c 100644 --- a/zebra-chain/Cargo.toml +++ b/zebra-chain/Cargo.toml @@ -10,7 +10,7 @@ edition = "2018" [features] default = [] -proptest-impl = ["proptest", "proptest-derive"] +proptest-impl = ["proptest", "proptest-derive", "itertools"] bench = ["zebra-test"] [dependencies] @@ -26,7 +26,6 @@ displaydoc = "0.2.1" equihash = "0.1" futures = "0.3" hex = "0.4" -itertools = "0.10.0" jubjub = "0.6.0" lazy_static = "1.4.0" primitive-types = "0.9.0" @@ -41,6 +40,7 @@ x25519-dalek = { version = "1.1", features = ["serde"] } proptest = { version = "0.10", optional = true } proptest-derive = { version = "0.3.0", optional = true } +itertools = { version = "0.10.0", optional = true } # ZF deps ed25519-zebra = "2.2.0" @@ -58,6 +58,7 @@ tracing = "0.1.25" proptest = "0.10" proptest-derive = "0.3" +itertools = "0.10.0" zebra-test = { path = "../zebra-test/" } From 3ffe9eb174a1b05045c71d935107048d42768191 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 27 Apr 2021 10:39:28 +1000 Subject: [PATCH 3/7] Check if V4 and V5 coinbase transactions contain PrevOut transparent inputs This is a bugfix on V4 transaction validation. The PrevOut consensus rule was not explicitly stated in the Zcash spec until April 2021. (But it was implied by Bitcoin, and partially implemented by Zebra.) Also do the shielded sapling input check for V5 transactions. --- zebra-chain/src/transaction.rs | 9 +++++++++ zebra-consensus/src/block/check.rs | 2 +- zebra-consensus/src/block/tests.rs | 2 +- zebra-consensus/src/error.rs | 5 ++++- zebra-consensus/src/transaction.rs | 2 +- zebra-consensus/src/transaction/check.rs | 14 ++++++++++++-- zebra-consensus/src/transaction/tests.rs | 2 +- 7 files changed, 29 insertions(+), 7 deletions(-) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index 05e52fa6d6a..4de9611b075 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -238,6 +238,15 @@ impl Transaction { .any(|input| matches!(input, transparent::Input::Coinbase { .. })) } + /// Returns `true` if transaction contains any `PrevOut` inputs. + /// + /// `PrevOut` inputs are also known as `transparent` inputs in the spec. + pub fn contains_prevout_input(&self) -> bool { + self.inputs() + .iter() + .any(|input| matches!(input, transparent::Input::PrevOut { .. })) + } + /// Returns `true` if this transaction is a coinbase transaction. pub fn is_coinbase(&self) -> bool { self.inputs().len() == 1 diff --git a/zebra-consensus/src/block/check.rs b/zebra-consensus/src/block/check.rs index 6e7e1a1fd66..f62afb2ca11 100644 --- a/zebra-consensus/src/block/check.rs +++ b/zebra-consensus/src/block/check.rs @@ -31,7 +31,7 @@ pub fn coinbase_is_first(block: &Block) -> Result<(), BlockError> { return Err(TransactionError::CoinbasePosition)?; } if rest.any(|tx| tx.contains_coinbase_input()) { - return Err(TransactionError::CoinbaseInputFound)?; + return Err(TransactionError::CoinbaseAfterFirst)?; } Ok(()) diff --git a/zebra-consensus/src/block/tests.rs b/zebra-consensus/src/block/tests.rs index e9675ce76d1..ad5e0715240 100644 --- a/zebra-consensus/src/block/tests.rs +++ b/zebra-consensus/src/block/tests.rs @@ -356,7 +356,7 @@ fn coinbase_validation_failure() -> Result<(), Report> { // Validate the block using coinbase_is_first let result = check::coinbase_is_first(&block).unwrap_err(); - let expected = BlockError::Transaction(TransactionError::CoinbaseInputFound); + let expected = BlockError::Transaction(TransactionError::CoinbaseAfterFirst); assert_eq!(expected, result); // Validate the block using subsidy_is_valid, which does not detect this error diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index 48cd020720e..d4e0d6a636a 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -24,7 +24,10 @@ pub enum TransactionError { CoinbasePosition, #[error("coinbase input found in non-coinbase transaction")] - CoinbaseInputFound, + CoinbaseAfterFirst, + + #[error("coinbase transaction MUST NOT have any transparent (PrevOut) inputs")] + CoinbaseHasPrevOutInput, #[error("coinbase transaction MUST NOT have any JoinSplit descriptions")] CoinbaseHasJoinSplit, diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 28bd0ad0057..1489e93d330 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -162,7 +162,7 @@ where // Handle transparent inputs and outputs. if tx.is_coinbase() { - check::coinbase_tx_no_joinsplit_or_spend(&tx)?; + check::coinbase_tx_no_prevout_joinsplit_spend(&tx)?; } else { // feed all of the inputs to the script and shielded verifiers // the script_verifier also checks transparent sighashes, using its own implementation diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 7981b11b5db..29b7c2be7a5 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -101,12 +101,20 @@ where } } -/// Check that a coinbase tx does not have any JoinSplit or Spend descriptions. +/// Check that a coinbase transaction has no PrevOut inputs, JoinSplits, or spends. +/// +/// A coinbase transaction MUST NOT have any transparent inputs, JoinSplit descriptions, +/// or Spend descriptions. +/// +/// In a version 5 coinbase transaction, the enableSpendsOrchard flag MUST be 0. /// /// https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus -pub fn coinbase_tx_no_joinsplit_or_spend(tx: &Transaction) -> Result<(), TransactionError> { +pub fn coinbase_tx_no_prevout_joinsplit_spend(tx: &Transaction) -> Result<(), TransactionError> { if tx.is_coinbase() { match tx { + // In Zebra, `inputs` contains both `PrevOut` (transparent) and `Coinbase` inputs. + tx if tx.contains_prevout_input() => Err(TransactionError::CoinbaseHasPrevOutInput), + // Check if there is any JoinSplitData. Transaction::V4 { joinsplit_data: Some(_), @@ -129,6 +137,8 @@ pub fn coinbase_tx_no_joinsplit_or_spend(tx: &Transaction) -> Result<(), Transac Err(TransactionError::CoinbaseHasSpend) } + // TODO: Orchard validation (#1980) + // In a version 5 coinbase transaction, the enableSpendsOrchard flag MUST be 0. Transaction::V4 { .. } | Transaction::V5 { .. } => Ok(()), Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => { diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 1c75b1c671b..f3ff4012283 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -38,7 +38,7 @@ fn v5_fake_transactions() -> Result<(), Report> { }; // make sure there are no joinsplits nor spends in coinbase - super::check::coinbase_tx_no_joinsplit_or_spend(&transaction)?; + super::check::coinbase_tx_no_prevout_joinsplit_spend(&transaction)?; // validate the sapling shielded data match transaction { From 23bc51f48209c295707cd17819dccd76f8bf424d Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 27 Apr 2021 10:47:24 +1000 Subject: [PATCH 4/7] Add spec and orchard TODOs to has_inputs_and_outputs Also make the variable names match the spec. --- zebra-consensus/src/transaction/check.rs | 39 +++++++++++++++--------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 29b7c2be7a5..3782faec0dd 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -11,10 +11,13 @@ use crate::error::TransactionError; /// Checks that the transaction has inputs and outputs. /// -/// More specifically: +/// For `Transaction::V4`: +/// * at least one of `tx_in_count`, `nSpendsSapling`, and `nJoinSplit` MUST be non-zero. +/// * at least one of `tx_out_count`, `nOutputsSapling`, and `nJoinSplit` MUST be non-zero. /// -/// * at least one of tx_in_count, nShieldedSpend, and nJoinSplit MUST be non-zero. -/// * at least one of tx_out_count, nShieldedOutput, and nJoinSplit MUST be non-zero. +/// For `Transaction::V5`: +/// * at least one of `tx_in_count`, `nSpendsSapling`, and `nActionsOrchard` MUST be non-zero. +/// * at least one of `tx_out_count`, `nOutputsSapling`, and `nActionsOrchard` MUST be non-zero. /// /// https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus pub fn has_inputs_and_outputs(tx: &Transaction) -> Result<(), TransactionError> { @@ -22,6 +25,7 @@ pub fn has_inputs_and_outputs(tx: &Transaction) -> Result<(), TransactionError> // hold enum'd data. Mixing pattern matching and numerical checks is risky, // so convert everything to counts and sum up. match tx { + // In Zebra, `inputs` contains both `PrevOut` (transparent) and `Coinbase` inputs. Transaction::V4 { inputs, outputs, @@ -35,51 +39,58 @@ pub fn has_inputs_and_outputs(tx: &Transaction) -> Result<(), TransactionError> .as_ref() .map(|d| d.joinsplits().count()) .unwrap_or(0); - let n_shielded_spend = sapling_shielded_data + let n_spends_sapling = sapling_shielded_data .as_ref() .map(|d| d.spends().count()) .unwrap_or(0); - let n_shielded_output = sapling_shielded_data + let n_outputs_sapling = sapling_shielded_data .as_ref() .map(|d| d.outputs().count()) .unwrap_or(0); - if tx_in_count + n_shielded_spend + n_joinsplit == 0 { + if tx_in_count + n_spends_sapling + n_joinsplit == 0 { Err(TransactionError::NoInputs) - } else if tx_out_count + n_shielded_output + n_joinsplit == 0 { + } else if tx_out_count + n_outputs_sapling + n_joinsplit == 0 { Err(TransactionError::NoOutputs) } else { Ok(()) } } - Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => { - unreachable!("tx version is checked first") - } + Transaction::V5 { inputs, outputs, sapling_shielded_data, + // TODO: Orchard validation (#1980) .. } => { let tx_in_count = inputs.len(); let tx_out_count = outputs.len(); - let n_shielded_spend = sapling_shielded_data + let n_spends_sapling = sapling_shielded_data .as_ref() .map(|d| d.spends().count()) .unwrap_or(0); - let n_shielded_output = sapling_shielded_data + let n_outputs_sapling = sapling_shielded_data .as_ref() .map(|d| d.outputs().count()) .unwrap_or(0); - if tx_in_count + n_shielded_spend == 0 { + // TODO: Orchard validation (#1980) + // For `Transaction::V5`: + // * at least one of `tx_in_count`, `nSpendsSapling`, and `nActionsOrchard` MUST be non-zero. + // * at least one of `tx_out_count`, `nOutputsSapling`, and `nActionsOrchard` MUST be non-zero. + if tx_in_count + n_spends_sapling == 0 { Err(TransactionError::NoInputs) - } else if tx_out_count + n_shielded_output == 0 { + } else if tx_out_count + n_outputs_sapling == 0 { Err(TransactionError::NoOutputs) } else { Ok(()) } } + + Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => { + unreachable!("tx version is checked first") + } } } From e298dd00e5b81c4c23405974a349958253bf6eb8 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 27 Apr 2021 11:01:37 +1000 Subject: [PATCH 5/7] Sort transaction functions to match v5 data order --- zebra-chain/src/transaction.rs | 146 ++++++++++++++++++--------------- 1 file changed, 79 insertions(+), 67 deletions(-) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index 4de9611b075..161839f73f0 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -113,11 +113,63 @@ pub enum Transaction { } impl Transaction { - /// Compute the hash of this transaction. + // hashes + + /// Compute the hash (id) of this transaction. pub fn hash(&self) -> Hash { Hash::from(self) } + /// Calculate the sighash for the current transaction + /// + /// # Details + /// + /// The `input` argument indicates the transparent Input for which we are + /// producing a sighash. It is comprised of the index identifying the + /// transparent::Input within the transaction and the transparent::Output + /// representing the UTXO being spent by that input. + /// + /// # Panics + /// + /// - if passed in any NetworkUpgrade from before NetworkUpgrade::Overwinter + /// - 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() + pub fn sighash( + &self, + network_upgrade: NetworkUpgrade, + hash_type: sighash::HashType, + input: Option<(u32, transparent::Output)>, + ) -> blake2b_simd::Hash { + sighash::SigHasher::new(self, hash_type, network_upgrade, input).sighash() + } + + // header + + /// Get this transaction's lock time. + pub fn lock_time(&self) -> LockTime { + match self { + Transaction::V1 { lock_time, .. } => *lock_time, + Transaction::V2 { lock_time, .. } => *lock_time, + Transaction::V3 { lock_time, .. } => *lock_time, + Transaction::V4 { lock_time, .. } => *lock_time, + Transaction::V5 { lock_time, .. } => *lock_time, + } + } + + /// Get this transaction's expiry height, if any. + pub fn expiry_height(&self) -> Option { + match self { + Transaction::V1 { .. } => None, + Transaction::V2 { .. } => None, + Transaction::V3 { expiry_height, .. } => Some(*expiry_height), + Transaction::V4 { expiry_height, .. } => Some(*expiry_height), + Transaction::V5 { expiry_height, .. } => Some(*expiry_height), + } + } + + // transparent + /// Access the transparent inputs of this transaction, regardless of version. pub fn inputs(&self) -> &[transparent::Input] { match self { @@ -140,28 +192,33 @@ impl Transaction { } } - /// Get this transaction's lock time. - pub fn lock_time(&self) -> LockTime { - match self { - Transaction::V1 { lock_time, .. } => *lock_time, - Transaction::V2 { lock_time, .. } => *lock_time, - Transaction::V3 { lock_time, .. } => *lock_time, - Transaction::V4 { lock_time, .. } => *lock_time, - Transaction::V5 { lock_time, .. } => *lock_time, - } + /// Returns `true` if this transaction is a coinbase transaction. + pub fn is_coinbase(&self) -> bool { + self.inputs().len() == 1 + && matches!( + self.inputs().get(0), + Some(transparent::Input::Coinbase { .. }) + ) } - /// Get this transaction's expiry height, if any. - pub fn expiry_height(&self) -> Option { - match self { - Transaction::V1 { .. } => None, - Transaction::V2 { .. } => None, - Transaction::V3 { expiry_height, .. } => Some(*expiry_height), - Transaction::V4 { expiry_height, .. } => Some(*expiry_height), - Transaction::V5 { expiry_height, .. } => Some(*expiry_height), - } + /// Returns `true` if transaction contains any coinbase inputs. + pub fn contains_coinbase_input(&self) -> bool { + self.inputs() + .iter() + .any(|input| matches!(input, transparent::Input::Coinbase { .. })) } + /// Returns `true` if transaction contains any `PrevOut` inputs. + /// + /// `PrevOut` inputs are also known as `transparent` inputs in the spec. + pub fn contains_prevout_input(&self) -> bool { + self.inputs() + .iter() + .any(|input| matches!(input, transparent::Input::PrevOut { .. })) + } + + // sprout + /// Access the sprout::Nullifiers in this transaction, regardless of version. pub fn sprout_nullifiers(&self) -> Box + '_> { // This function returns a boxed iterator because the different @@ -201,6 +258,8 @@ impl Transaction { } } + // sapling + /// Access the sapling::Nullifiers in this transaction, regardless of version. pub fn sapling_nullifiers(&self) -> Box + '_> { // This function returns a boxed iterator because the different @@ -231,52 +290,5 @@ impl Transaction { } } - /// Returns `true` if transaction contains any coinbase inputs. - pub fn contains_coinbase_input(&self) -> bool { - self.inputs() - .iter() - .any(|input| matches!(input, transparent::Input::Coinbase { .. })) - } - - /// Returns `true` if transaction contains any `PrevOut` inputs. - /// - /// `PrevOut` inputs are also known as `transparent` inputs in the spec. - pub fn contains_prevout_input(&self) -> bool { - self.inputs() - .iter() - .any(|input| matches!(input, transparent::Input::PrevOut { .. })) - } - - /// Returns `true` if this transaction is a coinbase transaction. - pub fn is_coinbase(&self) -> bool { - self.inputs().len() == 1 - && matches!( - self.inputs().get(0), - Some(transparent::Input::Coinbase { .. }) - ) - } - - /// Calculate the sighash for the current transaction - /// - /// # Details - /// - /// The `input` argument indicates the transparent Input for which we are - /// producing a sighash. It is comprised of the index identifying the - /// transparent::Input within the transaction and the transparent::Output - /// representing the UTXO being spent by that input. - /// - /// # Panics - /// - /// - if passed in any NetworkUpgrade from before NetworkUpgrade::Overwinter - /// - 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() - pub fn sighash( - &self, - network_upgrade: NetworkUpgrade, - hash_type: sighash::HashType, - input: Option<(u32, transparent::Output)>, - ) -> blake2b_simd::Hash { - sighash::SigHasher::new(self, hash_type, network_upgrade, input).sighash() - } + // TODO: orchard } From 3a3afe7f98bc71ebcdbaab9a5d3838ebd4b9b2ec Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 27 Apr 2021 11:10:21 +1000 Subject: [PATCH 6/7] Simplify transaction input and output checks Move counts or iterators into `Transaction` methods, so we can remove duplicate code, and make the consensus rule logic clearer. --- zebra-chain/src/transaction.rs | 98 ++++++++++++++++ zebra-consensus/src/transaction/check.rs | 137 ++++++----------------- 2 files changed, 130 insertions(+), 105 deletions(-) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index 161839f73f0..6b8b1042ddc 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -219,6 +219,41 @@ impl Transaction { // sprout + /// Returns the number of `JoinSplit`s in this transaction, regardless of version. + pub fn joinsplit_count(&self) -> usize { + match self { + // JoinSplits with Bctv14 Proofs + Transaction::V2 { + joinsplit_data: Some(joinsplit_data), + .. + } + | Transaction::V3 { + joinsplit_data: Some(joinsplit_data), + .. + } => joinsplit_data.joinsplits().count(), + // JoinSplits with Groth Proofs + Transaction::V4 { + joinsplit_data: Some(joinsplit_data), + .. + } => joinsplit_data.joinsplits().count(), + // No JoinSplits + Transaction::V1 { .. } + | Transaction::V2 { + joinsplit_data: None, + .. + } + | Transaction::V3 { + joinsplit_data: None, + .. + } + | Transaction::V4 { + joinsplit_data: None, + .. + } + | Transaction::V5 { .. } => 0, + } + } + /// Access the sprout::Nullifiers in this transaction, regardless of version. pub fn sprout_nullifiers(&self) -> Box + '_> { // This function returns a boxed iterator because the different @@ -260,6 +295,69 @@ impl Transaction { // sapling + /// Iterate over the sapling [`Spend`](sapling::Spend)s for this transaction, + /// returning `Spend` regardless of the underlying + /// transaction version. + /// + /// # Correctness + /// + /// Do not use this function for serialization. + pub fn sapling_spends_per_anchor( + &self, + ) -> Box> + '_> { + match self { + Transaction::V4 { + sapling_shielded_data: Some(sapling_shielded_data), + .. + } => Box::new(sapling_shielded_data.spends_per_anchor()), + Transaction::V5 { + sapling_shielded_data: Some(sapling_shielded_data), + .. + } => Box::new(sapling_shielded_data.spends_per_anchor()), + + // No Spends + Transaction::V1 { .. } + | Transaction::V2 { .. } + | Transaction::V3 { .. } + | Transaction::V4 { + sapling_shielded_data: None, + .. + } + | Transaction::V5 { + sapling_shielded_data: None, + .. + } => Box::new(std::iter::empty()), + } + } + + /// Iterate over the sapling [`Output`](sapling::Output)s for this + /// transaction + pub fn sapling_outputs(&self) -> Box + '_> { + match self { + Transaction::V4 { + sapling_shielded_data: Some(sapling_shielded_data), + .. + } => Box::new(sapling_shielded_data.outputs()), + Transaction::V5 { + sapling_shielded_data: Some(sapling_shielded_data), + .. + } => Box::new(sapling_shielded_data.outputs()), + + // No Outputs + Transaction::V1 { .. } + | Transaction::V2 { .. } + | Transaction::V3 { .. } + | Transaction::V4 { + sapling_shielded_data: None, + .. + } + | Transaction::V5 { + sapling_shielded_data: None, + .. + } => Box::new(std::iter::empty()), + } + } + /// Access the sapling::Nullifiers in this transaction, regardless of version. pub fn sapling_nullifiers(&self) -> Box + '_> { // This function returns a boxed iterator because the different diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 3782faec0dd..5bac3c5555a 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -19,78 +19,27 @@ use crate::error::TransactionError; /// * at least one of `tx_in_count`, `nSpendsSapling`, and `nActionsOrchard` MUST be non-zero. /// * at least one of `tx_out_count`, `nOutputsSapling`, and `nActionsOrchard` MUST be non-zero. /// +/// This check counts both `Coinbase` and `PrevOut` transparent inputs. +/// /// https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus pub fn has_inputs_and_outputs(tx: &Transaction) -> Result<(), TransactionError> { - // The consensus rule is written in terms of numbers, but our transactions - // hold enum'd data. Mixing pattern matching and numerical checks is risky, - // so convert everything to counts and sum up. - match tx { - // In Zebra, `inputs` contains both `PrevOut` (transparent) and `Coinbase` inputs. - Transaction::V4 { - inputs, - outputs, - joinsplit_data, - sapling_shielded_data, - .. - } => { - let tx_in_count = inputs.len(); - let tx_out_count = outputs.len(); - let n_joinsplit = joinsplit_data - .as_ref() - .map(|d| d.joinsplits().count()) - .unwrap_or(0); - let n_spends_sapling = sapling_shielded_data - .as_ref() - .map(|d| d.spends().count()) - .unwrap_or(0); - let n_outputs_sapling = sapling_shielded_data - .as_ref() - .map(|d| d.outputs().count()) - .unwrap_or(0); - - if tx_in_count + n_spends_sapling + n_joinsplit == 0 { - Err(TransactionError::NoInputs) - } else if tx_out_count + n_outputs_sapling + n_joinsplit == 0 { - Err(TransactionError::NoOutputs) - } else { - Ok(()) - } - } - - Transaction::V5 { - inputs, - outputs, - sapling_shielded_data, - // TODO: Orchard validation (#1980) - .. - } => { - let tx_in_count = inputs.len(); - let tx_out_count = outputs.len(); - let n_spends_sapling = sapling_shielded_data - .as_ref() - .map(|d| d.spends().count()) - .unwrap_or(0); - let n_outputs_sapling = sapling_shielded_data - .as_ref() - .map(|d| d.outputs().count()) - .unwrap_or(0); - - // TODO: Orchard validation (#1980) - // For `Transaction::V5`: - // * at least one of `tx_in_count`, `nSpendsSapling`, and `nActionsOrchard` MUST be non-zero. - // * at least one of `tx_out_count`, `nOutputsSapling`, and `nActionsOrchard` MUST be non-zero. - if tx_in_count + n_spends_sapling == 0 { - Err(TransactionError::NoInputs) - } else if tx_out_count + n_outputs_sapling == 0 { - Err(TransactionError::NoOutputs) - } else { - Ok(()) - } - } - - Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => { - unreachable!("tx version is checked first") - } + let tx_in_count = tx.inputs().len(); + let tx_out_count = tx.outputs().len(); + let n_joinsplit = tx.joinsplit_count(); + let n_spends_sapling = tx.sapling_spends_per_anchor().count(); + let n_outputs_sapling = tx.sapling_outputs().count(); + + // TODO: Orchard validation (#1980) + // For `Transaction::V5`: + // * at least one of `tx_in_count`, `nSpendsSapling`, and `nActionsOrchard` MUST be non-zero. + // * at least one of `tx_out_count`, `nOutputsSapling`, and `nActionsOrchard` MUST be non-zero. + + if tx_in_count + n_spends_sapling + n_joinsplit == 0 { + Err(TransactionError::NoInputs) + } else if tx_out_count + n_outputs_sapling + n_joinsplit == 0 { + Err(TransactionError::NoOutputs) + } else { + Ok(()) } } @@ -119,46 +68,24 @@ where /// /// In a version 5 coinbase transaction, the enableSpendsOrchard flag MUST be 0. /// +/// This check only counts `PrevOut` transparent inputs. +/// /// https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus pub fn coinbase_tx_no_prevout_joinsplit_spend(tx: &Transaction) -> Result<(), TransactionError> { if tx.is_coinbase() { - match tx { - // In Zebra, `inputs` contains both `PrevOut` (transparent) and `Coinbase` inputs. - tx if tx.contains_prevout_input() => Err(TransactionError::CoinbaseHasPrevOutInput), - - // Check if there is any JoinSplitData. - Transaction::V4 { - joinsplit_data: Some(_), - .. - } => Err(TransactionError::CoinbaseHasJoinSplit), - - // The ShieldedData contains both Spends and Outputs, and Outputs - // are allowed post-Heartwood, so we have to count Spends. - Transaction::V4 { - sapling_shielded_data: Some(sapling_shielded_data), - .. - } if sapling_shielded_data.spends().count() > 0 => { - Err(TransactionError::CoinbaseHasSpend) - } - - Transaction::V5 { - sapling_shielded_data: Some(sapling_shielded_data), - .. - } if sapling_shielded_data.spends().count() > 0 => { - Err(TransactionError::CoinbaseHasSpend) - } - - // TODO: Orchard validation (#1980) - // In a version 5 coinbase transaction, the enableSpendsOrchard flag MUST be 0. - Transaction::V4 { .. } | Transaction::V5 { .. } => Ok(()), - - Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => { - unreachable!("tx version is checked first") - } + if tx.contains_prevout_input() { + return Err(TransactionError::CoinbaseHasPrevOutInput); + } else if tx.joinsplit_count() > 0 { + return Err(TransactionError::CoinbaseHasJoinSplit); + } else if tx.sapling_spends_per_anchor().count() > 0 { + return Err(TransactionError::CoinbaseHasSpend); } - } else { - Ok(()) + + // TODO: Orchard validation (#1980) + // In a version 5 coinbase transaction, the enableSpendsOrchard flag MUST be 0. } + + Ok(()) } /// Check that a Spend description's cv and rk are not of small order, From acd86bab8c17113dc2ed754febc087cecc89cc24 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 27 Apr 2021 10:48:14 +1000 Subject: [PATCH 7/7] Update sapling_balances_match for Transaction v5 - Quote from the spec - Explain why the function is redunant for v5 - Rename the function so it's clear that it is sapling-specific --- zebra-consensus/src/transaction.rs | 2 +- zebra-consensus/src/transaction/check.rs | 19 +++++++++++++------ zebra-consensus/src/transaction/tests.rs | 2 +- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 28bd0ad0057..23d3a4c1212 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -215,7 +215,7 @@ where } if let Some(shielded_data) = sapling_shielded_data { - check::shielded_balances_match(&shielded_data)?; + check::sapling_balances_match(&shielded_data)?; for spend in shielded_data.spends_per_anchor() { // Consensus rule: cv and rk MUST NOT be of small diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 7981b11b5db..65ac55b5484 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -83,17 +83,24 @@ pub fn has_inputs_and_outputs(tx: &Transaction) -> Result<(), TransactionError> } } -/// Check that if there are no Spends or Outputs, that valueBalance is also 0. +/// Check that if there are no Spends or Outputs, the Sapling valueBalance is also 0. /// -/// https://zips.z.cash/protocol/protocol.pdf#consensusfrombitcoin -pub fn shielded_balances_match( - shielded_data: &ShieldedData, +/// If effectiveVersion = 4 and there are no Spend descriptions or Output descriptions, +/// then valueBalanceSapling MUST be 0. +/// +/// This check is redundant for `Transaction::V5`, because the transaction format +/// omits `valueBalanceSapling` when there are no spends and no outputs. But it's +/// simpler to just do the redundant check anyway. +/// +/// https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus +pub fn sapling_balances_match( + sapling_shielded_data: &ShieldedData, ) -> Result<(), TransactionError> where AnchorV: AnchorVariant + Clone, { - if (shielded_data.spends().count() + shielded_data.outputs().count() != 0) - || i64::from(shielded_data.value_balance) == 0 + if (sapling_shielded_data.spends().count() + sapling_shielded_data.outputs().count() != 0) + || i64::from(sapling_shielded_data.value_balance) == 0 { Ok(()) } else { diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 1c75b1c671b..4dba8d8f71c 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -47,7 +47,7 @@ fn v5_fake_transactions() -> Result<(), Report> { .. } => { if let Some(s) = sapling_shielded_data { - super::check::shielded_balances_match(&s)?; + super::check::sapling_balances_match(&s)?; for spend in s.spends_per_anchor() { super::check::spend_cv_rk_not_small_order(&spend)?