From a39dbc43d29eced87abd43f00d404efd659efa07 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Wed, 11 Aug 2021 11:41:49 -0300 Subject: [PATCH 01/16] Add validation of ZIP-221 and ZIP-244 commitments --- Cargo.lock | 1 + zebra-chain/src/block/commitment.rs | 12 ++ zebra-chain/src/block/merkle.rs | 12 ++ zebra-chain/src/history_tree.rs | 5 + zebra-state/Cargo.toml | 1 + zebra-state/src/error.rs | 3 + zebra-state/src/service.rs | 2 +- zebra-state/src/service/arbitrary.rs | 4 +- zebra-state/src/service/check.rs | 89 ++++++++++++-- .../src/service/non_finalized_state.rs | 5 + .../non_finalized_state/tests/vectors.rs | 112 +++++++++++++++++- zebra-state/src/tests.rs | 8 ++ 12 files changed, 237 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f28cbeecefe..8f0ee5ac6fd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4639,6 +4639,7 @@ name = "zebra-state" version = "1.0.0-alpha.14" dependencies = [ "bincode", + "blake2b_simd", "chrono", "color-eyre", "dirs", diff --git a/zebra-chain/src/block/commitment.rs b/zebra-chain/src/block/commitment.rs index 9b55adb37ad..eb2dda6a3cf 100644 --- a/zebra-chain/src/block/commitment.rs +++ b/zebra-chain/src/block/commitment.rs @@ -169,6 +169,18 @@ impl From for [u8; 32] { #[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)] pub struct ChainHistoryBlockTxAuthCommitmentHash([u8; 32]); +impl From<[u8; 32]> for ChainHistoryBlockTxAuthCommitmentHash { + fn from(hash: [u8; 32]) -> Self { + ChainHistoryBlockTxAuthCommitmentHash(hash) + } +} + +impl From for [u8; 32] { + fn from(hash: ChainHistoryBlockTxAuthCommitmentHash) -> Self { + hash.0 + } +} + /// Errors that can occur when checking RootHash consensus rules. /// /// Each error variant corresponds to a consensus rule, so enumerating diff --git a/zebra-chain/src/block/merkle.rs b/zebra-chain/src/block/merkle.rs index cf95737eb5f..270c0ae0018 100644 --- a/zebra-chain/src/block/merkle.rs +++ b/zebra-chain/src/block/merkle.rs @@ -155,6 +155,18 @@ impl fmt::Debug for AuthDataRoot { } } +impl From<[u8; 32]> for AuthDataRoot { + fn from(hash: [u8; 32]) -> Self { + AuthDataRoot(hash) + } +} + +impl From for [u8; 32] { + fn from(hash: AuthDataRoot) -> Self { + hash.0 + } +} + impl std::iter::FromIterator for AuthDataRoot where T: std::convert::AsRef, diff --git a/zebra-chain/src/history_tree.rs b/zebra-chain/src/history_tree.rs index 2485e4f25c4..852fc707caa 100644 --- a/zebra-chain/src/history_tree.rs +++ b/zebra-chain/src/history_tree.rs @@ -481,6 +481,11 @@ impl HistoryTree { }; Ok(()) } + + /// Return the hash of the tree root if the tree is not empty. + pub fn hash(&self) -> Option { + Some(self.0.as_ref()?.hash()) + } } impl From for HistoryTree { diff --git a/zebra-state/Cargo.toml b/zebra-state/Cargo.toml index 7fac2b1a9c8..708dbadae27 100644 --- a/zebra-state/Cargo.toml +++ b/zebra-state/Cargo.toml @@ -32,6 +32,7 @@ rlimit = "0.5.4" # TODO: this crate is not maintained anymore. Replace it? # https://github.com/ZcashFoundation/zebra/issues/2523 multiset = "0.0.5" +blake2b_simd = "0.5.11" proptest = { version = "0.10.1", optional = true } zebra-test = { path = "../zebra-test/", optional = true } diff --git a/zebra-state/src/error.rs b/zebra-state/src/error.rs index 7f57dcc7208..43dbdd3e845 100644 --- a/zebra-state/src/error.rs +++ b/zebra-state/src/error.rs @@ -188,6 +188,9 @@ pub enum ValidateContextError { #[error("error building the history tree")] HistoryTreeError(#[from] HistoryTreeError), + + #[error("block contains an invalid commitment")] + InvalidBlockCommitment(#[from] block::CommitmentError), } /// Trait for creating the corresponding duplicate nullifier error from a nullifier. diff --git a/zebra-state/src/service.rs b/zebra-state/src/service.rs index b2ddfdf0b87..065e2be1872 100644 --- a/zebra-state/src/service.rs +++ b/zebra-state/src/service.rs @@ -276,7 +276,7 @@ impl StateService { let relevant_chain = self.any_ancestor_blocks(prepared.block.header.previous_block_hash); // Security: check proof of work before any other checks - check::block_is_contextually_valid( + check::block_is_valid_for_recent_chain( prepared, self.network, self.disk.finalized_tip_height(), diff --git a/zebra-state/src/service/arbitrary.rs b/zebra-state/src/service/arbitrary.rs index 982b82831c7..5110c159918 100644 --- a/zebra-state/src/service/arbitrary.rs +++ b/zebra-state/src/service/arbitrary.rs @@ -62,7 +62,9 @@ pub struct PreparedChain { impl PreparedChain { /// Create a PreparedChain strategy with Heartwood-onward blocks. - #[cfg(test)] + // dead_code is allowed because the function is called only by tests, + // but the code is also compiled when proptest-impl is activated. + #[allow(dead_code)] pub(crate) fn new_heartwood() -> Self { // The history tree only works with Heartwood onward. // Since the network will be chosen later, we pick the larger diff --git a/zebra-state/src/service/check.rs b/zebra-state/src/service/check.rs index 4bde7dd8d2b..ca2ada2790b 100644 --- a/zebra-state/src/service/check.rs +++ b/zebra-state/src/service/check.rs @@ -1,11 +1,12 @@ //! Consensus critical contextual checks -use std::borrow::Borrow; +use std::{borrow::Borrow, convert::TryInto}; use chrono::Duration; use zebra_chain::{ - block::{self, Block}, + block::{self, Block, CommitmentError}, + history_tree::HistoryTree, parameters::POW_AVERAGING_WINDOW, parameters::{Network, NetworkUpgrade}, work::difficulty::CompactDifficulty, @@ -24,8 +25,11 @@ pub(crate) mod utxo; #[cfg(test)] mod tests; -/// Check that `block` is contextually valid for `network`, based on the -/// `finalized_tip_height` and `relevant_chain`. +/// Check that the `prepared` block is contextually valid for `network`, based +/// on the `finalized_tip_height` and `relevant_chain`. +/// +/// This function performs checks that require a small number of recent blocks, +/// including previous hash, previous height, and block difficulty. /// /// The relevant chain is an iterator over the ancestors of `block`, starting /// with its parent block. @@ -34,12 +38,8 @@ mod tests; /// /// If the state contains less than 28 /// (`POW_AVERAGING_WINDOW + POW_MEDIAN_BLOCK_SPAN`) blocks. -#[tracing::instrument( - name = "contextual_validation", - fields(?network), - skip(prepared, network, finalized_tip_height, relevant_chain) -)] -pub(crate) fn block_is_contextually_valid( +#[tracing::instrument(skip(prepared, finalized_tip_height, relevant_chain))] +pub(crate) fn block_is_valid_for_recent_chain( prepared: &PreparedBlock, network: Network, finalized_tip_height: Option, @@ -100,6 +100,75 @@ where Ok(()) } +/// Check that the `prepared` block is contextually valid for `network`, using +/// the `history_tree` up to and including the previous block. +#[tracing::instrument(skip(prepared))] +pub(crate) fn block_commitment_is_valid_for_chain_history( + prepared: &PreparedBlock, + network: Network, + history_tree: &HistoryTree, +) -> Result<(), ValidateContextError> { + match prepared.block.commitment(network)? { + block::Commitment::PreSaplingReserved(_) + | block::Commitment::FinalSaplingRoot(_) + | block::Commitment::ChainHistoryActivationReserved => { + // No contextual checks needed for those. + Ok(()) + } + block::Commitment::ChainHistoryRoot(actual_history_tree_root) => { + let history_tree_root = history_tree + .hash() + .expect("the history tree of the previous block must exist since the current block has a ChainHistoryRoot"); + if actual_history_tree_root == history_tree_root { + Ok(()) + } else { + Err(ValidateContextError::InvalidBlockCommitment( + CommitmentError::InvalidChainHistoryRoot { + actual: actual_history_tree_root.into(), + expected: history_tree_root.into(), + }, + )) + } + } + block::Commitment::ChainHistoryBlockTxAuthCommitment(actual_hash_block_commitments) => { + let actual_block_commitments: [u8; 32] = actual_hash_block_commitments.into(); + let history_tree_root = history_tree + .hash() + .expect("the history tree of the previous block must exist since the current block has a ChainHistoryBlockTxAuthCommitment"); + let auth_data_root = prepared.block.auth_data_root(); + + // > The value of this hash [hashBlockCommitments] is the BLAKE2b-256 hash personalized + // > by the string "ZcashBlockCommit" of the following elements: + // > hashLightClientRoot (as described in ZIP 221) + // > hashAuthDataRoot (as described below) + // > terminator [0u8;32] + // https://zips.z.cash/zip-0244#block-header-changes + let hash_block_commitments: [u8; 32] = blake2b_simd::Params::new() + .hash_length(32) + .personal(b"ZcashBlockCommit") + .to_state() + .update(&<[u8; 32]>::from(history_tree_root)[..]) + .update(&<[u8; 32]>::from(auth_data_root)) + .update(&[0u8; 32]) + .finalize() + .as_bytes() + .try_into() + .expect("32 byte array"); + + if actual_block_commitments == hash_block_commitments { + Ok(()) + } else { + Err(ValidateContextError::InvalidBlockCommitment( + CommitmentError::InvalidChainHistoryBlockTxAuthCommitment { + actual: actual_block_commitments, + expected: hash_block_commitments, + }, + )) + } + } + } +} + /// Returns `ValidateContextError::OrphanedBlock` if the height of the given /// block is less than or equal to the finalized tip height. fn block_is_not_orphaned( diff --git a/zebra-state/src/service/non_finalized_state.rs b/zebra-state/src/service/non_finalized_state.rs index 491f208abde..3a73d8f2525 100644 --- a/zebra-state/src/service/non_finalized_state.rs +++ b/zebra-state/src/service/non_finalized_state.rs @@ -191,6 +191,11 @@ impl NonFinalizedState { &parent_chain.spent_utxos, finalized_state, )?; + check::block_commitment_is_valid_for_chain_history( + &prepared, + self.network, + &parent_chain.history_tree, + )?; parent_chain.push(prepared) } diff --git a/zebra-state/src/service/non_finalized_state/tests/vectors.rs b/zebra-state/src/service/non_finalized_state/tests/vectors.rs index 712cf828c3d..81eb41f999b 100644 --- a/zebra-state/src/service/non_finalized_state/tests/vectors.rs +++ b/zebra-state/src/service/non_finalized_state/tests/vectors.rs @@ -2,6 +2,7 @@ use std::sync::Arc; use zebra_chain::{ block::Block, + history_tree::NonEmptyHistoryTree, parameters::{Network, NetworkUpgrade}, serialization::ZcashDeserializeInto, }; @@ -392,13 +393,13 @@ fn history_tree_is_updated_for_network_upgrade( .zcash_deserialize_into::() .expect("block is structurally valid"), ); - let activation_block = prev_block.make_fake_child(); - let next_block = activation_block.make_fake_child(); let mut state = NonFinalizedState::new(network); let finalized_state = FinalizedState::new(&Config::ephemeral(), network); - state.commit_new_chain(prev_block.prepare(), &finalized_state)?; + state + .commit_new_chain(prev_block.clone().prepare(), &finalized_state) + .unwrap(); let chain = state.best_chain().unwrap(); if network_upgrade == NetworkUpgrade::Heartwood { @@ -413,7 +414,12 @@ fn history_tree_is_updated_for_network_upgrade( ); } - state.commit_block(activation_block.prepare(), &finalized_state)?; + // The Heartwood activation block has an all-zero commitment + let activation_block = prev_block.make_fake_child().set_block_commitment([0u8; 32]); + + state + .commit_block(activation_block.clone().prepare(), &finalized_state) + .unwrap(); let chain = state.best_chain().unwrap(); assert!( @@ -426,7 +432,22 @@ fn history_tree_is_updated_for_network_upgrade( "history tree must have a single node" ); - state.commit_block(next_block.prepare(), &finalized_state)?; + // To fix the commitment in the next block we must recreate the history tree + let tree = NonEmptyHistoryTree::from_block( + Network::Mainnet, + activation_block.clone(), + &chain.sapling_note_commitment_tree.root(), + &chain.orchard_note_commitment_tree.root(), + ) + .unwrap(); + + let next_block = activation_block + .make_fake_child() + .set_block_commitment(tree.hash().into()); + + state + .commit_block(next_block.prepare(), &finalized_state) + .unwrap(); assert!( state.best_chain().unwrap().history_tree.as_ref().is_some(), @@ -435,3 +456,84 @@ fn history_tree_is_updated_for_network_upgrade( Ok(()) } + +#[test] +fn commitment_is_validated() { + commitment_is_validated_for_network_upgrade(Network::Mainnet, NetworkUpgrade::Heartwood); + commitment_is_validated_for_network_upgrade(Network::Testnet, NetworkUpgrade::Heartwood); + // TODO: we can't test other upgrades until we have a method for creating a FinalizedState + // with a HistoryTree. +} + +fn commitment_is_validated_for_network_upgrade(network: Network, network_upgrade: NetworkUpgrade) { + let blocks = match network { + Network::Mainnet => &*zebra_test::vectors::MAINNET_BLOCKS, + Network::Testnet => &*zebra_test::vectors::TESTNET_BLOCKS, + }; + let height = network_upgrade.activation_height(network).unwrap().0; + + let prev_block = Arc::new( + blocks + .get(&(height - 1)) + .expect("test vector exists") + .zcash_deserialize_into::() + .expect("block is structurally valid"), + ); + + let mut state = NonFinalizedState::new(network); + let finalized_state = FinalizedState::new(&Config::ephemeral(), network); + + state + .commit_new_chain(prev_block.clone().prepare(), &finalized_state) + .unwrap(); + + // The Heartwood activation block must have an all-zero commitment. + // Test error return when committing the block with the wrong commitment + let activation_block = prev_block.make_fake_child(); + let err = state + .commit_block(activation_block.clone().prepare(), &finalized_state) + .unwrap_err(); + match err { + crate::ValidateContextError::InvalidBlockCommitment( + zebra_chain::block::CommitmentError::InvalidChainHistoryActivationReserved { .. }, + ) => {}, + _ => panic!("Error must be InvalidBlockCommitment::InvalidChainHistoryActivationReserved instead of {:?}", err), + }; + + // Test committing the Heartwood activation block with the correct commitment + let activation_block = activation_block.set_block_commitment([0u8; 32]); + state + .commit_block(activation_block.clone().prepare(), &finalized_state) + .unwrap(); + + // To fix the commitment in the next block we must recreate the history tree + let chain = state.best_chain().unwrap(); + let tree = NonEmptyHistoryTree::from_block( + Network::Mainnet, + activation_block.clone(), + &chain.sapling_note_commitment_tree.root(), + &chain.orchard_note_commitment_tree.root(), + ) + .unwrap(); + + // Test committing the next block with the wrong commitment + let next_block = activation_block.make_fake_child(); + let err = state + .commit_block(next_block.clone().prepare(), &finalized_state) + .unwrap_err(); + match err { + crate::ValidateContextError::InvalidBlockCommitment( + zebra_chain::block::CommitmentError::InvalidChainHistoryRoot { .. }, + ) => {} + _ => panic!( + "Error must be InvalidBlockCommitment::InvalidChainHistoryRoot instead of {:?}", + err + ), + }; + + // Test committing the next block with the correct commitment + let next_block = next_block.set_block_commitment(tree.hash().into()); + state + .commit_block(next_block.prepare(), &finalized_state) + .unwrap(); +} diff --git a/zebra-state/src/tests.rs b/zebra-state/src/tests.rs index 4ed05bd8779..9c7296874f3 100644 --- a/zebra-state/src/tests.rs +++ b/zebra-state/src/tests.rs @@ -17,6 +17,8 @@ pub trait FakeChainHelper { fn make_fake_child(&self) -> Arc; fn set_work(self, work: u128) -> Arc; + + fn set_block_commitment(self, commitment: [u8; 32]) -> Arc; } impl FakeChainHelper for Arc { @@ -53,6 +55,12 @@ impl FakeChainHelper for Arc { block.header.difficulty_threshold = expanded.into(); self } + + fn set_block_commitment(mut self, block_commitment: [u8; 32]) -> Arc { + let block = Arc::make_mut(&mut self); + block.header.commitment_bytes = block_commitment; + self + } } fn work_to_expanded(work: U256) -> ExpandedDifficulty { From d497a2a4225dcbedc59328751ac27bd6bbf38c97 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Thu, 12 Aug 2021 15:16:10 -0300 Subject: [PATCH 02/16] Apply suggestions from code review Co-authored-by: teor --- zebra-state/src/service/check.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-state/src/service/check.rs b/zebra-state/src/service/check.rs index ca2ada2790b..c69b95de0df 100644 --- a/zebra-state/src/service/check.rs +++ b/zebra-state/src/service/check.rs @@ -102,7 +102,7 @@ where /// Check that the `prepared` block is contextually valid for `network`, using /// the `history_tree` up to and including the previous block. -#[tracing::instrument(skip(prepared))] +#[tracing::instrument(skip(prepared, history_tree))] pub(crate) fn block_commitment_is_valid_for_chain_history( prepared: &PreparedBlock, network: Network, From 1fab6c97316b34fc94f82fd143af568cd80336b5 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Fri, 13 Aug 2021 15:27:57 -0300 Subject: [PATCH 03/16] Add auth commitment check in the finalized state --- zebra-consensus/src/checkpoint.rs | 3 +- zebra-state/src/service/check.rs | 29 ++++++++++--- zebra-state/src/service/finalized_state.rs | 41 +++++++++++++------ .../src/service/non_finalized_state.rs | 2 +- 4 files changed, 54 insertions(+), 21 deletions(-) diff --git a/zebra-consensus/src/checkpoint.rs b/zebra-consensus/src/checkpoint.rs index b87c68e84b8..61936648e0c 100644 --- a/zebra-consensus/src/checkpoint.rs +++ b/zebra-consensus/src/checkpoint.rs @@ -919,8 +919,7 @@ where match state_service .oneshot(zs::Request::CommitFinalizedBlock(block.into())) .map_err(VerifyCheckpointError::CommitFinalized) - .await - .expect("state service commit block failed: verified checkpoints must be committed transactionally") + .await? { zs::Response::Committed(committed_hash) => { assert_eq!(committed_hash, hash, "state must commit correct hash"); diff --git a/zebra-state/src/service/check.rs b/zebra-state/src/service/check.rs index c69b95de0df..23845e0e91d 100644 --- a/zebra-state/src/service/check.rs +++ b/zebra-state/src/service/check.rs @@ -1,6 +1,6 @@ //! Consensus critical contextual checks -use std::{borrow::Borrow, convert::TryInto}; +use std::{borrow::Borrow, convert::TryInto, sync::Arc}; use chrono::Duration; @@ -12,7 +12,7 @@ use zebra_chain::{ work::difficulty::CompactDifficulty, }; -use crate::{PreparedBlock, ValidateContextError}; +use crate::{FinalizedBlock, PreparedBlock, ValidateContextError}; use super::check; @@ -103,12 +103,31 @@ where /// Check that the `prepared` block is contextually valid for `network`, using /// the `history_tree` up to and including the previous block. #[tracing::instrument(skip(prepared, history_tree))] -pub(crate) fn block_commitment_is_valid_for_chain_history( +pub(crate) fn prepared_block_commitment_is_valid_for_chain_history( prepared: &PreparedBlock, network: Network, history_tree: &HistoryTree, ) -> Result<(), ValidateContextError> { - match prepared.block.commitment(network)? { + block_commitment_is_valid_for_chain_history(prepared.block.clone(), network, history_tree) +} + +/// Check that the `finalized` block is contextually valid for `network`, using +/// the `history_tree` up to and including the previous block. +#[tracing::instrument(skip(finalized, history_tree))] +pub(crate) fn finalized_block_commitment_is_valid_for_chain_history( + finalized: &FinalizedBlock, + network: Network, + history_tree: &HistoryTree, +) -> Result<(), ValidateContextError> { + block_commitment_is_valid_for_chain_history(finalized.block.clone(), network, history_tree) +} + +fn block_commitment_is_valid_for_chain_history( + block: Arc, + network: Network, + history_tree: &HistoryTree, +) -> Result<(), ValidateContextError> { + match block.commitment(network)? { block::Commitment::PreSaplingReserved(_) | block::Commitment::FinalSaplingRoot(_) | block::Commitment::ChainHistoryActivationReserved => { @@ -135,7 +154,7 @@ pub(crate) fn block_commitment_is_valid_for_chain_history( let history_tree_root = history_tree .hash() .expect("the history tree of the previous block must exist since the current block has a ChainHistoryBlockTxAuthCommitment"); - let auth_data_root = prepared.block.auth_data_root(); + let auth_data_root = block.auth_data_root(); // > The value of this hash [hashBlockCommitments] is the BLAKE2b-256 hash personalized // > by the string "ZcashBlockCommit" of the following elements: diff --git a/zebra-state/src/service/finalized_state.rs b/zebra-state/src/service/finalized_state.rs index 4edf429ac15..0d75dc921ae 100644 --- a/zebra-state/src/service/finalized_state.rs +++ b/zebra-state/src/service/finalized_state.rs @@ -17,7 +17,7 @@ use zebra_chain::{ transparent, }; -use crate::{BoxError, Config, FinalizedBlock, HashOrHeight}; +use crate::{service::check, BoxError, Config, FinalizedBlock, HashOrHeight}; use self::disk_format::{DiskDeserialize, DiskSerialize, FromDisk, IntoDisk, TransactionLocation}; @@ -204,6 +204,8 @@ impl FinalizedState { /// /// - Propagates any errors from writing to the DB /// - Propagates any errors from updating history and note commitment trees + /// - If `hashFinalSaplingRoot` / `hashLightClientRoot` / `hashBlockCommitments` + /// does not match the expected value pub fn commit_finalized_direct( &mut self, finalized: FinalizedBlock, @@ -211,14 +213,6 @@ impl FinalizedState { ) -> Result { block_precommit_metrics(&finalized); - let FinalizedBlock { - block, - hash, - height, - new_outputs, - transaction_hashes, - } = finalized; - let finalized_tip_height = self.finalized_tip_height(); let hash_by_height = self.db.cf_handle("hash_by_height").unwrap(); @@ -243,27 +237,27 @@ impl FinalizedState { // Assert that callers (including unit tests) get the chain order correct if self.is_empty(hash_by_height) { assert_eq!( - GENESIS_PREVIOUS_BLOCK_HASH, block.header.previous_block_hash, + GENESIS_PREVIOUS_BLOCK_HASH, finalized.block.header.previous_block_hash, "the first block added to an empty state must be a genesis block, source: {}", source, ); assert_eq!( block::Height(0), - height, + finalized.height, "cannot commit genesis: invalid height, source: {}", source, ); } else { assert_eq!( finalized_tip_height.expect("state must have a genesis block committed") + 1, - Some(height), + Some(finalized.height), "committed block height must be 1 more than the finalized tip height, source: {}", source, ); assert_eq!( self.finalized_tip_hash(), - block.header.previous_block_hash, + finalized.block.header.previous_block_hash, "committed block must be a child of the finalized tip, source: {}", source, ); @@ -275,6 +269,27 @@ impl FinalizedState { let mut orchard_note_commitment_tree = self.orchard_note_commitment_tree(); let mut history_tree = self.history_tree(); + // Check the block commitment. For Nu5-onward, the block hash commits only + // to non-authorizing data (see ZIP-244). This checks the authorizing data + // commitment, making sure the entire block contents were commited to. + // The test is done here (and not during semantic validation) because it needs + // the history tree root. While it _is_ checked during contextual validation, + // that is not called by the checkpoint verifier, and keeping a history tree there + // would be harder to implement. + check::finalized_block_commitment_is_valid_for_chain_history( + &finalized, + self.network, + &history_tree, + )?; + + let FinalizedBlock { + block, + hash, + height, + new_outputs, + transaction_hashes, + } = finalized; + // Prepare a batch of DB modifications and return it (without actually writing anything). // We use a closure so we can use an early return for control flow in // the genesis case. diff --git a/zebra-state/src/service/non_finalized_state.rs b/zebra-state/src/service/non_finalized_state.rs index 3a73d8f2525..f4209747522 100644 --- a/zebra-state/src/service/non_finalized_state.rs +++ b/zebra-state/src/service/non_finalized_state.rs @@ -191,7 +191,7 @@ impl NonFinalizedState { &parent_chain.spent_utxos, finalized_state, )?; - check::block_commitment_is_valid_for_chain_history( + check::prepared_block_commitment_is_valid_for_chain_history( &prepared, self.network, &parent_chain.history_tree, From 9b7b0a152a64ee73c40f258eddd163107da96284 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Wed, 18 Aug 2021 13:42:19 -0300 Subject: [PATCH 04/16] Reset the verifier when comitting to state fails --- zebra-consensus/src/checkpoint.rs | 58 +++++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/zebra-consensus/src/checkpoint.rs b/zebra-consensus/src/checkpoint.rs index 61936648e0c..b510774c021 100644 --- a/zebra-consensus/src/checkpoint.rs +++ b/zebra-consensus/src/checkpoint.rs @@ -17,7 +17,10 @@ use std::{ collections::BTreeMap, ops::{Bound, Bound::*}, pin::Pin, - sync::Arc, + sync::{ + mpsc::{self, Receiver, Sender}, + Arc, + }, task::{Context, Poll}, }; @@ -132,6 +135,13 @@ where /// The current progress of this verifier. verifier_progress: Progress, + + /// A channel to receive requests to reset the verifier, + /// receiving the tip of the state. + reset_receiver: Receiver>, + /// A channel to send requests to reset the verifier, + /// passing the tip of the state. + reset_sender: Sender>, } impl CheckpointVerifier @@ -225,6 +235,7 @@ where // We start by verifying the genesis block, by itself None => (None, Progress::BeforeGenesis), }; + let (sender, receiver) = mpsc::channel(); CheckpointVerifier { checkpoint_list, network, @@ -232,9 +243,25 @@ where state_service, queued: BTreeMap::new(), verifier_progress, + reset_receiver: receiver, + reset_sender: sender, } } + /// Reset the verifier progress back to given tip. + fn reset_progress(&mut self, tip: Option<(block::Height, block::Hash)>) { + self.verifier_progress = match tip { + Some((height, _)) => { + if height >= self.checkpoint_list.max_height() { + Progress::FinalCheckpoint + } else { + Progress::InitialTip(height) + } + } + None => Progress::BeforeGenesis, + }; + } + /// Return the current verifier's progress. /// /// If verification has not started yet, returns `BeforeGenesis`, @@ -825,6 +852,8 @@ pub enum VerifyCheckpointError { #[error(transparent)] CommitFinalized(BoxError), #[error(transparent)] + Tip(BoxError), + #[error(transparent)] CheckpointList(BoxError), #[error(transparent)] VerifyBlock(BoxError), @@ -876,6 +905,12 @@ where #[instrument(name = "checkpoint", skip(self, block))] fn call(&mut self, block: Arc) -> Self::Future { + // Reset the verifier back to the state tip if requested + // (e.g. due to an error when committing a block to to the state) + if let Ok(tip) = self.reset_receiver.try_recv() { + self.reset_progress(tip); + } + // Immediately reject all incoming blocks that arrive after we've finished. if let FinalCheckpoint = self.previous_checkpoint_height() { return async { Err(VerifyCheckpointError::Finished) }.boxed(); @@ -929,6 +964,8 @@ where } }); + let state_service = self.state_service.clone(); + let reset_sender = self.reset_sender.clone(); async move { let result = commit_finalized_block.await; // Avoid a panic on shutdown @@ -938,11 +975,28 @@ where // so we don't need to panic here. The persistent state is correct even when the // task is cancelled, because block data is committed inside transactions, in // height order. - if zebra_chain::shutdown::is_shutting_down() { + let result = if zebra_chain::shutdown::is_shutting_down() { Err(VerifyCheckpointError::ShuttingDown) } else { result.expect("commit_finalized_block should not panic") + }; + if result.is_err() { + // If there was an error comitting the block, then this verifier + // will be out of sync with the state. In that case, reset + // its progress back to the state tip. + let tip = match state_service + .oneshot(zs::Request::Tip) + .await + .map_err(Into::into) + .map_err(VerifyCheckpointError::Tip)? + { + zs::Response::Tip(tip) => tip, + _ => unreachable!("wrong response for Tip"), + }; + // TODO: do we need to handle errors? + let _ = reset_sender.send(tip); } + result } .boxed() } From 96e95027fca8f5d4e2b7ffc50dcd792c3b991fb3 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Wed, 18 Aug 2021 13:50:53 -0300 Subject: [PATCH 05/16] Add explanation comment --- zebra-state/src/service/check.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/zebra-state/src/service/check.rs b/zebra-state/src/service/check.rs index 23845e0e91d..fe7872991de 100644 --- a/zebra-state/src/service/check.rs +++ b/zebra-state/src/service/check.rs @@ -162,6 +162,9 @@ fn block_commitment_is_valid_for_chain_history( // > hashAuthDataRoot (as described below) // > terminator [0u8;32] // https://zips.z.cash/zip-0244#block-header-changes + // + // Note that auth data root is from the current block, while + // the history tree root is from the previous block. let hash_block_commitments: [u8; 32] = blake2b_simd::Params::new() .hash_length(32) .personal(b"ZcashBlockCommit") From a3ff5b264b2d33eda6611a67ebe9b6e91aa0e4a5 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Fri, 13 Aug 2021 18:24:42 -0300 Subject: [PATCH 06/16] Add test with fake activation heights --- zebra-chain/Cargo.toml | 1 + zebra-chain/src/block/arbitrary.rs | 56 +++++++++++++++++- zebra-chain/src/parameters/network_upgrade.rs | 26 +++++++++ zebra-state/Cargo.toml | 3 +- .../src/service/finalized_state/tests/prop.rs | 57 ++++++++++++++++++- 5 files changed, 138 insertions(+), 5 deletions(-) diff --git a/zebra-chain/Cargo.toml b/zebra-chain/Cargo.toml index d09303546f9..9a39fdd7be0 100644 --- a/zebra-chain/Cargo.toml +++ b/zebra-chain/Cargo.toml @@ -11,6 +11,7 @@ edition = "2018" default = [] proptest-impl = ["proptest", "proptest-derive", "itertools", "zebra-test", "rand", "rand_chacha"] bench = ["zebra-test"] +test_fake_activation_heights = [] [dependencies] aes = "0.6" diff --git a/zebra-chain/src/block/arbitrary.rs b/zebra-chain/src/block/arbitrary.rs index f7aa470f320..492d4985ff4 100644 --- a/zebra-chain/src/block/arbitrary.rs +++ b/zebra-chain/src/block/arbitrary.rs @@ -11,6 +11,7 @@ use crate::{ amount::NonNegative, block, fmt::SummaryDebug, + history_tree::HistoryTree, parameters::{ Network, NetworkUpgrade::{self, *}, @@ -402,6 +403,9 @@ impl Block { let mut previous_block_hash = None; let mut utxos = HashMap::new(); let mut chain_value_pools = ValueBalance::zero(); + let mut sapling_tree = sapling::tree::NoteCommitmentTree::default(); + let mut orchard_tree = orchard::tree::NoteCommitmentTree::default(); + let mut history_tree = HistoryTree::default(); for (height, block) in vec.iter_mut() { // fixup the previous block hash @@ -419,6 +423,12 @@ impl Block { &mut utxos, check_transparent_coinbase_spend, ) { + for sapling_note_commitment in transaction.sapling_note_commitments() { + sapling_tree.append(*sapling_note_commitment).unwrap(); + } + for orchard_note_commitment in transaction.orchard_note_commitments() { + orchard_tree.append(*orchard_note_commitment).unwrap(); + } new_transactions.push(Arc::new(transaction)); } } @@ -426,13 +436,53 @@ impl Block { // delete invalid transactions block.transactions = new_transactions; - // TODO: if needed, fixup after modifying the block: - // - history and authorizing data commitments - // - the transaction merkle root + // fix commitment (must be done after finishing changing the block) + let current_height = block.coinbase_height().unwrap(); + let heartwood_height = NetworkUpgrade::Heartwood + .activation_height(current.network) + .unwrap(); + let nu5_height = NetworkUpgrade::Nu5.activation_height(current.network); + match current_height.cmp(&heartwood_height) { + std::cmp::Ordering::Less => {} + std::cmp::Ordering::Equal => { + block.header.commitment_bytes = [0u8; 32]; + } + std::cmp::Ordering::Greater => { + if nu5_height.is_some() && current_height >= nu5_height.unwrap() { + // From zebra-state/src/service/check.rs + let history_tree_root = history_tree.hash().unwrap(); + let auth_data_root = block.auth_data_root(); + let hash_block_commitments: [u8; 32] = blake2b_simd::Params::new() + .hash_length(32) + .personal(b"ZcashBlockCommit") + .to_state() + .update(&<[u8; 32]>::from(history_tree_root)[..]) + .update(&<[u8; 32]>::from(auth_data_root)) + .update(&[0u8; 32]) + .finalize() + .as_bytes() + .try_into() + .expect("32 byte array"); + block.header.commitment_bytes = hash_block_commitments; + } else { + block.header.commitment_bytes = history_tree.hash().unwrap().into(); + } + } + } // now that we've made all the changes, calculate our block hash, // so the next block can use it previous_block_hash = Some(block.hash()); + + // update history tree for the next block + history_tree + .push( + Network::Mainnet, + Arc::new(block.clone()), + sapling_tree.root(), + orchard_tree.root(), + ) + .unwrap(); } SummaryDebug( vec.into_iter() diff --git a/zebra-chain/src/parameters/network_upgrade.rs b/zebra-chain/src/parameters/network_upgrade.rs index 597eacc1ef0..b494832cf68 100644 --- a/zebra-chain/src/parameters/network_upgrade.rs +++ b/zebra-chain/src/parameters/network_upgrade.rs @@ -48,6 +48,7 @@ pub enum NetworkUpgrade { /// /// This is actually a bijective map, but it is const, so we use a vector, and /// do the uniqueness check in the unit tests. +#[cfg(not(feature = "test_fake_activation_heights"))] pub(crate) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[ (block::Height(0), Genesis), (block::Height(1), BeforeOverwinter), @@ -59,10 +60,23 @@ pub(crate) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] // TODO: Add Nu5 mainnet activation height ]; +#[cfg(feature = "test_fake_activation_heights")] +pub(crate) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[ + (block::Height(0), Genesis), + (block::Height(5), BeforeOverwinter), + (block::Height(10), Overwinter), + (block::Height(15), Sapling), + (block::Height(20), Blossom), + (block::Height(25), Heartwood), + (block::Height(30), Canopy), + (block::Height(35), Nu5), +]; + /// Testnet network upgrade activation heights. /// /// This is actually a bijective map, but it is const, so we use a vector, and /// do the uniqueness check in the unit tests. +#[cfg(not(feature = "test_fake_activation_heights"))] pub(crate) const TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[ (block::Height(0), Genesis), (block::Height(1), BeforeOverwinter), @@ -74,6 +88,18 @@ pub(crate) const TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] // TODO: Add Nu5 testnet activation height ]; +#[cfg(feature = "test_fake_activation_heights")] +pub(crate) const TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[ + (block::Height(0), Genesis), + (block::Height(5), BeforeOverwinter), + (block::Height(10), Overwinter), + (block::Height(15), Sapling), + (block::Height(20), Blossom), + (block::Height(25), Heartwood), + (block::Height(30), Canopy), + (block::Height(35), Nu5), +]; + /// The Consensus Branch Id, used to bind transactions and blocks to a /// particular network upgrade. #[derive(Copy, Clone, Debug, Eq, Hash, PartialEq)] diff --git a/zebra-state/Cargo.toml b/zebra-state/Cargo.toml index 89c540b80af..a472ffff558 100644 --- a/zebra-state/Cargo.toml +++ b/zebra-state/Cargo.toml @@ -7,6 +7,7 @@ edition = "2018" [features] proptest-impl = ["proptest", "zebra-test"] +test_fake_activation_heights = ["zebra-chain/test_fake_activation_heights"] [dependencies] zebra-chain = { path = "../zebra-chain" } @@ -38,7 +39,7 @@ proptest = { version = "0.10.1", optional = true } zebra-test = { path = "../zebra-test/", optional = true } [dev-dependencies] -zebra-chain = { path = "../zebra-chain", features = ["proptest-impl"] } +zebra-chain = { path = "../zebra-chain", features = ["proptest-impl", "test_fake_activation_heights"] } zebra-test = { path = "../zebra-test/" } color-eyre = "0.5.11" diff --git a/zebra-state/src/service/finalized_state/tests/prop.rs b/zebra-state/src/service/finalized_state/tests/prop.rs index 1dba29fa3c4..9b9b7909758 100644 --- a/zebra-state/src/service/finalized_state/tests/prop.rs +++ b/zebra-state/src/service/finalized_state/tests/prop.rs @@ -1,6 +1,6 @@ use std::env; -use zebra_chain::block::Height; +use zebra_chain::{block::Height, parameters::NetworkUpgrade}; use zebra_test::prelude::*; use crate::{ @@ -9,6 +9,7 @@ use crate::{ arbitrary::PreparedChain, finalized_state::{FinalizedBlock, FinalizedState}, }, + tests::FakeChainHelper, ContextuallyValidBlock, }; @@ -39,3 +40,57 @@ fn blocks_with_v5_transactions() -> Result<()> { Ok(()) } + +/// Test if committing blocks from all upgrades work correctly, to make +/// sure the contextual validation done by the finalized state works. +/// Also test if a block with the wrong commitment is correctly rejected. +#[allow(dead_code)] +#[cfg_attr(feature = "test_fake_activation_heights", test)] +fn all_upgrades_and_wrong_commitments() -> Result<()> { + zebra_test::init(); + // Use a single case and no_shrink() because this is more of a test vector, + // just using the existing proptest machinery to create test blocks. + proptest!(ProptestConfig::with_cases(1), + |((chain, _count, network, _history_tree) in PreparedChain::default().no_shrink())| { + + let mut state = FinalizedState::new(&Config::ephemeral(), network); + let mut height = Height(0); + let heartwood_height = NetworkUpgrade::Heartwood.activation_height(network).unwrap(); + let heartwood_height_plus1 = (heartwood_height + 1).unwrap(); + let nu5_height = NetworkUpgrade::Nu5.activation_height(network).unwrap(); + let nu5_height_plus1 = (nu5_height + 1).unwrap(); + + let mut failure_count = 0; + for block in chain.iter() { + let block_hash = block.hash; + let current_height = block.block.coinbase_height().unwrap(); + // For some specific heights, try to commit a block with + // corrupted commitment. + match current_height { + h if h == heartwood_height || + h == heartwood_height_plus1 || + h == nu5_height || + h == nu5_height_plus1 => { + let block = block.block.clone().set_block_commitment([0x42; 32]); + state.commit_finalized_direct( + FinalizedBlock::from(block), + "all_upgrades test" + ).expect_err("Must fail commitment check"); + failure_count += 1; + }, + _ => {}, + } + let hash = state.commit_finalized_direct( + FinalizedBlock::from(block.block.clone()), + "all_upgrades test" + ).unwrap(); + prop_assert_eq!(Some(height), state.finalized_tip_height()); + prop_assert_eq!(hash, block_hash); + height = Height(height.0 + 1); + } + // Make sure the failure path was triggered + prop_assert_eq!(failure_count, 4); + }); + + Ok(()) +} From 05978e359b67da93ad4c85f801fba5312e65d331 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Thu, 19 Aug 2021 12:22:02 -0300 Subject: [PATCH 07/16] Add generate_valid_commitments flag --- zebra-chain/src/block/arbitrary.rs | 105 +++++++++++------- zebra-chain/src/block/tests/prop.rs | 2 + zebra-state/src/service/arbitrary.rs | 8 ++ .../src/service/finalized_state/tests/prop.rs | 2 +- .../service/non_finalized_state/tests/prop.rs | 2 +- zebra-state/src/tests/setup.rs | 1 + 6 files changed, 78 insertions(+), 42 deletions(-) diff --git a/zebra-chain/src/block/arbitrary.rs b/zebra-chain/src/block/arbitrary.rs index 492d4985ff4..05cc1dbb3e5 100644 --- a/zebra-chain/src/block/arbitrary.rs +++ b/zebra-chain/src/block/arbitrary.rs @@ -380,6 +380,7 @@ impl Block { mut current: LedgerState, count: usize, check_transparent_coinbase_spend: F, + generate_valid_commitments: bool, ) -> BoxedStrategy>>> where F: Fn( @@ -405,7 +406,13 @@ impl Block { let mut chain_value_pools = ValueBalance::zero(); let mut sapling_tree = sapling::tree::NoteCommitmentTree::default(); let mut orchard_tree = orchard::tree::NoteCommitmentTree::default(); - let mut history_tree = HistoryTree::default(); + // The history tree usually takes care of "creating itself". But this + // only works when blocks are pushed to into starting from genesis + // (or at least pre-Heartwood, where the tree is not required). + // However, this strategy can generate blocks from an arbitrary height, + // so we must wait for the first block to create the history tree from it. + // This is why `Option` is used here. + let mut history_tree: Option = None; for (height, block) in vec.iter_mut() { // fixup the previous block hash @@ -437,52 +444,70 @@ impl Block { block.transactions = new_transactions; // fix commitment (must be done after finishing changing the block) - let current_height = block.coinbase_height().unwrap(); - let heartwood_height = NetworkUpgrade::Heartwood - .activation_height(current.network) - .unwrap(); - let nu5_height = NetworkUpgrade::Nu5.activation_height(current.network); - match current_height.cmp(&heartwood_height) { - std::cmp::Ordering::Less => {} - std::cmp::Ordering::Equal => { - block.header.commitment_bytes = [0u8; 32]; - } - std::cmp::Ordering::Greater => { - if nu5_height.is_some() && current_height >= nu5_height.unwrap() { - // From zebra-state/src/service/check.rs - let history_tree_root = history_tree.hash().unwrap(); - let auth_data_root = block.auth_data_root(); - let hash_block_commitments: [u8; 32] = blake2b_simd::Params::new() - .hash_length(32) - .personal(b"ZcashBlockCommit") - .to_state() - .update(&<[u8; 32]>::from(history_tree_root)[..]) - .update(&<[u8; 32]>::from(auth_data_root)) - .update(&[0u8; 32]) - .finalize() - .as_bytes() - .try_into() - .expect("32 byte array"); - block.header.commitment_bytes = hash_block_commitments; - } else { - block.header.commitment_bytes = history_tree.hash().unwrap().into(); + if generate_valid_commitments { + let current_height = block.coinbase_height().unwrap(); + let heartwood_height = NetworkUpgrade::Heartwood + .activation_height(current.network) + .unwrap(); + let nu5_height = NetworkUpgrade::Nu5.activation_height(current.network); + match current_height.cmp(&heartwood_height) { + std::cmp::Ordering::Less => {} + std::cmp::Ordering::Equal => { + block.header.commitment_bytes = [0u8; 32]; + } + std::cmp::Ordering::Greater => { + let history_tree_root = match &history_tree { + Some(tree) => tree.hash().unwrap_or_else(|| [0u8; 32].into()), + None => [0u8; 32].into(), + }; + if nu5_height.is_some() && current_height >= nu5_height.unwrap() { + // From zebra-state/src/service/check.rs + let auth_data_root = block.auth_data_root(); + let hash_block_commitments: [u8; 32] = blake2b_simd::Params::new() + .hash_length(32) + .personal(b"ZcashBlockCommit") + .to_state() + .update(&<[u8; 32]>::from(history_tree_root)[..]) + .update(&<[u8; 32]>::from(auth_data_root)) + .update(&[0u8; 32]) + .finalize() + .as_bytes() + .try_into() + .expect("32 byte array"); + block.header.commitment_bytes = hash_block_commitments; + } else { + block.header.commitment_bytes = history_tree_root.into(); + } } } + // update history tree for the next block + if history_tree.is_none() { + history_tree = Some( + HistoryTree::from_block( + current.network, + Arc::new(block.clone()), + &sapling_tree.root(), + &orchard_tree.root(), + ) + .unwrap(), + ); + } else { + history_tree + .as_mut() + .unwrap() + .push( + Network::Mainnet, + Arc::new(block.clone()), + sapling_tree.root(), + orchard_tree.root(), + ) + .unwrap(); + } } // now that we've made all the changes, calculate our block hash, // so the next block can use it previous_block_hash = Some(block.hash()); - - // update history tree for the next block - history_tree - .push( - Network::Mainnet, - Arc::new(block.clone()), - sapling_tree.root(), - orchard_tree.root(), - ) - .unwrap(); } SummaryDebug( vec.into_iter() diff --git a/zebra-chain/src/block/tests/prop.rs b/zebra-chain/src/block/tests/prop.rs index 8399fa0ed8d..b7bc34b6436 100644 --- a/zebra-chain/src/block/tests/prop.rs +++ b/zebra-chain/src/block/tests/prop.rs @@ -173,6 +173,7 @@ fn genesis_partial_chain_strategy() -> Result<()> { init, PREVOUTS_CHAIN_HEIGHT, allow_all_transparent_coinbase_spends, + false, ) }); @@ -222,6 +223,7 @@ fn arbitrary_height_partial_chain_strategy() -> Result<()> { init, PREVOUTS_CHAIN_HEIGHT, allow_all_transparent_coinbase_spends, + false, ) }); diff --git a/zebra-state/src/service/arbitrary.rs b/zebra-state/src/service/arbitrary.rs index 5110c159918..93a4125430c 100644 --- a/zebra-state/src/service/arbitrary.rs +++ b/zebra-state/src/service/arbitrary.rs @@ -58,6 +58,7 @@ pub struct PreparedChain { chain: std::sync::Mutex>>, HistoryTree)>>, // the strategy for generating LedgerStates. If None, it calls [`LedgerState::genesis_strategy`]. ledger_strategy: Option>, + generate_valid_commitments: bool, } impl PreparedChain { @@ -87,6 +88,12 @@ impl PreparedChain { ..Default::default() } } + + #[allow(dead_code)] + pub(crate) fn with_valid_commitments(mut self) -> Self { + self.generate_valid_commitments = true; + self + } } impl Strategy for PreparedChain { @@ -112,6 +119,7 @@ impl Strategy for PreparedChain { ledger, MAX_PARTIAL_CHAIN_BLOCKS, check::utxo::transparent_coinbase_spend, + self.generate_valid_commitments, ), ) }) diff --git a/zebra-state/src/service/finalized_state/tests/prop.rs b/zebra-state/src/service/finalized_state/tests/prop.rs index 9b9b7909758..fa4389aa4b1 100644 --- a/zebra-state/src/service/finalized_state/tests/prop.rs +++ b/zebra-state/src/service/finalized_state/tests/prop.rs @@ -51,7 +51,7 @@ fn all_upgrades_and_wrong_commitments() -> Result<()> { // Use a single case and no_shrink() because this is more of a test vector, // just using the existing proptest machinery to create test blocks. proptest!(ProptestConfig::with_cases(1), - |((chain, _count, network, _history_tree) in PreparedChain::default().no_shrink())| { + |((chain, _count, network, _history_tree) in PreparedChain::default().with_valid_commitments().no_shrink())| { let mut state = FinalizedState::new(&Config::ephemeral(), network); let mut height = Height(0); diff --git a/zebra-state/src/service/non_finalized_state/tests/prop.rs b/zebra-state/src/service/non_finalized_state/tests/prop.rs index fec8a17b672..e3829c5a3f0 100644 --- a/zebra-state/src/service/non_finalized_state/tests/prop.rs +++ b/zebra-state/src/service/non_finalized_state/tests/prop.rs @@ -233,7 +233,7 @@ fn different_blocks_different_chains() -> Result<()> { if is_nu5 && is_v5 { 5 } else { 4 }, true, )}) - .prop_map(|ledger_state| Block::partial_chain_strategy(ledger_state, 2, allow_all_transparent_coinbase_spends)) + .prop_map(|ledger_state| Block::partial_chain_strategy(ledger_state, 2, allow_all_transparent_coinbase_spends, false)) .prop_flat_map(|block_strategy| (block_strategy.clone(), block_strategy)) )| { let prev_block1 = vec1[0].clone(); diff --git a/zebra-state/src/tests/setup.rs b/zebra-state/src/tests/setup.rs index d490a561a05..d61d88d0524 100644 --- a/zebra-state/src/tests/setup.rs +++ b/zebra-state/src/tests/setup.rs @@ -70,6 +70,7 @@ pub(crate) fn partial_nu5_chain_strategy( init, blocks_after_nu_activation as usize, check::utxo::transparent_coinbase_spend, + false, ) }) .prop_map(move |partial_chain| (network, nu_activation, partial_chain)) From 0dd95572a2ce66f3fe1bbba0fe788ade9a5cc300 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Thu, 19 Aug 2021 12:22:36 -0300 Subject: [PATCH 08/16] Enable fake activation heights using env var instead of feature --- zebra-chain/Cargo.toml | 1 - zebra-chain/build.rs | 9 +++++++++ zebra-chain/src/parameters/network_upgrade.rs | 8 ++++---- zebra-state/Cargo.toml | 3 +-- zebra-state/build.rs | 9 +++++++++ zebra-state/src/service/finalized_state/tests/prop.rs | 2 +- 6 files changed, 24 insertions(+), 8 deletions(-) create mode 100644 zebra-chain/build.rs create mode 100644 zebra-state/build.rs diff --git a/zebra-chain/Cargo.toml b/zebra-chain/Cargo.toml index 9a39fdd7be0..d09303546f9 100644 --- a/zebra-chain/Cargo.toml +++ b/zebra-chain/Cargo.toml @@ -11,7 +11,6 @@ edition = "2018" default = [] proptest-impl = ["proptest", "proptest-derive", "itertools", "zebra-test", "rand", "rand_chacha"] bench = ["zebra-test"] -test_fake_activation_heights = [] [dependencies] aes = "0.6" diff --git a/zebra-chain/build.rs b/zebra-chain/build.rs new file mode 100644 index 00000000000..91ba008f76c --- /dev/null +++ b/zebra-chain/build.rs @@ -0,0 +1,9 @@ +use std::env; + +fn main() { + let use_fake_heights = env::var_os("TEST_FAKE_ACTIVATION_HEIGHTS").is_some(); + println!("cargo:rerun-if-env-changed=TEST_FAKE_ACTIVATION_HEIGHTS"); + if use_fake_heights { + println!("cargo:rustc-cfg=test_fake_activation_heights"); + } +} diff --git a/zebra-chain/src/parameters/network_upgrade.rs b/zebra-chain/src/parameters/network_upgrade.rs index b494832cf68..297dc0d1772 100644 --- a/zebra-chain/src/parameters/network_upgrade.rs +++ b/zebra-chain/src/parameters/network_upgrade.rs @@ -48,7 +48,7 @@ pub enum NetworkUpgrade { /// /// This is actually a bijective map, but it is const, so we use a vector, and /// do the uniqueness check in the unit tests. -#[cfg(not(feature = "test_fake_activation_heights"))] +#[cfg(not(test_fake_activation_heights))] pub(crate) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[ (block::Height(0), Genesis), (block::Height(1), BeforeOverwinter), @@ -60,7 +60,7 @@ pub(crate) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] // TODO: Add Nu5 mainnet activation height ]; -#[cfg(feature = "test_fake_activation_heights")] +#[cfg(test_fake_activation_heights)] pub(crate) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[ (block::Height(0), Genesis), (block::Height(5), BeforeOverwinter), @@ -76,7 +76,7 @@ pub(crate) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] /// /// This is actually a bijective map, but it is const, so we use a vector, and /// do the uniqueness check in the unit tests. -#[cfg(not(feature = "test_fake_activation_heights"))] +#[cfg(not(test_fake_activation_heights))] pub(crate) const TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[ (block::Height(0), Genesis), (block::Height(1), BeforeOverwinter), @@ -88,7 +88,7 @@ pub(crate) const TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] // TODO: Add Nu5 testnet activation height ]; -#[cfg(feature = "test_fake_activation_heights")] +#[cfg(test_fake_activation_heights)] pub(crate) const TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[ (block::Height(0), Genesis), (block::Height(5), BeforeOverwinter), diff --git a/zebra-state/Cargo.toml b/zebra-state/Cargo.toml index a472ffff558..89c540b80af 100644 --- a/zebra-state/Cargo.toml +++ b/zebra-state/Cargo.toml @@ -7,7 +7,6 @@ edition = "2018" [features] proptest-impl = ["proptest", "zebra-test"] -test_fake_activation_heights = ["zebra-chain/test_fake_activation_heights"] [dependencies] zebra-chain = { path = "../zebra-chain" } @@ -39,7 +38,7 @@ proptest = { version = "0.10.1", optional = true } zebra-test = { path = "../zebra-test/", optional = true } [dev-dependencies] -zebra-chain = { path = "../zebra-chain", features = ["proptest-impl", "test_fake_activation_heights"] } +zebra-chain = { path = "../zebra-chain", features = ["proptest-impl"] } zebra-test = { path = "../zebra-test/" } color-eyre = "0.5.11" diff --git a/zebra-state/build.rs b/zebra-state/build.rs new file mode 100644 index 00000000000..91ba008f76c --- /dev/null +++ b/zebra-state/build.rs @@ -0,0 +1,9 @@ +use std::env; + +fn main() { + let use_fake_heights = env::var_os("TEST_FAKE_ACTIVATION_HEIGHTS").is_some(); + println!("cargo:rerun-if-env-changed=TEST_FAKE_ACTIVATION_HEIGHTS"); + if use_fake_heights { + println!("cargo:rustc-cfg=test_fake_activation_heights"); + } +} diff --git a/zebra-state/src/service/finalized_state/tests/prop.rs b/zebra-state/src/service/finalized_state/tests/prop.rs index fa4389aa4b1..92c2038f0ce 100644 --- a/zebra-state/src/service/finalized_state/tests/prop.rs +++ b/zebra-state/src/service/finalized_state/tests/prop.rs @@ -45,7 +45,7 @@ fn blocks_with_v5_transactions() -> Result<()> { /// sure the contextual validation done by the finalized state works. /// Also test if a block with the wrong commitment is correctly rejected. #[allow(dead_code)] -#[cfg_attr(feature = "test_fake_activation_heights", test)] +#[cfg_attr(test_fake_activation_heights, test)] fn all_upgrades_and_wrong_commitments() -> Result<()> { zebra_test::init(); // Use a single case and no_shrink() because this is more of a test vector, From 80768e129bf9b75511dbd0b02e61b10ed53e48f2 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Thu, 19 Aug 2021 13:39:17 -0300 Subject: [PATCH 09/16] Also update initial_tip_hash; refactor into progress_from_tip() --- zebra-consensus/src/checkpoint.rs | 52 +++++++++++++++++-------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/zebra-consensus/src/checkpoint.rs b/zebra-consensus/src/checkpoint.rs index b510774c021..3881232e0f5 100644 --- a/zebra-consensus/src/checkpoint.rs +++ b/zebra-consensus/src/checkpoint.rs @@ -99,6 +99,26 @@ pub const MAX_CHECKPOINT_HEIGHT_GAP: usize = 400; /// serialized size. pub const MAX_CHECKPOINT_BYTE_COUNT: u64 = 32 * 1024 * 1024; +/// Convert a tip into its hash and matching progress. +fn progress_from_tip( + checkpoint_list: &CheckpointList, + tip: Option<(block::Height, block::Hash)>, +) -> (Option, Progress) { + match tip { + Some((height, hash)) => { + if height >= checkpoint_list.max_height() { + (None, Progress::FinalCheckpoint) + } else { + metrics::gauge!("checkpoint.verified.height", height.0 as f64); + metrics::gauge!("checkpoint.processing.next.height", height.0 as f64); + (Some(hash), Progress::InitialTip(height)) + } + } + // We start by verifying the genesis block, by itself + None => (None, Progress::BeforeGenesis), + } +} + /// A checkpointing block verifier. /// /// Verifies blocks using a supplied list of checkpoints. There must be at @@ -222,19 +242,9 @@ where ) -> Self { // All the initialisers should call this function, so we only have to // change fields or default values in one place. - let (initial_tip_hash, verifier_progress) = match initial_tip { - Some((height, hash)) => { - if height >= checkpoint_list.max_height() { - (None, Progress::FinalCheckpoint) - } else { - metrics::gauge!("checkpoint.verified.height", height.0 as f64); - metrics::gauge!("checkpoint.processing.next.height", height.0 as f64); - (Some(hash), Progress::InitialTip(height)) - } - } - // We start by verifying the genesis block, by itself - None => (None, Progress::BeforeGenesis), - }; + let (initial_tip_hash, verifier_progress) = + progress_from_tip(&checkpoint_list, initial_tip); + let (sender, receiver) = mpsc::channel(); CheckpointVerifier { checkpoint_list, @@ -250,16 +260,9 @@ where /// Reset the verifier progress back to given tip. fn reset_progress(&mut self, tip: Option<(block::Height, block::Hash)>) { - self.verifier_progress = match tip { - Some((height, _)) => { - if height >= self.checkpoint_list.max_height() { - Progress::FinalCheckpoint - } else { - Progress::InitialTip(height) - } - } - None => Progress::BeforeGenesis, - }; + let (initial_tip_hash, verifier_progress) = progress_from_tip(&self.checkpoint_list, tip); + self.initial_tip_hash = initial_tip_hash; + self.verifier_progress = verifier_progress; } /// Return the current verifier's progress. @@ -993,7 +996,8 @@ where zs::Response::Tip(tip) => tip, _ => unreachable!("wrong response for Tip"), }; - // TODO: do we need to handle errors? + // Ignore errors since send() can fail only when the verifier + // is being dropped, and then it doesn't matter anymore. let _ = reset_sender.send(tip); } result From 9e823e2ac0eccabb6fe1d3a930f26cdd4074606c Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Thu, 19 Aug 2021 14:14:40 -0300 Subject: [PATCH 10/16] Improve comments --- zebra-chain/src/block/arbitrary.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/zebra-chain/src/block/arbitrary.rs b/zebra-chain/src/block/arbitrary.rs index 05cc1dbb3e5..3ba46fb1e95 100644 --- a/zebra-chain/src/block/arbitrary.rs +++ b/zebra-chain/src/block/arbitrary.rs @@ -376,6 +376,10 @@ impl Block { /// `check_transparent_coinbase_spend` is used to check if /// transparent coinbase UTXOs are valid, before using them in blocks. /// Use [`allow_all_transparent_coinbase_spends`] to disable this check. + /// + /// `generate_valid_commitments` specifies if the generated blocks + /// should have valid commitments. This makes it much slower so it's better + /// to enable only when needed. pub fn partial_chain_strategy( mut current: LedgerState, count: usize, @@ -407,7 +411,7 @@ impl Block { let mut sapling_tree = sapling::tree::NoteCommitmentTree::default(); let mut orchard_tree = orchard::tree::NoteCommitmentTree::default(); // The history tree usually takes care of "creating itself". But this - // only works when blocks are pushed to into starting from genesis + // only works when blocks are pushed into it starting from genesis // (or at least pre-Heartwood, where the tree is not required). // However, this strategy can generate blocks from an arbitrary height, // so we must wait for the first block to create the history tree from it. From 8e2968f1fffe5e48a102db1342645371132e97e0 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Thu, 19 Aug 2021 16:42:09 -0300 Subject: [PATCH 11/16] Add fake activation heights test to CI --- .github/workflows/ci.yml | 9 +++++++++ zebra-state/src/service/finalized_state/tests/prop.rs | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fc4cd3f29c8..220f693fc1a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,6 +69,15 @@ jobs: with: command: test args: --verbose --all + + - name: Run tests with fake activation heights + uses: actions-rs/cargo@v1.0.3 + env: + TEST_FAKE_ACTIVATION_HEIGHTS: + with: + command: test + args: --verbose --all -- with_fake_activation_heights + # Explicitly run any tests that are usually #[ignored] - name: Run zebrad large sync tests diff --git a/zebra-state/src/service/finalized_state/tests/prop.rs b/zebra-state/src/service/finalized_state/tests/prop.rs index 92c2038f0ce..3b1067e03f7 100644 --- a/zebra-state/src/service/finalized_state/tests/prop.rs +++ b/zebra-state/src/service/finalized_state/tests/prop.rs @@ -46,7 +46,7 @@ fn blocks_with_v5_transactions() -> Result<()> { /// Also test if a block with the wrong commitment is correctly rejected. #[allow(dead_code)] #[cfg_attr(test_fake_activation_heights, test)] -fn all_upgrades_and_wrong_commitments() -> Result<()> { +fn all_upgrades_and_wrong_commitments_with_fake_activation_heights() -> Result<()> { zebra_test::init(); // Use a single case and no_shrink() because this is more of a test vector, // just using the existing proptest machinery to create test blocks. From e2e3b6bf2af2c2967aecd84f1a2012c6ae57d97f Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Fri, 20 Aug 2021 15:36:40 -0300 Subject: [PATCH 12/16] Fix bug that caused commitment trees to not match when generating partial arbitrary chains --- zebra-chain/src/block/arbitrary.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/zebra-chain/src/block/arbitrary.rs b/zebra-chain/src/block/arbitrary.rs index 3ba46fb1e95..b794931dcee 100644 --- a/zebra-chain/src/block/arbitrary.rs +++ b/zebra-chain/src/block/arbitrary.rs @@ -434,11 +434,18 @@ impl Block { &mut utxos, check_transparent_coinbase_spend, ) { - for sapling_note_commitment in transaction.sapling_note_commitments() { - sapling_tree.append(*sapling_note_commitment).unwrap(); - } - for orchard_note_commitment in transaction.orchard_note_commitments() { - orchard_tree.append(*orchard_note_commitment).unwrap(); + // The FinalizedState does not update the note commitment trees with the genesis block, + // because it doesn't need to (the trees are not used at that point) and updating them + // would be awkward since the genesis block is handled separatedly there. + // This forces us to skip the genesis block here too in order to able to use + // this to test the finalized state. + if *height != Height(0) { + for sapling_note_commitment in transaction.sapling_note_commitments() { + sapling_tree.append(*sapling_note_commitment).unwrap(); + } + for orchard_note_commitment in transaction.orchard_note_commitments() { + orchard_tree.append(*orchard_note_commitment).unwrap(); + } } new_transactions.push(Arc::new(transaction)); } @@ -500,7 +507,7 @@ impl Block { .as_mut() .unwrap() .push( - Network::Mainnet, + current.network, Arc::new(block.clone()), sapling_tree.root(), orchard_tree.root(), From 32a53cd3dd9bcc1d6c930a454c6a93d64ae4ce7a Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Fri, 20 Aug 2021 16:00:51 -0300 Subject: [PATCH 13/16] Add ChainHistoryBlockTxAuthCommitmentHash::from_commitments to organize and deduplicate code --- Cargo.lock | 1 - zebra-chain/src/block.rs | 4 ++- zebra-chain/src/block/arbitrary.rs | 18 +++++-------- zebra-chain/src/block/commitment.rs | 42 +++++++++++++++++++++++++---- zebra-state/Cargo.toml | 1 - zebra-state/src/service/check.rs | 35 +++++++----------------- 6 files changed, 55 insertions(+), 46 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c3bed145a7d..d7eb9fd04ce 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4639,7 +4639,6 @@ name = "zebra-state" version = "1.0.0-alpha.15" dependencies = [ "bincode", - "blake2b_simd", "chrono", "color-eyre", "dirs", diff --git a/zebra-chain/src/block.rs b/zebra-chain/src/block.rs index 73f3553d5be..ecea4a1c580 100644 --- a/zebra-chain/src/block.rs +++ b/zebra-chain/src/block.rs @@ -16,7 +16,9 @@ pub mod tests; use std::{collections::HashMap, convert::TryInto, fmt, ops::Neg}; -pub use commitment::{ChainHistoryMmrRootHash, Commitment, CommitmentError}; +pub use commitment::{ + ChainHistoryBlockTxAuthCommitmentHash, ChainHistoryMmrRootHash, Commitment, CommitmentError, +}; pub use hash::Hash; pub use header::{BlockTimeError, CountedHeader, Header}; pub use height::Height; diff --git a/zebra-chain/src/block/arbitrary.rs b/zebra-chain/src/block/arbitrary.rs index b794931dcee..de5bef4ed59 100644 --- a/zebra-chain/src/block/arbitrary.rs +++ b/zebra-chain/src/block/arbitrary.rs @@ -474,18 +474,12 @@ impl Block { if nu5_height.is_some() && current_height >= nu5_height.unwrap() { // From zebra-state/src/service/check.rs let auth_data_root = block.auth_data_root(); - let hash_block_commitments: [u8; 32] = blake2b_simd::Params::new() - .hash_length(32) - .personal(b"ZcashBlockCommit") - .to_state() - .update(&<[u8; 32]>::from(history_tree_root)[..]) - .update(&<[u8; 32]>::from(auth_data_root)) - .update(&[0u8; 32]) - .finalize() - .as_bytes() - .try_into() - .expect("32 byte array"); - block.header.commitment_bytes = hash_block_commitments; + let hash_block_commitments = + ChainHistoryBlockTxAuthCommitmentHash::from_commitments( + &history_tree_root, + &auth_data_root, + ); + block.header.commitment_bytes = hash_block_commitments.into(); } else { block.header.commitment_bytes = history_tree_root.into(); } diff --git a/zebra-chain/src/block/commitment.rs b/zebra-chain/src/block/commitment.rs index eb2dda6a3cf..8b98125ffe0 100644 --- a/zebra-chain/src/block/commitment.rs +++ b/zebra-chain/src/block/commitment.rs @@ -2,10 +2,13 @@ use thiserror::Error; +use std::convert::TryInto; + use crate::parameters::{Network, NetworkUpgrade, NetworkUpgrade::*}; use crate::sapling; use super::super::block; +use super::merkle::AuthDataRoot; /// Zcash blocks contain different kinds of commitments to their contents, /// depending on the network and height. @@ -161,11 +164,6 @@ impl From for [u8; 32] { /// - the transaction authorising data in this block. /// /// Introduced in NU5. -// -// TODO: -// - add auth data type -// - add a method for hashing chain history and auth data together -// - move to a separate file #[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)] pub struct ChainHistoryBlockTxAuthCommitmentHash([u8; 32]); @@ -181,6 +179,40 @@ impl From for [u8; 32] { } } +impl ChainHistoryBlockTxAuthCommitmentHash { + /// Compute the block commitment from the history tree root and the + /// authorization data root, as specified in [ZIP-244]. + /// + /// `history_tree_root` is the root of the history tree up to and including + /// the *previous* block. + /// `auth_data_root` is the root of the Merkle tree of authorizing data + /// commmitments of each transaction in the *current* block. + /// + /// [ZIP-244]: https://zips.z.cash/zip-0244#block-header-changes + pub fn from_commitments( + history_tree_root: &ChainHistoryMmrRootHash, + auth_data_root: &AuthDataRoot, + ) -> Self { + // > The value of this hash [hashBlockCommitments] is the BLAKE2b-256 hash personalized + // > by the string "ZcashBlockCommit" of the following elements: + // > hashLightClientRoot (as described in ZIP 221) + // > hashAuthDataRoot (as described below) + // > terminator [0u8;32] + let hash_block_commitments: [u8; 32] = blake2b_simd::Params::new() + .hash_length(32) + .personal(b"ZcashBlockCommit") + .to_state() + .update(&<[u8; 32]>::from(*history_tree_root)[..]) + .update(&<[u8; 32]>::from(*auth_data_root)) + .update(&[0u8; 32]) + .finalize() + .as_bytes() + .try_into() + .expect("32 byte array"); + Self(hash_block_commitments) + } +} + /// Errors that can occur when checking RootHash consensus rules. /// /// Each error variant corresponds to a consensus rule, so enumerating diff --git a/zebra-state/Cargo.toml b/zebra-state/Cargo.toml index 89c540b80af..00a30e8a638 100644 --- a/zebra-state/Cargo.toml +++ b/zebra-state/Cargo.toml @@ -32,7 +32,6 @@ rlimit = "0.5.4" # TODO: this crate is not maintained anymore. Replace it? # https://github.com/ZcashFoundation/zebra/issues/2523 multiset = "0.0.5" -blake2b_simd = "0.5.11" proptest = { version = "0.10.1", optional = true } zebra-test = { path = "../zebra-test/", optional = true } diff --git a/zebra-state/src/service/check.rs b/zebra-state/src/service/check.rs index fe7872991de..88cc4ba260b 100644 --- a/zebra-state/src/service/check.rs +++ b/zebra-state/src/service/check.rs @@ -1,11 +1,11 @@ //! Consensus critical contextual checks -use std::{borrow::Borrow, convert::TryInto, sync::Arc}; +use std::{borrow::Borrow, sync::Arc}; use chrono::Duration; use zebra_chain::{ - block::{self, Block, CommitmentError}, + block::{self, Block, ChainHistoryBlockTxAuthCommitmentHash, CommitmentError}, history_tree::HistoryTree, parameters::POW_AVERAGING_WINDOW, parameters::{Network, NetworkUpgrade}, @@ -150,40 +150,23 @@ fn block_commitment_is_valid_for_chain_history( } } block::Commitment::ChainHistoryBlockTxAuthCommitment(actual_hash_block_commitments) => { - let actual_block_commitments: [u8; 32] = actual_hash_block_commitments.into(); let history_tree_root = history_tree .hash() .expect("the history tree of the previous block must exist since the current block has a ChainHistoryBlockTxAuthCommitment"); let auth_data_root = block.auth_data_root(); - // > The value of this hash [hashBlockCommitments] is the BLAKE2b-256 hash personalized - // > by the string "ZcashBlockCommit" of the following elements: - // > hashLightClientRoot (as described in ZIP 221) - // > hashAuthDataRoot (as described below) - // > terminator [0u8;32] - // https://zips.z.cash/zip-0244#block-header-changes - // - // Note that auth data root is from the current block, while - // the history tree root is from the previous block. - let hash_block_commitments: [u8; 32] = blake2b_simd::Params::new() - .hash_length(32) - .personal(b"ZcashBlockCommit") - .to_state() - .update(&<[u8; 32]>::from(history_tree_root)[..]) - .update(&<[u8; 32]>::from(auth_data_root)) - .update(&[0u8; 32]) - .finalize() - .as_bytes() - .try_into() - .expect("32 byte array"); + let hash_block_commitments = ChainHistoryBlockTxAuthCommitmentHash::from_commitments( + &history_tree_root, + &auth_data_root, + ); - if actual_block_commitments == hash_block_commitments { + if actual_hash_block_commitments == hash_block_commitments { Ok(()) } else { Err(ValidateContextError::InvalidBlockCommitment( CommitmentError::InvalidChainHistoryBlockTxAuthCommitment { - actual: actual_block_commitments, - expected: hash_block_commitments, + actual: actual_hash_block_commitments.into(), + expected: hash_block_commitments.into(), }, )) } From e3d1907b32f2ee8be5f8d0914c548adf03be4b3f Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Fri, 20 Aug 2021 16:16:00 -0300 Subject: [PATCH 14/16] Remove stale comment, improve readability --- zebra-consensus/src/checkpoint.rs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/zebra-consensus/src/checkpoint.rs b/zebra-consensus/src/checkpoint.rs index 3881232e0f5..23fc6124ed5 100644 --- a/zebra-consensus/src/checkpoint.rs +++ b/zebra-consensus/src/checkpoint.rs @@ -17,10 +17,7 @@ use std::{ collections::BTreeMap, ops::{Bound, Bound::*}, pin::Pin, - sync::{ - mpsc::{self, Receiver, Sender}, - Arc, - }, + sync::{mpsc, Arc}, task::{Context, Poll}, }; @@ -158,10 +155,10 @@ where /// A channel to receive requests to reset the verifier, /// receiving the tip of the state. - reset_receiver: Receiver>, + reset_receiver: mpsc::Receiver>, /// A channel to send requests to reset the verifier, /// passing the tip of the state. - reset_sender: Sender>, + reset_sender: mpsc::Sender>, } impl CheckpointVerifier @@ -948,10 +945,6 @@ where .map_err(VerifyCheckpointError::CommitFinalized) .expect("CheckpointVerifier does not leave dangling receivers")?; - // Once we get a verified hash, we must commit it to the chain state - // as a finalized block, or exit the program, so .expect rather than - // propagate errors from the state service. - // // We use a `ServiceExt::oneshot`, so that every state service // `poll_ready` has a corresponding `call`. See #1593. match state_service From c0abcdeb3a4bda94fb2faa099885dfa412d4b191 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Fri, 20 Aug 2021 16:24:02 -0300 Subject: [PATCH 15/16] Allow overriding with PROPTEST_CASES --- zebra-state/src/service/finalized_state/tests/prop.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/zebra-state/src/service/finalized_state/tests/prop.rs b/zebra-state/src/service/finalized_state/tests/prop.rs index 3b1067e03f7..d5e66b2feec 100644 --- a/zebra-state/src/service/finalized_state/tests/prop.rs +++ b/zebra-state/src/service/finalized_state/tests/prop.rs @@ -48,9 +48,11 @@ fn blocks_with_v5_transactions() -> Result<()> { #[cfg_attr(test_fake_activation_heights, test)] fn all_upgrades_and_wrong_commitments_with_fake_activation_heights() -> Result<()> { zebra_test::init(); - // Use a single case and no_shrink() because this is more of a test vector, - // just using the existing proptest machinery to create test blocks. - proptest!(ProptestConfig::with_cases(1), + // Use no_shrink() because we're ignoring _count and there is nothing to actually shrink. + proptest!(ProptestConfig::with_cases(env::var("PROPTEST_CASES") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(DEFAULT_PARTIAL_CHAIN_PROPTEST_CASES)), |((chain, _count, network, _history_tree) in PreparedChain::default().with_valid_commitments().no_shrink())| { let mut state = FinalizedState::new(&Config::ephemeral(), network); From f6844224ad75c31d2610a32a366698edb8840499 Mon Sep 17 00:00:00 2001 From: Conrado Gouvea Date: Fri, 20 Aug 2021 17:04:17 -0300 Subject: [PATCH 16/16] partial_chain_strategy(): don't update note commitment trees when not needed; add comment --- zebra-chain/src/block/arbitrary.rs | 2 +- zebra-state/src/service/arbitrary.rs | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/zebra-chain/src/block/arbitrary.rs b/zebra-chain/src/block/arbitrary.rs index de5bef4ed59..408c69c6f42 100644 --- a/zebra-chain/src/block/arbitrary.rs +++ b/zebra-chain/src/block/arbitrary.rs @@ -439,7 +439,7 @@ impl Block { // would be awkward since the genesis block is handled separatedly there. // This forces us to skip the genesis block here too in order to able to use // this to test the finalized state. - if *height != Height(0) { + if generate_valid_commitments && *height != Height(0) { for sapling_note_commitment in transaction.sapling_note_commitments() { sapling_tree.append(*sapling_note_commitment).unwrap(); } diff --git a/zebra-state/src/service/arbitrary.rs b/zebra-state/src/service/arbitrary.rs index 93a4125430c..3d99cf6b045 100644 --- a/zebra-state/src/service/arbitrary.rs +++ b/zebra-state/src/service/arbitrary.rs @@ -89,6 +89,9 @@ impl PreparedChain { } } + /// Transform the strategy to use valid commitments in the block. + /// + /// This is slower so it should be used only when needed. #[allow(dead_code)] pub(crate) fn with_valid_commitments(mut self) -> Self { self.generate_valid_commitments = true;