From 9df78ffdbaf54b458cba4cc3b8ffe4d20a1feea7 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 6 Jul 2023 11:04:28 +1000 Subject: [PATCH] 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 --- zebra-chain/src/orchard/tree.rs | 45 ++++++--- zebra-chain/src/sapling/tree.rs | 45 ++++++--- zebra-chain/src/sprout/tree.rs | 43 +++++--- .../service/finalized_state/tests/vectors.rs | 99 +++++++++++++++++-- 4 files changed, 187 insertions(+), 45 deletions(-) diff --git a/zebra-chain/src/orchard/tree.rs b/zebra-chain/src/orchard/tree.rs index 6af9680c..0924b3d8 100644 --- a/zebra-chain/src/orchard/tree.rs +++ b/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 - .cached_root - .read() - .expect("a thread that previously held exclusive lock access panicked") - .deref() - { + if let Some(root) = self.cached_root() { // 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 { + *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 - .cached_root - .read() - .expect("a thread that previously held exclusive lock access panicked"); + let cached_root = self.cached_root(); Self { inner: self.inner.clone(), diff --git a/zebra-chain/src/sapling/tree.rs b/zebra-chain/src/sapling/tree.rs index f5307a8e..2b6feeb0 100644 --- a/zebra-chain/src/sapling/tree.rs +++ b/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 - .cached_root - .read() - .expect("a thread that previously held exclusive lock access panicked") - .deref() - { + if let Some(root) = self.cached_root() { // 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 { + *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 - .cached_root - .read() - .expect("a thread that previously held exclusive lock access panicked"); + let cached_root = self.cached_root(); Self { inner: self.inner.clone(), diff --git a/zebra-chain/src/sprout/tree.rs b/zebra-chain/src/sprout/tree.rs index ab597fc9..d28738be 100644 --- a/zebra-chain/src/sprout/tree.rs +++ b/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 - .cached_root - .read() - .expect("a thread that previously held exclusive lock access panicked") - .deref() - { + if let Some(root) = self.cached_root() { // 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 { + *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 - .cached_root - .read() - .expect("a thread that previously held exclusive lock access panicked"); + let cached_root = self.cached_root(); Self { inner: self.inner.clone(), diff --git a/zebra-state/src/service/finalized_state/tests/vectors.rs b/zebra-state/src/service/finalized_state/tests/vectors.rs index 98975646..078c0267 100644 --- a/zebra-state/src/service/finalized_state/tests/vectors.rs +++ b/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); }