From a6cf3834e5d98e387a24eeec3fbc014699c7af84 Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 29 Mar 2021 13:52:07 +1000 Subject: [PATCH 1/4] 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 Date: Mon, 29 Mar 2021 13:52:43 +1000 Subject: [PATCH 2/4] 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 Date: Mon, 29 Mar 2021 14:02:22 +1000 Subject: [PATCH 3/4] 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 Date: Mon, 29 Mar 2021 15:12:45 +1000 Subject: [PATCH 4/4] 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 { - 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 { + 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::(), any::()) .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 { 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