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

@@ -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,
 }

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

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,

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::<V1>::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();

zebra-chain/src/sapling/arbitrary.rs

@@ -125,7 +125,7 @@ impl Arbitrary for tree::Root {
         (vec(any::<u8>(), 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()
     }

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()
+            .unique_by(|raw| raw.0.to_bytes())
-            .dedup()
     }
 
     /// Iterate over the [`Spend`]s for this transaction, returning

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<SharedAnchor> {
 impl ZcashSerialize for Spend<PerSpendAnchor> {
     fn zcash_serialize<W: io::Write>(&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<PerSpendAnchor> {
             // 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

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::<Block>()
             .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;

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)]
+#[derive(Clone, Copy, Default, Eq, Serialize, Deserialize)]
-pub struct Root(pub [u8; 32]);
+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<Root> for [u8; 32] {
     fn from(root: Root) -> Self {
-        root.0
+        root.0.to_bytes()
-    }
-}
-
-impl From<&[u8; 32]> for Root {
-    fn from(bytes: &[u8; 32]) -> Root {
-        (*bytes).into()
     }
 }
 
@@ -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<H: Hasher>(&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<Self, Self::Error> {
+        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<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
+        writer.write_all(&<[u8; 32]>::from(*self)[..])?;
+
+        Ok(())
+    }
+}
+
+impl ZcashDeserialize for Root {
+    fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
+        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
             }

zebra-chain/src/transaction/serialize.rs

@@ -215,7 +215,7 @@ impl ZcashDeserialize for Option<sapling::ShieldedData<SharedAnchor>> {
         //
         // 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
         };