From 1902aae03ca19c321eb7961a02112ad68a6260d6 Mon Sep 17 00:00:00 2001 From: Philipp Gackstatter Date: Mon, 21 Oct 2024 15:28:42 +0200 Subject: [PATCH 1/5] feat(core): Rename `EqHash` -> `MastNodeFingerprint` --- assembly/src/assembler/mast_forest_builder.rs | 46 +++-- core/src/mast/merger/mod.rs | 32 +-- core/src/mast/merger/tests.rs | 16 +- core/src/mast/mod.rs | 169 +--------------- core/src/mast/node_fingerprint.rs | 190 ++++++++++++++++++ core/src/operations/decorators/mod.rs | 6 +- 6 files changed, 247 insertions(+), 212 deletions(-) create mode 100644 core/src/mast/node_fingerprint.rs diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index 72139b1a0..7e899c212 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -5,8 +5,10 @@ use alloc::{ use core::ops::{Index, IndexMut}; use vm_core::{ - crypto::hash::{Blake3Digest, RpoDigest}, - mast::{DecoratorId, EqHash, MastForest, MastNode, MastNodeId}, + crypto::hash::RpoDigest, + mast::{ + DecoratorFingerprint, DecoratorId, MastForest, MastNode, MastNodeFingerprint, MastNodeId, + }, Decorator, DecoratorList, Operation, }; @@ -46,13 +48,13 @@ pub struct MastForestBuilder { /// map, this map contains only the first inserted procedure for procedures with the same MAST /// root. proc_gid_by_mast_root: BTreeMap, - /// A map of MAST node eq hashes to their corresponding positions in the MAST forest. - node_id_by_hash: BTreeMap, - /// The reverse mapping of `node_id_by_hash`. This map caches the eq hashes of all nodes (for - /// performance reasons). - hash_by_node_id: BTreeMap, - /// A map of decorator hashes to their corresponding positions in the MAST forest. - decorator_id_by_hash: BTreeMap, DecoratorId>, + /// A map of MAST node fingerprints to their corresponding positions in the MAST forest. + node_id_by_fingerprint: BTreeMap, + /// The reverse mapping of `node_id_by_fingerprint`. This map caches the fingerprints of all + /// nodes (for performance reasons). + hash_by_node_id: BTreeMap, + /// A map of decorator fingerprints to their corresponding positions in the MAST forest. + decorator_id_by_fingerprint: BTreeMap, /// A set of IDs for basic blocks which have been merged into a bigger basic blocks. This is /// used as a candidate set of nodes that may be eliminated if the are not referenced by any /// other node in the forest and are not a root of any procedure. @@ -339,14 +341,14 @@ impl MastForestBuilder { impl MastForestBuilder { /// Adds a decorator to the forest, and returns the [`Decorator`] associated with it. pub fn ensure_decorator(&mut self, decorator: Decorator) -> Result { - let decorator_hash = decorator.eq_hash(); + let decorator_hash = decorator.fingerprint(); - if let Some(decorator_id) = self.decorator_id_by_hash.get(&decorator_hash) { + if let Some(decorator_id) = self.decorator_id_by_fingerprint.get(&decorator_hash) { // decorator already exists in the forest; return previously assigned id Ok(*decorator_id) } else { let new_decorator_id = self.mast_forest.add_decorator(decorator)?; - self.decorator_id_by_hash.insert(decorator_hash, new_decorator_id); + self.decorator_id_by_fingerprint.insert(decorator_hash, new_decorator_id); Ok(new_decorator_id) } @@ -358,15 +360,15 @@ impl MastForestBuilder { /// MAST forest; two nodes that have the same MAST root and decorators will have the same /// [`MastNodeId`]. pub fn ensure_node(&mut self, node: MastNode) -> Result { - let node_hash = self.eq_hash_for_node(&node); + let node_fingerprint = self.fingerprint_for_node(&node); - if let Some(node_id) = self.node_id_by_hash.get(&node_hash) { + if let Some(node_id) = self.node_id_by_fingerprint.get(&node_fingerprint) { // node already exists in the forest; return previously assigned id Ok(*node_id) } else { let new_node_id = self.mast_forest.add_node(node)?; - self.node_id_by_hash.insert(node_hash, new_node_id); - self.hash_by_node_id.insert(new_node_id, node_hash); + self.node_id_by_fingerprint.insert(node_fingerprint, new_node_id); + self.hash_by_node_id.insert(new_node_id, node_fingerprint); Ok(new_node_id) } @@ -433,21 +435,21 @@ impl MastForestBuilder { pub fn set_before_enter(&mut self, node_id: MastNodeId, decorator_ids: Vec) { self.mast_forest[node_id].set_before_enter(decorator_ids); - let new_node_hash = self.eq_hash_for_node(&self[node_id]); - self.hash_by_node_id.insert(node_id, new_node_hash); + let new_node_fingerprint = self.fingerprint_for_node(&self[node_id]); + self.hash_by_node_id.insert(node_id, new_node_fingerprint); } pub fn set_after_exit(&mut self, node_id: MastNodeId, decorator_ids: Vec) { self.mast_forest[node_id].set_after_exit(decorator_ids); - let new_node_hash = self.eq_hash_for_node(&self[node_id]); - self.hash_by_node_id.insert(node_id, new_node_hash); + let new_node_fingerprint = self.fingerprint_for_node(&self[node_id]); + self.hash_by_node_id.insert(node_id, new_node_fingerprint); } } impl MastForestBuilder { - fn eq_hash_for_node(&self, node: &MastNode) -> EqHash { - EqHash::from_mast_node(&self.mast_forest, &self.hash_by_node_id, node) + fn fingerprint_for_node(&self, node: &MastNode) -> MastNodeFingerprint { + MastNodeFingerprint::from_mast_node(&self.mast_forest, &self.hash_by_node_id, node) } } diff --git a/core/src/mast/merger/mod.rs b/core/src/mast/merger/mod.rs index d8e522dc0..ae5e8cacc 100644 --- a/core/src/mast/merger/mod.rs +++ b/core/src/mast/merger/mod.rs @@ -3,7 +3,7 @@ use alloc::{collections::BTreeMap, vec::Vec}; use miden_crypto::hash::blake::Blake3Digest; use crate::mast::{ - DecoratorId, EqHash, MastForest, MastForestError, MastNode, MastNodeId, + DecoratorId, MastForest, MastForestError, MastNode, MastNodeFingerprint, MastNodeId, MultiMastForestIteratorItem, MultiMastForestNodeIter, }; @@ -15,12 +15,13 @@ mod tests; /// This functionality is exposed via [`MastForest::merge`]. See its documentation for more details. pub(crate) struct MastForestMerger { mast_forest: MastForest, - // Internal indices needed for efficient duplicate checking and EqHash computation. + // Internal indices needed for efficient duplicate checking and MastNodeFingerprint + // computation. // // These are always in-sync with the nodes in `mast_forest`, i.e. all nodes added to the // `mast_forest` are also added to the indices. - node_id_by_hash: BTreeMap, - hash_by_node_id: BTreeMap, + node_id_by_hash: BTreeMap, + hash_by_node_id: BTreeMap, decorators_by_hash: BTreeMap, DecoratorId>, /// Mappings from old decorator and node ids to their new ids. /// @@ -142,7 +143,7 @@ impl MastForestMerger { let mut decorator_id_remapping = DecoratorIdMap::new(other_forest.decorators.len()); for (merging_id, merging_decorator) in other_forest.decorators.iter().enumerate() { - let merging_decorator_hash = merging_decorator.eq_hash(); + let merging_decorator_hash = merging_decorator.fingerprint(); let new_decorator_id = if let Some(existing_decorator) = self.decorators_by_hash.get(&merging_decorator_hash) { @@ -168,12 +169,12 @@ impl MastForestMerger { merging_id: MastNodeId, node: &MastNode, ) -> Result<(), MastForestError> { - // We need to remap the node prior to computing the EqHash. + // We need to remap the node prior to computing the MastNodeFingerprint. // - // This is because the EqHash computation looks up its descendants and decorators in - // the internal index, and if we were to pass the original node to that - // computation, it would look up the incorrect descendants and decorators (since the - // descendant's indices may have changed). + // This is because the MastNodeFingerprint computation looks up its descendants and + // decorators in the internal index, and if we were to pass the original node to + // that computation, it would look up the incorrect descendants and decorators + // (since the descendant's indices may have changed). // // Remapping at this point is guaranteed to be "complete", meaning all ids of children // will be present in the node id mapping since the DFS iteration guarantees @@ -181,8 +182,11 @@ impl MastForestMerger { // their indices have been added to the mappings. let remapped_node = self.remap_node(forest_idx, node)?; - let node_fingerprint = - EqHash::from_mast_node(&self.mast_forest, &self.hash_by_node_id, &remapped_node); + let node_fingerprint = MastNodeFingerprint::from_mast_node( + &self.mast_forest, + &self.hash_by_node_id, + &remapped_node, + ); match self.lookup_node_by_fingerprint(&node_fingerprint) { Some(matching_node_id) => { @@ -197,7 +201,7 @@ impl MastForestMerger { self.node_id_mappings[forest_idx].insert(merging_id, new_node_id); // We need to update the indices with the newly inserted nodes - // since the EqHash computation requires all descendants of a node + // since the MastNodeFingerprint computation requires all descendants of a node // to be in this index. Hence when we encounter a node in the merging forest // which has descendants (Call, Loop, Split, ...), then their descendants need to be // in the indices. @@ -314,7 +318,7 @@ impl MastForestMerger { // ================================================================================================ /// Returns a slice of nodes in the merged forest which have the given `mast_root`. - fn lookup_node_by_fingerprint(&self, fingerprint: &EqHash) -> Option { + fn lookup_node_by_fingerprint(&self, fingerprint: &MastNodeFingerprint) -> Option { self.node_id_by_hash.get(fingerprint).copied() } } diff --git a/core/src/mast/merger/tests.rs b/core/src/mast/merger/tests.rs index 881e22c2c..b33ae9729 100644 --- a/core/src/mast/merger/tests.rs +++ b/core/src/mast/merger/tests.rs @@ -484,12 +484,12 @@ fn mast_forest_merge_external_node_reference_with_decorator() { .enumerate() { let id_foo_a_fingerprint = - EqHash::from_mast_node(&forest_a, &BTreeMap::new(), &forest_a[id_foo_a]); + MastNodeFingerprint::from_mast_node(&forest_a, &BTreeMap::new(), &forest_a[id_foo_a]); let fingerprints: Vec<_> = merged .nodes() .iter() - .map(|node| EqHash::from_mast_node(&merged, &BTreeMap::new(), node)) + .map(|node| MastNodeFingerprint::from_mast_node(&merged, &BTreeMap::new(), node)) .collect(); assert_eq!(merged.nodes.len(), 1); @@ -552,12 +552,12 @@ fn mast_forest_merge_external_node_with_decorator() { assert_eq!(merged.nodes.len(), 1); let id_foo_b_fingerprint = - EqHash::from_mast_node(&forest_a, &BTreeMap::new(), &forest_b[id_foo_b]); + MastNodeFingerprint::from_mast_node(&forest_a, &BTreeMap::new(), &forest_b[id_foo_b]); let fingerprints: Vec<_> = merged .nodes() .iter() - .map(|node| EqHash::from_mast_node(&merged, &BTreeMap::new(), node)) + .map(|node| MastNodeFingerprint::from_mast_node(&merged, &BTreeMap::new(), node)) .collect(); // Block foo should be unmodified. @@ -622,12 +622,12 @@ fn mast_forest_merge_external_node_and_referenced_node_have_decorators() { assert_eq!(merged.nodes.len(), 1); let id_foo_b_fingerprint = - EqHash::from_mast_node(&forest_b, &BTreeMap::new(), &forest_b[id_foo_b]); + MastNodeFingerprint::from_mast_node(&forest_b, &BTreeMap::new(), &forest_b[id_foo_b]); let fingerprints: Vec<_> = merged .nodes() .iter() - .map(|node| EqHash::from_mast_node(&merged, &BTreeMap::new(), node)) + .map(|node| MastNodeFingerprint::from_mast_node(&merged, &BTreeMap::new(), node)) .collect(); // Block foo should be unmodified. @@ -700,12 +700,12 @@ fn mast_forest_merge_multiple_external_nodes_with_decorator() { assert_eq!(merged.nodes.len(), 1); let id_foo_b_fingerprint = - EqHash::from_mast_node(&forest_a, &BTreeMap::new(), &forest_b[id_foo_b]); + MastNodeFingerprint::from_mast_node(&forest_a, &BTreeMap::new(), &forest_b[id_foo_b]); let fingerprints: Vec<_> = merged .nodes() .iter() - .map(|node| EqHash::from_mast_node(&merged, &BTreeMap::new(), node)) + .map(|node| MastNodeFingerprint::from_mast_node(&merged, &BTreeMap::new(), node)) .collect(); // Block foo should be unmodified. diff --git a/core/src/mast/mod.rs b/core/src/mast/mod.rs index a105ae996..062898da0 100644 --- a/core/src/mast/mod.rs +++ b/core/src/mast/mod.rs @@ -7,11 +7,7 @@ use core::{ ops::{Index, IndexMut}, }; -use miden_crypto::hash::{ - blake::{Blake3Digest, Blake3_256}, - rpo::RpoDigest, - Digest, -}; +use miden_crypto::hash::rpo::RpoDigest; mod node; pub use node::{ @@ -31,6 +27,9 @@ pub use merger::MastForestRootMap; mod multi_forest_node_iterator; pub(crate) use multi_forest_node_iterator::*; +mod node_fingerprint; +pub use node_fingerprint::{DecoratorFingerprint, MastNodeFingerprint}; + #[cfg(test)] mod tests; @@ -642,166 +641,6 @@ impl Serializable for DecoratorId { } } -// MAST NODE EQUALITY -// ================================================================================================ - -/// Represents the hash used to test for equality between [`MastNode`]s. -/// -/// The decorator root will be `None` if and only if there are no decorators attached to the node, -/// and all children have no decorator roots (meaning that there are no decorators in all the -/// descendants). -#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] -pub struct EqHash { - mast_root: RpoDigest, - decorator_root: Option>, -} - -// TODO: Document public functions and assumptions about forest and the index map. -impl EqHash { - pub fn new(mast_root: RpoDigest) -> Self { - Self { mast_root, decorator_root: None } - } - - pub fn with_decorator_root(mast_root: RpoDigest, decorator_root: Blake3Digest<32>) -> Self { - Self { - mast_root, - decorator_root: Some(decorator_root), - } - } - - pub fn from_mast_node( - forest: &MastForest, - hash_by_node_id: &BTreeMap, - node: &MastNode, - ) -> EqHash { - match node { - MastNode::Block(node) => { - let mut bytes_to_hash = Vec::new(); - - for &(idx, decorator_id) in node.decorators() { - bytes_to_hash.extend(idx.to_le_bytes()); - bytes_to_hash.extend(forest[decorator_id].eq_hash().as_bytes()); - } - - // Add any `Assert` or `U32assert2` opcodes present, since these are not included in - // the MAST root. - for (op_idx, op) in node.operations().enumerate() { - if let Operation::U32assert2(inner_value) - | Operation::Assert(inner_value) - | Operation::MpVerify(inner_value) = op - { - let op_idx: u32 = op_idx - .try_into() - .expect("there are more than 2^{32}-1 operations in basic block"); - - // we include the opcode to differentiate between `Assert` and `U32assert2` - bytes_to_hash.push(op.op_code()); - // we include the operation index to distinguish between basic blocks that - // would have the same assert instructions, but in a different order - bytes_to_hash.extend(op_idx.to_le_bytes()); - bytes_to_hash.extend(inner_value.to_le_bytes()); - } - } - - if bytes_to_hash.is_empty() { - EqHash::new(node.digest()) - } else { - let decorator_root = Blake3_256::hash(&bytes_to_hash); - EqHash::with_decorator_root(node.digest(), decorator_root) - } - }, - MastNode::Join(node) => eq_hash_from_parts( - forest, - hash_by_node_id, - node.before_enter(), - node.after_exit(), - &[node.first(), node.second()], - node.digest(), - ), - MastNode::Split(node) => eq_hash_from_parts( - forest, - hash_by_node_id, - node.before_enter(), - node.after_exit(), - &[node.on_true(), node.on_false()], - node.digest(), - ), - MastNode::Loop(node) => eq_hash_from_parts( - forest, - hash_by_node_id, - node.before_enter(), - node.after_exit(), - &[node.body()], - node.digest(), - ), - MastNode::Call(node) => eq_hash_from_parts( - forest, - hash_by_node_id, - node.before_enter(), - node.after_exit(), - &[node.callee()], - node.digest(), - ), - MastNode::Dyn(node) => eq_hash_from_parts( - forest, - hash_by_node_id, - node.before_enter(), - node.after_exit(), - &[], - node.digest(), - ), - MastNode::External(node) => eq_hash_from_parts( - forest, - hash_by_node_id, - node.before_enter(), - node.after_exit(), - &[], - node.digest(), - ), - } - } -} - -fn eq_hash_from_parts( - forest: &MastForest, - hash_by_node_id: &BTreeMap, - before_enter_ids: &[DecoratorId], - after_exit_ids: &[DecoratorId], - children_ids: &[MastNodeId], - node_digest: RpoDigest, -) -> EqHash { - let pre_decorator_hash_bytes = - before_enter_ids.iter().flat_map(|&id| forest[id].eq_hash().as_bytes()); - let post_decorator_hash_bytes = - after_exit_ids.iter().flat_map(|&id| forest[id].eq_hash().as_bytes()); - - // Reminder: the `EqHash`'s decorator root will be `None` if and only if there are no - // decorators attached to the node, and all children have no decorator roots (meaning that - // there are no decorators in all the descendants). - if pre_decorator_hash_bytes.clone().next().is_none() - && post_decorator_hash_bytes.clone().next().is_none() - && children_ids - .iter() - .filter_map(|child_id| hash_by_node_id[child_id].decorator_root) - .next() - .is_none() - { - EqHash::new(node_digest) - } else { - let children_decorator_roots = children_ids - .iter() - .filter_map(|child_id| hash_by_node_id[child_id].decorator_root) - .flat_map(|decorator_root| decorator_root.as_bytes()); - let decorator_bytes_to_hash: Vec = pre_decorator_hash_bytes - .chain(post_decorator_hash_bytes) - .chain(children_decorator_roots) - .collect(); - - let decorator_root = Blake3_256::hash(&decorator_bytes_to_hash); - EqHash::with_decorator_root(node_digest, decorator_root) - } -} - // MAST FOREST ERROR // ================================================================================================ diff --git a/core/src/mast/node_fingerprint.rs b/core/src/mast/node_fingerprint.rs new file mode 100644 index 000000000..54fd35280 --- /dev/null +++ b/core/src/mast/node_fingerprint.rs @@ -0,0 +1,190 @@ +use alloc::{collections::BTreeMap, vec::Vec}; + +use miden_crypto::hash::{ + blake::{Blake3Digest, Blake3_256}, + rpo::RpoDigest, + Digest, +}; + +use crate::{ + mast::{DecoratorId, MastForest, MastNode, MastNodeId}, + Operation, +}; + +// MAST NODE EQUALITY +// ================================================================================================ + +pub type DecoratorFingerprint = Blake3Digest<32>; + +/// Represents the hash used to test for equality between [`MastNode`]s. +/// +/// The decorator root will be `None` if and only if there are no decorators attached to the node, +/// and all children have no decorator roots (meaning that there are no decorators in all the +/// descendants). +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub struct MastNodeFingerprint { + mast_root: RpoDigest, + decorator_root: Option, +} + +// ------------------------------------------------------------------------------------------------ +/// Constructors +impl MastNodeFingerprint { + /// Creates a new [`MastNodeFingerprint`] from the given MAST root with an empty decorator root. + pub fn new(mast_root: RpoDigest) -> Self { + Self { mast_root, decorator_root: None } + } + + /// Creates a new [`MastNodeFingerprint`] from the given MAST root and the given + /// [`DecoratorFingerprint`]. + pub fn with_decorator_root(mast_root: RpoDigest, decorator_root: DecoratorFingerprint) -> Self { + Self { + mast_root, + decorator_root: Some(decorator_root), + } + } + + /// Creates a [`MastNodeFingerprint`] from a [`MastNode`]. + /// + /// The `hash_by_node_id` map must contain all children of the node for efficient lookup of + /// their fingerprints. + pub fn from_mast_node( + forest: &MastForest, + hash_by_node_id: &BTreeMap, + node: &MastNode, + ) -> MastNodeFingerprint { + match node { + MastNode::Block(node) => { + let mut bytes_to_hash = Vec::new(); + + for &(idx, decorator_id) in node.decorators() { + bytes_to_hash.extend(idx.to_le_bytes()); + bytes_to_hash.extend(forest[decorator_id].fingerprint().as_bytes()); + } + + // Add any `Assert` or `U32assert2` opcodes present, since these are not included in + // the MAST root. + for (op_idx, op) in node.operations().enumerate() { + if let Operation::U32assert2(inner_value) + | Operation::Assert(inner_value) + | Operation::MpVerify(inner_value) = op + { + let op_idx: u32 = op_idx + .try_into() + .expect("there are more than 2^{32}-1 operations in basic block"); + + // we include the opcode to differentiate between `Assert` and `U32assert2` + bytes_to_hash.push(op.op_code()); + // we include the operation index to distinguish between basic blocks that + // would have the same assert instructions, but in a different order + bytes_to_hash.extend(op_idx.to_le_bytes()); + bytes_to_hash.extend(inner_value.to_le_bytes()); + } + } + + if bytes_to_hash.is_empty() { + MastNodeFingerprint::new(node.digest()) + } else { + let decorator_root = Blake3_256::hash(&bytes_to_hash); + MastNodeFingerprint::with_decorator_root(node.digest(), decorator_root) + } + }, + MastNode::Join(node) => fingerprint_from_parts( + forest, + hash_by_node_id, + node.before_enter(), + node.after_exit(), + &[node.first(), node.second()], + node.digest(), + ), + MastNode::Split(node) => fingerprint_from_parts( + forest, + hash_by_node_id, + node.before_enter(), + node.after_exit(), + &[node.on_true(), node.on_false()], + node.digest(), + ), + MastNode::Loop(node) => fingerprint_from_parts( + forest, + hash_by_node_id, + node.before_enter(), + node.after_exit(), + &[node.body()], + node.digest(), + ), + MastNode::Call(node) => fingerprint_from_parts( + forest, + hash_by_node_id, + node.before_enter(), + node.after_exit(), + &[node.callee()], + node.digest(), + ), + MastNode::Dyn(node) => fingerprint_from_parts( + forest, + hash_by_node_id, + node.before_enter(), + node.after_exit(), + &[], + node.digest(), + ), + MastNode::External(node) => fingerprint_from_parts( + forest, + hash_by_node_id, + node.before_enter(), + node.after_exit(), + &[], + node.digest(), + ), + } + } +} + +// ------------------------------------------------------------------------------------------------ +/// Accessors +impl MastNodeFingerprint { + pub fn mast_root(&self) -> &RpoDigest { + &self.mast_root + } +} + +fn fingerprint_from_parts( + forest: &MastForest, + hash_by_node_id: &BTreeMap, + before_enter_ids: &[DecoratorId], + after_exit_ids: &[DecoratorId], + children_ids: &[MastNodeId], + node_digest: RpoDigest, +) -> MastNodeFingerprint { + let pre_decorator_hash_bytes = + before_enter_ids.iter().flat_map(|&id| forest[id].fingerprint().as_bytes()); + let post_decorator_hash_bytes = + after_exit_ids.iter().flat_map(|&id| forest[id].fingerprint().as_bytes()); + + // Reminder: the `MastNodeFingerprint`'s decorator root will be `None` if and only if there are + // no decorators attached to the node, and all children have no decorator roots (meaning + // that there are no decorators in all the descendants). + if pre_decorator_hash_bytes.clone().next().is_none() + && post_decorator_hash_bytes.clone().next().is_none() + && children_ids + .iter() + .filter_map(|child_id| hash_by_node_id[child_id].decorator_root) + .next() + .is_none() + { + MastNodeFingerprint::new(node_digest) + } else { + let children_decorator_roots = children_ids + .iter() + .filter_map(|child_id| hash_by_node_id[child_id].decorator_root) + .flat_map(|decorator_root| decorator_root.as_bytes()); + let decorator_bytes_to_hash: Vec = pre_decorator_hash_bytes + .chain(post_decorator_hash_bytes) + .chain(children_decorator_roots) + .collect(); + + let decorator_root = Blake3_256::hash(&decorator_bytes_to_hash); + MastNodeFingerprint::with_decorator_root(node_digest, decorator_root) + } +} diff --git a/core/src/operations/decorators/mod.rs b/core/src/operations/decorators/mod.rs index ecfac643d..02e054eb0 100644 --- a/core/src/operations/decorators/mod.rs +++ b/core/src/operations/decorators/mod.rs @@ -1,7 +1,7 @@ use alloc::{string::ToString, vec::Vec}; use core::fmt; -use miden_crypto::hash::blake::{Blake3Digest, Blake3_256}; +use miden_crypto::hash::blake::Blake3_256; use num_traits::ToBytes; mod advice; @@ -13,7 +13,7 @@ pub use assembly_op::AssemblyOp; mod debug; pub use debug::DebugOptions; -use crate::mast::DecoratorId; +use crate::mast::{DecoratorFingerprint, DecoratorId}; // DECORATORS // ================================================================================================ @@ -40,7 +40,7 @@ pub enum Decorator { } impl Decorator { - pub fn eq_hash(&self) -> Blake3Digest<32> { + pub fn fingerprint(&self) -> DecoratorFingerprint { match self { Self::Advice(advice) => Blake3_256::hash(advice.to_string().as_bytes()), Self::AsmOp(asm_op) => { From 23cc4072a8e7fa193355fa7a9709b66452ec1ad6 Mon Sep 17 00:00:00 2001 From: Philipp Gackstatter Date: Mon, 28 Oct 2024 09:54:38 +0100 Subject: [PATCH 2/5] chore(core): Document panic in fingerprint computation --- core/src/mast/node_fingerprint.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/mast/node_fingerprint.rs b/core/src/mast/node_fingerprint.rs index 54fd35280..106c5d946 100644 --- a/core/src/mast/node_fingerprint.rs +++ b/core/src/mast/node_fingerprint.rs @@ -46,8 +46,10 @@ impl MastNodeFingerprint { /// Creates a [`MastNodeFingerprint`] from a [`MastNode`]. /// + /// # Panics + /// /// The `hash_by_node_id` map must contain all children of the node for efficient lookup of - /// their fingerprints. + /// their fingerprints. This function panics if a child of the given `node` is not in this map. pub fn from_mast_node( forest: &MastForest, hash_by_node_id: &BTreeMap, From 19ae6bf376bc1faf7dc8b0ab31cc1fbbd61bfa87 Mon Sep 17 00:00:00 2001 From: Philipp Gackstatter Date: Mon, 28 Oct 2024 09:58:22 +0100 Subject: [PATCH 3/5] chore(core): Add `MpVerify` to comments in fingerprint computation --- core/src/mast/node_fingerprint.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/mast/node_fingerprint.rs b/core/src/mast/node_fingerprint.rs index 106c5d946..00b2d617f 100644 --- a/core/src/mast/node_fingerprint.rs +++ b/core/src/mast/node_fingerprint.rs @@ -64,8 +64,8 @@ impl MastNodeFingerprint { bytes_to_hash.extend(forest[decorator_id].fingerprint().as_bytes()); } - // Add any `Assert` or `U32assert2` opcodes present, since these are not included in - // the MAST root. + // Add any `Assert`, `U32assert2` and `MpVerify` opcodes present, since these are + // not included in the MAST root. for (op_idx, op) in node.operations().enumerate() { if let Operation::U32assert2(inner_value) | Operation::Assert(inner_value) From 16475fb99afce4772145c1cafcfcae437b90a946 Mon Sep 17 00:00:00 2001 From: Philipp Gackstatter Date: Mon, 28 Oct 2024 10:46:39 +0100 Subject: [PATCH 4/5] feat(core): Error instead of panic in fingerprint computation --- assembly/src/assembler/mast_forest_builder.rs | 1 + core/src/mast/merger/mod.rs | 3 ++ core/src/mast/mod.rs | 2 + core/src/mast/node_fingerprint.rs | 46 +++++++++++-------- 4 files changed, 32 insertions(+), 20 deletions(-) diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index 7e899c212..56de6f939 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -450,6 +450,7 @@ impl MastForestBuilder { impl MastForestBuilder { fn fingerprint_for_node(&self, node: &MastNode) -> MastNodeFingerprint { MastNodeFingerprint::from_mast_node(&self.mast_forest, &self.hash_by_node_id, node) + .expect("hash_by_node_id should contain the fingerprints of all children of `node`") } } diff --git a/core/src/mast/merger/mod.rs b/core/src/mast/merger/mod.rs index ae5e8cacc..ae0273371 100644 --- a/core/src/mast/merger/mod.rs +++ b/core/src/mast/merger/mod.rs @@ -186,6 +186,9 @@ impl MastForestMerger { &self.mast_forest, &self.hash_by_node_id, &remapped_node, + ) + .expect( + "hash_by_node_id should contain the fingerprints of all children of `remapped_node`", ); match self.lookup_node_by_fingerprint(&node_fingerprint) { diff --git a/core/src/mast/mod.rs b/core/src/mast/mod.rs index 062898da0..5185bc3cd 100644 --- a/core/src/mast/mod.rs +++ b/core/src/mast/mod.rs @@ -663,4 +663,6 @@ pub enum MastForestError { DecoratorIdOverflow(DecoratorId, usize), #[error("basic block cannot be created from an empty list of operations")] EmptyBasicBlock, + #[error("decorator root of child with node id {0} is missing but required for fingerprint computation")] + ChildFingerprintMissing(MastNodeId), } diff --git a/core/src/mast/node_fingerprint.rs b/core/src/mast/node_fingerprint.rs index 00b2d617f..6bf41a2ce 100644 --- a/core/src/mast/node_fingerprint.rs +++ b/core/src/mast/node_fingerprint.rs @@ -7,7 +7,7 @@ use miden_crypto::hash::{ }; use crate::{ - mast::{DecoratorId, MastForest, MastNode, MastNodeId}, + mast::{DecoratorId, MastForest, MastForestError, MastNode, MastNodeId}, Operation, }; @@ -46,15 +46,14 @@ impl MastNodeFingerprint { /// Creates a [`MastNodeFingerprint`] from a [`MastNode`]. /// - /// # Panics - /// /// The `hash_by_node_id` map must contain all children of the node for efficient lookup of - /// their fingerprints. This function panics if a child of the given `node` is not in this map. + /// their fingerprints. This function returns an error if a child of the given `node` is not in + /// this map. pub fn from_mast_node( forest: &MastForest, hash_by_node_id: &BTreeMap, node: &MastNode, - ) -> MastNodeFingerprint { + ) -> Result { match node { MastNode::Block(node) => { let mut bytes_to_hash = Vec::new(); @@ -85,10 +84,10 @@ impl MastNodeFingerprint { } if bytes_to_hash.is_empty() { - MastNodeFingerprint::new(node.digest()) + Ok(MastNodeFingerprint::new(node.digest())) } else { let decorator_root = Blake3_256::hash(&bytes_to_hash); - MastNodeFingerprint::with_decorator_root(node.digest(), decorator_root) + Ok(MastNodeFingerprint::with_decorator_root(node.digest(), decorator_root)) } }, MastNode::Join(node) => fingerprint_from_parts( @@ -158,35 +157,42 @@ fn fingerprint_from_parts( after_exit_ids: &[DecoratorId], children_ids: &[MastNodeId], node_digest: RpoDigest, -) -> MastNodeFingerprint { +) -> Result { let pre_decorator_hash_bytes = before_enter_ids.iter().flat_map(|&id| forest[id].fingerprint().as_bytes()); let post_decorator_hash_bytes = after_exit_ids.iter().flat_map(|&id| forest[id].fingerprint().as_bytes()); + let children_decorator_roots = children_ids + .iter() + .filter_map(|child_id| { + hash_by_node_id + .get(child_id) + .ok_or(MastForestError::ChildFingerprintMissing(*child_id)) + .map(|child_fingerprint| child_fingerprint.decorator_root) + .transpose() + }) + .collect::, MastForestError>>()?; + // Reminder: the `MastNodeFingerprint`'s decorator root will be `None` if and only if there are // no decorators attached to the node, and all children have no decorator roots (meaning // that there are no decorators in all the descendants). if pre_decorator_hash_bytes.clone().next().is_none() && post_decorator_hash_bytes.clone().next().is_none() - && children_ids - .iter() - .filter_map(|child_id| hash_by_node_id[child_id].decorator_root) - .next() - .is_none() + && children_decorator_roots.is_empty() { - MastNodeFingerprint::new(node_digest) + Ok(MastNodeFingerprint::new(node_digest)) } else { - let children_decorator_roots = children_ids - .iter() - .filter_map(|child_id| hash_by_node_id[child_id].decorator_root) - .flat_map(|decorator_root| decorator_root.as_bytes()); let decorator_bytes_to_hash: Vec = pre_decorator_hash_bytes .chain(post_decorator_hash_bytes) - .chain(children_decorator_roots) + .chain( + children_decorator_roots + .into_iter() + .flat_map(|decorator_root| decorator_root.as_bytes()), + ) .collect(); let decorator_root = Blake3_256::hash(&decorator_bytes_to_hash); - MastNodeFingerprint::with_decorator_root(node_digest, decorator_root) + Ok(MastNodeFingerprint::with_decorator_root(node_digest, decorator_root)) } } From e50b5209e54e41efcb660373e34d611ebe15ba13 Mon Sep 17 00:00:00 2001 From: Philipp Gackstatter Date: Mon, 28 Oct 2024 16:22:56 +0100 Subject: [PATCH 5/5] chore: Add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1204ac13..f90220aaa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ - Migrated to new padding rule for RPO (#1343). - Migrated to `miden-crypto` v0.11.0 (#1343). - Implemented `MastForest` merging (#1534) +- Rename `EqHash` to `MastNodeFingerprint` and make it `pub` (#1539) #### Fixes