Clean up block commitment enum and parsing (#1978)

* Rename RootHash to Commitment based on ZIP-244

Interactive replace using:
```sh
fastmod RootHash Commitment
fastmod root_hash commitment
fastmod root_bytes commitment_bytes
git mv zebra-chain/src/block/root_hash.rs zebra-chain/src/block/commitment.rs
```

All replacements were accepted.

* rustfmt

* Comment and format cleanups after interactive replace

* Distinguish Sapling tree roots from other tree roots

* Add the NU5 BlockCommitmentsHash variant to block::Commitment

This change parses the hash, but does not perform validation.

* Validate reserved values in Block::commitment

- change Block::commitment to return a Result rather than an Option
- enforce the all-zeroes reserved value consensus rules
- change `PreSaplingReserved([u8; 32])` to `PreSaplingReserved`
- change `ChainHistoryActivationReserved([u8; 32])` to `ChainHistoryActivationReserved`
- update the function comments to describe when each variant is verified

* Fix comment whitespace

zebra-chain/src/block.rs

@@ -16,10 +16,9 @@ mod tests;
 
 use std::fmt;
 
-pub use commitment::Commitment;
+pub use commitment::{Commitment, CommitmentError};
 pub use hash::Hash;
-pub use header::BlockTimeError;
+pub use header::{BlockTimeError, CountedHeader, Header};
-pub use header::{CountedHeader, Header};
 pub use height::Height;
 pub use serialize::MAX_BLOCK_BYTES;
 
@@ -70,15 +69,20 @@ impl Block {
         Hash::from(self)
     }
 
-    /// Get the parsed root hash for this block.
+    /// Get the parsed block [`Commitment`] for this block.
     ///
-    /// The interpretation of the root hash depends on the
+    /// The interpretation of the commitment depends on the
     /// configured `network`, and this block's height.
     ///
-    /// Returns None if this block does not have a block height.
+    /// Returns an error if this block does not have a block height,
-    pub fn commitment(&self, network: Network) -> Option<Commitment> {
+    /// or if the commitment value is structurally invalid.
-        self.coinbase_height()
+    pub fn commitment(&self, network: Network) -> Result<Commitment, CommitmentError> {
-            .map(|height| Commitment::from_bytes(self.header.commitment_bytes, network, height))
+        match self.coinbase_height() {
+            None => Err(CommitmentError::MissingBlockHeight {
+                block_hash: self.hash(),
+            }),
+            Some(height) => Commitment::from_bytes(self.header.commitment_bytes, network, height),
+        }
     }
 }
 

zebra-chain/src/block/arbitrary.rs

@@ -53,7 +53,16 @@ impl Arbitrary for Commitment {
     fn arbitrary_with(_args: ()) -> Self::Strategy {
         (any::<[u8; 32]>(), any::<Network>(), any::<Height>())
             .prop_map(|(commitment_bytes, network, block_height)| {
-                Commitment::from_bytes(commitment_bytes, network, block_height)
+                match Commitment::from_bytes(commitment_bytes, network, block_height) {
+                    Ok(commitment) => commitment,
+                    // just fix up the reserved values when they fail
+                    Err(_) => Commitment::from_bytes(
+                        super::commitment::RESERVED_BYTES,
+                        network,
+                        block_height,
+                    )
+                    .expect("from_bytes only fails due to reserved bytes"),
+                }
             })
             .boxed()
     }

zebra-chain/src/block/commitment.rs

@@ -1,36 +1,55 @@
 //! The Commitment enum, used for the corresponding block header field.
 
+use thiserror::Error;
+
 use crate::parameters::{Network, NetworkUpgrade, NetworkUpgrade::*};
-use crate::sapling::tree::Root;
+use crate::sapling;
 
-use super::Height;
+use super::super::block;
 
-/// Zcash blocks contain different kinds of root hashes, depending on the network upgrade.
+/// Zcash blocks contain different kinds of commitments to their contents,
+/// depending on the network and height.
 ///
-/// The `BlockHeader.commitment_bytes` field is interpreted differently,
+/// The `Header.commitment_bytes` field is interpreted differently, based on the
-/// based on the current block height. The interpretation changes at or after
+/// network and height. The interpretation changes in the network upgrade
-/// network upgrades.
+/// activation block, or in the block immediately after network upgrade
+/// activation.
 #[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)]
 pub enum Commitment {
     /// [Pre-Sapling] Reserved field.
     ///
-    /// All zeroes.
+    /// The value of this field MUST be all zeroes.
-    PreSaplingReserved([u8; 32]),
+    ///
+    /// This field is verified in `Commitment::from_bytes`.
+    PreSaplingReserved,
 
     /// [Sapling and Blossom] The final Sapling treestate of this block.
     ///
     /// The root LEBS2OSP256(rt) of the Sapling note commitment tree
     /// corresponding to the final Sapling treestate of this block.
-    FinalSaplingRoot(Root),
+    ///
+    /// Subsequent `Commitment` variants also commit to the `FinalSaplingRoot`,
+    /// via their `EarliestSaplingRoot` and `LatestSaplingRoot` fields.
+    ///
+    /// TODO: this field is verified during semantic verification
+    ///
+    /// Since Zebra checkpoints on Canopy, we don't need to validate this
+    /// field, but since it's included in the ChainHistoryRoot, we are
+    /// already calculating it, so we might as well validate it.
+    FinalSaplingRoot(sapling::tree::Root),
 
     /// [Heartwood activation block] Reserved field.
     ///
-    /// All zeroes. This MUST NOT be interpreted as a root hash.
+    /// The value of this field MUST be all zeroes.
+    ///
+    /// This MUST NOT be interpreted as a root hash.
     /// See ZIP-221 for details.
-    ChainHistoryActivationReserved([u8; 32]),
+    ///
+    /// This field is verified in `Commitment::from_bytes`.
+    ChainHistoryActivationReserved,
 
-    /// [After Heartwood activation block] The root of a Merkle Mountain
+    /// [(Heartwood activation block + 1) to Canopy] The root of a Merkle
-    /// Range chain history tree.
+    /// Mountain Range chain history tree.
     ///
     /// This root hash commits to various features of the chain's history,
     /// including the Sapling commitment tree. This commitment supports the
@@ -39,23 +58,65 @@ pub enum Commitment {
     /// The commitment in each block covers the chain history from the most
     /// recent network upgrade, through to the previous block. In particular,
     /// an activation block commits to the entire previous network upgrade, and
-    /// the block after activation commits only to the activation block.
+    /// the block after activation commits only to the activation block. (And
+    /// therefore transitively to all previous network upgrades covered by a
+    /// chain history hash in their activation block, via the previous block
+    /// hash field.)
+    ///
+    /// TODO: this field is verified during semantic verification
     ChainHistoryRoot(ChainHistoryMmrRootHash),
+
+    /// [NU5 activation onwards] A commitment to:
+    /// - the chain history Merkle Mountain Range tree, and
+    /// - the auth data merkle tree covering this block.
+    ///
+    /// The chain history Merkle Mountain Range tree commits to the previous
+    /// block and all ancestors in the current network upgrade. The auth data
+    /// merkle tree commits to this block.
+    ///
+    /// This commitment supports the FlyClient protocol and non-malleable
+    /// transaction IDs. See ZIP-221 and ZIP-244 for details.
+    ///
+    /// See also the [`ChainHistoryRoot`] variant.
+    ///
+    /// TODO: this field is verified during semantic verification
+    //
+    // TODO: Do block commitments activate at NU5 activation, or (NU5 + 1)?
+    // https://github.com/zcash/zips/pull/474
+    BlockCommitments(BlockCommitmentsHash),
 }
 
+/// The required value of reserved `Commitment`s.
+pub(crate) const RESERVED_BYTES: [u8; 32] = [0; 32];
+
 impl Commitment {
     /// Returns `bytes` as the Commitment variant for `network` and `height`.
-    pub(super) fn from_bytes(bytes: [u8; 32], network: Network, height: Height) -> Commitment {
+    pub(super) fn from_bytes(
+        bytes: [u8; 32],
+        network: Network,
+        height: block::Height,
+    ) -> Result<Commitment, CommitmentError> {
         use Commitment::*;
+        use CommitmentError::*;
 
         match NetworkUpgrade::current(network, height) {
-            Genesis | BeforeOverwinter | Overwinter => PreSaplingReserved(bytes),
+            Genesis | BeforeOverwinter | Overwinter => {
-            Sapling | Blossom => FinalSaplingRoot(Root(bytes)),
+                if bytes == RESERVED_BYTES {
-            Heartwood if Some(height) == Heartwood.activation_height(network) => {
+                    Ok(PreSaplingReserved)
-                ChainHistoryActivationReserved(bytes)
+                } else {
+                    Err(InvalidPreSaplingReserved { actual: bytes })
+                }
             }
-            Heartwood | Canopy => ChainHistoryRoot(ChainHistoryMmrRootHash(bytes)),
+            Sapling | Blossom => Ok(FinalSaplingRoot(sapling::tree::Root(bytes))),
-            Nu5 => unimplemented!("Nu5 uses hashAuthDataRoot as specified in ZIP-244"),
+            Heartwood if Some(height) == Heartwood.activation_height(network) => {
+                if bytes == RESERVED_BYTES {
+                    Ok(ChainHistoryActivationReserved)
+                } else {
+                    Err(InvalidChainHistoryActivationReserved { actual: bytes })
+                }
+            }
+            Heartwood | Canopy => Ok(ChainHistoryRoot(ChainHistoryMmrRootHash(bytes))),
+            Nu5 => Ok(BlockCommitments(BlockCommitmentsHash(bytes))),
         }
     }
 
@@ -65,10 +126,11 @@ impl Commitment {
         use Commitment::*;
 
         match self {
-            PreSaplingReserved(b) => b,
+            PreSaplingReserved => RESERVED_BYTES,
-            FinalSaplingRoot(v) => v.0,
+            FinalSaplingRoot(hash) => hash.0,
-            ChainHistoryActivationReserved(b) => b,
+            ChainHistoryActivationReserved => RESERVED_BYTES,
-            ChainHistoryRoot(v) => v.0,
+            ChainHistoryRoot(hash) => hash.0,
+            BlockCommitments(hash) => hash.0,
         }
     }
 }
@@ -76,7 +138,60 @@ impl Commitment {
 /// The root hash of a Merkle Mountain Range chain history tree.
 // TODO:
 //    - add methods for maintaining the MMR peaks, and calculating the root
-//      hash from the current set of peaks.
+//      hash from the current set of peaks
-//    - move to a separate file.
+//    - move to a separate file
 #[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)]
 pub struct ChainHistoryMmrRootHash([u8; 32]);
+
+/// The Block Commitments for a block. As of NU5, these cover:
+/// - the chain history tree for all ancestors in the current network upgrade,
+///   and
+/// - the transaction authorising data in this block.
+//
+// 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 BlockCommitmentsHash([u8; 32]);
+
+/// Errors that can occur when checking RootHash consensus rules.
+///
+/// Each error variant corresponds to a consensus rule, so enumerating
+/// all possible verification failures enumerates the consensus rules we
+/// implement, and ensures that we don't reject blocks or transactions
+/// for a non-enumerated reason.
+#[allow(dead_code)]
+#[derive(Error, Debug, PartialEq)]
+pub enum CommitmentError {
+    #[error("invalid pre-Sapling reserved committment: expected all zeroes, actual: {actual:?}")]
+    InvalidPreSaplingReserved {
+        // TODO: are these fields a security risk? If so, open a ticket to remove
+        // similar fields across Zebra
+        actual: [u8; 32],
+    },
+
+    #[error("invalid final sapling root: expected {expected:?}, actual: {actual:?}")]
+    InvalidFinalSaplingRoot {
+        expected: [u8; 32],
+        actual: [u8; 32],
+    },
+
+    #[error("invalid chain history activation reserved block committment: expected all zeroes, actual: {actual:?}")]
+    InvalidChainHistoryActivationReserved { actual: [u8; 32] },
+
+    #[error("invalid chain history root: expected {expected:?}, actual: {actual:?}")]
+    InvalidChainHistoryRoot {
+        expected: [u8; 32],
+        actual: [u8; 32],
+    },
+
+    #[error("invalid block commitment: expected {expected:?}, actual: {actual:?}")]
+    InvalidBlockCommitment {
+        expected: [u8; 32],
+        actual: [u8; 32],
+    },
+
+    #[error("missing required block height: block commitments can't be parsed without a block height, block hash: {block_hash:?}")]
+    MissingBlockHeight { block_hash: block::Hash },
+}

zebra-chain/src/block/header.rs

@@ -45,12 +45,14 @@ pub struct Header {
     /// valid.
     pub merkle_root: merkle::Root,
 
-    /// Some kind of root hash.
+    /// Zcash blocks contain different kinds of commitments to their contents,
+    /// depending on the network and height.
     ///
-    /// Unfortunately, the interpretation of this field was changed without
+    /// The interpretation of this field has been changed multiple times, without
-    /// incrementing the version, so it cannot be parsed without the block height
+    /// incrementing the block [`version`]. Therefore, this field cannot be
-    /// and network. Use [`Block::commitment`](super::Block::commitment) to get the
+    /// parsed without the network and height. Use
-    /// parsed [`Commitment`](super::Commitment).
+    /// [`Block::commitment`](super::Block::commitment) to get the parsed
+    /// [`Commitment`](super::Commitment).
     pub commitment_bytes: [u8; 32],
 
     /// The block timestamp is a Unix epoch time (UTC) when the miner

zebra-chain/src/block/tests/prop.rs

@@ -47,10 +47,13 @@ proptest! {
     ) {
         zebra_test::init();
 
-        let commitment = Commitment::from_bytes(bytes, network, block_height);
+        // just skip the test if the bytes don't parse, because there's nothing
-        let other_bytes = commitment.to_bytes();
+        // to compare with
+        if let Ok(commitment) = Commitment::from_bytes(bytes, network, block_height) {
+            let other_bytes = commitment.to_bytes();
 
-        prop_assert_eq![bytes, other_bytes];
+            prop_assert_eq![bytes, other_bytes];
+        }
     }
 }
 
@@ -69,13 +72,11 @@ proptest! {
         let bytes = block.zcash_serialize_to_vec()?;
         let bytes = &mut bytes.as_slice();
 
-        // Check the root hash
+        // Check the block commitment
         let commitment = block.commitment(network);
-        if let Some(commitment) = commitment {
+        if let Ok(commitment) = commitment {
             let commitment_bytes = commitment.to_bytes();
             prop_assert_eq![block.header.commitment_bytes, commitment_bytes];
-        } else {
-            prop_assert_eq![block.coinbase_height(), None];
         }
 
         // Check the block size limit