From 29621b6d2aac4a95a729a30cb493d3508daf1abe Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Mon, 21 Jun 2021 18:35:34 +0000 Subject: [PATCH 1/5] Refactor to create `verify_sprout_shielded_data` Move the join split verification code into a new `verify_sprout_shielded_data` helper method that returns an `AsyncChecks` set. --- zebra-consensus/src/transaction.rs | 71 +++++++++++++++++------------- 1 file changed, 40 insertions(+), 31 deletions(-) diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index c6ae2c2cb1c..428443fb047 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -235,7 +235,6 @@ where let mut spend_verifier = primitives::groth16::SPEND_VERIFIER.clone(); let mut output_verifier = primitives::groth16::OUTPUT_VERIFIER.clone(); - let mut ed25519_verifier = primitives::ed25519::VERIFIER.clone(); let mut redjubjub_verifier = primitives::redjubjub::VERIFIER.clone(); // A set of asynchronous checks which must all succeed. @@ -255,36 +254,10 @@ where let shielded_sighash = tx.sighash(upgrade, HashType::ALL, None); - 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. - - // Consensus rule: The joinSplitSig MUST represent a - // valid signature, under joinSplitPubKey, of the - // sighash. - // - // Queue the validation of the JoinSplit signature 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#sproutnonmalleability - // https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus - let rsp = ed25519_verifier.ready_and().await?.call( - ( - joinsplit_data.pub_key, - joinsplit_data.sig, - &shielded_sighash, - ) - .into(), - ); - - async_checks.push(rsp.boxed()); - } + async_checks.extend(Self::verify_sprout_shielded_data( + joinsplit_data, + &shielded_sighash, + )); if let Some(sapling_shielded_data) = sapling_shielded_data { for spend in sapling_shielded_data.spends_per_anchor() { @@ -489,6 +462,42 @@ where } } + /// Verifies a transaction's Sprout shielded join split data. + fn verify_sprout_shielded_data( + joinsplit_data: &Option>, + shielded_sighash: &blake2b_simd::Hash, + ) -> AsyncChecks { + let 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. + + // Consensus rule: The joinSplitSig MUST represent a + // valid signature, under joinSplitPubKey, of the + // sighash. + // + // Queue the validation of the JoinSplit signature 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#sproutnonmalleability + // https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus + let ed25519_verifier = primitives::ed25519::VERIFIER.clone(); + let ed25519_item = + (joinsplit_data.pub_key, joinsplit_data.sig, shielded_sighash).into(); + + checks.push(ed25519_verifier.oneshot(ed25519_item).boxed()); + } + + checks + } + /// Await a set of checks that should all succeed. /// /// If any of the checks fail, this method immediately returns the error and cancels all other From 36a6329364a8c3b7ac1fe9d6a4e2cceb7a2653d7 Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Wed, 23 Jun 2021 01:35:39 +0000 Subject: [PATCH 2/5] Test if signed V4 tx. join splits are accepted Create a fake V4 transaction with a dummy join split, and sign it appropriately. Check if the transaction verifier accepts the transaction. --- Cargo.lock | 1 + zebra-consensus/Cargo.toml | 1 + zebra-consensus/src/transaction/tests.rs | 118 ++++++++++++++++++++++- 3 files changed, 118 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b8cc399b498..84f9c457483 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4447,6 +4447,7 @@ dependencies = [ "lazy_static", "metrics", "once_cell", + "rand 0.7.3", "rand 0.8.4", "serde", "spandoc", diff --git a/zebra-consensus/Cargo.toml b/zebra-consensus/Cargo.toml index cd45be89615..30837567cf0 100644 --- a/zebra-consensus/Cargo.toml +++ b/zebra-consensus/Cargo.toml @@ -36,6 +36,7 @@ wagyu-zcash-parameters = "0.2.0" [dev-dependencies] color-eyre = "0.5.11" +rand07 = { package = "rand", version = "0.7" } spandoc = "0.2" tokio = { version = "0.3.6", features = ["full"] } tracing-error = "0.1.2" diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 1a64cd30fd9..a47fffe070e 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, convert::TryFrom, sync::Arc}; +use std::{collections::HashMap, convert::TryFrom, convert::TryInto, sync::Arc}; use tower::{service_fn, ServiceExt}; @@ -6,9 +6,12 @@ use zebra_chain::{ amount::Amount, block, orchard, parameters::{Network, NetworkUpgrade}, + primitives::{ed25519, x25519, Groth16Proof}, + serialization::ZcashDeserialize, + sprout, transaction::{ arbitrary::{fake_v5_transactions_for_network, insert_fake_orchard_shielded_data}, - Hash, LockTime, Transaction, + Hash, HashType, JoinSplitData, LockTime, Transaction, }, transparent, }; @@ -451,6 +454,64 @@ async fn v5_transaction_with_transparent_transfer_is_rejected_by_the_script() { ); } +/// Test if signed V4 transaction with a dummy [`sprout::JoinSplit`] is accepted. +/// +/// This test verifies if the transaction verifier correctly accepts a signed transaction. +#[tokio::test] +async fn v4_with_signed_sprout_transfer_is_accepted() { + 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"); + + // 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_bytes()), + _ => unreachable!("Mock transaction was created incorrectly"), + } + + // Store the expected resulting hash + let expected_hash = transaction.hash(); + + // Test 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); + + let result = verifier + .oneshot(Request::Block { + transaction: Arc::new(transaction), + known_utxos: Arc::new(HashMap::new()), + height: transaction_block_height, + }) + .await; + + assert_eq!(result, Ok(expected_hash)); +} + // Utility functions /// Create a mock transparent transfer to be included in a transaction. @@ -524,3 +585,56 @@ fn mock_transparent_transfer( (input, output, known_utxos) } + +/// Create a mock [`sprout::JoinSplit`] and include it in a [`transaction::JoinSplitData`]. +/// +/// This creates a dummy join split. By itself it is invalid, but it is useful for including in a +/// transaction to check the signatures. +/// +/// The [`transaction::JoinSplitData`] with the dummy [`sprout::JoinSplit`] is returned together +/// with the [`ed25519::SigningKey`] that can be used to create a signature to later add to the +/// returned join split data. +fn mock_sprout_join_split_data() -> (JoinSplitData, ed25519::SigningKey) { + // Prepare dummy inputs for the join split + let zero_amount = 0_i32 + .try_into() + .expect("Invalid JoinSplit transparent input"); + let anchor = sprout::tree::Root::default(); + let nullifier = sprout::note::Nullifier([0u8; 32]); + let commitment = sprout::commitment::NoteCommitment::from([0u8; 32]); + let ephemeral_key = + x25519::PublicKey::from(&x25519::EphemeralSecret::new(rand07::thread_rng())); + let random_seed = [0u8; 32]; + let mac = sprout::note::Mac::zcash_deserialize(&[0u8; 32][..]) + .expect("Failure to deserialize dummy MAC"); + let zkproof = Groth16Proof([0u8; 192]); + let encrypted_note = sprout::note::EncryptedNote([0u8; 601]); + + // Create an dummy join split + let joinsplit = sprout::JoinSplit { + vpub_old: zero_amount, + vpub_new: zero_amount, + anchor, + nullifiers: [nullifier; 2], + commitments: [commitment; 2], + ephemeral_key, + random_seed, + vmacs: [mac.clone(), mac], + zkproof, + enc_ciphertexts: [encrypted_note; 2], + }; + + // Create a usable signing key + let signing_key = ed25519::SigningKey::new(rand::thread_rng()); + let verification_key = ed25519::VerificationKey::from(&signing_key); + + // Populate join split data with the dummy join split. + let joinsplit_data = JoinSplitData { + first: joinsplit, + rest: vec![], + pub_key: verification_key.into(), + sig: [0u8; 64].into(), + }; + + (joinsplit_data, signing_key) +} From 27df12734ef3e0d1cb5943d1c8fa949c4f3a5b55 Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Wed, 23 Jun 2021 01:45:17 +0000 Subject: [PATCH 3/5] Test if unsigned V4 tx. joinsplit data is rejected Create a fake V4 transaction with a dummy join split. Do NOT sign this transaction's join split data, and check that the verifier rejects the transaction. --- zebra-consensus/src/transaction/tests.rs | 52 ++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index a47fffe070e..7fe6c2a2e73 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -512,6 +512,58 @@ async fn v4_with_signed_sprout_transfer_is_accepted() { assert_eq!(result, Ok(expected_hash)); } +/// Test if an unsigned V4 transaction with a dummy [`sprout::JoinSplit`] is rejected. +/// +/// This test verifies if the transaction verifier correctly rejects the transaction because of the +/// invalid signature. +#[tokio::test] +async fn v4_with_unsigned_sprout_transfer_is_rejected() { + 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"); + + // 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 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); + + let result = verifier + .oneshot(Request::Block { + transaction: Arc::new(transaction), + known_utxos: Arc::new(HashMap::new()), + height: transaction_block_height, + }) + .await; + + assert_eq!( + result, + // TODO: Fix error downcast + // Err(TransactionError::Ed25519(ed25519::Error::InvalidSignature)) + Err(TransactionError::InternalDowncastError( + "downcast to redjubjub::Error failed, original error: InvalidSignature".to_string() + )) + ); +} + // Utility functions /// Create a mock transparent transfer to be included in a transaction. From fa44c704eee9fc36eed4212782c5abaf573d6549 Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Thu, 24 Jun 2021 16:06:48 +0000 Subject: [PATCH 4/5] Join tests to share Tokio runtime Otherwise one of the tests might fail incorrectly because of a limitation in the test environment. `Batch` services spawn a task in the Tokio runtime, but separate tests can have separate runtimes, so sharing a `Batch` service can lead to the worker task only being available for one of the tests. --- zebra-consensus/src/transaction/tests.rs | 140 +++++++++-------------- 1 file changed, 55 insertions(+), 85 deletions(-) diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 7fe6c2a2e73..b49910b5499 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -454,11 +454,22 @@ async fn v5_transaction_with_transparent_transfer_is_rejected_by_the_script() { ); } -/// Test if signed V4 transaction with a dummy [`sprout::JoinSplit`] is accepted. +/// Tests transactions with Sprout transfers. /// -/// This test verifies if the transaction verifier correctly accepts a signed transaction. +/// This is actually two tests: +/// - Test if signed V4 transaction with a dummy [`sprout::JoinSplit`] is accepted. +/// - Test if an unsigned V4 transaction with a dummy [`sprout::JoinSplit`] is rejected. +/// +/// The first test verifies if the transaction verifier correctly accepts a signed transaction. The +/// second test verifies if the transaction verifier correctly rejects the transaction because of +/// the invalid signature. +/// +/// These tests are grouped together because of a limitation to test shared [`tower_batch::Batch`] +/// services. Such services spawn a Tokio task in the runtime, and `#[tokio::test]` can create a +/// separate runtime for each test. This means that the worker task is created for one test and +/// destroyed before the other gets a chance to use it. #[tokio::test] -async fn v4_with_signed_sprout_transfer_is_accepted() { +async fn v4_with_sprout_transfers() { let network = Network::Mainnet; let network_upgrade = NetworkUpgrade::Canopy; @@ -469,99 +480,58 @@ async fn v4_with_signed_sprout_transfer_is_accepted() { let transaction_block_height = (canopy_activation_height + 10).expect("Canopy activation height is too large"); - // 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_bytes()), - _ => unreachable!("Mock transaction was created incorrectly"), - } - - // Store the expected resulting hash - let expected_hash = transaction.hash(); - - // Test the verifier + // 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); - let result = verifier - .oneshot(Request::Block { - transaction: Arc::new(transaction), - known_utxos: Arc::new(HashMap::new()), - height: transaction_block_height, - }) - .await; - - assert_eq!(result, Ok(expected_hash)); -} - -/// Test if an unsigned V4 transaction with a dummy [`sprout::JoinSplit`] is rejected. -/// -/// This test verifies if the transaction verifier correctly rejects the transaction because of the -/// invalid signature. -#[tokio::test] -async fn v4_with_unsigned_sprout_transfer_is_rejected() { - 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"); + for should_sign in [true, false] { + // Create a fake Sprout join split + let (joinsplit_data, signing_key) = mock_sprout_join_split_data(); - let transaction_block_height = - (canopy_activation_height + 10).expect("Canopy activation height is too large"); + 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, + }; - // Create a fake Sprout join split - let (joinsplit_data, _) = mock_sprout_join_split_data(); + let expected_result = if should_sign { + // Sign the transaction + let sighash = transaction.sighash(network_upgrade, HashType::ALL, None); - 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, - }; + match &mut transaction { + Transaction::V4 { + joinsplit_data: Some(joinsplit_data), + .. + } => joinsplit_data.sig = signing_key.sign(sighash.as_bytes()), + _ => unreachable!("Mock transaction was created incorrectly"), + } - // Test 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); + Ok(transaction.hash()) + } else { + // TODO: Fix error downcast + // Err(TransactionError::Ed25519(ed25519::Error::InvalidSignature)) + Err(TransactionError::InternalDowncastError( + "downcast to redjubjub::Error failed, original error: InvalidSignature".to_string(), + )) + }; - let result = verifier - .oneshot(Request::Block { - transaction: Arc::new(transaction), - known_utxos: Arc::new(HashMap::new()), - height: transaction_block_height, - }) - .await; + // 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, + }) + .await; - assert_eq!( - result, - // TODO: Fix error downcast - // Err(TransactionError::Ed25519(ed25519::Error::InvalidSignature)) - Err(TransactionError::InternalDowncastError( - "downcast to redjubjub::Error failed, original error: InvalidSignature".to_string() - )) - ); + assert_eq!(result, expected_result); + } } // Utility functions From e8cc11ef2ff35b23ae0bf4357f7d30cf6b1f4e74 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 25 Jun 2021 10:29:28 +1000 Subject: [PATCH 5/5] Reference Zebra ticket --- zebra-consensus/src/transaction/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index b49910b5499..9f2922a9e20 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -467,7 +467,7 @@ async fn v5_transaction_with_transparent_transfer_is_rejected_by_the_script() { /// These tests are grouped together because of a limitation to test shared [`tower_batch::Batch`] /// services. Such services spawn a Tokio task in the runtime, and `#[tokio::test]` can create a /// separate runtime for each test. This means that the worker task is created for one test and -/// destroyed before the other gets a chance to use it. +/// destroyed before the other gets a chance to use it. (We'll fix this in #2390.) #[tokio::test] async fn v4_with_sprout_transfers() { let network = Network::Mainnet;