Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename EqHash to MastNodeFingerprint #1539

Merged
merged 5 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
47 changes: 25 additions & 22 deletions assembly/src/assembler/mast_forest_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -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<RpoDigest, GlobalProcedureIndex>,
/// A map of MAST node eq hashes to their corresponding positions in the MAST forest.
node_id_by_hash: BTreeMap<EqHash, MastNodeId>,
/// 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<MastNodeId, EqHash>,
/// A map of decorator hashes to their corresponding positions in the MAST forest.
decorator_id_by_hash: BTreeMap<Blake3Digest<32>, DecoratorId>,
/// A map of MAST node fingerprints to their corresponding positions in the MAST forest.
node_id_by_fingerprint: BTreeMap<MastNodeFingerprint, MastNodeId>,
/// The reverse mapping of `node_id_by_fingerprint`. This map caches the fingerprints of all
/// nodes (for performance reasons).
hash_by_node_id: BTreeMap<MastNodeId, MastNodeFingerprint>,
/// A map of decorator fingerprints to their corresponding positions in the MAST forest.
decorator_id_by_fingerprint: BTreeMap<DecoratorFingerprint, DecoratorId>,
/// 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.
Expand Down Expand Up @@ -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<DecoratorId, AssemblyError> {
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)
}
Expand All @@ -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<MastNodeId, AssemblyError> {
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)
}
Expand Down Expand Up @@ -433,21 +435,22 @@ impl MastForestBuilder {
pub fn set_before_enter(&mut self, node_id: MastNodeId, decorator_ids: Vec<DecoratorId>) {
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<DecoratorId>) {
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)
.expect("hash_by_node_id should contain the fingerprints of all children of `node`")
}
}

Expand Down
35 changes: 21 additions & 14 deletions core/src/mast/merger/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand All @@ -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<EqHash, MastNodeId>,
hash_by_node_id: BTreeMap<MastNodeId, EqHash>,
node_id_by_hash: BTreeMap<MastNodeFingerprint, MastNodeId>,
hash_by_node_id: BTreeMap<MastNodeId, MastNodeFingerprint>,
decorators_by_hash: BTreeMap<Blake3Digest<32>, DecoratorId>,
/// Mappings from old decorator and node ids to their new ids.
///
Expand Down Expand Up @@ -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)
{
Expand All @@ -168,21 +169,27 @@ 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
// that all children of this `node` have been processed before this node and
// 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,
)
.expect(
"hash_by_node_id should contain the fingerprints of all children of `remapped_node`",
);

match self.lookup_node_by_fingerprint(&node_fingerprint) {
Some(matching_node_id) => {
Expand All @@ -197,7 +204,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.
Expand Down Expand Up @@ -314,7 +321,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<MastNodeId> {
fn lookup_node_by_fingerprint(&self, fingerprint: &MastNodeFingerprint) -> Option<MastNodeId> {
self.node_id_by_hash.get(fingerprint).copied()
}
}
Expand Down
16 changes: 8 additions & 8 deletions core/src/mast/merger/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
Loading
Loading