From 0d3892f61f39dde44427a2f91a3fa60282cd43ef Mon Sep 17 00:00:00 2001 From: Marek Date: Mon, 30 Oct 2023 21:06:54 +0100 Subject: [PATCH] change(state): Refactor the naming of note commitment subtrees (#7855) * Rename `node` & `Node` to `root` & `Root` * Rename `end` to `end_height` * Rename `Root` to `SubtreeRoot` --- zebra-chain/src/subtree.rs | 45 +++++++++-------- zebra-rpc/src/methods.rs | 8 ++-- .../finalized_state/disk_format/shielded.rs | 4 +- .../finalized_state/disk_format/tests/prop.rs | 6 +-- .../disk_format/upgrade/add_subtrees.rs | 48 +++++++++---------- .../finalized_state/zebra_db/shielded.rs | 4 +- .../src/service/non_finalized_state/chain.rs | 4 +- zebra-state/src/service/read/tests/vectors.rs | 5 +- 8 files changed, 65 insertions(+), 59 deletions(-) diff --git a/zebra-chain/src/subtree.rs b/zebra-chain/src/subtree.rs index 52e3ef3d7..6402b0e07 100644 --- a/zebra-chain/src/subtree.rs +++ b/zebra-chain/src/subtree.rs @@ -50,31 +50,38 @@ impl From for u64 { // TODO: // - consider defining sapling::SubtreeRoot and orchard::SubtreeRoot types or type wrappers, // to avoid type confusion between the leaf Node and subtree root types. -// - rename the `Node` generic to `SubtreeRoot` /// Subtree root of Sapling or Orchard note commitment tree, /// with its associated block height and subtree index. #[derive(Copy, Clone, Debug, Eq, PartialEq)] #[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))] -pub struct NoteCommitmentSubtree { +pub struct NoteCommitmentSubtree { /// Index of this subtree pub index: NoteCommitmentSubtreeIndex, /// Root of this subtree. - pub node: Node, + pub root: SubtreeRoot, /// End boundary of this subtree, the block height of its last leaf. - pub end: Height, + pub end_height: Height, } -impl NoteCommitmentSubtree { +impl NoteCommitmentSubtree { /// Creates new [`NoteCommitmentSubtree`] - pub fn new(index: impl Into, end: Height, node: Node) -> Self { + pub fn new( + index: impl Into, + end_height: Height, + root: SubtreeRoot, + ) -> Self { let index = index.into(); - Self { index, end, node } + Self { + index, + end_height, + root, + } } /// Converts struct to [`NoteCommitmentSubtreeData`]. - pub fn into_data(self) -> NoteCommitmentSubtreeData { - NoteCommitmentSubtreeData::new(self.end, self.node) + pub fn into_data(self) -> NoteCommitmentSubtreeData { + NoteCommitmentSubtreeData::new(self.end_height, self.root) } } @@ -82,29 +89,25 @@ impl NoteCommitmentSubtree { /// Used for database key-value serialization, where the subtree index is the key, and this struct is the value. #[derive(Copy, Clone, Debug, Eq, PartialEq, serde::Serialize)] #[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))] -pub struct NoteCommitmentSubtreeData { +pub struct NoteCommitmentSubtreeData { /// Merkle root of the 2^16-leaf subtree. - // - // TODO: rename both Rust fields to match the RPC field names - #[serde(rename = "root")] - pub node: Node, + pub root: SubtreeRoot, /// Height of the block containing the note that completed this subtree. - #[serde(rename = "end_height")] - pub end: Height, + pub end_height: Height, } -impl NoteCommitmentSubtreeData { +impl NoteCommitmentSubtreeData { /// Creates new [`NoteCommitmentSubtreeData`] - pub fn new(end: Height, node: Node) -> Self { - Self { end, node } + pub fn new(end_height: Height, root: SubtreeRoot) -> Self { + Self { end_height, root } } /// Creates new [`NoteCommitmentSubtree`] from a [`NoteCommitmentSubtreeData`] and index pub fn with_index( self, index: impl Into, - ) -> NoteCommitmentSubtree { - NoteCommitmentSubtree::new(index, self.end, self.node) + ) -> NoteCommitmentSubtree { + NoteCommitmentSubtree::new(index, self.end_height, self.root) } } diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index 2df10ca72..179f30ff5 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -1172,8 +1172,8 @@ where let subtrees = subtrees .values() .map(|subtree| SubtreeRpcData { - node: subtree.node.encode_hex(), - end: subtree.end, + root: subtree.root.encode_hex(), + end_height: subtree.end_height, }) .collect(); @@ -1202,8 +1202,8 @@ where let subtrees = subtrees .values() .map(|subtree| SubtreeRpcData { - node: subtree.node.encode_hex(), - end: subtree.end, + root: subtree.root.encode_hex(), + end_height: subtree.end_height, }) .collect(); diff --git a/zebra-state/src/service/finalized_state/disk_format/shielded.rs b/zebra-state/src/service/finalized_state/disk_format/shielded.rs index 622b1ae1a..d97168476 100644 --- a/zebra-state/src/service/finalized_state/disk_format/shielded.rs +++ b/zebra-state/src/service/finalized_state/disk_format/shielded.rs @@ -178,11 +178,11 @@ impl IntoDisk for orchard::tree::Node { } } -impl>> IntoDisk for NoteCommitmentSubtreeData { +impl>> IntoDisk for NoteCommitmentSubtreeData { type Bytes = Vec; fn as_bytes(&self) -> Self::Bytes { - [self.end.as_bytes().to_vec(), self.node.as_bytes()].concat() + [self.end_height.as_bytes().to_vec(), self.root.as_bytes()].concat() } } diff --git a/zebra-state/src/service/finalized_state/disk_format/tests/prop.rs b/zebra-state/src/service/finalized_state/disk_format/tests/prop.rs index cd3c6cf26..a76f9efff 100644 --- a/zebra-state/src/service/finalized_state/disk_format/tests/prop.rs +++ b/zebra-state/src/service/finalized_state/disk_format/tests/prop.rs @@ -376,7 +376,7 @@ fn roundtrip_sapling_subtree_data() { let _init_guard = zebra_test::init(); proptest!(|(mut val in any::>())| { - val.end.0 %= MAX_ON_DISK_HEIGHT.0 + 1; + val.end_height.0 %= MAX_ON_DISK_HEIGHT.0 + 1; assert_value_properties(val) }); } @@ -461,8 +461,8 @@ fn roundtrip_orchard_subtree_data() { let _init_guard = zebra_test::init(); proptest!(|(mut val in any::>())| { - val.end = val.end.clamp(Height(0), MAX_ON_DISK_HEIGHT); - val.end.0 %= MAX_ON_DISK_HEIGHT.0 + 1; + val.end_height = val.end_height.clamp(Height(0), MAX_ON_DISK_HEIGHT); + val.end_height.0 %= MAX_ON_DISK_HEIGHT.0 + 1; assert_value_properties(val) }); } diff --git a/zebra-state/src/service/finalized_state/disk_format/upgrade/add_subtrees.rs b/zebra-state/src/service/finalized_state/disk_format/upgrade/add_subtrees.rs index 72dc4d55c..d84392ebf 100644 --- a/zebra-state/src/service/finalized_state/disk_format/upgrade/add_subtrees.rs +++ b/zebra-state/src/service/finalized_state/disk_format/upgrade/add_subtrees.rs @@ -171,11 +171,11 @@ fn first_sapling_mainnet_subtree() -> NoteCommitmentSubtree // ``` NoteCommitmentSubtree { index: 0.into(), - node: hex!("754bb593ea42d231a7ddf367640f09bbf59dc00f2c1d2003cc340e0c016b5b13") + root: hex!("754bb593ea42d231a7ddf367640f09bbf59dc00f2c1d2003cc340e0c016b5b13") .as_slice() .try_into() .expect("test vector is valid"), - end: Height(558822), + end_height: Height(558822), } } @@ -187,11 +187,11 @@ fn first_orchard_mainnet_subtree() -> NoteCommitmentSubtree // ``` NoteCommitmentSubtree { index: 0.into(), - node: hex!("d4e323b3ae0cabfb6be4087fec8c66d9a9bbfc354bf1d9588b6620448182063b") + root: hex!("d4e323b3ae0cabfb6be4087fec8c66d9a9bbfc354bf1d9588b6620448182063b") .as_slice() .try_into() .expect("test vector is valid"), - end: Height(1707429), + end_height: Height(1707429), } } @@ -360,9 +360,9 @@ fn check_sapling_subtrees( }; // Check that there was a sapling note at the subtree's end height. - let Some(tree) = db.sapling_tree_by_height(&subtree.end) else { + let Some(tree) = db.sapling_tree_by_height(&subtree.end_height) else { result = Err("missing note commitment tree at subtree completion height"); - error!(?result, ?subtree.end); + error!(?result, ?subtree.end_height); continue; }; @@ -373,7 +373,7 @@ fn check_sapling_subtrees( error!(?result); } - if subtree.node != node { + if subtree.root != node { result = Err("completed subtree roots should match"); error!(?result); } @@ -381,13 +381,13 @@ fn check_sapling_subtrees( // Check that the final note has a greater subtree index if it didn't complete a subtree. else { let prev_height = subtree - .end + .end_height .previous() .expect("Note commitment subtrees should not end at the minimal height."); let Some(prev_tree) = db.sapling_tree_by_height(&prev_height) else { result = Err("missing note commitment tree below subtree completion height"); - error!(?result, ?subtree.end); + error!(?result, ?subtree.end_height); continue; }; @@ -430,15 +430,15 @@ fn check_sapling_subtrees( }; // Check that the subtree end height matches that in the sapling trees. - if subtree.end != height { + if subtree.end_height != height { let is_complete = tree.is_complete_subtree(); result = Err("bad sapling subtree end height"); - error!(?result, ?subtree.end, ?height, ?index, ?is_complete, ); + error!(?result, ?subtree.end_height, ?height, ?index, ?is_complete, ); } // Check the root if the sapling note commitment tree at this height is a complete subtree. if let Some((_index, node)) = tree.completed_subtree_index_and_root() { - if subtree.node != node { + if subtree.root != node { result = Err("completed subtree roots should match"); error!(?result); } @@ -490,9 +490,9 @@ fn check_orchard_subtrees( }; // Check that there was a orchard note at the subtree's end height. - let Some(tree) = db.orchard_tree_by_height(&subtree.end) else { + let Some(tree) = db.orchard_tree_by_height(&subtree.end_height) else { result = Err("missing note commitment tree at subtree completion height"); - error!(?result, ?subtree.end); + error!(?result, ?subtree.end_height); continue; }; @@ -503,7 +503,7 @@ fn check_orchard_subtrees( error!(?result); } - if subtree.node != node { + if subtree.root != node { result = Err("completed subtree roots should match"); error!(?result); } @@ -511,13 +511,13 @@ fn check_orchard_subtrees( // Check that the final note has a greater subtree index if it didn't complete a subtree. else { let prev_height = subtree - .end + .end_height .previous() .expect("Note commitment subtrees should not end at the minimal height."); let Some(prev_tree) = db.orchard_tree_by_height(&prev_height) else { result = Err("missing note commitment tree below subtree completion height"); - error!(?result, ?subtree.end); + error!(?result, ?subtree.end_height); continue; }; @@ -560,15 +560,15 @@ fn check_orchard_subtrees( }; // Check that the subtree end height matches that in the orchard trees. - if subtree.end != height { + if subtree.end_height != height { let is_complete = tree.is_complete_subtree(); result = Err("bad orchard subtree end height"); - error!(?result, ?subtree.end, ?height, ?index, ?is_complete, ); + error!(?result, ?subtree.end_height, ?height, ?index, ?is_complete, ); } // Check the root if the orchard note commitment tree at this height is a complete subtree. if let Some((_index, node)) = tree.completed_subtree_index_and_root() { - if subtree.node != node { + if subtree.root != node { result = Err("completed subtree roots should match"); error!(?result); } @@ -851,10 +851,10 @@ fn write_sapling_subtree( .expect("writing sapling note commitment subtrees should always succeed."); if subtree.index.0 % 100 == 0 { - info!(end_height = ?subtree.end, index = ?subtree.index.0, "calculated and added sapling subtree"); + info!(end_height = ?subtree.end_height, index = ?subtree.index.0, "calculated and added sapling subtree"); } // This log happens about once per second on recent machines with SSD disks. - debug!(end_height = ?subtree.end, index = ?subtree.index.0, "calculated and added sapling subtree"); + debug!(end_height = ?subtree.end_height, index = ?subtree.index.0, "calculated and added sapling subtree"); } /// Writes an Orchard note commitment subtree to `upgrade_db`. @@ -871,8 +871,8 @@ fn write_orchard_subtree( .expect("writing orchard note commitment subtrees should always succeed."); if subtree.index.0 % 100 == 0 { - info!(end_height = ?subtree.end, index = ?subtree.index.0, "calculated and added orchard subtree"); + info!(end_height = ?subtree.end_height, index = ?subtree.index.0, "calculated and added orchard subtree"); } // This log happens about once per second on recent machines with SSD disks. - debug!(end_height = ?subtree.end, index = ?subtree.index.0, "calculated and added orchard subtree"); + debug!(end_height = ?subtree.end_height, index = ?subtree.index.0, "calculated and added orchard subtree"); } diff --git a/zebra-state/src/service/finalized_state/zebra_db/shielded.rs b/zebra-state/src/service/finalized_state/zebra_db/shielded.rs index 260d202ea..a6733afa4 100644 --- a/zebra-state/src/service/finalized_state/zebra_db/shielded.rs +++ b/zebra-state/src/service/finalized_state/zebra_db/shielded.rs @@ -275,7 +275,7 @@ impl ZebraDb { ) = self.db.zs_last_key_value(&sapling_subtrees)?; let tip_height = self.finalized_tip_height()?; - if subtree_data.end != tip_height { + if subtree_data.end_height != tip_height { return None; } @@ -401,7 +401,7 @@ impl ZebraDb { ) = self.db.zs_last_key_value(&orchard_subtrees)?; let tip_height = self.finalized_tip_height()?; - if subtree_data.end != tip_height { + if subtree_data.end_height != tip_height { return None; } diff --git a/zebra-state/src/service/non_finalized_state/chain.rs b/zebra-state/src/service/non_finalized_state/chain.rs index e4ccf9c30..fc3160587 100644 --- a/zebra-state/src/service/non_finalized_state/chain.rs +++ b/zebra-state/src/service/non_finalized_state/chain.rs @@ -694,7 +694,7 @@ impl Chain { self.sapling_subtrees .iter() - .find(|(_index, subtree)| subtree.end == height) + .find(|(_index, subtree)| subtree.end_height == height) .map(|(index, subtree)| subtree.with_index(*index)) } @@ -898,7 +898,7 @@ impl Chain { self.orchard_subtrees .iter() - .find(|(_index, subtree)| subtree.end == height) + .find(|(_index, subtree)| subtree.end_height == height) .map(|(index, subtree)| subtree.with_index(*index)) } diff --git a/zebra-state/src/service/read/tests/vectors.rs b/zebra-state/src/service/read/tests/vectors.rs index 3ff14cb42..54d668d2f 100644 --- a/zebra-state/src/service/read/tests/vectors.rs +++ b/zebra-state/src/service/read/tests/vectors.rs @@ -179,7 +179,10 @@ async fn test_read_subtrees() -> Result<()> { .pop_first() .expect("chain_subtrees should not be empty"); assert_eq!(first_chain_index, index, "subtree indexes should match"); - assert_eq!(end_height, subtree.end, "subtree end heights should match"); + assert_eq!( + end_height, subtree.end_height, + "subtree end heights should match" + ); // Check that Zebra retrieves subtrees correctly when using a range with an Excluded start bound