ZIP-221/244 auth data commitment validation in checkpoint verifier (#2633)

* Add validation of ZIP-221 and ZIP-244 commitments

* Apply suggestions from code review

Co-authored-by: teor <teor@riseup.net>

* Add auth commitment check in the finalized state

* Reset the verifier when comitting to state fails

* Add explanation comment

* Add test with fake activation heights

* Add generate_valid_commitments flag

* Enable fake activation heights using env var instead of feature

* Also update initial_tip_hash; refactor into progress_from_tip()

* Improve comments

* Add fake activation heights test to CI

* Fix bug that caused commitment trees to not match when generating partial arbitrary chains

* Add ChainHistoryBlockTxAuthCommitmentHash::from_commitments to organize and deduplicate code

* Remove stale comment, improve readability

* Allow overriding with PROPTEST_CASES

* partial_chain_strategy(): don't update note commitment trees when not needed; add comment

Co-authored-by: teor <teor@riseup.net>

.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

Cargo.lock

@@ -4639,7 +4639,6 @@ name = "zebra-state"
 version = "1.0.0-alpha.15"
 dependencies = [
  "bincode",
- "blake2b_simd",
  "chrono",
  "color-eyre",
  "dirs",

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");
+    }
+}

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;

zebra-chain/src/block/arbitrary.rs

@@ -11,6 +11,7 @@ use crate::{
     amount::NonNegative,
     block,
     fmt::SummaryDebug,
+    history_tree::HistoryTree,
     parameters::{
         Network,
         NetworkUpgrade::{self, *},
@@ -375,10 +376,15 @@ 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<F, T, E>(
         mut current: LedgerState,
         count: usize,
         check_transparent_coinbase_spend: F,
+        generate_valid_commitments: bool,
     ) -> BoxedStrategy<SummaryDebug<Vec<Arc<Self>>>>
     where
         F: Fn(
@@ -402,6 +408,15 @@ 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();
+            // The history tree usually takes care of "creating itself". But this
+            // 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.
+            // This is why `Option` is used here.
+            let mut history_tree: Option<HistoryTree> = None;
 
             for (height, block) in vec.iter_mut() {
                 // fixup the previous block hash
@@ -419,6 +434,19 @@ impl Block {
                         &mut utxos,
                         check_transparent_coinbase_spend,
                     ) {
+                        // 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 generate_valid_commitments && *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));
                     }
                 }
@@ -426,9 +454,61 @@ impl Block {
                 // delete invalid transactions
                 block.transactions = new_transactions;
 
-                // TODO: if needed, fixup after modifying the block:
+                // fix commitment (must be done after finishing changing the block)
-                // - history and authorizing data commitments
+                if generate_valid_commitments {
-                // - the transaction merkle root
+                    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 =
+                                    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();
+                            }
+                        }
+                    }
+                    // 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(
+                                current.network,
+                                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

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<ChainHistoryMmrRootHash> 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<ChainHistoryBlockTxAuthCommitmentHash> 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

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,
             )
         });
 

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(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(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(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(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)]

zebra-consensus/src/checkpoint.rs

@@ -17,7 +17,7 @@ use std::{
     collections::BTreeMap,
     ops::{Bound, Bound::*},
     pin::Pin,
-    sync::Arc,
+    sync::{mpsc, Arc},
     task::{Context, Poll},
 };
 
@@ -96,6 +96,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<block::Hash>, Progress<block::Height>) {
+    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
@@ -132,6 +152,13 @@ where
 
     /// The current progress of this verifier.
     verifier_progress: Progress<block::Height>,
+
+    /// A channel to receive requests to reset the verifier,
+    /// receiving the tip of the state.
+    reset_receiver: mpsc::Receiver<Option<(block::Height, block::Hash)>>,
+    /// A channel to send requests to reset the verifier,
+    /// passing the tip of the state.
+    reset_sender: mpsc::Sender<Option<(block::Height, block::Hash)>>,
 }
 
 impl<S> CheckpointVerifier<S>
@@ -212,19 +239,10 @@ 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 {
+        let (initial_tip_hash, verifier_progress) =
-            Some((height, hash)) => {
+            progress_from_tip(&checkpoint_list, initial_tip);
-                if height >= checkpoint_list.max_height() {
+
-                    (None, Progress::FinalCheckpoint)
+        let (sender, receiver) = mpsc::channel();
-                } 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),
-        };
         CheckpointVerifier {
             checkpoint_list,
             network,
@@ -232,9 +250,18 @@ 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)>) {
+        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.
     ///
     /// 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<Block>) -> 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();
@@ -910,17 +945,12 @@ 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
                 .oneshot(zs::Request::CommitFinalizedBlock(block.into()))
                 .map_err(VerifyCheckpointError::CommitFinalized)
-                .await
+                .await?
-                .expect("state service commit block failed: verified checkpoints must be committed transactionally")
             {
                 zs::Response::Committed(committed_hash) => {
                     assert_eq!(committed_hash, hash, "state must commit correct hash");
@@ -930,6 +960,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
@@ -939,11 +971,29 @@ 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"),
+                };
+                // 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
         }
         .boxed()
     }

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 }

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");
+    }
+}

zebra-state/src/service/arbitrary.rs

@@ -58,6 +58,7 @@ pub struct PreparedChain {
     chain: std::sync::Mutex<Option<(Network, Arc<SummaryDebug<Vec<PreparedBlock>>>, HistoryTree)>>,
     // the strategy for generating LedgerStates. If None, it calls [`LedgerState::genesis_strategy`].
     ledger_strategy: Option<BoxedStrategy<LedgerState>>,
+    generate_valid_commitments: bool,
 }
 
 impl PreparedChain {
@@ -87,6 +88,15 @@ impl PreparedChain {
             ..Default::default()
         }
     }
+
+    /// 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;
+        self
+    }
 }
 
 impl Strategy for PreparedChain {
@@ -112,6 +122,7 @@ impl Strategy for PreparedChain {
                             ledger,
                             MAX_PARTIAL_CHAIN_BLOCKS,
                             check::utxo::transparent_coinbase_spend,
+                            self.generate_valid_commitments,
                         ),
                     )
                 })

zebra-state/src/service/check.rs

@@ -1,18 +1,18 @@
 //! Consensus critical contextual checks
 
-use std::{borrow::Borrow, convert::TryInto};
+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},
     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<Block>,
+    network: Network,
+    history_tree: &HistoryTree,
+) -> Result<(), ValidateContextError> {
+    match block.commitment(network)? {
         block::Commitment::PreSaplingReserved(_)
         | block::Commitment::FinalSaplingRoot(_)
         | block::Commitment::ChainHistoryActivationReserved => {
@@ -131,37 +150,23 @@ pub(crate) 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 = 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
+            let hash_block_commitments = ChainHistoryBlockTxAuthCommitmentHash::from_commitments(
-            // > by the string "ZcashBlockCommit" of the following elements:
+                &history_tree_root,
-            // >   hashLightClientRoot (as described in ZIP 221)
+                &auth_data_root,
-            // >   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 {
+            if actual_hash_block_commitments == hash_block_commitments {
                 Ok(())
             } else {
                 Err(ValidateContextError::InvalidBlockCommitment(
                     CommitmentError::InvalidChainHistoryBlockTxAuthCommitment {
-                        actual: actual_block_commitments,
+                        actual: actual_hash_block_commitments.into(),
-                        expected: hash_block_commitments,
+                        expected: hash_block_commitments.into(),
                     },
                 ))
             }

zebra-state/src/service/finalized_state.rs

@@ -19,7 +19,7 @@ use zebra_chain::{
     value_balance::ValueBalance,
 };
 
-use crate::{BoxError, Config, FinalizedBlock, HashOrHeight};
+use crate::{service::check, BoxError, Config, FinalizedBlock, HashOrHeight};
 
 use self::disk_format::{DiskDeserialize, DiskSerialize, FromDisk, IntoDisk, TransactionLocation};
 
@@ -207,6 +207,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,
@@ -214,14 +216,6 @@ impl FinalizedState {
     ) -> Result<block::Hash, BoxError> {
         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();
@@ -248,27 +242,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,
             );
@@ -280,6 +274,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.

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,
 };
 
 const DEFAULT_PARTIAL_CHAIN_PROPTEST_CASES: u32 = 1;
@@ -38,3 +39,59 @@ 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(test_fake_activation_heights, test)]
+fn all_upgrades_and_wrong_commitments_with_fake_activation_heights() -> Result<()> {
+    zebra_test::init();
+    // 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);
+            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(())
+}

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,

zebra-state/src/service/non_finalized_state/tests/prop.rs

@@ -244,7 +244,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();

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))