From 29893f2b9b9fcf6fbe0cf1ec153b1f0c5a8df20e Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Sun, 9 May 2021 22:31:45 -0300 Subject: [PATCH] Validate nConsensusBranchId (#2100) * validate nConsensusBranchId * add tests * fix bug in transaction_to_fake_v5 Co-authored-by: teor --- zebra-chain/src/transaction/arbitrary.rs | 21 +++-- zebra-chain/src/transaction/tests/vectors.rs | 27 +++++- zebra-consensus/src/block.rs | 2 +- zebra-consensus/src/block/check.rs | 38 ++++++++ zebra-consensus/src/block/tests.rs | 92 +++++++++++++++++++- zebra-consensus/src/checkpoint.rs | 2 +- zebra-consensus/src/error.rs | 3 + zebra-consensus/src/transaction/tests.rs | 21 ++++- 8 files changed, 188 insertions(+), 18 deletions(-) diff --git a/zebra-chain/src/transaction/arbitrary.rs b/zebra-chain/src/transaction/arbitrary.rs index 47da8040e..b7bf8216d 100644 --- a/zebra-chain/src/transaction/arbitrary.rs +++ b/zebra-chain/src/transaction/arbitrary.rs @@ -6,7 +6,7 @@ use proptest::{arbitrary::any, array, collection::vec, option, prelude::*}; use crate::{ amount::Amount, block, - parameters::NetworkUpgrade, + parameters::{Network, NetworkUpgrade}, primitives::{Bctv14Proof, Groth16Proof, ZkSnarkProof}, sapling, sprout, transparent, LedgerState, }; @@ -349,21 +349,24 @@ impl Arbitrary for Transaction { // Utility functions -/// The network upgrade for any fake transactions we will create. -const FAKE_NETWORK_UPGRADE: NetworkUpgrade = NetworkUpgrade::Nu5; - /// 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 { +pub fn transaction_to_fake_v5( + trans: &Transaction, + network: Network, + height: block::Height, +) -> Transaction { use Transaction::*; + let block_nu = NetworkUpgrade::current(network, height); + match trans { V1 { inputs, outputs, lock_time, } => V5 { - network_upgrade: FAKE_NETWORK_UPGRADE, + network_upgrade: block_nu, inputs: inputs.to_vec(), outputs: outputs.to_vec(), lock_time: *lock_time, @@ -376,7 +379,7 @@ pub fn transaction_to_fake_v5(trans: &Transaction) -> Transaction { lock_time, .. } => V5 { - network_upgrade: FAKE_NETWORK_UPGRADE, + network_upgrade: block_nu, inputs: inputs.to_vec(), outputs: outputs.to_vec(), lock_time: *lock_time, @@ -390,7 +393,7 @@ pub fn transaction_to_fake_v5(trans: &Transaction) -> Transaction { expiry_height, .. } => V5 { - network_upgrade: FAKE_NETWORK_UPGRADE, + network_upgrade: block_nu, inputs: inputs.to_vec(), outputs: outputs.to_vec(), lock_time: *lock_time, @@ -405,7 +408,7 @@ pub fn transaction_to_fake_v5(trans: &Transaction) -> Transaction { sapling_shielded_data, .. } => V5 { - network_upgrade: FAKE_NETWORK_UPGRADE, + network_upgrade: block_nu, inputs: inputs.to_vec(), outputs: outputs.to_vec(), lock_time: *lock_time, diff --git a/zebra-chain/src/transaction/tests/vectors.rs b/zebra-chain/src/transaction/tests/vectors.rs index 14f414b88..1ade0cfcb 100644 --- a/zebra-chain/src/transaction/tests/vectors.rs +++ b/zebra-chain/src/transaction/tests/vectors.rs @@ -1,7 +1,8 @@ use super::super::*; use crate::{ - block::{Block, MAX_BLOCK_BYTES}, + block::{Block, Height, MAX_BLOCK_BYTES}, + parameters::{Network, NetworkUpgrade}, serialization::{ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize}, }; @@ -164,11 +165,31 @@ fn empty_v4_round_trip() { fn fake_v5_round_trip() { zebra_test::init(); - for original_bytes in zebra_test::vectors::BLOCKS.iter() { + fake_v5_round_trip_for_network(Network::Mainnet); + fake_v5_round_trip_for_network(Network::Testnet); +} + +fn fake_v5_round_trip_for_network(network: Network) { + let block_iter = match network { + Network::Mainnet => zebra_test::vectors::MAINNET_BLOCKS.iter(), + Network::Testnet => zebra_test::vectors::TESTNET_BLOCKS.iter(), + }; + + for (height, original_bytes) in block_iter { let original_block = original_bytes .zcash_deserialize_into::() .expect("block is structurally valid"); + // skip blocks that are before overwinter as they will not have a valid consensus branch id + if *height + < NetworkUpgrade::Overwinter + .activation_height(network) + .expect("a valid height") + .0 + { + continue; + } + // skip this block if it only contains v5 transactions, // the block round-trip test covers it already if original_block @@ -184,7 +205,7 @@ fn fake_v5_round_trip() { .transactions .iter() .map(AsRef::as_ref) - .map(arbitrary::transaction_to_fake_v5) + .map(|t| arbitrary::transaction_to_fake_v5(t, network, Height(*height))) .map(Into::into) .collect(); diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index 4de70472d..433ef4e8e 100644 --- a/zebra-consensus/src/block.rs +++ b/zebra-consensus/src/block.rs @@ -161,7 +161,7 @@ where .map(|t| t.hash()) .collect::>(); - check::merkle_root_validity(&block, &transaction_hashes)?; + check::merkle_root_validity(network, &block, &transaction_hashes)?; // Since errors cause an early exit, try to do the // quick checks first. diff --git a/zebra-consensus/src/block/check.rs b/zebra-consensus/src/block/check.rs index f62afb2ca..495f6f1bd 100644 --- a/zebra-consensus/src/block/check.rs +++ b/zebra-consensus/src/block/check.rs @@ -168,10 +168,48 @@ pub fn time_is_valid_at( /// Check Merkle root validity. /// /// `transaction_hashes` is a precomputed list of transaction hashes. +/// +/// # Consensus rules: +/// +/// - The nConsensusBranchId field MUST match the consensus branch ID used for +/// SIGHASH transaction hashes, as specifed in [ZIP-244] ([7.1]). +/// - A SHA-256d hash in internal byte order. The merkle root is derived from the +/// hashes of all transactions included in this block, ensuring that none of +/// those transactions can be modified without modifying the header. [7.6] +/// +/// # Panics +/// +/// - If block does not have a coinbase transaction. +/// +/// [ZIP-244]: https://zips.z.cash/zip-0244 +/// [7.1]: https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus +/// [7.6]: https://zips.z.cash/protocol/nu5.pdf#blockheader pub fn merkle_root_validity( + network: Network, block: &Block, transaction_hashes: &[transaction::Hash], ) -> Result<(), BlockError> { + use transaction::Transaction; + let block_nu = + NetworkUpgrade::current(network, block.coinbase_height().expect("a valid height")); + + if block + .transactions + .iter() + .filter_map(|trans| match *trans.as_ref() { + Transaction::V5 { + network_upgrade, .. + } => Some(network_upgrade), + Transaction::V1 { .. } + | Transaction::V2 { .. } + | Transaction::V3 { .. } + | Transaction::V4 { .. } => None, + }) + .any(|trans_nu| trans_nu != block_nu) + { + return Err(BlockError::WrongTransactionConsensusBranchId); + } + let merkle_root = transaction_hashes.iter().cloned().collect(); if block.header.merkle_root != merkle_root { diff --git a/zebra-consensus/src/block/tests.rs b/zebra-consensus/src/block/tests.rs index 45359a57d..a8cafa987 100644 --- a/zebra-consensus/src/block/tests.rs +++ b/zebra-consensus/src/block/tests.rs @@ -13,8 +13,9 @@ use tower::buffer::Buffer; use zebra_chain::{ block::{self, Block, Height}, - parameters::Network, + parameters::{Network, NetworkUpgrade}, serialization::{ZcashDeserialize, ZcashDeserializeInto}, + transaction::{arbitrary::transaction_to_fake_v5, Transaction}, work::difficulty::{ExpandedDifficulty, INVALID_COMPACT_DIFFICULTY}, }; use zebra_test::transcript::{TransError, Transcript}; @@ -435,3 +436,92 @@ fn time_is_valid_for_historical_blocks() -> Result<(), Report> { Ok(()) } + +#[test] +fn merkle_root_is_valid() -> Result<(), Report> { + zebra_test::init(); + + // test all original blocks available, all blocks validate + merkle_root_is_valid_for_network(Network::Mainnet)?; + merkle_root_is_valid_for_network(Network::Testnet)?; + + // create and test fake blocks with v5 transactions, all blocks fail validation + merkle_root_fake_v5_for_network(Network::Mainnet)?; + merkle_root_fake_v5_for_network(Network::Testnet)?; + + Ok(()) +} + +fn merkle_root_is_valid_for_network(network: Network) -> Result<(), Report> { + let block_iter = match network { + Network::Mainnet => zebra_test::vectors::MAINNET_BLOCKS.iter(), + Network::Testnet => zebra_test::vectors::TESTNET_BLOCKS.iter(), + }; + + for (_height, block) in block_iter { + let block = block + .zcash_deserialize_into::() + .expect("block is structurally valid"); + + let transaction_hashes = block + .transactions + .iter() + .map(|tx| tx.hash()) + .collect::>(); + + check::merkle_root_validity(network, &block, &transaction_hashes) + .expect("merkle root should be valid for this block"); + } + + Ok(()) +} + +fn merkle_root_fake_v5_for_network(network: Network) -> Result<(), Report> { + let block_iter = match network { + Network::Mainnet => zebra_test::vectors::MAINNET_BLOCKS.iter(), + Network::Testnet => zebra_test::vectors::TESTNET_BLOCKS.iter(), + }; + + for (height, block) in block_iter { + let mut block = block + .zcash_deserialize_into::() + .expect("block is structurally valid"); + + // skip blocks that are before overwinter as they will not have a valid consensus branch id + if *height + < NetworkUpgrade::Overwinter + .activation_height(network) + .expect("a valid overwinter activation height") + .0 + { + continue; + } + + // convert all transactions from the block to V5 + let transactions: Vec> = block + .transactions + .iter() + .map(AsRef::as_ref) + .map(|t| transaction_to_fake_v5(t, network, Height(*height))) + .map(Into::into) + .collect(); + + block.transactions = transactions; + + let transaction_hashes = block + .transactions + .iter() + .map(|tx| tx.hash()) + .collect::>(); + + // Replace the merkle root so that it matches the modified transactions. + // This test provides some transaction id and merkle root coverage, + // but we also need to test against zcashd test vectors. + block.header.merkle_root = transaction_hashes.iter().cloned().collect(); + + check::merkle_root_validity(network, &block, &transaction_hashes) + .expect("merkle root should be valid for this block"); + } + + Ok(()) +} diff --git a/zebra-consensus/src/checkpoint.rs b/zebra-consensus/src/checkpoint.rs index 2ae92a7c2..0a4036972 100644 --- a/zebra-consensus/src/checkpoint.rs +++ b/zebra-consensus/src/checkpoint.rs @@ -464,7 +464,7 @@ where .map(|tx| tx.hash()) .collect::>(); - crate::block::check::merkle_root_validity(&block, &transaction_hashes)?; + crate::block::check::merkle_root_validity(self.network, &block, &transaction_hashes)?; Ok(height) } diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index d4e0d6a63..63f2ac998 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -146,4 +146,7 @@ pub enum BlockError { zebra_chain::work::difficulty::ExpandedDifficulty, zebra_chain::parameters::Network, ), + + #[error("transaction has wrong consensus branch id for block network upgrade")] + WrongTransactionConsensusBranchId, } diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 3481e5fa0..999483414 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -1,5 +1,6 @@ use zebra_chain::{ - block::Block, + block::{Block, Height}, + parameters::Network, serialization::ZcashDeserializeInto, transaction::{arbitrary::transaction_to_fake_v5, Transaction}, }; @@ -11,8 +12,22 @@ use color_eyre::eyre::Report; fn v5_fake_transactions() -> Result<(), Report> { zebra_test::init(); + v5_fake_transactions_for_network(Network::Mainnet)?; + v5_fake_transactions_for_network(Network::Testnet)?; + + Ok(()) +} + +fn v5_fake_transactions_for_network(network: Network) -> Result<(), Report> { + zebra_test::init(); + // get all the blocks we have available - for original_bytes in zebra_test::vectors::BLOCKS.iter() { + let block_iter = match network { + Network::Mainnet => zebra_test::vectors::MAINNET_BLOCKS.iter(), + Network::Testnet => zebra_test::vectors::TESTNET_BLOCKS.iter(), + }; + + for (height, original_bytes) in block_iter { let original_block = original_bytes .zcash_deserialize_into::() .expect("block is structurally valid"); @@ -22,7 +37,7 @@ fn v5_fake_transactions() -> Result<(), Report> { .transactions .iter() .map(AsRef::as_ref) - .map(transaction_to_fake_v5) + .map(|t| transaction_to_fake_v5(t, network, Height(*height))) .map(Into::into) .collect();