Bind MAST commitments to external dependency digests & advice map by huitseeker · Pull Request #3311 · 0xMiden/miden-vm

huitseeker · 2026-06-30T20:30:43Z

This makes MAST forest commitments include external dependency digests.

Before this change, two forests could have the same commitment even when their external dependencies were different. Now the dependency digests are part of the commitment input, so changing an external dependency changes the forest commitment.

This is stacked on #3294 and uses its sorted commitment input format.

bobbinth

Thank you! Looks good. Not a full review yet, but I left some comments which may have impact on the approach.

bobbinth · 2026-07-02T00:03:19Z

+    pub(in crate::mast) fn add_external_node(
+        &mut self,
+        node: MastNode,
+    ) -> Result<MastNodeId, MastForestError> {
+        debug_assert!(node.is_external());
+        let node_id = self.nodes.push(node).map_err(|_| MastForestError::TooManyNodes)?;
+        self.commitment = self.compute_mast_forest_commitment();
+        Ok(node_id)
+    }


Why do we need to add this method here? I think we should try to remove the existing mutators from MastForest rather than add new ones. I understand that adding it here doesn't change much because the same mutation was previously happening via MastForestContributor directly - but I think we should probably try to get rid of MastForestContributor at some point in the future to make sure direct mutations of MastForest are not possible.

We now makes finalized dense MastForest values canonical. External nodes come first and are strictly sorted by digest. Basic blocks follow. Internal nodes come last, with children before parents.

Finalization canonicalizes the MastForest node order. Serialization expects finalized forests, but still has a temporary fallback for construction-phase forests that were built through the low-level append API.

I think that's all?

MastForestContributor is still needed because it is the current common builder interface for adding all node types to a MastForest. It is used across core tests, merger code, package tests/serialization/arbitrary support, assembly finalization, and helper builders. We can make MastForestContributor operate on another structure, but I'd recommend another PR.

That other PR is #3329 now.

bobbinth · 2026-07-02T00:05:16Z

+fn compute_dependency_commitment(nodes: &IndexVec<MastNodeId, MastNode>) -> Word {
+    let mut digests: Vec<Word> = nodes
+        .iter()
+        .filter(|node| node.is_external())
+        .map(MastNodeExt::digest)
+        .collect();
+    digests.sort_unstable();
+    miden_crypto::hash::poseidon2::Poseidon2::merge_many(&digests)
+}


This is fine for now, but ideally, we'd avoid this need for iteration/sorting by making sure external nodes are already sorted and contiguous upon construction. Is this coming in a future PR?

The reader now treats dense order as part of the format. It checks the order while reading instead of sorting the payload into shape. The dependency digest section must match the external prefix exactly.

The function you're commenting on is used in construction-phase forests.

bobbinth

Looks good! Thank you. Still not a full review, but I left some more comments/questions inline.

bobbinth · 2026-07-03T00:08:47Z

+    miden_crypto::hash::poseidon2::Poseidon2::merge_many(&digests)
+}
+
+pub(in crate::mast) fn compute_advice_commitment(advice_map: &AdviceMap) -> Word {


This could be a method on the AdviceMap type, right?

I would also add a comment to this describing commitment methodology. I understand that we build a list of elements from concatenating key-value pairs (sorted by key) and than hash this list.

AdviceMap now owns commitment(), with a doc comment for the key-ordered entry hashing.

bobbinth · 2026-07-03T00:12:36Z

+            let value_len = values.len() as u64;
+            elements.push(Felt::from_u32(value_len as u32));
+            elements.push(Felt::from_u32((value_len >> 32) as u32));


I'm not sure if mixing the length into the hash is really needed (the overall length of the vector is used for one of the capacity elements in hash_elements() - but if we do want to include, we could probably safely assume that the length fits into a single field element since these are actual lengths of in-memory structures.

I removed the explicit length fields. Each entry is key || values; the key is fixed width, and hash_elements() already binds the element count.

bobbinth · 2026-07-03T00:29:26Z

            .push(ExternalNode { digest: self.digest }.into())
            .map_err(|_| MastForestError::TooManyNodes)?;
-
+        forest.commitment = forest.compute_mast_forest_commitment();


This is probably fine for now, but recomputing the entire commitment after every external node is added is probably pretty expensive. Do we need to create an issue to cover this or do we already have one?

#3311 keeps this "repair" path because that branch still has direct append construction. #3329 removes the production append trait path and makes dense finalization return a canonical MastForest, so this per-append commitment recompute is no longer part of normal construction.

bobbinth · 2026-07-03T00:32:52Z

This means that root digests are not sorted in the serialized representation, right?

Yes. In #3311, the serialized root digest commitment section is canonical. It must equal the procedure root digests sorted lexicographically.

That section is not the same as the serialized root ID list. The root ID list is written in forest root order. It tells the reader which node IDs are roots. It is not a digest commitment input.

#3294 is where these explicit commitment input sections were added to the wire format. It bumped the MAST wire version to [0, 0, 5] for "sorted root digest and sorted external digest commitment input sections". It writes the sorted root digest section and then the sorted external dependency digest section after the node digest data. It also made readers compare those serialized sections with the digests derived from the serialized forest. #3311 builds on that by using those sections as the canonical input lists for the forest commitment changes.

The root digest commitment section is written from sorted_root_digests(self), and sorted_root_digests() sorts those digests before writing them.

On read, validate_commitment_input_sections() derives the expected root digests from the root IDs, sorts that expected list, rejects duplicate root digests, and compares each serialized digest entry to the expected digest at the same index. The sort is for canonicalizing the expected list, not for checking that the original list was ordered. The validation constrains the wire section by equality against that canonical list. It is not a direct adjacent-pair check over the serialized section.

This aims to match your design requirements as I could gather them:

a comment in Issue #3067 asks for sorted root and dependency digest commitments.

PR #2765 asks for external nodes to precede internal nodes and be sorted in the MastForest.

Issue #3043 extends the in-memory order to external nodes, then basic blocks, then internal nodes, with children before parents.

PR #3055 says sorted wire digest sections are less useful if the in-memory forest order differs.

bobbinth · 2026-07-03T00:51:05Z

+    /// Cached commitment to the original MAST forest's roots, external dependencies, and advice
+    /// map.
    commitment_cache: Word,

    /// Sorted source root digests used as the forest commitment input.
    commitment_root_digests: Vec<Word>,

-    /// Sorted full external node digests represented in this sparse forest.
+    /// Sorted source external node digests used as the dependency commitment input.
    dependency_digests: Vec<Word>,


Question: why do we need to store these instead of using the MastForestCommitment struct?

Because a SparseMastForest is not the full input to the source forest commitment, but a replay view whose builder keeps original node IDs, copies only full visits, and stores some referenced nodes as digest-only entries, it must also carry the sorted root and dependency digest lists used by the source forest, so deserialization can recompute the same commitment and check the retained roots and full external nodes against those lists even though the sparse node set alone cannot recreate the dense forest's commitment input.

bobbinth · 2026-07-03T00:53:28Z

+        if self.validate_dense_node_order().is_err() {
+            // Construction-phase forests may still be append-ordered. Finalized forests take the
+            // direct path below; this fallback prevents public serialization from panicking before
+            // callers have moved through a finalization path.
+            let canonical = MastForest::from_raw_parts(
+                self.nodes.clone(),
+                self.roots.clone(),
+                self.advice_map.clone(),
+            )
+            .expect("dense MAST forest must be valid before serialization");
+            canonical.write_into_with_options(target, hashless);
+            return;


I'm not sure I follow the invariants here - my understanding is that once we have a MastForest struct, everything is guaranteed to be in the correct order. Is this not the case?

This is addressed in the stack. #3311 still repairs at the boundary because the branch still has direct append construction. #3329 makes finalized dense MastForest values canonical in memory, so serialization and dependency commitment can trust the external-node prefix.

bobbinth · 2026-07-03T01:02:32Z

Probably not for this PR, but this file has become quite big and difficult to review. We should probably split it up (e.g., test-related code could go into a separate module).

I would keep the broad file split out of #3311. #3329 moves the dense builder into core/src/mast/dense_builder.rs, but it does not split all of core/src/mast/mod.rs. This PR is already double the recommended size, but I'll stack a PR to make that change (on top of the sequence of #3313, #3294, #3311, #3329).

bobbinth · 2026-07-03T01:04:41Z

+    Ok(())
+}
+
+fn canonicalize_dense_parts(parts: MastForestParts) -> Result<MastForestParts, MastForestError> {


Could we add some doc comments explaining what this does? Also, would it make sense to put this as a method on MastForestParts?

#3329 makes the finalization API clearer by adding MastForest::from_raw_parts_with_id_map(), which documents that canonicalization returns an ID remap. I kept canonicalize_dense_parts() private under MastForest because it is an implementation step inside finalization. MastForestParts is still just the crate-local bag of fields used to finish construction.

huitseeker · 2026-07-06T19:51:01Z

Closing this in favor of #3334.

huitseeker force-pushed the issue-3067-rationalize-digests branch from 1ec96ab to 1a01b77 Compare June 30, 2026 20:32

huitseeker marked this pull request as ready for review June 30, 2026 20:34

huitseeker force-pushed the issue-3067-rationalize-digests branch from 1a01b77 to d8ebcaa Compare June 30, 2026 20:34

huitseeker requested review from Al-Kindi-0 and bobbinth June 30, 2026 20:43

huitseeker marked this pull request as draft June 30, 2026 20:46

huitseeker force-pushed the issue-3067-rationalize-digests branch from d8ebcaa to dc1b2ab Compare June 30, 2026 20:49

huitseeker force-pushed the issue-3291-commitment-inputs branch from 3452482 to b775170 Compare June 30, 2026 20:49

huitseeker linked an issue Jun 30, 2026 that may be closed by this pull request

Rationalize digests throughout programs #3067

Closed

huitseeker added this to the v0.25.0 milestone Jun 30, 2026

huitseeker force-pushed the issue-3291-commitment-inputs branch from b775170 to 51b783f Compare June 30, 2026 23:25

huitseeker force-pushed the issue-3067-rationalize-digests branch from dc1b2ab to 915065a Compare June 30, 2026 23:25

huitseeker marked this pull request as ready for review July 1, 2026 01:20

huitseeker force-pushed the issue-3291-commitment-inputs branch from 51b783f to fa34b31 Compare July 1, 2026 01:29

huitseeker force-pushed the issue-3067-rationalize-digests branch from 915065a to 27cdecc Compare July 1, 2026 01:29

bobbinth reviewed Jul 2, 2026

View reviewed changes

huitseeker marked this pull request as draft July 2, 2026 18:17

huitseeker marked this pull request as ready for review July 2, 2026 18:24

bobbinth reviewed Jul 3, 2026

View reviewed changes

This was referenced Jul 3, 2026

Make dense MastForest construction produce canonical node order #3325

Closed

refactor: Build dense MAST forests through finalization #3326

Merged

bobbinth reviewed Jul 3, 2026

View reviewed changes

Comment thread core/src/advice/map.rs

huitseeker changed the title ~~Bind MAST commitments to external dependency digests~~ Bind MAST commitments to external dependency digests & advice map Jul 3, 2026

huitseeker force-pushed the issue-3067-rationalize-digests branch from 88aef51 to 1454efa Compare July 3, 2026 20:49

huitseeker mentioned this pull request Jul 3, 2026

refactor: Build dense MAST forests through finalization #3329

Closed

huitseeker force-pushed the issue-3067-rationalize-digests branch from 1454efa to f2ee07d Compare July 3, 2026 21:12

huitseeker force-pushed the issue-3291-commitment-inputs branch from fa34b31 to cad30a2 Compare July 3, 2026 21:12

huitseeker added 3 commits July 3, 2026 19:48

fix: Commit MAST digests to dependencies

991cbc3

chore: Changelog

ad1a1cb

fix: Commit MAST digests to advice maps

91dd067

huitseeker added 4 commits July 3, 2026 19:48

fix: Canonicalize dense MAST forest order

04e0efc

fix: Canonicalize MAST forest before writing

84348b8

fix: Simplify MAST advice commitment checks

8221896

fix: Narrow MAST commitment changes

74beb7b

huitseeker force-pushed the issue-3291-commitment-inputs branch from cad30a2 to e042008 Compare July 3, 2026 23:54

huitseeker force-pushed the issue-3067-rationalize-digests branch from f2ee07d to 74beb7b Compare July 3, 2026 23:54

fix: Update MAST dependency digest test offset

bbca320

huitseeker mentioned this pull request Jul 6, 2026

Bind dense MastForest commitments and final order #3334

Merged

huitseeker closed this Jul 6, 2026

huitseeker deleted the issue-3067-rationalize-digests branch July 21, 2026 14:09

Uh oh!

Conversation

huitseeker commented Jun 30, 2026

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huitseeker Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huitseeker Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huitseeker Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huitseeker Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huitseeker Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huitseeker Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huitseeker Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huitseeker Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

huitseeker commented Jul 6, 2026

Uh oh!

Reviewers

Assignees

Labels

huitseeker Jul 2, 2026 •

edited

Loading

huitseeker Jul 2, 2026 •

edited

Loading

huitseeker Jul 2, 2026 •

edited

Loading

huitseeker Jul 3, 2026 •

edited

Loading

huitseeker Jul 3, 2026 •

edited

Loading

huitseeker Jul 3, 2026 •

edited

Loading

huitseeker Jul 3, 2026 •

edited

Loading

huitseeker Jul 3, 2026 •

edited

Loading