Factor serialization out from merkle tree data structures.

zcash_client_sqlite/src/prepared.rs

@@ -12,6 +12,7 @@ use zcash_primitives::{
     block::BlockHash,
     consensus::{self, BlockHeight},
     memo::MemoBytes,
+    merkle_tree::{write_commitment_tree, write_incremental_witness},
     sapling::{self, Diversifier, Nullifier},
     transaction::{components::Amount, TxId},
     zip32::{AccountId, DiversifierIndex},
@@ -279,7 +280,7 @@ impl<'a, P> DataConnStmtCache<'a, P> {
         commitment_tree: &sapling::CommitmentTree,
     ) -> Result<(), SqliteClientError> {
         let mut encoded_tree = Vec::new();
-        commitment_tree.write(&mut encoded_tree).unwrap();
+        write_commitment_tree(commitment_tree, &mut encoded_tree).unwrap();
 
         self.stmt_insert_block.execute(params![
             u32::from(block_height),
@@ -784,7 +785,7 @@ impl<'a, P> DataConnStmtCache<'a, P> {
         }?;
 
         let mut encoded = Vec::new();
-        witness.write(&mut encoded).unwrap();
+        write_incremental_witness(witness, &mut encoded).unwrap();
 
         self.stmt_insert_witness
             .execute(params![note_id, u32::from(height), encoded])?;

zcash_client_sqlite/src/wallet.rs

@@ -73,6 +73,7 @@ use zcash_primitives::{
     block::BlockHash,
     consensus::{self, BlockHeight, BranchId, NetworkUpgrade, Parameters},
     memo::{Memo, MemoBytes},
+    merkle_tree::{read_commitment_tree, read_incremental_witness},
     sapling::{self, Note, Nullifier},
     transaction::{components::Amount, Transaction, TxId},
     zip32::{
@@ -693,7 +694,7 @@ pub(crate) fn get_sapling_commitment_tree<P>(
             [u32::from(block_height)],
             |row| {
                 let row_data: Vec<u8> = row.get(0)?;
-                sapling::CommitmentTree::read(&row_data[..]).map_err(|e| {
+                read_commitment_tree(&row_data[..]).map_err(|e| {
                     rusqlite::Error::FromSqlConversionFailure(
                         row_data.len(),
                         rusqlite::types::Type::Blob,
@@ -719,7 +720,7 @@ pub(crate) fn get_sapling_witnesses<P>(
         .query_map([u32::from(block_height)], |row| {
             let id_note = NoteId::ReceivedNoteId(row.get(0)?);
             let wdb: Vec<u8> = row.get(1)?;
-            Ok(sapling::IncrementalWitness::read(&wdb[..]).map(|witness| (id_note, witness)))
+            Ok(read_incremental_witness(&wdb[..]).map(|witness| (id_note, witness)))
         })
         .map_err(SqliteClientError::from)?;
 

zcash_client_sqlite/src/wallet/transact.rs

@@ -7,7 +7,7 @@ use group::ff::PrimeField;
 
 use zcash_primitives::{
     consensus::BlockHeight,
-    merkle_tree::IncrementalWitness,
+    merkle_tree::read_incremental_witness,
     sapling::{Diversifier, Rseed},
     transaction::components::Amount,
     zip32::AccountId,
@@ -50,7 +50,7 @@ fn to_spendable_note(row: &Row) -> Result<SpendableNote<NoteId>, SqliteClientErr
 
     let witness = {
         let d: Vec<_> = row.get(4)?;
-        IncrementalWitness::read(&d[..])?
+        read_incremental_witness(&d[..])?
     };
 
     Ok(SpendableNote {

zcash_primitives/CHANGELOG.md

@@ -14,11 +14,17 @@ and this library adheres to Rust's notion of
   type has been removed.
 - `sapling::SAPLING_COMMITMENT_TREE_DEPTH_U8` and `sapling::SAPLING_COMMITMENT_TREE_DEPTH` 
   have been removed; use `sapling::NOTE_COMMITMENT_TREE_DEPTH` instead.
-- `merkle_tree::incremental::write_auth_fragment_v1`
+- `merkle_tree:` removals:
+  - `incremental::write_auth_fragment_v1`
+  - `CommitmentTree::write` and `CommitmentTree::read` have been replaced by
+    `write_commitment_tree` and `read_commitment_tree` respectively.
+  - `IncrementalWitness::write` and `IncrementalWitness::read` have been
+    replaced by `write_incremental_witness` and `read_incremental_witness` 
+    respectively.
+  - `MerklePath::from_slice` has been replaced by `merkle_path_from_slice`
 
 ### Added
-- `merkle_tree::incremental::{read_address, write_address}`
+- `merkle_tree::incremental::{read_address, write_address, read_bridge_v2}`
-- `merkle_tree::incremental::read_bridge_v2`
 - `sapling::{CommitmentTree, IncrementalWitness, MerklePath, NOTE_COMMITMENT_TREE_DEPTH}`
 
 ### Changed

zcash_primitives/src/merkle_tree.rs

@@ -150,35 +150,34 @@ impl<Node, const DEPTH: u8> CommitmentTree<Node, DEPTH> {
     }
 }
 
+/// Reads a `CommitmentTree` from its serialized form.
+pub fn read_commitment_tree<Node: HashSer, R: Read, const DEPTH: u8>(
+    mut reader: R,
+) -> io::Result<CommitmentTree<Node, DEPTH>> {
+    let left = Optional::read(&mut reader, Node::read)?;
+    let right = Optional::read(&mut reader, Node::read)?;
+    let parents = Vector::read(&mut reader, |r| Optional::read(r, Node::read))?;
+
+    Ok(CommitmentTree {
+        left,
+        right,
+        parents,
+    })
+}
+
+/// Serializes this tree as an array of bytes.
+pub fn write_commitment_tree<Node: HashSer, W: Write, const DEPTH: u8>(
+    tree: &CommitmentTree<Node, DEPTH>,
+    mut writer: W,
+) -> io::Result<()> {
+    Optional::write(&mut writer, tree.left.as_ref(), |w, n| n.write(w))?;
+    Optional::write(&mut writer, tree.right.as_ref(), |w, n| n.write(w))?;
+    Vector::write(&mut writer, &tree.parents, |w, e| {
+        Optional::write(w, e.as_ref(), |w, n| n.write(w))
+    })
+}
+
 impl<Node: Hashable + Clone, const DEPTH: u8> CommitmentTree<Node, DEPTH> {
-    /// Reads a `CommitmentTree` from its serialized form.
-    pub fn read<R: Read>(mut reader: R) -> io::Result<Self>
-    where
-        Node: HashSer,
-    {
-        let left = Optional::read(&mut reader, Node::read)?;
-        let right = Optional::read(&mut reader, Node::read)?;
-        let parents = Vector::read(&mut reader, |r| Optional::read(r, Node::read))?;
-
-        Ok(CommitmentTree {
-            left,
-            right,
-            parents,
-        })
-    }
-
-    /// Serializes this tree as an array of bytes.
-    pub fn write<W: Write>(&self, mut writer: W) -> io::Result<()>
-    where
-        Node: HashSer,
-    {
-        Optional::write(&mut writer, self.left.as_ref(), |w, n| n.write(w))?;
-        Optional::write(&mut writer, self.right.as_ref(), |w, n| n.write(w))?;
-        Vector::write(&mut writer, &self.parents, |w, e| {
-            Optional::write(w, e.as_ref(), |w, n| n.write(w))
-        })
-    }
-
     /// Adds a leaf node to the tree.
     ///
     /// Returns an error if the tree is full.
@@ -307,38 +306,47 @@ impl<Node, const DEPTH: u8> IncrementalWitness<Node, DEPTH> {
     }
 }
 
-impl<Node: Hashable + HashSer + Clone, const DEPTH: u8> IncrementalWitness<Node, DEPTH> {
+/// Reads an `IncrementalWitness` from its serialized form.
-    /// Reads an `IncrementalWitness` from its serialized form.
+#[allow(clippy::redundant_closure)]
-    #[allow(clippy::redundant_closure)]
+pub fn read_incremental_witness<Node: HashSer, R: Read, const DEPTH: u8>(
-    pub fn read<R: Read>(mut reader: R) -> io::Result<IncrementalWitness<Node, DEPTH>> {
+    mut reader: R,
-        let tree = CommitmentTree::<Node, DEPTH>::read(&mut reader)?;
+) -> io::Result<IncrementalWitness<Node, DEPTH>> {
-        let filled = Vector::read(&mut reader, |r| Node::read(r))?;
+    let tree = read_commitment_tree(&mut reader)?;
-        let cursor = Optional::read(&mut reader, CommitmentTree::<Node, DEPTH>::read)?;
+    let filled = Vector::read(&mut reader, |r| Node::read(r))?;
+    let cursor = Optional::read(&mut reader, read_commitment_tree)?;
 
-        let mut witness = IncrementalWitness {
+    let mut witness = IncrementalWitness {
-            tree,
+        tree,
-            filled,
+        filled,
-            cursor_depth: 0,
+        cursor_depth: 0,
-            cursor,
+        cursor,
-        };
+    };
 
-        witness.cursor_depth = witness.next_depth();
+    witness.cursor_depth = witness.next_depth();
 
-        Ok(witness)
+    Ok(witness)
-    }
+}
 
-    /// Serializes this `IncrementalWitness` as an array of bytes.
+/// Serializes this `IncrementalWitness` as an array of bytes.
-    pub fn write<W: Write>(&self, mut writer: W) -> io::Result<()> {
+pub fn write_incremental_witness<Node: HashSer, W: Write, const DEPTH: u8>(
-        self.tree.write(&mut writer)?;
+    witness: &IncrementalWitness<Node, DEPTH>,
-        Vector::write(&mut writer, &self.filled, |w, n| n.write(w))?;
+    mut writer: W,
-        Optional::write(&mut writer, self.cursor.as_ref(), |w, t| t.write(w))
+) -> io::Result<()> {
-    }
+    write_commitment_tree(&witness.tree, &mut writer)?;
+    Vector::write(&mut writer, &witness.filled, |w, n| n.write(w))?;
+    Optional::write(&mut writer, witness.cursor.as_ref(), |w, t| {
+        write_commitment_tree(t, w)
+    })
+}
 
+impl<Node, const DEPTH: u8> IncrementalWitness<Node, DEPTH> {
     /// Returns the position of the witnessed leaf node in the commitment tree.
     pub fn position(&self) -> usize {
         self.tree.size() - 1
     }
+}
 
+impl<Node: Hashable + Clone, const DEPTH: u8> IncrementalWitness<Node, DEPTH> {
     fn filler(&self) -> PathFiller<Node> {
         let cursor_root = self
             .cursor
@@ -349,7 +357,9 @@ impl<Node: Hashable + HashSer + Clone, const DEPTH: u8> IncrementalWitness<Node,
             queue: self.filled.iter().cloned().chain(cursor_root).collect(),
         }
     }
+}
 
+impl<Node, const DEPTH: u8> IncrementalWitness<Node, DEPTH> {
     /// Finds the next "depth" of an unfilled subtree.
     fn next_depth(&self) -> u8 {
         let mut skip: u8 = self
@@ -388,7 +398,9 @@ impl<Node: Hashable + HashSer + Clone, const DEPTH: u8> IncrementalWitness<Node,
 
         d + skip
     }
+}
 
+impl<Node: Hashable + Clone, const DEPTH: u8> IncrementalWitness<Node, DEPTH> {
     /// Tracks a leaf node that has been added to the underlying tree.
     ///
     /// Returns an error if the tree is full.
@@ -499,68 +511,70 @@ impl<Node, const DEPTH: u8> MerklePath<Node, DEPTH> {
     }
 }
 
-impl<Node: Hashable + HashSer, const DEPTH: u8> MerklePath<Node, DEPTH> {
+/// Reads a Merkle path from its serialized form.
-    /// Reads a Merkle path from its serialized form.
+pub fn merkle_path_from_slice<Node: HashSer, const DEPTH: u8>(
-    pub fn from_slice(mut witness: &[u8]) -> Result<Self, ()> {
+    mut witness: &[u8],
-        // Skip the first byte, which should be DEPTH to signify the length of
+) -> Result<MerklePath<Node, DEPTH>, ()> {
-        // the following vector of Pedersen hashes.
+    // Skip the first byte, which should be DEPTH to signify the length of
-        if witness[0] != DEPTH {
+    // the following vector of Pedersen hashes.
-            return Err(());
+    if witness[0] != DEPTH {
-        }
+        return Err(());
-        witness = &witness[1..];
+    }
+    witness = &witness[1..];
 
-        // Begin to construct the authentication path
+    // Begin to construct the authentication path
-        let iter = witness.chunks_exact(33);
+    let iter = witness.chunks_exact(33);
-        witness = iter.remainder();
+    witness = iter.remainder();
 
-        // The vector works in reverse
+    // The vector works in reverse
-        let mut auth_path = iter
+    let mut auth_path = iter
-            .rev()
+        .rev()
-            .map(|bytes| {
+        .map(|bytes| {
-                // Length of inner vector should be the length of a Pedersen hash
+            // Length of inner vector should be the length of a Pedersen hash
-                if bytes[0] == 32 {
+            if bytes[0] == 32 {
-                    // Sibling node should be an element of Fr
+                // Sibling node should be an element of Fr
-                    Node::read(&bytes[1..])
+                Node::read(&bytes[1..])
-                        .map(|sibling| {
+                    .map(|sibling| {
-                            // Set the value in the auth path; we put false here
+                        // Set the value in the auth path; we put false here
-                            // for now (signifying the position bit) which we'll
+                        // for now (signifying the position bit) which we'll
-                            // fill in later.
+                        // fill in later.
-                            (sibling, false)
+                        (sibling, false)
-                        })
+                    })
-                        .map_err(|_| ())
+                    .map_err(|_| ())
-                } else {
+            } else {
-                    Err(())
+                Err(())
-                }
+            }
-            })
+        })
-            .collect::<Result<Vec<_>, _>>()?;
+        .collect::<Result<Vec<_>, _>>()?;
-        if auth_path.len() != usize::from(DEPTH) {
+    if auth_path.len() != usize::from(DEPTH) {
-            return Err(());
+        return Err(());
-        }
-
-        // Read the position from the witness
-        let position = witness.read_u64::<LittleEndian>().map_err(|_| ())?;
-
-        // Given the position, let's finish constructing the authentication
-        // path
-        let mut tmp = position;
-        for entry in auth_path.iter_mut() {
-            entry.1 = (tmp & 1) == 1;
-            tmp >>= 1;
-        }
-
-        // The witness should be empty now; if it wasn't, the caller would
-        // have provided more information than they should have, indicating
-        // a bug downstream
-        if witness.is_empty() {
-            Ok(MerklePath {
-                auth_path,
-                position,
-            })
-        } else {
-            Err(())
-        }
     }
 
+    // Read the position from the witness
+    let position = witness.read_u64::<LittleEndian>().map_err(|_| ())?;
+
+    // Given the position, let's finish constructing the authentication
+    // path
+    let mut tmp = position;
+    for entry in auth_path.iter_mut() {
+        entry.1 = (tmp & 1) == 1;
+        tmp >>= 1;
+    }
+
+    // The witness should be empty now; if it wasn't, the caller would
+    // have provided more information than they should have, indicating
+    // a bug downstream
+    if witness.is_empty() {
+        Ok(MerklePath {
+            auth_path,
+            position,
+        })
+    } else {
+        Err(())
+    }
+}
+
+impl<Node: Hashable + HashSer, const DEPTH: u8> MerklePath<Node, DEPTH> {
     /// Returns the root of the tree corresponding to this path applied to `leaf`.
     pub fn root(&self, leaf: Node) -> Node {
         self.auth_path
@@ -587,8 +601,10 @@ mod tests {
     use crate::sapling::{self, testing::arb_node, Node};
 
     use super::{
+        merkle_path_from_slice, read_commitment_tree, read_incremental_witness,
         testing::{arb_commitment_tree, TestNode},
-        CommitmentTree, HashSer, IncrementalWitness, MerklePath, PathFiller,
+        write_commitment_tree, write_incremental_witness, CommitmentTree, HashSer,
+        IncrementalWitness, PathFiller,
     };
 
     const HEX_EMPTY_ROOTS: [&str; 33] = [
@@ -1001,14 +1017,14 @@ mod tests {
         fn assert_tree_ser_eq(tree: &CommitmentTree<Node, TESTING_DEPTH>, expected: &str) {
             // Check that the tree matches its encoding
             let mut tmp = Vec::new();
-            tree.write(&mut tmp).unwrap();
+            write_commitment_tree(tree, &mut tmp).unwrap();
             assert_eq!(hex::encode(&tmp[..]), expected);
 
             // Check round-trip encoding
             let decoded: CommitmentTree<Node, TESTING_DEPTH> =
-                CommitmentTree::read(&hex::decode(expected).unwrap()[..]).unwrap();
+                read_commitment_tree(&hex::decode(expected).unwrap()[..]).unwrap();
             tmp.clear();
-            decoded.write(&mut tmp).unwrap();
+            write_commitment_tree(&decoded, &mut tmp).unwrap();
             assert_eq!(hex::encode(tmp), expected);
         }
 
@@ -1018,14 +1034,14 @@ mod tests {
         ) {
             // Check that the witness matches its encoding
             let mut tmp = Vec::new();
-            witness.write(&mut tmp).unwrap();
+            write_incremental_witness(witness, &mut tmp).unwrap();
             assert_eq!(hex::encode(&tmp[..]), expected);
 
             // Check round-trip encoding
             let decoded: IncrementalWitness<Node, TESTING_DEPTH> =
-                IncrementalWitness::read(&hex::decode(expected).unwrap()[..]).unwrap();
+                read_incremental_witness(&hex::decode(expected).unwrap()[..]).unwrap();
             tmp.clear();
-            decoded.write(&mut tmp).unwrap();
+            write_incremental_witness(&decoded, &mut tmp).unwrap();
             assert_eq!(hex::encode(tmp), expected);
         }
 
@@ -1063,7 +1079,7 @@ mod tests {
                 if let Some(leaf) = leaf {
                     let path = witness.path().expect("should be able to create a path");
                     let expected =
-                        MerklePath::from_slice(&hex::decode(paths[paths_i]).unwrap()).unwrap();
+                        merkle_path_from_slice(&hex::decode(paths[paths_i]).unwrap()).unwrap();
                     assert_eq!(path, expected);
 
                     assert_eq!(path.root(*leaf), witness.root());
@@ -1136,8 +1152,8 @@ mod tests {
         #[test]
         fn prop_commitment_tree_roundtrip_ser(ct in arb_commitment_tree::<_, _, 8>(32, arb_node())) {
             let mut serialized = vec![];
-            assert_matches!(ct.write(&mut serialized), Ok(()));
+            assert_matches!(write_commitment_tree(&ct, &mut serialized), Ok(()));
-            assert_matches!(CommitmentTree::<_, 8>::read(&serialized[..]), Ok(ct_out) if ct == ct_out);
+            assert_matches!(read_commitment_tree::<_, _, 8>(&serialized[..]), Ok(ct_out) if ct == ct_out);
         }
     }
 

zcash_primitives/src/merkle_tree/incremental.rs

@@ -9,7 +9,7 @@ use incrementalmerkletree::{Address, Hashable, Level, Position};
 use orchard::tree::MerkleHashOrchard;
 use zcash_encoding::{Optional, Vector};
 
-use super::{CommitmentTree, HashSer};
+use super::{read_commitment_tree, HashSer};
 use crate::sapling;
 
 pub const SER_V1: u8 = 1;
@@ -80,8 +80,7 @@ pub fn read_address<R: Read>(mut reader: R) -> io::Result<Address> {
 pub fn read_frontier_v0<H: Hashable + HashSer + Clone, R: Read>(
     mut reader: R,
 ) -> io::Result<Frontier<H, { sapling::NOTE_COMMITMENT_TREE_DEPTH }>> {
-    let tree = CommitmentTree::<H, { sapling::NOTE_COMMITMENT_TREE_DEPTH }>::read(&mut reader)?;
+    let tree = read_commitment_tree(&mut reader)?;
-
     Ok(tree.to_frontier())
 }
 
@@ -312,7 +311,10 @@ mod tests {
 
     use super::*;
     use crate::{
-        merkle_tree::testing::{arb_commitment_tree, TestNode},
+        merkle_tree::{
+            testing::{arb_commitment_tree, TestNode},
+            write_commitment_tree,
+        },
         sapling::{testing as sapling, Node},
     };
 
@@ -321,7 +323,7 @@ mod tests {
         fn frontier_serialization_v0(t in arb_commitment_tree::<_, _, 32>(0, sapling::arb_node()))
         {
             let mut buffer = vec![];
-            t.write(&mut buffer).unwrap();
+            write_commitment_tree(&t, &mut buffer).unwrap();
             let frontier: Frontier<Node, 32> = read_frontier_v0(&buffer[..]).unwrap();
 
             let expected: Frontier<Node, 32> = t.to_frontier();