Improve type safety of note commitment tree node construction.

zcash_client_backend/CHANGELOG.md

@@ -177,7 +177,8 @@ and this library adheres to Rust's notion of
   - Setters (use direct field access instead).
 - The hardcoded `data_api::wallet::ANCHOR_OFFSET` constant.
 - `zcash_client_backend::wallet::AccountId` (moved to `zcash_primitives::zip32::AccountId`).
-
+- The implementation of `welding_rig::ScanningKey` for `ExtendedFullViewingKey`
+  has been removed. Use `DiversifiableFullViewingKey` instead.
 
 ## [0.5.0] - 2021-03-26
 ### Added

zcash_client_backend/src/data_api/wallet.rs

@@ -2,7 +2,7 @@ use std::fmt::Debug;
 use zcash_primitives::{
     consensus::{self, NetworkUpgrade},
     memo::MemoBytes,
-    sapling::{prover::TxProver, Node},
+    sapling::prover::TxProver,
     transaction::{
         builder::Builder,
         components::amount::{Amount, BalanceError, DEFAULT_FEE},
@@ -322,10 +322,9 @@ where
 
             let expected_root = selected.witness.root();
             external_note
-                .filter(|n| expected_root == merkle_path.root(Node::from_scalar(n.cmu())))
+                .filter(|n| expected_root == merkle_path.root(n.commitment()))
                 .or_else(|| {
-                    internal_note
-                        .filter(|n| expected_root == merkle_path.root(Node::from_scalar(n.cmu())))
+                    internal_note.filter(|n| expected_root == merkle_path.root(n.commitment()))
                 })
                 .ok_or_else(|| E::from(Error::NoteMismatch))
         }?;

zcash_client_backend/src/encoding.rs

@@ -217,7 +217,7 @@ pub fn decode_extended_spending_key(
 /// use zcash_primitives::zip32::ExtendedFullViewingKey;
 ///
 /// let extsk = sapling::spending_key(&[0; 32][..], COIN_TYPE, AccountId::from(0));
-/// let extfvk = ExtendedFullViewingKey::from(&extsk);
+/// let extfvk = extsk.to_extended_full_viewing_key();
 /// let encoded = encode_extended_full_viewing_key(HRP_SAPLING_EXTENDED_FULL_VIEWING_KEY, &extfvk);
 /// ```
 /// [`ExtendedFullViewingKey`]: zcash_primitives::zip32::ExtendedFullViewingKey

zcash_client_backend/src/welding_rig.rs

@@ -3,7 +3,6 @@
 use std::collections::{HashMap, HashSet};
 use std::convert::TryFrom;
 
-use group::ff::PrimeField;
 use subtle::{ConditionallySelectable, ConstantTimeEq, CtOption};
 use zcash_note_encryption::batch;
 use zcash_primitives::{
@@ -333,7 +332,7 @@ pub(crate) fn scan_block_with_runner<
                     .collect();
 
                 // Increment tree and witnesses
-                let node = Node::new(output.cmu.to_repr());
+                let node = Node::from_scalar(output.cmu);
                 for witness in &mut *existing_witnesses {
                     witness.append(node).unwrap();
                 }

zcash_extensions/src/transparent/demo.rs

@@ -477,7 +477,7 @@ impl<'a, B: ExtensionTxBuilder<'a>> DemoBuilder<B> {
 #[cfg(test)]
 mod tests {
     use blake2b_simd::Params;
-    use ff::{Field, PrimeField};
+    use ff::Field;
     use rand_core::OsRng;
 
     use zcash_proofs::prover::LocalTxProver;
@@ -488,7 +488,7 @@ mod tests {
         extensions::transparent::{self as tze, Extension, FromPayload, ToPayload},
         legacy::TransparentAddress,
         merkle_tree::{CommitmentTree, IncrementalWitness},
-        sapling::{Node, Rseed},
+        sapling::Rseed,
         transaction::{
             builder::Builder,
             components::{
@@ -817,7 +817,7 @@ mod tests {
         let note1 = to
             .create_note(101000, Rseed::BeforeZip212(jubjub::Fr::random(&mut rng)))
             .unwrap();
-        let cm1 = Node::new(note1.cmu().to_repr());
+        let cm1 = note1.commitment();
         let mut tree = CommitmentTree::empty();
         // fake that the note appears in some previous
         // shielded output

zcash_primitives/CHANGELOG.md

@@ -56,6 +56,8 @@ and this library adheres to Rust's notion of
    has been removed; change outputs must now be added by the caller.
 - The `From<&ExtendedSpendingKey>` instance for `ExtendedFullViewingKey` has been
   removed. Use `ExtendedSpendingKey::to_diversifiable_full_viewing_key` instead.
+- `zcash_primitives::sapling::Node::new` has been removed. Use
+  `Node::from_scalar` or (preferably) `Note::commitment` instead.
 
 ## [0.8.1] - 2022-10-19
 ### Added

zcash_primitives/src/merkle_tree.rs

@@ -323,13 +323,13 @@ impl<Node: Hashable> CommitmentTree<Node> {
 ///
 /// let mut tree = CommitmentTree::<Node>::empty();
 ///
-/// tree.append(Node::new(bls12_381::Scalar::random(&mut rng).to_repr()));
-/// tree.append(Node::new(bls12_381::Scalar::random(&mut rng).to_repr()));
+/// tree.append(Node::from_scalar(bls12_381::Scalar::random(&mut rng)));
+/// tree.append(Node::from_scalar(bls12_381::Scalar::random(&mut rng)));
 /// let mut witness = IncrementalWitness::from_tree(&tree);
 /// assert_eq!(witness.position(), 1);
 /// assert_eq!(tree.root(), witness.root());
 ///
-/// let cmu = Node::new(bls12_381::Scalar::random(&mut rng).to_repr());
+/// let cmu = Node::from_scalar(bls12_381::Scalar::random(&mut rng));
 /// tree.append(cmu);
 /// witness.append(cmu);
 /// assert_eq!(tree.root(), witness.root());

zcash_primitives/src/sapling.rs

@@ -77,10 +77,12 @@ pub struct Node {
 }
 
 impl Node {
-    pub fn new(repr: [u8; 32]) -> Self {
+    #[cfg(test)]
+    pub(crate) fn new(repr: [u8; 32]) -> Self {
         Node { repr }
     }
 
+    /// Constructs a new note commitment tree node from a [`bls12_381::Scalar`]
     pub fn from_scalar(cmu: bls12_381::Scalar) -> Self {
         Self {
             repr: cmu.to_repr(),
@@ -110,7 +112,7 @@ impl HashSer for Node {
     fn read<R: Read>(mut reader: R) -> io::Result<Self> {
         let mut repr = [0u8; 32];
         reader.read_exact(&mut repr)?;
-        Ok(Node::new(repr))
+        Ok(Node { repr })
     }
 
     fn write<W: Write>(&self, mut writer: W) -> io::Result<()> {
@@ -523,6 +525,12 @@ impl Note {
             )),
         }
     }
+
+    pub fn commitment(&self) -> Node {
+        Node {
+            repr: self.cmu().to_repr(),
+        }
+    }
 }
 
 #[cfg(any(test, feature = "test-dependencies"))]
@@ -569,7 +577,9 @@ pub mod testing {
 
     prop_compose! {
         pub fn arb_node()(value in prop::array::uniform32(prop::num::u8::ANY)) -> Node {
-            Node::new(value)
+            Node {
+                repr: value
+            }
         }
     }
 

zcash_primitives/src/transaction/builder.rs

@@ -544,7 +544,7 @@ mod testing {
 
 #[cfg(test)]
 mod tests {
-    use ff::{Field, PrimeField};
+    use ff::Field;
     use rand_core::OsRng;
 
     use crate::{
@@ -552,7 +552,7 @@ mod tests {
         legacy::TransparentAddress,
         memo::MemoBytes,
         merkle_tree::{CommitmentTree, IncrementalWitness},
-        sapling::{Node, Rseed},
+        sapling::Rseed,
         transaction::components::{
             amount::{Amount, DEFAULT_FEE},
             sapling::builder::{self as build_s},
@@ -673,7 +673,7 @@ mod tests {
         let note1 = to
             .create_note(50000, Rseed::BeforeZip212(jubjub::Fr::random(&mut rng)))
             .unwrap();
-        let cmu1 = Node::new(note1.cmu().to_repr());
+        let cmu1 = note1.commitment();
         let mut tree = CommitmentTree::empty();
         tree.append(cmu1).unwrap();
         let witness1 = IncrementalWitness::from_tree(&tree);
@@ -783,7 +783,7 @@ mod tests {
         let note1 = to
             .create_note(50999, Rseed::BeforeZip212(jubjub::Fr::random(&mut rng)))
             .unwrap();
-        let cmu1 = Node::new(note1.cmu().to_repr());
+        let cmu1 = note1.commitment();
         let mut tree = CommitmentTree::empty();
         tree.append(cmu1).unwrap();
         let mut witness1 = IncrementalWitness::from_tree(&tree);
@@ -823,7 +823,7 @@ mod tests {
         let note2 = to
             .create_note(1, Rseed::BeforeZip212(jubjub::Fr::random(&mut rng)))
             .unwrap();
-        let cmu2 = Node::new(note2.cmu().to_repr());
+        let cmu2 = note2.commitment();
         tree.append(cmu2).unwrap();
         witness1.append(cmu2).unwrap();
         let witness2 = IncrementalWitness::from_tree(&tree);

zcash_primitives/src/transaction/components/sapling/builder.rs

@@ -272,14 +272,14 @@ impl<P: consensus::Parameters> SaplingBuilder<P> {
         merkle_path: MerklePath<Node>,
     ) -> Result<(), Error> {
         // Consistency check: all anchors must equal the first one
-        let cmu = Node::new(note.cmu().into());
+        let node = note.commitment();
         if let Some(anchor) = self.anchor {
-            let path_root: bls12_381::Scalar = merkle_path.root(cmu).into();
+            let path_root: bls12_381::Scalar = merkle_path.root(node).into();
             if path_root != anchor {
                 return Err(Error::AnchorMismatch);
             }
         } else {
-            self.anchor = Some(merkle_path.root(cmu).into())
+            self.anchor = Some(merkle_path.root(node).into())
         }
 
         let alpha = jubjub::Fr::random(&mut rng);