change(tests): Do round-trip tests for note commitment tree data structure and RPC serialisation (#7147)

* Add an assert_frontier_eq() method to note commitment trees for tests

* Check round-trip serialization for note commitment trees

* fix typos

---------

Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>

zebra-chain/src/orchard/tree.rs

@@ -14,7 +14,6 @@ use std::{
     fmt,
     hash::{Hash, Hasher},
     io,
-    ops::Deref,
     sync::Arc,
 };
 
@@ -330,14 +329,9 @@ impl NoteCommitmentTree {
     /// Returns the current root of the tree, used as an anchor in Orchard
     /// shielded transactions.
     pub fn root(&self) -> Root {
-        if let Some(root) = self
+        if let Some(root) = self.cached_root() {
-            .cached_root
-            .read()
-            .expect("a thread that previously held exclusive lock access panicked")
-            .deref()
-        {
             // Return cached root.
-            return *root;
+            return root;
         }
 
         // Get exclusive access, compute the root, and cache it.
@@ -358,6 +352,15 @@ impl NoteCommitmentTree {
         }
     }
 
+    /// Returns the current root of the tree, if it has already been cached.
+    #[allow(clippy::unwrap_in_result)]
+    pub fn cached_root(&self) -> Option<Root> {
+        *self
+            .cached_root
+            .read()
+            .expect("a thread that previously held exclusive lock access panicked")
+    }
+
     /// Get the Pallas-based Sinsemilla hash / root node of this merkle tree of
     /// note commitments.
     pub fn hash(&self) -> [u8; 32] {
@@ -379,15 +382,33 @@ impl NoteCommitmentTree {
     pub fn count(&self) -> u64 {
         self.inner.position().map_or(0, |pos| u64::from(pos) + 1)
     }
+
+    /// Checks if the tree roots and inner data structures of `self` and `other` are equal.
+    ///
+    /// # Panics
+    ///
+    /// If they aren't equal, with a message explaining the differences.
+    ///
+    /// Only for use in tests.
+    #[cfg(any(test, feature = "proptest-impl"))]
+    pub fn assert_frontier_eq(&self, other: &Self) {
+        // It's technically ok for the cached root not to be preserved,
+        // but it can result in expensive cryptographic operations,
+        // so we fail the tests if it happens.
+        assert_eq!(self.cached_root(), other.cached_root());
+
+        // Check the data in the internal data structure
+        assert_eq!(self.inner, other.inner);
+
+        // Check the RPC serialization format (not the same as the Zebra database format)
+        assert_eq!(SerializedTree::from(self), SerializedTree::from(other));
+    }
 }
 
 impl Clone for NoteCommitmentTree {
     /// Clones the inner tree, and creates a new `RwLock` with the cloned root data.
     fn clone(&self) -> Self {
-        let cached_root = *self
+        let cached_root = self.cached_root();
-            .cached_root
-            .read()
-            .expect("a thread that previously held exclusive lock access panicked");
 
         Self {
             inner: self.inner.clone(),

zebra-chain/src/sapling/tree.rs

@@ -14,7 +14,6 @@ use std::{
     fmt,
     hash::{Hash, Hasher},
     io,
-    ops::Deref,
     sync::Arc,
 };
 
@@ -333,14 +332,9 @@ impl NoteCommitmentTree {
     /// Returns the current root of the tree, used as an anchor in Sapling
     /// shielded transactions.
     pub fn root(&self) -> Root {
-        if let Some(root) = self
+        if let Some(root) = self.cached_root() {
-            .cached_root
-            .read()
-            .expect("a thread that previously held exclusive lock access panicked")
-            .deref()
-        {
             // Return cached root.
-            return *root;
+            return root;
         }
 
         // Get exclusive access, compute the root, and cache it.
@@ -361,6 +355,15 @@ impl NoteCommitmentTree {
         }
     }
 
+    /// Returns the current root of the tree, if it has already been cached.
+    #[allow(clippy::unwrap_in_result)]
+    pub fn cached_root(&self) -> Option<Root> {
+        *self
+            .cached_root
+            .read()
+            .expect("a thread that previously held exclusive lock access panicked")
+    }
+
     /// Gets the Jubjub-based Pedersen hash of root node of this merkle tree of
     /// note commitments.
     pub fn hash(&self) -> [u8; 32] {
@@ -382,16 +385,34 @@ impl NoteCommitmentTree {
     pub fn count(&self) -> u64 {
         self.inner.position().map_or(0, |pos| u64::from(pos) + 1)
     }
+
+    /// Checks if the tree roots and inner data structures of `self` and `other` are equal.
+    ///
+    /// # Panics
+    ///
+    /// If they aren't equal, with a message explaining the differences.
+    ///
+    /// Only for use in tests.
+    #[cfg(any(test, feature = "proptest-impl"))]
+    pub fn assert_frontier_eq(&self, other: &Self) {
+        // It's technically ok for the cached root not to be preserved,
+        // but it can result in expensive cryptographic operations,
+        // so we fail the tests if it happens.
+        assert_eq!(self.cached_root(), other.cached_root());
+
+        // Check the data in the internal data structure
+        assert_eq!(self.inner, other.inner);
+
+        // Check the RPC serialization format (not the same as the Zebra database format)
+        assert_eq!(SerializedTree::from(self), SerializedTree::from(other));
+    }
 }
 
 impl Clone for NoteCommitmentTree {
     /// Clones the inner tree, and creates a new [`RwLock`](std::sync::RwLock)
     /// with the cloned root data.
     fn clone(&self) -> Self {
-        let cached_root = *self
+        let cached_root = self.cached_root();
-            .cached_root
-            .read()
-            .expect("a thread that previously held exclusive lock access panicked");
 
         Self {
             inner: self.inner.clone(),

zebra-chain/src/sprout/tree.rs

@@ -10,7 +10,7 @@
 //!
 //! A root of a note commitment tree is associated with each treestate.
 
-use std::{fmt, ops::Deref};
+use std::fmt;
 
 use byteorder::{BigEndian, ByteOrder};
 use incrementalmerkletree::{bridgetree, Frontier};
@@ -266,14 +266,9 @@ impl NoteCommitmentTree {
     /// Returns the current root of the tree; used as an anchor in Sprout
     /// shielded transactions.
     pub fn root(&self) -> Root {
-        if let Some(root) = self
+        if let Some(root) = self.cached_root() {
-            .cached_root
-            .read()
-            .expect("a thread that previously held exclusive lock access panicked")
-            .deref()
-        {
             // Return cached root.
-            return *root;
+            return root;
         }
 
         // Get exclusive access, compute the root, and cache it.
@@ -294,6 +289,15 @@ impl NoteCommitmentTree {
         }
     }
 
+    /// Returns the current root of the tree, if it has already been cached.
+    #[allow(clippy::unwrap_in_result)]
+    pub fn cached_root(&self) -> Option<Root> {
+        *self
+            .cached_root
+            .read()
+            .expect("a thread that previously held exclusive lock access panicked")
+    }
+
     /// Returns a hash of the Sprout note commitment tree root.
     pub fn hash(&self) -> [u8; 32] {
         self.root().into()
@@ -316,15 +320,30 @@ impl NoteCommitmentTree {
     pub fn count(&self) -> u64 {
         self.inner.position().map_or(0, |pos| u64::from(pos) + 1)
     }
+
+    /// Checks if the tree roots and inner data structures of `self` and `other` are equal.
+    ///
+    /// # Panics
+    ///
+    /// If they aren't equal, with a message explaining the differences.
+    ///
+    /// Only for use in tests.
+    #[cfg(any(test, feature = "proptest-impl"))]
+    pub fn assert_frontier_eq(&self, other: &Self) {
+        // It's technically ok for the cached root not to be preserved,
+        // but it can result in expensive cryptographic operations,
+        // so we fail the tests if it happens.
+        assert_eq!(self.cached_root(), other.cached_root());
+
+        // Check the data in the internal data structure
+        assert_eq!(self.inner, other.inner);
+    }
 }
 
 impl Clone for NoteCommitmentTree {
     /// Clones the inner tree, and creates a new `RwLock` with the cloned root data.
     fn clone(&self) -> Self {
-        let cached_root = *self
+        let cached_root = self.cached_root();
-            .cached_root
-            .read()
-            .expect("a thread that previously held exclusive lock access panicked");
 
         Self {
             inner: self.inner.clone(),

zebra-state/src/service/finalized_state/tests/vectors.rs

@@ -48,9 +48,18 @@ fn sprout_note_commitment_tree_serialization() {
     let serialized_tree = incremental_tree.as_bytes();
     assert_eq!(hex::encode(&serialized_tree), expected_serialized_tree_hex);
 
-    let deserialized_tree = sprout::tree::NoteCommitmentTree::from_bytes(serialized_tree);
+    let deserialized_tree = sprout::tree::NoteCommitmentTree::from_bytes(&serialized_tree);
 
+    // This check isn't enough to show that the entire struct is the same, because it just compares
+    // the cached serialized/deserialized roots. (NoteCommitmentTree::eq() also just compares
+    // roots.)
     assert_eq!(incremental_tree.root(), deserialized_tree.root());
+
+    incremental_tree.assert_frontier_eq(&deserialized_tree);
+
+    // Double-check that the internal format is the same by re-serializing the tree.
+    let re_serialized_tree = deserialized_tree.as_bytes();
+    assert_eq!(serialized_tree, re_serialized_tree);
 }
 
 /// Check that the sprout tree database serialization format has not changed for one commitment.
@@ -85,9 +94,18 @@ fn sprout_note_commitment_tree_serialization_one() {
     let serialized_tree = incremental_tree.as_bytes();
     assert_eq!(hex::encode(&serialized_tree), expected_serialized_tree_hex);
 
-    let deserialized_tree = sprout::tree::NoteCommitmentTree::from_bytes(serialized_tree);
+    let deserialized_tree = sprout::tree::NoteCommitmentTree::from_bytes(&serialized_tree);
 
+    // This check isn't enough to show that the entire struct is the same, because it just compares
+    // the cached serialized/deserialized roots. (NoteCommitmentTree::eq() also just compares
+    // roots.)
     assert_eq!(incremental_tree.root(), deserialized_tree.root());
+
+    incremental_tree.assert_frontier_eq(&deserialized_tree);
+
+    // Double-check that the internal format is the same by re-serializing the tree.
+    let re_serialized_tree = deserialized_tree.as_bytes();
+    assert_eq!(serialized_tree, re_serialized_tree);
 }
 
 /// Check that the sprout tree database serialization format has not changed when the number of
@@ -131,9 +149,18 @@ fn sprout_note_commitment_tree_serialization_pow2() {
     let serialized_tree = incremental_tree.as_bytes();
     assert_eq!(hex::encode(&serialized_tree), expected_serialized_tree_hex);
 
-    let deserialized_tree = sprout::tree::NoteCommitmentTree::from_bytes(serialized_tree);
+    let deserialized_tree = sprout::tree::NoteCommitmentTree::from_bytes(&serialized_tree);
 
+    // This check isn't enough to show that the entire struct is the same, because it just compares
+    // the cached serialized/deserialized roots. (NoteCommitmentTree::eq() also just compares
+    // roots.)
     assert_eq!(incremental_tree.root(), deserialized_tree.root());
+
+    incremental_tree.assert_frontier_eq(&deserialized_tree);
+
+    // Double-check that the internal format is the same by re-serializing the tree.
+    let re_serialized_tree = deserialized_tree.as_bytes();
+    assert_eq!(serialized_tree, re_serialized_tree);
 }
 
 /// Check that the sapling tree database serialization format has not changed.
@@ -172,9 +199,18 @@ fn sapling_note_commitment_tree_serialization() {
     let serialized_tree = incremental_tree.as_bytes();
     assert_eq!(hex::encode(&serialized_tree), expected_serialized_tree_hex);
 
-    let deserialized_tree = sapling::tree::NoteCommitmentTree::from_bytes(serialized_tree);
+    let deserialized_tree = sapling::tree::NoteCommitmentTree::from_bytes(&serialized_tree);
 
+    // This check isn't enough to show that the entire struct is the same, because it just compares
+    // the cached serialized/deserialized roots. (NoteCommitmentTree::eq() also just compares
+    // roots.)
     assert_eq!(incremental_tree.root(), deserialized_tree.root());
+
+    incremental_tree.assert_frontier_eq(&deserialized_tree);
+
+    // Double-check that the internal format is the same by re-serializing the tree.
+    let re_serialized_tree = deserialized_tree.as_bytes();
+    assert_eq!(serialized_tree, re_serialized_tree);
 }
 
 /// Check that the sapling tree database serialization format has not changed for one commitment.
@@ -209,9 +245,18 @@ fn sapling_note_commitment_tree_serialization_one() {
     let serialized_tree = incremental_tree.as_bytes();
     assert_eq!(hex::encode(&serialized_tree), expected_serialized_tree_hex);
 
-    let deserialized_tree = sapling::tree::NoteCommitmentTree::from_bytes(serialized_tree);
+    let deserialized_tree = sapling::tree::NoteCommitmentTree::from_bytes(&serialized_tree);
 
+    // This check isn't enough to show that the entire struct is the same, because it just compares
+    // the cached serialized/deserialized roots. (NoteCommitmentTree::eq() also just compares
+    // roots.)
     assert_eq!(incremental_tree.root(), deserialized_tree.root());
+
+    incremental_tree.assert_frontier_eq(&deserialized_tree);
+
+    // Double-check that the internal format is the same by re-serializing the tree.
+    let re_serialized_tree = deserialized_tree.as_bytes();
+    assert_eq!(serialized_tree, re_serialized_tree);
 }
 
 /// Check that the sapling tree database serialization format has not changed when the number of
@@ -259,9 +304,18 @@ fn sapling_note_commitment_tree_serialization_pow2() {
     let serialized_tree = incremental_tree.as_bytes();
     assert_eq!(hex::encode(&serialized_tree), expected_serialized_tree_hex);
 
-    let deserialized_tree = sapling::tree::NoteCommitmentTree::from_bytes(serialized_tree);
+    let deserialized_tree = sapling::tree::NoteCommitmentTree::from_bytes(&serialized_tree);
 
+    // This check isn't enough to show that the entire struct is the same, because it just compares
+    // the cached serialized/deserialized roots. (NoteCommitmentTree::eq() also just compares
+    // roots.)
     assert_eq!(incremental_tree.root(), deserialized_tree.root());
+
+    incremental_tree.assert_frontier_eq(&deserialized_tree);
+
+    // Double-check that the internal format is the same by re-serializing the tree.
+    let re_serialized_tree = deserialized_tree.as_bytes();
+    assert_eq!(serialized_tree, re_serialized_tree);
 }
 
 /// Check that the orchard tree database serialization format has not changed.
@@ -310,9 +364,18 @@ fn orchard_note_commitment_tree_serialization() {
     let serialized_tree = incremental_tree.as_bytes();
     assert_eq!(hex::encode(&serialized_tree), expected_serialized_tree_hex);
 
-    let deserialized_tree = orchard::tree::NoteCommitmentTree::from_bytes(serialized_tree);
+    let deserialized_tree = orchard::tree::NoteCommitmentTree::from_bytes(&serialized_tree);
 
+    // This check isn't enough to show that the entire struct is the same, because it just compares
+    // the cached serialized/deserialized roots. (NoteCommitmentTree::eq() also just compares
+    // roots.)
     assert_eq!(incremental_tree.root(), deserialized_tree.root());
+
+    incremental_tree.assert_frontier_eq(&deserialized_tree);
+
+    // Double-check that the internal format is the same by re-serializing the tree.
+    let re_serialized_tree = deserialized_tree.as_bytes();
+    assert_eq!(serialized_tree, re_serialized_tree);
 }
 
 /// Check that the orchard tree database serialization format has not changed for one commitment.
@@ -349,9 +412,18 @@ fn orchard_note_commitment_tree_serialization_one() {
     let serialized_tree = incremental_tree.as_bytes();
     assert_eq!(hex::encode(&serialized_tree), expected_serialized_tree_hex);
 
-    let deserialized_tree = orchard::tree::NoteCommitmentTree::from_bytes(serialized_tree);
+    let deserialized_tree = orchard::tree::NoteCommitmentTree::from_bytes(&serialized_tree);
 
+    // This check isn't enough to show that the entire struct is the same, because it just compares
+    // the cached serialized/deserialized roots. (NoteCommitmentTree::eq() also just compares
+    // roots.)
     assert_eq!(incremental_tree.root(), deserialized_tree.root());
+
+    incremental_tree.assert_frontier_eq(&deserialized_tree);
+
+    // Double-check that the internal format is the same by re-serializing the tree.
+    let re_serialized_tree = deserialized_tree.as_bytes();
+    assert_eq!(serialized_tree, re_serialized_tree);
 }
 
 /// Check that the orchard tree database serialization format has not changed when the number of
@@ -399,7 +471,16 @@ fn orchard_note_commitment_tree_serialization_pow2() {
     let serialized_tree = incremental_tree.as_bytes();
     assert_eq!(hex::encode(&serialized_tree), expected_serialized_tree_hex);
 
-    let deserialized_tree = orchard::tree::NoteCommitmentTree::from_bytes(serialized_tree);
+    let deserialized_tree = orchard::tree::NoteCommitmentTree::from_bytes(&serialized_tree);
 
+    // This check isn't enough to show that the entire struct is the same, because it just compares
+    // the cached serialized/deserialized roots. (NoteCommitmentTree::eq() also just compares
+    // roots.)
     assert_eq!(incremental_tree.root(), deserialized_tree.root());
+
+    incremental_tree.assert_frontier_eq(&deserialized_tree);
+
+    // Double-check that the internal format is the same by re-serializing the tree.
+    let re_serialized_tree = deserialized_tree.as_bytes();
+    assert_eq!(serialized_tree, re_serialized_tree);
 }