From eac201c6189e03060e049857567c85e285e06a9f Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Tue, 15 Feb 2022 18:51:30 -0300 Subject: [PATCH 1/7] change `anchorSapling` type --- zebra-chain/src/block/commitment.rs | 6 +- 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 | 5 +- zebra-chain/src/sapling/shielded_data.rs | 1 - zebra-chain/src/sapling/spend.rs | 4 +- zebra-chain/src/sapling/tests/tree.rs | 6 +- zebra-chain/src/sapling/tree.rs | 68 ++++++++++++++----- zebra-chain/src/transaction/serialize.rs | 2 +- 10 files changed, 75 insertions(+), 37 deletions(-) diff --git a/zebra-chain/src/block/commitment.rs b/zebra-chain/src/block/commitment.rs index 41b03e37a49..41cf7628cd5 100644 --- a/zebra-chain/src/block/commitment.rs +++ b/zebra-chain/src/block/commitment.rs @@ -108,7 +108,9 @@ impl Commitment { match NetworkUpgrade::current(network, height) { Genesis | BeforeOverwinter | Overwinter => Ok(PreSaplingReserved(bytes)), - Sapling | Blossom => Ok(FinalSaplingRoot(sapling::tree::Root(bytes))), + Sapling | Blossom => Ok(FinalSaplingRoot( + sapling::tree::Root::try_from(bytes).expect("we should have valid bytes"), + )), Heartwood if Some(height) == Heartwood.activation_height(network) => { if bytes == CHAIN_HISTORY_ACTIVATION_RESERVED { Ok(ChainHistoryActivationReserved) @@ -130,7 +132,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, diff --git a/zebra-chain/src/block/tests/vectors.rs b/zebra-chain/src/block/tests/vectors.rs index c609b5e1989..6ab78054890 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 f2e7ca71df5..ef9295204cb 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 80bb6181cbe..b2b5a0c1bbf 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 923f49f4deb..a7d336c42b7 100644 --- a/zebra-chain/src/sapling/arbitrary.rs +++ b/zebra-chain/src/sapling/arbitrary.rs @@ -125,7 +125,10 @@ 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() + jubjub::Fq::from_bytes_wide(&bytes) + .to_bytes() + .try_into() + .expect("bytes are always valid") }) .boxed() } diff --git a/zebra-chain/src/sapling/shielded_data.rs b/zebra-chain/src/sapling/shielded_data.rs index cc1e0f8ae6b..206099db18b 100644 --- a/zebra-chain/src/sapling/shielded_data.rs +++ b/zebra-chain/src/sapling/shielded_data.rs @@ -199,7 +199,6 @@ where // TODO: use TransferData::shared_anchor to improve performance for V5 transactions self.spends_per_anchor() .map(|spend| spend.per_spend_anchor) - .sorted() .dedup() } diff --git a/zebra-chain/src/sapling/spend.rs b/zebra-chain/src/sapling/spend.rs index 0e0e1f1820c..b22b97cad9e 100644 --- a/zebra-chain/src/sapling/spend.rs +++ b/zebra-chain/src/sapling/spend.rs @@ -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[..])?; + writer.write_all(&self.per_spend_anchor.0.to_bytes())?; 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: tree::Root::try_from(reader.read_32_bytes()?)?, // 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 afa5f86fb3c..599c2fe0581 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 15af1ba76fe..a30b0f04fc4 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,36 +89,62 @@ 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, PartialEq, 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() + f.debug_tuple("Root") + .field(&hex::encode(&self.0.to_bytes())) + .finish() } } -impl From<[u8; 32]> for Root { - fn from(bytes: [u8; 32]) -> Root { - Self(bytes) +impl From for [u8; 32] { + fn from(root: Root) -> Self { + root.0.to_bytes() } } -impl From for [u8; 32] { - fn from(root: Root) -> Self { - root.0 +impl From<&Root> for [u8; 32] { + fn from(root: &Root) -> Self { + (*root).into() } } -impl From<&[u8; 32]> for Root { - fn from(bytes: &[u8; 32]) -> Root { - (*bytes).into() +impl Hash for Root { + fn hash(&self, state: &mut H) { + self.0.to_bytes().hash(state) } } -impl From<&Root> for [u8; 32] { - fn from(root: &Root) -> Self { - (*root).into() +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()?) } } @@ -241,7 +275,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 ac7def0cde2..3c65ef6228f 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(reader.read_32_bytes()?.try_into()?) } else { None }; From 6f767dd17c71ea8b4b59ccc1279bfe8d684b00a8 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Tue, 15 Feb 2022 20:01:59 -0300 Subject: [PATCH 2/7] implement PartialEq manually for clippy --- zebra-chain/src/sapling/tree.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/zebra-chain/src/sapling/tree.rs b/zebra-chain/src/sapling/tree.rs index a30b0f04fc4..fb997187495 100644 --- a/zebra-chain/src/sapling/tree.rs +++ b/zebra-chain/src/sapling/tree.rs @@ -89,7 +89,7 @@ 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, Eq, PartialEq, Serialize, Deserialize)] +#[derive(Clone, Copy, Default, Eq, Serialize, Deserialize)] pub struct Root(#[serde(with = "serde_helpers::Fq")] pub(crate) jubjub::Base); impl fmt::Debug for Root { @@ -112,6 +112,12 @@ 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) From 4a8509244fbdaadcbb2d7820b9ab17a1c95f0131 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 16 Feb 2022 13:11:03 -0300 Subject: [PATCH 3/7] use `unique_by` in place of `sorted` --- zebra-chain/src/sapling/shielded_data.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zebra-chain/src/sapling/shielded_data.rs b/zebra-chain/src/sapling/shielded_data.rs index 206099db18b..1d5491b74f5 100644 --- a/zebra-chain/src/sapling/shielded_data.rs +++ b/zebra-chain/src/sapling/shielded_data.rs @@ -199,6 +199,7 @@ where // TODO: use TransferData::shared_anchor to improve performance for V5 transactions self.spends_per_anchor() .map(|spend| spend.per_spend_anchor) + .unique_by(|raw| raw.0.to_bytes()) .dedup() } From 07b48b3f4761809b598a38b085818bd576cc597d Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 16 Feb 2022 14:40:05 -0300 Subject: [PATCH 4/7] replace panic with new error --- zebra-chain/src/block/commitment.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/zebra-chain/src/block/commitment.rs b/zebra-chain/src/block/commitment.rs index 41cf7628cd5..3db28fbb3c8 100644 --- a/zebra-chain/src/block/commitment.rs +++ b/zebra-chain/src/block/commitment.rs @@ -108,9 +108,10 @@ impl Commitment { match NetworkUpgrade::current(network, height) { Genesis | BeforeOverwinter | Overwinter => Ok(PreSaplingReserved(bytes)), - Sapling | Blossom => Ok(FinalSaplingRoot( - sapling::tree::Root::try_from(bytes).expect("we should have valid 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) @@ -249,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, } From 6a8b4b8a18f4206da6e712cf6fc31d8ee5a55c78 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 16 Feb 2022 14:57:40 -0300 Subject: [PATCH 5/7] improve some serialize/deserialize calls for sapling anchors --- zebra-chain/src/sapling/spend.rs | 6 +++--- zebra-chain/src/transaction/serialize.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/zebra-chain/src/sapling/spend.rs b/zebra-chain/src/sapling/spend.rs index b22b97cad9e..8b532edeb5a 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.to_bytes())?; + 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::try_from(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/transaction/serialize.rs b/zebra-chain/src/transaction/serialize.rs index 3c65ef6228f..55ca113507a 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()?.try_into()?) + Some((&mut reader).zcash_deserialize_into()?) } else { None }; From ac4a75e7dfd6c1d008b8fe2eb80f8fabd4c54e9d Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 16 Feb 2022 15:09:06 -0300 Subject: [PATCH 6/7] fix arbitrary for sapling::tree::Root --- zebra-chain/src/sapling/arbitrary.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/zebra-chain/src/sapling/arbitrary.rs b/zebra-chain/src/sapling/arbitrary.rs index a7d336c42b7..4840849ae61 100644 --- a/zebra-chain/src/sapling/arbitrary.rs +++ b/zebra-chain/src/sapling/arbitrary.rs @@ -125,10 +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() - .try_into() - .expect("bytes are always valid") + tree::Root(jubjub::Base::from_bytes_wide(&bytes)) }) .boxed() } From 55504246e6cd26a6610c3caa4823527635684655 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 16 Feb 2022 20:37:51 -0300 Subject: [PATCH 7/7] remove dedup() --- zebra-chain/src/sapling/shielded_data.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/zebra-chain/src/sapling/shielded_data.rs b/zebra-chain/src/sapling/shielded_data.rs index 1d5491b74f5..ff337da3178 100644 --- a/zebra-chain/src/sapling/shielded_data.rs +++ b/zebra-chain/src/sapling/shielded_data.rs @@ -200,7 +200,6 @@ where self.spends_per_anchor() .map(|spend| spend.per_spend_anchor) .unique_by(|raw| raw.0.to_bytes()) - .dedup() } /// Iterate over the [`Spend`]s for this transaction, returning