From 137ae4e0418b67fcbffadbb17c3a591d76fcd0ad Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Thu, 17 Feb 2022 00:20:22 -0300 Subject: [PATCH] refactor(anchorSapling): Change type to force consensus rule validation (#3544) * change `anchorSapling` type * implement PartialEq manually for clippy * use `unique_by` in place of `sorted` * replace panic with new error * improve some serialize/deserialize calls for sapling anchors * fix arbitrary for sapling::tree::Root * remove dedup() Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- zebra-chain/src/block/commitment.rs | 10 ++- zebra-chain/src/block/tests/vectors.rs | 2 +- zebra-chain/src/history_tree/tests/vectors.rs | 12 +-- .../primitives/zcash_history/tests/vectors.rs | 6 +- zebra-chain/src/sapling/arbitrary.rs | 2 +- zebra-chain/src/sapling/shielded_data.rs | 3 +- zebra-chain/src/sapling/spend.rs | 6 +- zebra-chain/src/sapling/tests/tree.rs | 6 +- zebra-chain/src/sapling/tree.rs | 76 ++++++++++++++----- zebra-chain/src/transaction/serialize.rs | 2 +- 10 files changed, 85 insertions(+), 40 deletions(-) diff --git a/zebra-chain/src/block/commitment.rs b/zebra-chain/src/block/commitment.rs index 41b03e37a..3db28fbb3 100644 --- a/zebra-chain/src/block/commitment.rs +++ b/zebra-chain/src/block/commitment.rs @@ -108,7 +108,10 @@ impl Commitment { match NetworkUpgrade::current(network, height) { Genesis | BeforeOverwinter | Overwinter => Ok(PreSaplingReserved(bytes)), - Sapling | Blossom => Ok(FinalSaplingRoot(sapling::tree::Root(bytes))), + Sapling | Blossom => match sapling::tree::Root::try_from(bytes) { + Ok(root) => Ok(FinalSaplingRoot(root)), + _ => Err(InvalidSapingRootBytes), + }, Heartwood if Some(height) == Heartwood.activation_height(network) => { if bytes == CHAIN_HISTORY_ACTIVATION_RESERVED { Ok(ChainHistoryActivationReserved) @@ -130,7 +133,7 @@ impl Commitment { match self { PreSaplingReserved(bytes) => bytes, - FinalSaplingRoot(hash) => hash.0, + FinalSaplingRoot(hash) => hash.0.into(), ChainHistoryActivationReserved => CHAIN_HISTORY_ACTIVATION_RESERVED, ChainHistoryRoot(hash) => hash.0, ChainHistoryBlockTxAuthCommitment(hash) => hash.0, @@ -247,4 +250,7 @@ pub enum CommitmentError { #[error("missing required block height: block commitments can't be parsed without a block height, block hash: {block_hash:?}")] MissingBlockHeight { block_hash: block::Hash }, + + #[error("provided bytes are not a valid sapling root")] + InvalidSapingRootBytes, } diff --git a/zebra-chain/src/block/tests/vectors.rs b/zebra-chain/src/block/tests/vectors.rs index c609b5e19..6ab780548 100644 --- a/zebra-chain/src/block/tests/vectors.rs +++ b/zebra-chain/src/block/tests/vectors.rs @@ -237,7 +237,7 @@ fn block_commitment(network: Network) { .expect("unexpected missing final sapling root test vector"); assert_eq!( final_sapling_root, - expected_final_sapling_root.into(), + crate::sapling::tree::Root::try_from(*expected_final_sapling_root).unwrap(), "unexpected invalid final sapling root commitment at {} {}", network, height diff --git a/zebra-chain/src/history_tree/tests/vectors.rs b/zebra-chain/src/history_tree/tests/vectors.rs index f2e7ca71d..ef9295204 100644 --- a/zebra-chain/src/history_tree/tests/vectors.rs +++ b/zebra-chain/src/history_tree/tests/vectors.rs @@ -62,7 +62,7 @@ fn push_and_prune_for_network_upgrade( // Build initial history tree tree with only the first block let first_sapling_root = - sapling::tree::Root(**sapling_roots.get(&height).expect("test vector exists")); + sapling::tree::Root::try_from(**sapling_roots.get(&height).expect("test vector exists"))?; let mut tree = NonEmptyHistoryTree::from_block( network, first_block, @@ -91,11 +91,11 @@ fn push_and_prune_for_network_upgrade( assert_eq!(second_commitment, Commitment::ChainHistoryRoot(first_root)); // Append second block to history tree - let second_sapling_root = sapling::tree::Root( + let second_sapling_root = sapling::tree::Root::try_from( **sapling_roots .get(&(height + 1)) .expect("test vector exists"), - ); + )?; tree.push(second_block, &second_sapling_root, &Default::default()) .unwrap(); @@ -139,7 +139,7 @@ fn upgrade_for_network_upgrade(network: Network, network_upgrade: NetworkUpgrade // This tree will not match the actual tree (which has all the blocks since the previous // network upgrade), so we won't be able to check if its root is correct. let sapling_root_prev = - sapling::tree::Root(**sapling_roots.get(&height).expect("test vector exists")); + sapling::tree::Root::try_from(**sapling_roots.get(&height).expect("test vector exists"))?; let mut tree = NonEmptyHistoryTree::from_block( network, block_prev, @@ -162,11 +162,11 @@ fn upgrade_for_network_upgrade(network: Network, network_upgrade: NetworkUpgrade // Append block to history tree. This must trigger a upgrade of the tree, // which should be recreated. - let activation_sapling_root = sapling::tree::Root( + let activation_sapling_root = sapling::tree::Root::try_from( **sapling_roots .get(&(height + 1)) .expect("test vector exists"), - ); + )?; tree.push( activation_block, &activation_sapling_root, diff --git a/zebra-chain/src/primitives/zcash_history/tests/vectors.rs b/zebra-chain/src/primitives/zcash_history/tests/vectors.rs index 80bb6181c..b2b5a0c1b 100644 --- a/zebra-chain/src/primitives/zcash_history/tests/vectors.rs +++ b/zebra-chain/src/primitives/zcash_history/tests/vectors.rs @@ -48,7 +48,7 @@ fn tree_for_network_upgrade(network: Network, network_upgrade: NetworkUpgrade) - // Build initial MMR tree with only Block 0 let sapling_root0 = - sapling::tree::Root(**sapling_roots.get(&height).expect("test vector exists")); + sapling::tree::Root::try_from(**sapling_roots.get(&height).expect("test vector exists"))?; let (mut tree, _) = Tree::::new_from_block(network, block0, &sapling_root0, &Default::default())?; @@ -69,11 +69,11 @@ fn tree_for_network_upgrade(network: Network, network_upgrade: NetworkUpgrade) - assert_eq!(commitment1, Commitment::ChainHistoryRoot(hash0)); // Append Block to MMR tree - let sapling_root1 = sapling::tree::Root( + let sapling_root1 = sapling::tree::Root::try_from( **sapling_roots .get(&(height + 1)) .expect("test vector exists"), - ); + )?; let append = tree .append_leaf(block1, &sapling_root1, &Default::default()) .unwrap(); diff --git a/zebra-chain/src/sapling/arbitrary.rs b/zebra-chain/src/sapling/arbitrary.rs index 923f49f4d..4840849ae 100644 --- a/zebra-chain/src/sapling/arbitrary.rs +++ b/zebra-chain/src/sapling/arbitrary.rs @@ -125,7 +125,7 @@ impl Arbitrary for tree::Root { (vec(any::(), 64)) .prop_map(|bytes| { let bytes = bytes.try_into().expect("vec is the correct length"); - jubjub::Fq::from_bytes_wide(&bytes).to_bytes().into() + tree::Root(jubjub::Base::from_bytes_wide(&bytes)) }) .boxed() } diff --git a/zebra-chain/src/sapling/shielded_data.rs b/zebra-chain/src/sapling/shielded_data.rs index cc1e0f8ae..ff337da31 100644 --- a/zebra-chain/src/sapling/shielded_data.rs +++ b/zebra-chain/src/sapling/shielded_data.rs @@ -199,8 +199,7 @@ where // TODO: use TransferData::shared_anchor to improve performance for V5 transactions self.spends_per_anchor() .map(|spend| spend.per_spend_anchor) - .sorted() - .dedup() + .unique_by(|raw| raw.0.to_bytes()) } /// Iterate over the [`Spend`]s for this transaction, returning diff --git a/zebra-chain/src/sapling/spend.rs b/zebra-chain/src/sapling/spend.rs index 0e0e1f182..8b532edeb 100644 --- a/zebra-chain/src/sapling/spend.rs +++ b/zebra-chain/src/sapling/spend.rs @@ -13,7 +13,7 @@ use crate::{ }, serialization::{ ReadZcashExt, SerializationError, TrustedPreallocate, WriteZcashExt, ZcashDeserialize, - ZcashSerialize, + ZcashDeserializeInto, ZcashSerialize, }, }; @@ -161,7 +161,7 @@ impl Spend { impl ZcashSerialize for Spend { fn zcash_serialize(&self, mut writer: W) -> Result<(), io::Error> { self.cv.zcash_serialize(&mut writer)?; - writer.write_all(&self.per_spend_anchor.0[..])?; + self.per_spend_anchor.zcash_serialize(&mut writer)?; writer.write_32_bytes(&self.nullifier.into())?; writer.write_all(&<[u8; 32]>::from(self.rk.clone())[..])?; self.zkproof.zcash_serialize(&mut writer)?; @@ -199,7 +199,7 @@ impl ZcashDeserialize for Spend { // See [`commitment::NotSmallOrderValueCommitment::zcash_deserialize`]. cv: commitment::NotSmallOrderValueCommitment::zcash_deserialize(&mut reader)?, // Type is `B^{[ℓ_{Sapling}_{Merkle}]}`, i.e. 32 bytes - per_spend_anchor: tree::Root(reader.read_32_bytes()?), + per_spend_anchor: (&mut reader).zcash_deserialize_into()?, // Type is `B^Y^{[ℓ_{PRFnfSapling}/8]}`, i.e. 32 bytes nullifier: note::Nullifier::from(reader.read_32_bytes()?), // Type is `SpendAuthSig^{Sapling}.Public`, i.e. J diff --git a/zebra-chain/src/sapling/tests/tree.rs b/zebra-chain/src/sapling/tests/tree.rs index afa5f86fb..599c2fe05 100644 --- a/zebra-chain/src/sapling/tests/tree.rs +++ b/zebra-chain/src/sapling/tests/tree.rs @@ -91,7 +91,7 @@ fn incremental_roots_with_blocks_for_network(network: Network) -> Result<()> { // Check if root of the tree of the activation block is correct let sapling_activation_block_root = - sapling::tree::Root(**sapling_roots.get(&height).expect("test vector exists")); + sapling::tree::Root::try_from(**sapling_roots.get(&height).expect("test vector exists"))?; assert_eq!(sapling_activation_block_root, tree.root()); // Load the block immediately after Sapling activation (activation + 1) @@ -102,11 +102,11 @@ fn incremental_roots_with_blocks_for_network(network: Network) -> Result<()> { .zcash_deserialize_into::() .expect("block is structurally valid"), ); - let block_after_sapling_activation_root = sapling::tree::Root( + let block_after_sapling_activation_root = sapling::tree::Root::try_from( **sapling_roots .get(&(height + 1)) .expect("test vector exists"), - ); + )?; // Add note commitments from the block after Sapling activatoin to the tree let mut appended_count = 0; diff --git a/zebra-chain/src/sapling/tree.rs b/zebra-chain/src/sapling/tree.rs index 15af1ba76..fb9971874 100644 --- a/zebra-chain/src/sapling/tree.rs +++ b/zebra-chain/src/sapling/tree.rs @@ -13,7 +13,12 @@ #![allow(clippy::unit_arg)] #![allow(dead_code)] -use std::{cell::Cell, fmt}; +use std::{ + cell::Cell, + fmt, + hash::{Hash, Hasher}, + io, +}; use bitvec::prelude::*; use incrementalmerkletree::{bridgetree, Frontier}; @@ -22,6 +27,9 @@ use thiserror::Error; use super::commitment::pedersen_hashes::pedersen_hash; +use crate::serialization::{ + serde_helpers, ReadZcashExt, SerializationError, ZcashDeserialize, ZcashSerialize, +}; pub(super) const MERKLE_DEPTH: usize = 32; /// MerkleCRH^Sapling Hash Function @@ -81,30 +89,20 @@ pub struct Position(pub(crate) u64); /// commitment tree corresponding to the final Sapling treestate of /// this block. A root of a note commitment tree is associated with /// each treestate. -#[derive(Clone, Copy, Default, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)] -pub struct Root(pub [u8; 32]); +#[derive(Clone, Copy, Default, Eq, Serialize, Deserialize)] +pub struct Root(#[serde(with = "serde_helpers::Fq")] pub(crate) jubjub::Base); impl fmt::Debug for Root { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.debug_tuple("Root").field(&hex::encode(&self.0)).finish() - } -} - -impl From<[u8; 32]> for Root { - fn from(bytes: [u8; 32]) -> Root { - Self(bytes) + f.debug_tuple("Root") + .field(&hex::encode(&self.0.to_bytes())) + .finish() } } impl From for [u8; 32] { fn from(root: Root) -> Self { - root.0 - } -} - -impl From<&[u8; 32]> for Root { - fn from(bytes: &[u8; 32]) -> Root { - (*bytes).into() + root.0.to_bytes() } } @@ -114,6 +112,48 @@ impl From<&Root> for [u8; 32] { } } +impl PartialEq for Root { + fn eq(&self, other: &Self) -> bool { + self.0 == other.0 + } +} + +impl Hash for Root { + fn hash(&self, state: &mut H) { + self.0.to_bytes().hash(state) + } +} + +impl TryFrom<[u8; 32]> for Root { + type Error = SerializationError; + + fn try_from(bytes: [u8; 32]) -> Result { + let possible_point = jubjub::Base::from_bytes(&bytes); + + if possible_point.is_some().into() { + Ok(Self(possible_point.unwrap())) + } else { + Err(SerializationError::Parse( + "Invalid jubjub::Base value for Sapling note commitment tree root", + )) + } + } +} + +impl ZcashSerialize for Root { + fn zcash_serialize(&self, mut writer: W) -> Result<(), io::Error> { + writer.write_all(&<[u8; 32]>::from(*self)[..])?; + + Ok(()) + } +} + +impl ZcashDeserialize for Root { + fn zcash_deserialize(mut reader: R) -> Result { + Self::try_from(reader.read_32_bytes()?) + } +} + /// A node of the Sapling Incremental Note Commitment Tree. /// /// Note that it's handled as a byte buffer and not a point coordinate (jubjub::Fq) @@ -241,7 +281,7 @@ impl NoteCommitmentTree { Some(root) => root, None => { // Compute root and cache it - let root = Root(self.inner.root().0); + let root = Root::try_from(self.inner.root().0).unwrap(); self.cached_root.replace(Some(root)); root } diff --git a/zebra-chain/src/transaction/serialize.rs b/zebra-chain/src/transaction/serialize.rs index ac7def0cd..55ca11350 100644 --- a/zebra-chain/src/transaction/serialize.rs +++ b/zebra-chain/src/transaction/serialize.rs @@ -215,7 +215,7 @@ impl ZcashDeserialize for Option> { // // Type is `B^{[ℓ_{Sapling}_{Merkle}]}`, i.e. 32 bytes let shared_anchor = if spends_count > 0 { - Some(reader.read_32_bytes()?.into()) + Some((&mut reader).zcash_deserialize_into()?) } else { None };