diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index b5dbf9e26..0234c8971 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -1020,7 +1020,7 @@ impl Transaction { } /// Returns the `vpub_old` fields from `JoinSplit`s in this transaction, - /// regardless of version. + /// regardless of version, in the order they appear in the transaction. /// /// These values are added to the sprout chain value pool, /// and removed from the value pool of this transaction. @@ -1067,7 +1067,7 @@ impl Transaction { } /// Modify the `vpub_old` fields from `JoinSplit`s in this transaction, - /// regardless of version. + /// regardless of version, in the order they appear in the transaction. /// /// See `output_values_to_sprout` for details. #[cfg(any(test, feature = "proptest-impl"))] @@ -1116,7 +1116,7 @@ impl Transaction { } /// Returns the `vpub_new` fields from `JoinSplit`s in this transaction, - /// regardless of version. + /// regardless of version, in the order they appear in the transaction. /// /// These values are removed from the value pool of this transaction. /// and added to the sprout chain value pool. @@ -1163,7 +1163,7 @@ impl Transaction { } /// Modify the `vpub_new` fields from `JoinSplit`s in this transaction, - /// regardless of version. + /// regardless of version, in the order they appear in the transaction. /// /// See `input_values_from_sprout` for details. #[cfg(any(test, feature = "proptest-impl"))] diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index a6e719dd2..a37d5f30d 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -130,10 +130,14 @@ pub enum TransactionError { #[error("spend description cv and rk MUST NOT be of small order")] SmallOrder, - // XXX change this when we align groth16 verifier errors with bellman - // and add a from annotation when the error type is more precise + // XXX: the underlying error is bellman::VerificationError, but it does not implement + // Arbitrary as required here. #[error("spend proof MUST be valid given a primary input formed from the other fields except spendAuthSig")] - Groth16, + Groth16(String), + + // XXX: the underlying error is io::Error, but it does not implement Clone as required here. + #[error("Groth16 proof is malformed")] + MalformedGroth16(String), #[error( "Sprout joinSplitSig MUST represent a valid signature under joinSplitPubKey of dataToBeSigned" @@ -153,6 +157,9 @@ pub enum TransactionError { #[error("Downcast from BoxError to redjubjub::Error failed")] InternalDowncastError(String), + #[error("either vpub_old or vpub_new must be zero")] + BothVPubsNonZero, + #[error("adding to the sprout pool is disabled after Canopy")] DisabledAddToSproutPool, diff --git a/zebra-consensus/src/primitives/groth16.rs b/zebra-consensus/src/primitives/groth16.rs index 46a1cd85e..5e7b4fa49 100644 --- a/zebra-consensus/src/primitives/groth16.rs +++ b/zebra-consensus/src/primitives/groth16.rs @@ -1,7 +1,8 @@ //! Async Groth16 batch verifier service use std::{ - convert::TryInto, + convert::{TryFrom, TryInto}, + error::Error, fmt, future::Future, mem, @@ -22,7 +23,7 @@ use tokio::sync::broadcast::{channel, error::RecvError, Sender}; use tower::{util::ServiceFn, Service}; use tower_batch::{Batch, BatchControl}; -use tower_fallback::Fallback; +use tower_fallback::{BoxedError, Fallback}; use zebra_chain::{ primitives::{ @@ -41,6 +42,8 @@ mod vectors; pub use params::{Groth16Parameters, GROTH16_PARAMETERS}; +use crate::error::TransactionError; + /// Global batch verification context for Groth16 proofs of Spend statements. /// /// This service transparently batches contemporaneous proof verifications, @@ -115,7 +118,7 @@ pub static OUTPUT_VERIFIER: Lazy< /// Note that making a `Service` call requires mutable access to the service, so /// you should call `.clone()` on the global handle to create a local, mutable /// handle. -pub static JOINSPLIT_VERIFIER: Lazy Ready>>> = +pub static JOINSPLIT_VERIFIER: Lazy Ready>>> = Lazy::new(|| { // We need a Service to use. The obvious way to do this would // be to write a closure that returns an async block. But because we @@ -126,19 +129,19 @@ pub static JOINSPLIT_VERIFIER: Lazy Ready _, ) }); -/// A Groth16 verification item, used as the request type of the service. -pub type Item = batch::Item; - -/// A wrapper to workaround the missing `ServiceExt::map_err` method. -pub struct ItemWrapper(Item); - /// A Groth16 Description (JoinSplit, Spend, or Output) with a Groth16 proof /// and its inputs encoded as scalars. pub trait Description { @@ -296,22 +299,26 @@ impl Description for (&JoinSplit, &ed25519::VerificationKeyBytes) } } -impl From<&T> for ItemWrapper +/// A Groth16 verification item, used as the request type of the service. +pub type Item = batch::Item; + +/// A wrapper to allow a TryFrom blanket implementation of the [`Description`] +/// trait for the [`Item`] struct. +/// See https://github.com/rust-lang/rust/issues/50133 for more details. +pub struct DescriptionWrapper(pub T); + +impl TryFrom> for Item where T: Description, { - /// Convert a [`Description`] into an [`ItemWrapper`]. - fn from(input: &T) -> Self { - Self(Item::from(( - bellman::groth16::Proof::read(&input.proof().0[..]).unwrap(), - input.primary_inputs(), - ))) - } -} + type Error = TransactionError; -impl From for Item { - fn from(item_wrapper: ItemWrapper) -> Self { - item_wrapper.0 + fn try_from(input: DescriptionWrapper<&T>) -> Result { + Ok(Item::from(( + bellman::groth16::Proof::read(&input.0.proof().0[..]) + .map_err(|e| TransactionError::MalformedGroth16(e.to_string()))?, + input.0.primary_inputs(), + ))) } } diff --git a/zebra-consensus/src/primitives/groth16/tests.rs b/zebra-consensus/src/primitives/groth16/tests.rs index bb6859b91..665df39ec 100644 --- a/zebra-consensus/src/primitives/groth16/tests.rs +++ b/zebra-consensus/src/primitives/groth16/tests.rs @@ -11,7 +11,7 @@ use zebra_chain::{ transaction::Transaction, }; -use crate::primitives::groth16::{self, *}; +use crate::primitives::groth16::*; async fn verify_groth16_spends_and_outputs( spend_verifier: &mut V, @@ -37,10 +37,11 @@ where for spend in spends { tracing::trace!(?spend); - let spend_rsp = spend_verifier - .ready() - .await? - .call(groth16::ItemWrapper::from(&spend).into()); + let spend_rsp = spend_verifier.ready().await?.call( + DescriptionWrapper(&spend) + .try_into() + .map_err(tower_fallback::BoxedError::from)?, + ); async_checks.push(spend_rsp); } @@ -48,10 +49,11 @@ where for output in outputs { tracing::trace!(?output); - let output_rsp = output_verifier - .ready() - .await? - .call(groth16::ItemWrapper::from(output).into()); + let output_rsp = output_verifier.ready().await?.call( + DescriptionWrapper(output) + .try_into() + .map_err(tower_fallback::BoxedError::from)?, + ); async_checks.push(output_rsp); } @@ -110,9 +112,21 @@ async fn verify_sapling_groth16() { .unwrap() } +#[derive(Clone, Copy)] +enum Groth16OutputModification { + ZeroCMU, + ZeroProof, +} + +static GROTH16_OUTPUT_MODIFICATIONS: [Groth16OutputModification; 2] = [ + Groth16OutputModification::ZeroCMU, + Groth16OutputModification::ZeroProof, +]; + async fn verify_invalid_groth16_output_description( output_verifier: &mut V, transactions: Vec>, + modification: Groth16OutputModification, ) -> Result<(), V::Error> where V: tower::Service, @@ -132,14 +146,18 @@ where // This changes the primary inputs to the proof // verification, causing it to fail for this proof. let mut modified_output = output.clone(); - modified_output.cm_u = jubjub::Fq::zero(); + match modification { + Groth16OutputModification::ZeroCMU => modified_output.cm_u = jubjub::Fq::zero(), + Groth16OutputModification::ZeroProof => modified_output.zkproof.0 = [0; 192], + } tracing::trace!(?modified_output); - let output_rsp = output_verifier - .ready() - .await? - .call(groth16::ItemWrapper::from(&modified_output).into()); + let output_rsp = output_verifier.ready().await?.call( + DescriptionWrapper(&modified_output) + .try_into() + .map_err(tower_fallback::BoxedError::from)?, + ); async_checks.push(output_rsp); } @@ -174,9 +192,15 @@ async fn correctly_err_on_invalid_output_proof() { .zcash_deserialize_into::() .expect("a valid block"); - verify_invalid_groth16_output_description(&mut output_verifier, block.transactions) + for modification in GROTH16_OUTPUT_MODIFICATIONS { + verify_invalid_groth16_output_description( + &mut output_verifier, + block.transactions.clone(), + modification, + ) .await .expect_err("unexpected success checking invalid groth16 inputs"); + } } async fn verify_groth16_joinsplits( @@ -204,10 +228,11 @@ where let pub_key = tx .sprout_joinsplit_pub_key() .expect("pub key must exist since there are joinsplits"); - let joinsplit_rsp = verifier - .ready() - .await? - .call(groth16::ItemWrapper::from(&(joinsplit, &pub_key)).into()); + let joinsplit_rsp = verifier.ready().await?.call( + DescriptionWrapper(&(joinsplit, &pub_key)) + .try_into() + .map_err(tower_fallback::BoxedError::from)?, + ); async_checks.push(joinsplit_rsp); } @@ -268,10 +293,11 @@ where tracing::trace!(?joinsplit); - let joinsplit_rsp = verifier - .ready() - .await? - .call(groth16::ItemWrapper::from(&(joinsplit, pub_key)).into()); + let joinsplit_rsp = verifier.ready().await?.call( + DescriptionWrapper(&(joinsplit, pub_key)) + .try_into() + .map_err(tower_fallback::BoxedError::from)?, + ); async_checks.push(joinsplit_rsp); @@ -388,10 +414,11 @@ where // Use an arbitrary public key which is not the correct one, // which will make the verification fail. let modified_pub_key = [0x42; 32].into(); - let joinsplit_rsp = verifier - .ready() - .await? - .call(groth16::ItemWrapper::from(&(joinsplit, &modified_pub_key)).into()); + let joinsplit_rsp = verifier.ready().await?.call( + DescriptionWrapper(&(joinsplit, &modified_pub_key)) + .try_into() + .map_err(tower_fallback::BoxedError::from)?, + ); async_checks.push(joinsplit_rsp); } diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 47d88fd41..734521e68 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -2,6 +2,7 @@ //! use std::{ collections::HashMap, + convert::TryInto, future::Future, iter::FromIterator, pin::Pin, @@ -33,7 +34,7 @@ use zebra_chain::{ use zebra_script::CachedFfiTransaction; use zebra_state as zs; -use crate::{error::TransactionError, primitives, script, BoxError}; +use crate::{error::TransactionError, groth16::DescriptionWrapper, primitives, script, BoxError}; pub mod check; #[cfg(test)] @@ -314,6 +315,13 @@ where check::non_coinbase_expiry_height(&req.height(), &tx)?; } + // Consensus rule: + // + // > Either v_{pub}^{old} or v_{pub}^{new} MUST be zero. + // + // https://zips.z.cash/protocol/protocol.pdf#joinsplitdesc + check::joinsplit_has_vpub_zero(&tx)?; + // [Canopy onward]: `vpub_old` MUST be zero. // https://zips.z.cash/protocol/protocol.pdf#joinsplitdesc check::disabled_add_to_sprout_pool(&tx, req.height(), network)?; @@ -461,7 +469,7 @@ where .and(Self::verify_sprout_shielded_data( joinsplit_data, &shielded_sighash, - )) + )?) .and(Self::verify_sapling_shielded_data( sapling_shielded_data, &shielded_sighash, @@ -632,16 +640,25 @@ where fn verify_sprout_shielded_data( joinsplit_data: &Option>, shielded_sighash: &SigHash, - ) -> AsyncChecks { + ) -> Result { let mut checks = AsyncChecks::new(); if let Some(joinsplit_data) = joinsplit_data { - // XXX create a method on JoinSplitData - // that prepares groth16::Items with the correct proofs - // and proof inputs, handling interstitial treestates - // correctly. - - // Then, pass those items to self.joinsplit to verify them. + for joinsplit in joinsplit_data.joinsplits() { + // Consensus rule: The proof π_ZKSpend MUST be valid given a + // primary input formed from the relevant other fields and h_{Sig} + // + // Queue the verification of the Groth16 spend proof + // for each JoinSplit description while adding the + // resulting future to our collection of async + // checks that (at a minimum) must pass for the + // transaction to verify. + // + // https://zips.z.cash/protocol/protocol.pdf#joinsplitdesc + checks.push(primitives::groth16::JOINSPLIT_VERIFIER.oneshot( + DescriptionWrapper(&(joinsplit, &joinsplit_data.pub_key)).try_into()?, + )); + } // Consensus rule: The joinSplitSig MUST represent a // valid signature, under joinSplitPubKey, of the @@ -661,7 +678,7 @@ where checks.push(ed25519_verifier.oneshot(ed25519_item)); } - checks + Ok(checks) } /// Verifies a transaction's Sapling shielded data. @@ -696,7 +713,7 @@ where async_checks.push( primitives::groth16::SPEND_VERIFIER .clone() - .oneshot(primitives::groth16::ItemWrapper::from(&spend).into()), + .oneshot(DescriptionWrapper(&spend).try_into()?), ); // Consensus rule: The spend authorization signature @@ -735,7 +752,7 @@ where async_checks.push( primitives::groth16::OUTPUT_VERIFIER .clone() - .oneshot(primitives::groth16::ItemWrapper::from(output).into()), + .oneshot(DescriptionWrapper(output).try_into()?), ); } diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 15d9ba6a3..a20ba6b1b 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -164,6 +164,27 @@ pub fn output_cv_epk_not_small_order(output: &Output) -> Result<(), TransactionE } } +/// Check if JoinSplits in the transaction have one of its v_{pub} values equal +/// to zero. +/// +/// +pub fn joinsplit_has_vpub_zero(tx: &Transaction) -> Result<(), TransactionError> { + let zero = Amount::::try_from(0).expect("an amount of 0 is always valid"); + + let vpub_pairs = tx + .output_values_to_sprout() + .zip(tx.input_values_from_sprout()); + for (vpub_old, vpub_new) in vpub_pairs { + // > Either v_{pub}^{old} or v_{pub}^{new} MUST be zero. + // https://zips.z.cash/protocol/protocol.pdf#joinsplitdesc + if *vpub_old != zero && *vpub_new != zero { + return Err(TransactionError::BothVPubsNonZero); + } + } + + Ok(()) +} + /// Check if a transaction is adding to the sprout pool after Canopy /// network upgrade given a block height and a network. /// diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index d7ba64eb1..8024deead 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -904,14 +904,17 @@ fn v4_with_signed_sprout_transfer_is_accepted() { zebra_test::init(); zebra_test::RUNTIME.block_on(async { let network = Network::Mainnet; - let network_upgrade = NetworkUpgrade::Canopy; - let canopy_activation_height = network_upgrade - .activation_height(network) - .expect("Canopy activation height is not set"); + let (height, transaction) = test_transactions(network) + .rev() + .filter(|(_, transaction)| { + !transaction.has_valid_coinbase_transaction_inputs() + && transaction.inputs().is_empty() + }) + .find(|(_, transaction)| transaction.sprout_groth16_joinsplits().next().is_some()) + .expect("No transaction found with Groth16 JoinSplits"); - let transaction_block_height = - (canopy_activation_height + 10).expect("Canopy activation height is too large"); + let expected_hash = transaction.unmined_id(); // Initialize the verifier let state_service = @@ -919,38 +922,13 @@ fn v4_with_signed_sprout_transfer_is_accepted() { let script_verifier = script::Verifier::new(state_service); let verifier = Verifier::new(network, script_verifier); - // Create a fake Sprout join split - let (joinsplit_data, signing_key) = mock_sprout_join_split_data(); - - let mut transaction = Transaction::V4 { - inputs: vec![], - outputs: vec![], - lock_time: LockTime::Height(block::Height(0)), - expiry_height: (transaction_block_height + 1).expect("expiry height is too large"), - joinsplit_data: Some(joinsplit_data), - sapling_shielded_data: None, - }; - - // Sign the transaction - let sighash = transaction.sighash(network_upgrade, HashType::ALL, None); - - match &mut transaction { - Transaction::V4 { - joinsplit_data: Some(joinsplit_data), - .. - } => joinsplit_data.sig = signing_key.sign(sighash.as_ref()), - _ => unreachable!("Mock transaction was created incorrectly"), - } - - let expected_hash = transaction.unmined_id(); - // Test the transaction verifier let result = verifier .clone() .oneshot(Request::Block { - transaction: Arc::new(transaction), + transaction, known_utxos: Arc::new(HashMap::new()), - height: transaction_block_height, + height, time: chrono::MAX_DATETIME, }) .await; @@ -962,67 +940,76 @@ fn v4_with_signed_sprout_transfer_is_accepted() { }); } -/// Test if an unsigned V4 transaction with a dummy [`sprout::JoinSplit`] is rejected. +/// Test if an V4 transaction with a modified [`sprout::JoinSplit`] is rejected. /// /// This test verifies if the transaction verifier correctly rejects the transaction because of the -/// invalid signature. +/// invalid JoinSplit. #[test] -fn v4_with_unsigned_sprout_transfer_is_rejected() { +fn v4_with_modified_joinsplit_is_rejected() { zebra_test::init(); zebra_test::RUNTIME.block_on(async { - let network = Network::Mainnet; - let network_upgrade = NetworkUpgrade::Canopy; - - let canopy_activation_height = network_upgrade - .activation_height(network) - .expect("Canopy activation height is not set"); - - let transaction_block_height = - (canopy_activation_height + 10).expect("Canopy activation height is too large"); - - // Initialize the verifier - let state_service = - service_fn(|_| async { unreachable!("State service should not be called") }); - let script_verifier = script::Verifier::new(state_service); - let verifier = Verifier::new(network, script_verifier); - - // Create a fake Sprout join split - let (joinsplit_data, _) = mock_sprout_join_split_data(); - - let transaction = Transaction::V4 { - inputs: vec![], - outputs: vec![], - lock_time: LockTime::Height(block::Height(0)), - expiry_height: (transaction_block_height + 1).expect("expiry height is too large"), - joinsplit_data: Some(joinsplit_data), - sapling_shielded_data: None, - }; - - // Test the transaction verifier - let result = verifier - .clone() - .oneshot(Request::Block { - transaction: Arc::new(transaction), - known_utxos: Arc::new(HashMap::new()), - height: transaction_block_height, - time: chrono::MAX_DATETIME, - }) - .await; - - assert_eq!( - result, - Err( - // TODO: Fix error downcast - // Err(TransactionError::Ed25519(ed25519::Error::InvalidSignature)) - TransactionError::InternalDowncastError( - "downcast to known transaction error type failed, original error: InvalidSignature" - .to_string(), - ) - ) - ); + v4_with_joinsplit_is_rejected_for_modification( + JoinSplitModification::CorruptSignature, + // TODO: Fix error downcast + // Err(TransactionError::Ed25519(ed25519::Error::InvalidSignature)) + TransactionError::InternalDowncastError( + "downcast to known transaction error type failed, original error: InvalidSignature" + .to_string(), + ), + ) + .await; + v4_with_joinsplit_is_rejected_for_modification( + JoinSplitModification::CorruptProof, + TransactionError::Groth16("proof verification failed".to_string()), + ) + .await; + v4_with_joinsplit_is_rejected_for_modification( + JoinSplitModification::ZeroProof, + TransactionError::MalformedGroth16("invalid G1".to_string()), + ) + .await; }); } +async fn v4_with_joinsplit_is_rejected_for_modification( + modification: JoinSplitModification, + expected_error: TransactionError, +) { + let network = Network::Mainnet; + + let (height, mut transaction) = test_transactions(network) + .rev() + .filter(|(_, transaction)| { + !transaction.has_valid_coinbase_transaction_inputs() && transaction.inputs().is_empty() + }) + .find(|(_, transaction)| transaction.sprout_groth16_joinsplits().next().is_some()) + .expect("No transaction found with Groth16 JoinSplits"); + + modify_joinsplit( + Arc::get_mut(&mut transaction).expect("Transaction only has one active reference"), + modification, + ); + + // Initialize the verifier + let state_service = + service_fn(|_| async { unreachable!("State service should not be called") }); + let script_verifier = script::Verifier::new(state_service); + let verifier = Verifier::new(network, script_verifier); + + // Test the transaction verifier + let result = verifier + .clone() + .oneshot(Request::Block { + transaction, + known_utxos: Arc::new(HashMap::new()), + height, + time: chrono::MAX_DATETIME, + }) + .await; + + assert_eq!(result, Err(expected_error)); +} + /// Test if a V4 transaction with Sapling spends is accepted by the verifier. #[test] fn v4_with_sapling_spends() { @@ -1476,6 +1463,63 @@ fn mock_sprout_join_split_data() -> (JoinSplitData, ed25519::Signi (joinsplit_data, signing_key) } +/// A type of JoinSplit modification to test. +#[derive(Clone, Copy)] +enum JoinSplitModification { + // Corrupt a signature, making it invalid. + CorruptSignature, + // Corrupt a proof, making it invalid, but still well-formed. + CorruptProof, + // Make a proof all-zeroes, making it malformed. + ZeroProof, +} + +/// Modify a JoinSplit in the transaction following the given modification type. +fn modify_joinsplit(transaction: &mut Transaction, modification: JoinSplitModification) { + match transaction { + Transaction::V4 { + joinsplit_data: Some(ref mut joinsplit_data), + .. + } => modify_joinsplit_data(joinsplit_data, modification), + _ => unreachable!("Transaction has no JoinSplit shielded data"), + } +} + +/// Modify a [`JoinSplitData`] following the given modification type. +fn modify_joinsplit_data( + joinsplit_data: &mut JoinSplitData, + modification: JoinSplitModification, +) { + match modification { + JoinSplitModification::CorruptSignature => { + let mut sig_bytes: [u8; 64] = joinsplit_data.sig.into(); + // Flip a bit from an arbitrary byte of the signature. + sig_bytes[10] ^= 0x01; + joinsplit_data.sig = sig_bytes.into(); + } + JoinSplitModification::CorruptProof => { + let joinsplit = joinsplit_data + .joinsplits_mut() + .next() + .expect("must have a JoinSplit"); + { + // A proof is composed of three field elements, the first and last having 48 bytes. + // (The middle one has 96 bytes.) To corrupt the proof without making it malformed, + // simply swap those first and last elements. + let (first, rest) = joinsplit.zkproof.0.split_at_mut(48); + first.swap_with_slice(&mut rest[96..144]); + } + } + JoinSplitModification::ZeroProof => { + let joinsplit = joinsplit_data + .joinsplits_mut() + .next() + .expect("must have a JoinSplit"); + joinsplit.zkproof.0 = [0; 192]; + } + } +} + /// Duplicate a Sapling spend inside a `transaction`. /// /// Returns the nullifier of the duplicate spend.