From 597f5be46101d84f9ef7edbecae6f28eb7261af0 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 29 Mar 2021 13:14:43 +1000
Subject: [PATCH 1/7] 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.
---
 zebra-chain/src/block.rs                       |  8 ++++----
 zebra-chain/src/block/arbitrary.rs             | 10 +++++-----
 .../src/block/{root_hash.rs => commitment.rs}  | 18 +++++++++---------
 zebra-chain/src/block/header.rs                |  6 +++---
 zebra-chain/src/block/serialize.rs             |  4 ++--
 zebra-chain/src/block/tests/prop.rs            | 14 +++++++-------
 6 files changed, 30 insertions(+), 30 deletions(-)
 rename zebra-chain/src/block/{root_hash.rs => commitment.rs} (87%)

diff --git a/zebra-chain/src/block.rs b/zebra-chain/src/block.rs
index 6f741832ed0..7e3acb3e045 100644
--- a/zebra-chain/src/block.rs
+++ b/zebra-chain/src/block.rs
@@ -4,7 +4,7 @@
 mod hash;
 mod header;
 mod height;
-mod root_hash;
+mod commitment;
 mod serialize;
 
 pub mod merkle;
@@ -20,7 +20,7 @@ pub use hash::Hash;
 pub use header::BlockTimeError;
 pub use header::{CountedHeader, Header};
 pub use height::Height;
-pub use root_hash::RootHash;
+pub use commitment::Commitment;
 
 use serde::{Deserialize, Serialize};
 
@@ -69,9 +69,9 @@ impl Block {
     /// configured `network`, and this block's height.
     ///
     /// Returns None if this block does not have a block height.
-    pub fn root_hash(&self, network: Network) -> Option<RootHash> {
+    pub fn commitment(&self, network: Network) -> Option<Commitment> {
         self.coinbase_height()
-            .map(|height| RootHash::from_bytes(self.header.root_bytes, network, height))
+            .map(|height| Commitment::from_bytes(self.header.commitment_bytes, network, height))
     }
 }
 
diff --git a/zebra-chain/src/block/arbitrary.rs b/zebra-chain/src/block/arbitrary.rs
index fe96c4d6957..f50d1f9347d 100644
--- a/zebra-chain/src/block/arbitrary.rs
+++ b/zebra-chain/src/block/arbitrary.rs
@@ -47,13 +47,13 @@ impl Block {
     }
 }
 
-impl Arbitrary for RootHash {
+impl Arbitrary for Commitment {
     type Parameters = ();
 
     fn arbitrary_with(_args: ()) -> Self::Strategy {
         (any::<[u8; 32]>(), any::<Network>(), any::<Height>())
-            .prop_map(|(root_bytes, network, block_height)| {
-                RootHash::from_bytes(root_bytes, network, block_height)
+            .prop_map(|(commitment_bytes, network, block_height)| {
+                Commitment::from_bytes(commitment_bytes, network, block_height)
             })
             .boxed()
     }
@@ -82,7 +82,7 @@ impl Arbitrary for Header {
                     version,
                     previous_block_hash,
                     merkle_root,
-                    root_bytes,
+                    commitment_bytes,
                     timestamp,
                     difficulty_threshold,
                     nonce,
@@ -91,7 +91,7 @@ impl Arbitrary for Header {
                     version,
                     previous_block_hash,
                     merkle_root,
-                    root_bytes,
+                    commitment_bytes,
                     time: Utc.timestamp(timestamp, 0),
                     difficulty_threshold,
                     nonce,
diff --git a/zebra-chain/src/block/root_hash.rs b/zebra-chain/src/block/commitment.rs
similarity index 87%
rename from zebra-chain/src/block/root_hash.rs
rename to zebra-chain/src/block/commitment.rs
index 146124de09d..de250357275 100644
--- a/zebra-chain/src/block/root_hash.rs
+++ b/zebra-chain/src/block/commitment.rs
@@ -1,4 +1,4 @@
-//! The RootHash enum, used for the corresponding block header field.
+//! The Commitment enum, used for the corresponding block header field.
 
 use crate::parameters::{Network, NetworkUpgrade, NetworkUpgrade::*};
 use crate::sapling::tree::Root;
@@ -7,11 +7,11 @@ use super::Height;
 
 /// Zcash blocks contain different kinds of root hashes, depending on the network upgrade.
 ///
-/// The `BlockHeader.root_bytes` field is interpreted differently,
+/// The `BlockHeader.commitment_bytes` field is interpreted differently,
 /// based on the current block height. The interpretation changes at or after
 /// network upgrades.
 #[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)]
-pub enum RootHash {
+pub enum Commitment {
     /// [Pre-Sapling] Reserved field.
     ///
     /// All zeroes.
@@ -43,10 +43,10 @@ pub enum RootHash {
     ChainHistoryRoot(ChainHistoryMmrRootHash),
 }
 
-impl RootHash {
-    /// Returns `bytes` as the RootHash variant for `network` and `height`.
-    pub(super) fn from_bytes(bytes: [u8; 32], network: Network, height: Height) -> RootHash {
-        use RootHash::*;
+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 {
+        use Commitment::*;
 
         match NetworkUpgrade::current(network, height) {
             Genesis | BeforeOverwinter | Overwinter => PreSaplingReserved(bytes),
@@ -59,10 +59,10 @@ impl RootHash {
         }
     }
 
-    /// Returns the serialized bytes for this RootHash.
+    /// Returns the serialized bytes for this Commitment.
     #[allow(dead_code)]
     pub(super) fn to_bytes(self) -> [u8; 32] {
-        use RootHash::*;
+        use Commitment::*;
 
         match self {
             PreSaplingReserved(b) => b,
diff --git a/zebra-chain/src/block/header.rs b/zebra-chain/src/block/header.rs
index a5b05728706..8ab3492127b 100644
--- a/zebra-chain/src/block/header.rs
+++ b/zebra-chain/src/block/header.rs
@@ -44,9 +44,9 @@ pub struct Header {
     ///
     /// Unfortunately, the interpretation of this field was changed without
     /// incrementing the version, so it cannot be parsed without the block height
-    /// and network. Use [`Block::root_hash`](super::Block::root_hash) to get the
-    /// parsed [`RootHash`](super::RootHash).
-    pub root_bytes: [u8; 32],
+    /// and network. Use [`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
     /// started hashing the header (according to the miner).
diff --git a/zebra-chain/src/block/serialize.rs b/zebra-chain/src/block/serialize.rs
index 2042f12f0ea..0bb1f6b4219 100644
--- a/zebra-chain/src/block/serialize.rs
+++ b/zebra-chain/src/block/serialize.rs
@@ -26,7 +26,7 @@ impl ZcashSerialize for Header {
         writer.write_u32::<LittleEndian>(self.version)?;
         self.previous_block_hash.zcash_serialize(&mut writer)?;
         writer.write_all(&self.merkle_root.0[..])?;
-        writer.write_all(&self.root_bytes[..])?;
+        writer.write_all(&self.commitment_bytes[..])?;
         // this is a truncating cast, rather than a saturating cast
         // but u32 times are valid until 2106, and our block verification time
         // checks should detect any truncation.
@@ -74,7 +74,7 @@ impl ZcashDeserialize for Header {
             version,
             previous_block_hash: Hash::zcash_deserialize(&mut reader)?,
             merkle_root: merkle::Root(reader.read_32_bytes()?),
-            root_bytes: reader.read_32_bytes()?,
+            commitment_bytes: reader.read_32_bytes()?,
             // This can't panic, because all u32 values are valid `Utc.timestamp`s
             time: Utc.timestamp(reader.read_u32::<LittleEndian>()? as i64, 0),
             difficulty_threshold: CompactDifficulty(reader.read_u32::<LittleEndian>()?),
diff --git a/zebra-chain/src/block/tests/prop.rs b/zebra-chain/src/block/tests/prop.rs
index 7cfee309e02..f2006186914 100644
--- a/zebra-chain/src/block/tests/prop.rs
+++ b/zebra-chain/src/block/tests/prop.rs
@@ -40,15 +40,15 @@ proptest! {
     }
 
     #[test]
-    fn root_hash_roundtrip(
+    fn commitment_roundtrip(
         bytes in any::<[u8; 32]>(),
         network in any::<Network>(),
         block_height in any::<Height>()
     ) {
         zebra_test::init();
 
-        let root_hash = RootHash::from_bytes(bytes, network, block_height);
-        let other_bytes = root_hash.to_bytes();
+        let commitment = Commitment::from_bytes(bytes, network, block_height);
+        let other_bytes = commitment.to_bytes();
 
         prop_assert_eq![bytes, other_bytes];
     }
@@ -70,10 +70,10 @@ proptest! {
         let bytes = &mut bytes.as_slice();
 
         // Check the root hash
-        let root_hash = block.root_hash(network);
-        if let Some(root_hash) = root_hash {
-            let root_hash_bytes = root_hash.to_bytes();
-            prop_assert_eq![block.header.root_bytes, root_hash_bytes];
+        let commitment = block.commitment(network);
+        if let Some(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];
         }

From 75e08167af47b1a6f054ed84a21d52a87b86d883 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 29 Mar 2021 15:32:07 +1000
Subject: [PATCH 2/7] rustfmt

---
 zebra-chain/src/block.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/zebra-chain/src/block.rs b/zebra-chain/src/block.rs
index 7e3acb3e045..0bbd4d14fe2 100644
--- a/zebra-chain/src/block.rs
+++ b/zebra-chain/src/block.rs
@@ -1,10 +1,10 @@
 //! Blocks and block-related structures (heights, headers, etc.)
 #![allow(clippy::unit_arg)]
 
+mod commitment;
 mod hash;
 mod header;
 mod height;
-mod commitment;
 mod serialize;
 
 pub mod merkle;
@@ -16,11 +16,11 @@ mod tests;
 
 use std::fmt;
 
+pub use commitment::Commitment;
 pub use hash::Hash;
 pub use header::BlockTimeError;
 pub use header::{CountedHeader, Header};
 pub use height::Height;
-pub use commitment::Commitment;
 
 use serde::{Deserialize, Serialize};
 

From a6cf3834e5d98e387a24eeec3fbc014699c7af84 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 29 Mar 2021 13:52:07 +1000
Subject: [PATCH 3/7] Comment and format cleanups after interactive replace

---
 zebra-chain/src/block.rs            |  4 ++--
 zebra-chain/src/block/commitment.rs | 18 +++++++++++++-----
 zebra-chain/src/block/header.rs     | 12 +++++++-----
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/zebra-chain/src/block.rs b/zebra-chain/src/block.rs
index 0bbd4d14fe2..9c614293e95 100644
--- a/zebra-chain/src/block.rs
+++ b/zebra-chain/src/block.rs
@@ -63,9 +63,9 @@ 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.
diff --git a/zebra-chain/src/block/commitment.rs b/zebra-chain/src/block/commitment.rs
index de250357275..930f556355a 100644
--- a/zebra-chain/src/block/commitment.rs
+++ b/zebra-chain/src/block/commitment.rs
@@ -5,11 +5,13 @@ use crate::sapling::tree::Root;
 
 use super::Height;
 
-/// 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,
-/// based on the current block height. The interpretation changes at or after
-/// network upgrades.
+/// The `Header.commitment_bytes` field is interpreted differently, based on the
+/// network and height. The interpretation changes in the network upgrade
+/// 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.
@@ -21,6 +23,9 @@ pub enum Commitment {
     ///
     /// The root LEBS2OSP256(rt) of the Sapling note commitment tree
     /// corresponding to the final Sapling treestate of this block.
+    ///
+    /// Subsequent `Commitment` variants also commit to the `FinalSaplingRoot`,
+    /// via their `EarliestSaplingRoot` and `LatestSaplingRoot` fields.
     FinalSaplingRoot(Root),
 
     /// [Heartwood activation block] Reserved field.
@@ -39,7 +44,10 @@ 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.)
     ChainHistoryRoot(ChainHistoryMmrRootHash),
 }
 
diff --git a/zebra-chain/src/block/header.rs b/zebra-chain/src/block/header.rs
index 8ab3492127b..4d903498d05 100644
--- a/zebra-chain/src/block/header.rs
+++ b/zebra-chain/src/block/header.rs
@@ -40,12 +40,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
-    /// incrementing the version, so it cannot be parsed without the block height
-    /// and network. Use [`Block::commitment`](super::Block::commitment) to get the
-    /// parsed [`Commitment`](super::Commitment).
+    /// The interpretation of this field has been changed multiple times, without
+    /// incrementing the block [`version`]. Therefore, this field cannot be
+    /// parsed without the network and height. Use
+    /// [`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

From 37f657bdd92b84e102390bb940424ed90f9f4665 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 29 Mar 2021 13:52:43 +1000
Subject: [PATCH 4/7] Distinguish Sapling tree roots from other tree roots

---
 zebra-chain/src/block/commitment.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/zebra-chain/src/block/commitment.rs b/zebra-chain/src/block/commitment.rs
index 930f556355a..d37654ec2f5 100644
--- a/zebra-chain/src/block/commitment.rs
+++ b/zebra-chain/src/block/commitment.rs
@@ -1,7 +1,7 @@
 //! The Commitment enum, used for the corresponding block header field.
 
 use crate::parameters::{Network, NetworkUpgrade, NetworkUpgrade::*};
-use crate::sapling::tree::Root;
+use crate::sapling;
 
 use super::Height;
 
@@ -26,7 +26,7 @@ pub enum Commitment {
     ///
     /// Subsequent `Commitment` variants also commit to the `FinalSaplingRoot`,
     /// via their `EarliestSaplingRoot` and `LatestSaplingRoot` fields.
-    FinalSaplingRoot(Root),
+    FinalSaplingRoot(sapling::tree::Root),
 
     /// [Heartwood activation block] Reserved field.
     ///
@@ -58,7 +58,7 @@ impl Commitment {
 
         match NetworkUpgrade::current(network, height) {
             Genesis | BeforeOverwinter | Overwinter => PreSaplingReserved(bytes),
-            Sapling | Blossom => FinalSaplingRoot(Root(bytes)),
+            Sapling | Blossom => FinalSaplingRoot(sapling::tree::Root(bytes)),
             Heartwood if Some(height) == Heartwood.activation_height(network) => {
                 ChainHistoryActivationReserved(bytes)
             }

From 1d8384255a3645230f24313cc6f2332f2c2ba134 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 29 Mar 2021 14:02:22 +1000
Subject: [PATCH 5/7] Add the NU5 BlockCommitmentsHash variant to
 block::Commitment

This change parses the hash, but does not perform validation.
---
 zebra-chain/src/block/commitment.rs | 44 ++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/zebra-chain/src/block/commitment.rs b/zebra-chain/src/block/commitment.rs
index d37654ec2f5..a1c08941aa9 100644
--- a/zebra-chain/src/block/commitment.rs
+++ b/zebra-chain/src/block/commitment.rs
@@ -34,8 +34,8 @@ pub enum Commitment {
     /// See ZIP-221 for details.
     ChainHistoryActivationReserved([u8; 32]),
 
-    /// [After Heartwood activation block] The root of a Merkle Mountain
-    /// Range chain history tree.
+    /// [(Heartwood activation block + 1) to Canopy] The root of a Merkle
+    /// 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
@@ -49,6 +49,23 @@ pub enum Commitment {
     /// chain history hash in their activation block, via the previous block
     /// hash field.)
     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: Do block commitments activate at NU5 activation, or (NU5 + 1)?
+    // https://github.com/zcash/zips/pull/474
+    BlockCommitments(BlockCommitmentsHash),
 }
 
 impl Commitment {
@@ -63,7 +80,7 @@ impl Commitment {
                 ChainHistoryActivationReserved(bytes)
             }
             Heartwood | Canopy => ChainHistoryRoot(ChainHistoryMmrRootHash(bytes)),
-            Nu5 => unimplemented!("Nu5 uses hashAuthDataRoot as specified in ZIP-244"),
+            Nu5 => BlockCommitments(BlockCommitmentsHash(bytes)),
         }
     }
 
@@ -74,9 +91,10 @@ impl Commitment {
 
         match self {
             PreSaplingReserved(b) => b,
-            FinalSaplingRoot(v) => v.0,
+            FinalSaplingRoot(hash) => hash.0,
             ChainHistoryActivationReserved(b) => b,
-            ChainHistoryRoot(v) => v.0,
+            ChainHistoryRoot(hash) => hash.0,
+            BlockCommitments(hash) => hash.0,
         }
     }
 }
@@ -84,7 +102,19 @@ 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.
-//    - move to a separate file.
+//      hash from the current set of peaks
+//    - 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]);

From 4ba9a4595e31822dbbe3affd8afc86263d3d90f7 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 29 Mar 2021 15:12:45 +1000
Subject: [PATCH 6/7] 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
---
 zebra-chain/src/block.rs            |  18 +++--
 zebra-chain/src/block/arbitrary.rs  |  11 ++-
 zebra-chain/src/block/commitment.rs | 103 ++++++++++++++++++++++++----
 zebra-chain/src/block/tests/prop.rs |  15 ++--
 4 files changed, 119 insertions(+), 28 deletions(-)

diff --git a/zebra-chain/src/block.rs b/zebra-chain/src/block.rs
index 9c614293e95..2762bbbbc61 100644
--- a/zebra-chain/src/block.rs
+++ b/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::{CountedHeader, Header};
+pub use header::{BlockTimeError, CountedHeader, Header};
 pub use height::Height;
 
 use serde::{Deserialize, Serialize};
@@ -68,10 +67,15 @@ impl Block {
     /// 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.
-    pub fn commitment(&self, network: Network) -> Option<Commitment> {
-        self.coinbase_height()
-            .map(|height| Commitment::from_bytes(self.header.commitment_bytes, network, height))
+    /// Returns an error if this block does not have a block height,
+    /// or if the commitment value is structurally invalid.
+    pub fn commitment(&self, network: Network) -> Result<Commitment, CommitmentError> {
+        match self.coinbase_height() {
+            None => Err(CommitmentError::MissingBlockHeight {
+                block_hash: self.hash(),
+            }),
+            Some(height) => Commitment::from_bytes(self.header.commitment_bytes, network, height),
+        }
     }
 }
 
diff --git a/zebra-chain/src/block/arbitrary.rs b/zebra-chain/src/block/arbitrary.rs
index f50d1f9347d..a9993a516ee 100644
--- a/zebra-chain/src/block/arbitrary.rs
+++ b/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()
     }
diff --git a/zebra-chain/src/block/commitment.rs b/zebra-chain/src/block/commitment.rs
index a1c08941aa9..ebf972c403c 100644
--- a/zebra-chain/src/block/commitment.rs
+++ b/zebra-chain/src/block/commitment.rs
@@ -1,9 +1,11 @@
 //! The Commitment enum, used for the corresponding block header field.
 
+use thiserror::Error;
+
 use crate::parameters::{Network, NetworkUpgrade, NetworkUpgrade::*};
 use crate::sapling;
 
-use super::Height;
+use super::super::block;
 
 /// Zcash blocks contain different kinds of commitments to their contents,
 /// depending on the network and height.
@@ -16,8 +18,10 @@ use super::Height;
 pub enum Commitment {
     /// [Pre-Sapling] Reserved field.
     ///
-    /// All zeroes.
-    PreSaplingReserved([u8; 32]),
+    /// The value of this field MUST be all zeroes.
+    ///
+    /// This field is verified in `Commitment::from_bytes`.
+    PreSaplingReserved,
 
     /// [Sapling and Blossom] The final Sapling treestate of this block.
     ///
@@ -26,13 +30,23 @@ pub enum Commitment {
     ///
     /// 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,
 
     /// [(Heartwood activation block + 1) to Canopy] The root of a Merkle
     /// Mountain Range chain history tree.
@@ -48,6 +62,8 @@ pub enum Commitment {
     /// 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:
@@ -62,25 +78,45 @@ pub enum Commitment {
     /// 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),
-            Sapling | Blossom => FinalSaplingRoot(sapling::tree::Root(bytes)),
+            Genesis | BeforeOverwinter | Overwinter => {
+                if bytes == RESERVED_BYTES {
+                    Ok(PreSaplingReserved)
+                } else {
+                    Err(InvalidPreSaplingReserved { actual: bytes })
+                }
+            }
+            Sapling | Blossom => Ok(FinalSaplingRoot(sapling::tree::Root(bytes))),
             Heartwood if Some(height) == Heartwood.activation_height(network) => {
-                ChainHistoryActivationReserved(bytes)
+                if bytes == RESERVED_BYTES {
+                    Ok(ChainHistoryActivationReserved)
+                } else {
+                    Err(InvalidChainHistoryActivationReserved { actual: bytes })
+                }
             }
-            Heartwood | Canopy => ChainHistoryRoot(ChainHistoryMmrRootHash(bytes)),
-            Nu5 => BlockCommitments(BlockCommitmentsHash(bytes)),
+            Heartwood | Canopy => Ok(ChainHistoryRoot(ChainHistoryMmrRootHash(bytes))),
+            Nu5 => Ok(BlockCommitments(BlockCommitmentsHash(bytes))),
         }
     }
 
@@ -90,9 +126,9 @@ impl Commitment {
         use Commitment::*;
 
         match self {
-            PreSaplingReserved(b) => b,
+            PreSaplingReserved => RESERVED_BYTES,
             FinalSaplingRoot(hash) => hash.0,
-            ChainHistoryActivationReserved(b) => b,
+            ChainHistoryActivationReserved => RESERVED_BYTES,
             ChainHistoryRoot(hash) => hash.0,
             BlockCommitments(hash) => hash.0,
         }
@@ -118,3 +154,44 @@ pub struct ChainHistoryMmrRootHash([u8; 32]);
 //    - 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 },
+}
diff --git a/zebra-chain/src/block/tests/prop.rs b/zebra-chain/src/block/tests/prop.rs
index f2006186914..3cf47b13093 100644
--- a/zebra-chain/src/block/tests/prop.rs
+++ b/zebra-chain/src/block/tests/prop.rs
@@ -47,10 +47,13 @@ proptest! {
     ) {
         zebra_test::init();
 
-        let commitment = Commitment::from_bytes(bytes, network, block_height);
-        let other_bytes = commitment.to_bytes();
+        // just skip the test if the bytes don't parse, because there's nothing
+        // 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

From 9e943398a18f168935b951d96bba6f62cd4633d9 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Tue, 6 Apr 2021 16:51:39 +1000
Subject: [PATCH 7/7] Fix comment whitespace

---
 zebra-chain/src/block/commitment.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/zebra-chain/src/block/commitment.rs b/zebra-chain/src/block/commitment.rs
index ebf972c403c..1251bc7cb0b 100644
--- a/zebra-chain/src/block/commitment.rs
+++ b/zebra-chain/src/block/commitment.rs
@@ -33,9 +33,9 @@ pub enum Commitment {
     ///
     /// 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
+    /// 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.