Skip to content

Implement Keccak-based MMR frontier#2245

Merged
mmagician merged 20 commits intoagglayerfrom
andrew-keccak-based-mmr-frontier
Jan 23, 2026
Merged

Implement Keccak-based MMR frontier#2245
mmagician merged 20 commits intoagglayerfrom
andrew-keccak-based-mmr-frontier

Conversation

@Fumuran
Copy link
Copy Markdown
Contributor

@Fumuran Fumuran commented Jan 8, 2026

This PR implements a new Keccak-based MMR frontier collection. See the details in the documentation in the related masm file.

As a related change the structure of the miden-agglayer crate was slightly updated: bridge and newly created collection modules were moved to the lib folder. That was done to make the creation of the agglayer library a bit more simple: that way we can compile the whole lib folder.

Closes: #2105

@Fumuran Fumuran mentioned this pull request Jan 8, 2026
3 tasks
@Fumuran Fumuran force-pushed the andrew-keccak-based-mmr-frontier branch from 7b40afe to fb09b70 Compare January 9, 2026 21:12
@Fumuran Fumuran marked this pull request as ready for review January 11, 2026 21:16
Copy link
Copy Markdown
Collaborator

@mmagician mmagician left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though it's not a full review yet.

Thanks for outlining the details of the computation so well ✅ very helpful

Comment on lines +50 to +51
# this implementation this maximum height is set to 32), and the leaves equal to the MMR frontier
# leaves or a zero hash (Keccak256::hash(&[0u8; 32])).
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the leaves of the SMT can't be represented by the leaves of the MMR frontier, because the frontier doesn't have all the leaves

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was struggling with how can I explain that, but the idea is that we need to compute the root of the SMT, which has all the leaves from the MMR and all other missing leaves are zeros. For example, if we have an MMR with leaves A, B, C, the leaves of the SMT which root we want to compute will look like A, B, C, ZERO_HASH_3, ..., ZERO_HASH_N.

Copy link
Copy Markdown
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! My assumption for the PR review is the algorithm described in #2105 (comment).

I left mostly small comments. The only important question I have is about whether leaf values should be hashed or not.

Comment on lines +38 to +40
# relevant frontier values in the frontier array for the current height. For example, if we have 10
# leaves (1010 in binary representation), relevant frontier values will be stored at frontier[1] and
# frontier[3].
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to spell out what a "frontier value" is. For height 0 it is the leaf (or maybe the hash of the leaf - see other comment) while for height > 0, it is the subtree root. If we end up hashing the leaves, I think "frontier hash" would be a more precise name.

Comment on lines +14 to +16
// Push the zero of height 0 to the zeros vec. This is done separately because it requires
// `Keccak256::hash` instead of `Keccak256::merge`
zeros_by_height.push(Keccak256::hash(&[0u8; 32]));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, the canonical leaf with value 0 has the value hash(0) in the tree? If so, shouldn't we be hashing leaves as well in append_and_update_frontier? Otherwise, it seems like adding a 0 into an empty frontier via append_and_update_frontier would result in a different tree root than the canonical tree root.

I would also add a test for adding (many) zeros to a frontier and make sure it matches the canonical subtree roots.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the canonical zero at height 0 is actually just a "pure" 0u32 rather than hash(0u32): https://github.com/agglayer/agglayer-contracts/blob/e468f9b0967334403069aa650d9f1164b1731ebb/contracts/v2/lib/DepositContractBase.sol#L43

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format!(
r#"
# load the provided leaf onto the stack
push.{LEAF}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
push.{LEAF}
push.[{LEAF}]

Nit: Might be worth adapting to avoid a breakage once 0xMiden/miden-vm#2572 is implemented. I think this requires dropping the rev in the stringify function, but please double check.

Copy link
Copy Markdown
Contributor Author

@Fumuran Fumuran Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, we can support square brackets only for the word literals, but the LEAF here is a double word value, so I'm not sure that we can use them directly. Though it is a good idea to adapt the code preliminarily, so I just need to update the way LEAF is generated.

Edit: it seems like right now push.[a, b, c, d] pushes values on the stack in the same order as push.a.b.c.d, so now it works the same way, but after mentioned PR will be merged we will have to just un-reverse the resulting string.

Comment on lines +71 to +77
// create a leaf from a random hex
let first_leaf = Keccak256Digest::try_from(
"0x110527e2a134fcb367f3bc770acc0d75b9c47bb4c5a78f0de02c80143340df62",
)
.unwrap();
let first_root = mmr_frontier.append_and_update_frontier(first_leaf);
let first_leaf_count = mmr_frontier.leaves_num;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ideally this would test many more rounds (16 or 32 or something like that), but at least 4, so we also test the case when the number of MMR peaks is "shrinking". With three leaves, the number only grows.

So could we rewrite this to a test loop that generates a couple of frontier updates?

Ideally we can create a small reusable test setup where we can run the MASM code and then write two concrete tests where one is similar to the current one, with more values, and the other one tests that adding a bunch of zeros to the frontier results in the expected canonical zero subtree roots.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assertion of all empty subtree roots will require to add 2^32 - 1 zero leaves to the MMR (that way we can compare the frontier with the zero hashes), but it will require too many calculations.
Instead I implemented the test in which I add 32 zero leaves and check that after each insertion the MMR root remains to be equal to the empty MMR root. I don't check subtree roots directly, but probably checking the MMR root should be sufficient?

Base automatically changed from agglayer to next January 16, 2026 08:48
Copy link
Copy Markdown
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Comment on lines +109 to +116
let mut source = "use miden::agglayer::collections::mmr_frontier32_keccak begin".to_string();

for round in 1..=32 {
// check that pushing the zero leaves into the MMR doesn't change its root
source.push_str(&leaf_assertion_code(zero_leaf, empty_mmr_root, round));
}

source.push_str("end");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: mmr_frontier_ptr is never mentioned or explicitly initialized anywhere, and while that is technically correct due to "empty memory being a valid mmr frontier", this is very subtle and could make it harder to understand the test in the future.

Copy link
Copy Markdown
Collaborator

@mmagician mmagician left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great, well documented and nicely structured.
One thing that is missing here are the Solidity compatibility tests. So I quickly built few test vectors locally with foundry and verified that things check out 1-to-1 ✅

I'll make a separate PR against the agglayer branch that adds the Solidity-compat tests. I think a small foundry project will be a good addition under the miden-agglayer crate. It will be useful not just for these MMR frontier tests, but also for addLeafValue that @partylikeits1983 was working on and verifyLeaf that I added.

This one is ready to merge from my perspective.
We could still iron out the terminology (MMR/frontier/SMT) but let's do that in a separate PR, I'll open a tracking issue.

in leaf assertion tests
@mmagician mmagician changed the base branch from next to agglayer January 19, 2026 11:06
@mmagician
Copy link
Copy Markdown
Collaborator

bridge and newly created collection modules were moved to the lib folder.

I think this was an overall improvement, but since we'll be restructuring the agglayer directories further as per #2294, let's tackle it all in one go when the time comes. For now I've reverted the directory structure changes as part of chore: revert to using old dir structure

@mmagician mmagician added the agglayer PRs or issues related to AggLayer bridging integration label Jan 21, 2026
@mmagician mmagician requested a review from bobbinth January 22, 2026 15:53
@mmagician mmagician merged commit 4c5ccae into agglayer Jan 23, 2026
17 checks passed
@mmagician mmagician deleted the andrew-keccak-based-mmr-frontier branch January 23, 2026 09:46
mmagician added a commit that referenced this pull request Jan 27, 2026
* feat: impl fist frontier version, add (yet buggy) test

* test: fix the test, fix the bug in algorithm

* chore: update changelog

* docs: add docs for the MMR frontier in the masm file

* refactor: update the doc comments, slightly update code

* refactor: update docs and comments, add overflow check, update test

* test: add more leaves

* test: add zero root test

* chore: rename `root` -> `expected_root`

in leaf assertion tests

* chore: lint

* chore: revert to using old dir structure

* fix: update max leaves constants and comments

* chore: regen errors file

* fix: first assert valid u32, only then u32lte

---------

Co-authored-by: Marti <marti@miden.team>
mmagician added a commit that referenced this pull request Jan 27, 2026
* feat: impl fist frontier version, add (yet buggy) test

* test: fix the test, fix the bug in algorithm

* chore: update changelog

* docs: add docs for the MMR frontier in the masm file

* refactor: update the doc comments, slightly update code

* refactor: update docs and comments, add overflow check, update test

* test: add more leaves

* test: add zero root test

* chore: rename `root` -> `expected_root`

in leaf assertion tests

* chore: lint

* chore: revert to using old dir structure

* fix: update max leaves constants and comments

* chore: regen errors file

* fix: first assert valid u32, only then u32lte

---------

Co-authored-by: Marti <marti@miden.team>
mmagician added a commit that referenced this pull request Jan 27, 2026
* feat: impl fist frontier version, add (yet buggy) test

* test: fix the test, fix the bug in algorithm

* chore: update changelog

* docs: add docs for the MMR frontier in the masm file

* refactor: update the doc comments, slightly update code

* refactor: update docs and comments, add overflow check, update test

* test: add more leaves

* test: add zero root test

* chore: rename `root` -> `expected_root`

in leaf assertion tests

* chore: lint

* chore: revert to using old dir structure

* fix: update max leaves constants and comments

* chore: regen errors file

* fix: first assert valid u32, only then u32lte

---------

Co-authored-by: Marti <marti@miden.team>
mmagician added a commit that referenced this pull request Jan 27, 2026
* feat: impl fist frontier version, add (yet buggy) test

* test: fix the test, fix the bug in algorithm

* chore: update changelog

* docs: add docs for the MMR frontier in the masm file

* refactor: update the doc comments, slightly update code

* refactor: update docs and comments, add overflow check, update test

* test: add more leaves

* test: add zero root test

* chore: rename `root` -> `expected_root`

in leaf assertion tests

* chore: lint

* chore: revert to using old dir structure

* fix: update max leaves constants and comments

* chore: regen errors file

* fix: first assert valid u32, only then u32lte

---------

Co-authored-by: Marti <marti@miden.team>
mmagician added a commit that referenced this pull request Jan 27, 2026
* feat: impl fist frontier version, add (yet buggy) test

* test: fix the test, fix the bug in algorithm

* chore: update changelog

* docs: add docs for the MMR frontier in the masm file

* refactor: update the doc comments, slightly update code

* refactor: update docs and comments, add overflow check, update test

* test: add more leaves

* test: add zero root test

* chore: rename `root` -> `expected_root`

in leaf assertion tests

* chore: lint

* chore: revert to using old dir structure

* fix: update max leaves constants and comments

* chore: regen errors file

* fix: first assert valid u32, only then u32lte

---------

Co-authored-by: Marti <marti@miden.team>
mmagician added a commit that referenced this pull request Jan 27, 2026
* feat: impl fist frontier version, add (yet buggy) test

* test: fix the test, fix the bug in algorithm

* chore: update changelog

* docs: add docs for the MMR frontier in the masm file

* refactor: update the doc comments, slightly update code

* refactor: update docs and comments, add overflow check, update test

* test: add more leaves

* test: add zero root test

* chore: rename `root` -> `expected_root`

in leaf assertion tests

* chore: lint

* chore: revert to using old dir structure

* fix: update max leaves constants and comments

* chore: regen errors file

* fix: first assert valid u32, only then u32lte

---------

Co-authored-by: Marti <marti@miden.team>
partylikeits1983 pushed a commit that referenced this pull request Feb 12, 2026
* feat: impl fist frontier version, add (yet buggy) test

* test: fix the test, fix the bug in algorithm

* chore: update changelog

* docs: add docs for the MMR frontier in the masm file

* refactor: update the doc comments, slightly update code

* refactor: update docs and comments, add overflow check, update test

* test: add more leaves

* test: add zero root test

* chore: rename `root` -> `expected_root`

in leaf assertion tests

* chore: lint

* chore: revert to using old dir structure

* fix: update max leaves constants and comments

* chore: regen errors file

* fix: first assert valid u32, only then u32lte

---------

Co-authored-by: Marti <marti@miden.team>
afa7789 pushed a commit to afa7789/miden-base that referenced this pull request Mar 9, 2026
* feat: impl fist frontier version, add (yet buggy) test

* test: fix the test, fix the bug in algorithm

* chore: update changelog

* docs: add docs for the MMR frontier in the masm file

* refactor: update the doc comments, slightly update code

* refactor: update docs and comments, add overflow check, update test

* test: add more leaves

* test: add zero root test

* chore: rename `root` -> `expected_root`

in leaf assertion tests

* chore: lint

* chore: revert to using old dir structure

* fix: update max leaves constants and comments

* chore: regen errors file

* fix: first assert valid u32, only then u32lte

---------

Co-authored-by: Marti <marti@miden.team>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agglayer PRs or issues related to AggLayer bridging integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants