Conversation
d477ed9 to
154e6f3
Compare
huitseeker
left a comment
There was a problem hiding this comment.
The code looks great to me. I have further questions:
- how would our benchmarks evolve on an APi that would consider this the principal commitment API? That includes downstream benches, e.g. I would expect
get
operations will require fewer hash calls when the hamming weight oflenis large, but
unpackitself, which is required foraddoperations, will require roughly twice as many hash calls. How does that translate on e.g. the transaction benchmark of miden-vm? - how complex would it be to have our the downstream APIs, notably the SyncMMR evolve to adopt this?
8843c61 to
bffac78
Compare
bffac78 to
8ff28bb
Compare
|
As a first step, I added a benchmark to get an initial It compares For the verification comparison, I’m using:
|
huitseeker
left a comment
There was a problem hiding this comment.
Thanks for the benchmarks, this is very useful! /cc @bobbinth
| - Added `Signature::from_der()` for EdDSA signatures ([#979](https://github.com/0xMiden/crypto/pull/979)). | ||
| - Fixed `SimpleSmt::set_subtree()` to clear stale leaves and inner nodes in the replaced subtree region ([#981](https://github.com/0xMiden/crypto/pull/981)). | ||
| - Fixed `SliceReader` bounds checking to reject overflowing read lengths ([#987](https://github.com/0xMiden/crypto/pull/987)). | ||
| - Added `MerkleFrontier` as append-only `len + peaks` state with a normal Merkle root commitment, append support, legacy `MmrPeaks` conversion, and standard Merkle proof bridging ([#984](https://github.com/0xMiden/crypto/pull/984)). |
There was a problem hiding this comment.
Could this move to the 0.26.0 (TBD) section? 0.25.0 was released on 2026-05-01, so this new API would otherwise read like it shipped in the previous release.
There was a problem hiding this comment.
Moved to the 0.26.0 section.
| /// | ||
| /// The root is computed by folding the path to `len`: set bits consume peaks on the left and | ||
| /// unset bits imply empty subtrees on the right. | ||
| pub fn root(&self) -> Word { |
There was a problem hiding this comment.
Could we either bind len into this commitment, or make the public check use an authenticated (len, root) pair? The root alone does not distinguish a shorter frontier from one extended by empty leaves.
With normal empty padding, a proof for a leaf past the frontier can still verify when the value is the empty leaf. For example, after one appended empty_subtree_root(0), root() is empty_subtree_root(1), so position 1 with empty_subtree_root(0) verifies against the same root even though len == 1. We may need to include len in the authenticated value, or make the verify API require checking position < len along with the root.
This local unit test repros both the root collision and the out-of-range proof verification:
#[test]
fn test_merkle_frontier_root_collision_does_not_bind_len() {
let empty_leaf = empty_subtree_root(0);
let mut mmr = Mmr::new();
mmr.add(empty_leaf).unwrap();
mmr.add(empty_leaf).unwrap();
let frontier_len2 = mmr.frontier();
let mut frontier_len3 = frontier_len2.clone();
frontier_len3.append(empty_leaf).unwrap();
assert_eq!(frontier_len2.len(), 2);
assert_eq!(frontier_len3.len(), 3);
assert_eq!(frontier_len2.root(), frontier_len3.root());
let out_of_range_path = MerklePath::new(vec![empty_leaf, frontier_len2.peaks()[0]]);
out_of_range_path
.verify(2, empty_leaf, &frontier_len2.root())
.unwrap();
}There was a problem hiding this comment.
I went with the authenticated (len, root) option.
MerkleFrontier::root() remains the raw Merkle root, and the public verification path now checks position < len before delegating to MerklePath::verify.
I also added your empty-padding repro as a regression: raw root-only verification still demonstrates the ambiguity, while MerkleFrontier::verify() rejects the out-of-range opening.
71781a0 to
94b1151
Compare
94b1151 to
1a7abbf
Compare
|
Update after the latest fix: the rooted-frontier verification benchmark now uses the authenticated
|
Describe your changes
This adds
MerkleFrontieras append-onlylen + peaksstate with a normal Merkle root commitment, plus append support and conversions to/from legacyMmrPeaks.It also adds
Mmr/PartialMmrhelpers for producing plain Merkle proofs against the frontier root, and documentsMmrPeaks::hash_peaks()as the legacy commitment path.Tests added for frontier root computation, append parity with
Mmr, legacy conversion, and frontier proof verification.Closes 0xMiden/miden-vm#3526.
Checklist before requesting a review
nextaccording to naming convention.