Implement Keccak-based MMR frontier#2245
Conversation
7b40afe to
fb09b70
Compare
mmagician
left a comment
There was a problem hiding this comment.
LGTM, though it's not a full review yet.
Thanks for outlining the details of the computation so well ✅ very helpful
crates/miden-agglayer/asm/lib/collections/mmr_frontier32_keccak.masm
Outdated
Show resolved
Hide resolved
crates/miden-agglayer/asm/lib/collections/mmr_frontier32_keccak.masm
Outdated
Show resolved
Hide resolved
| # 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])). |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
crates/miden-agglayer/asm/lib/collections/mmr_frontier32_keccak.masm
Outdated
Show resolved
Hide resolved
crates/miden-agglayer/asm/lib/collections/mmr_frontier32_keccak.masm
Outdated
Show resolved
Hide resolved
PhilippGackstatter
left a comment
There was a problem hiding this comment.
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.
| # 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]. |
There was a problem hiding this comment.
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.
crates/miden-agglayer/asm/lib/collections/mmr_frontier32_keccak.masm
Outdated
Show resolved
Hide resolved
crates/miden-agglayer/asm/lib/collections/mmr_frontier32_keccak.masm
Outdated
Show resolved
Hide resolved
crates/miden-agglayer/asm/lib/collections/mmr_frontier32_keccak.masm
Outdated
Show resolved
Hide resolved
crates/miden-agglayer/asm/lib/collections/mmr_frontier32_keccak.masm
Outdated
Show resolved
Hide resolved
| // 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])); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
and when we add the leaf, it's already hashed, so no need to hash the value again, see https://github.com/agglayer/agglayer-contracts/blob/e468f9b0967334403069aa650d9f1164b1731ebb/contracts/v2/lib/DepositContractBase.sol#L66
| format!( | ||
| r#" | ||
| # load the provided leaf onto the stack | ||
| push.{LEAF} |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
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.
| // 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
crates/miden-agglayer/asm/lib/collections/mmr_frontier32_keccak.masm
Outdated
Show resolved
Hide resolved
crates/miden-agglayer/asm/lib/collections/mmr_frontier32_keccak.masm
Outdated
Show resolved
Hide resolved
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me!
| 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"); |
There was a problem hiding this comment.
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.
mmagician
left a comment
There was a problem hiding this comment.
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
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 |
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
This PR implements a new Keccak-based MMR frontier collection. See the details in the documentation in the related
masmfile.As a related change the structure of the
miden-agglayercrate was slightly updated:bridgeand newly createdcollectionmodules were moved to thelibfolder. That was done to make the creation of theagglayerlibrary a bit more simple: that way we can compile the wholelibfolder.Closes: #2105