Implement Keccak-based MMR frontier #2202
Conversation
…/miden-base into andrew-migrate-to-v20-vm
Fumuran
force-pushed
the
andrew-keccak-mmr-frontier
branch
from
f83c1bd to
107d4a2
Compare
|
Without having looked at the code, I would also probably add implementation of this data structure in Rust. For now, it would probably be purely for testing purposes. |
mmagician
left a comment
mmagician
left a comment
There was a problem hiding this comment.
Really great work! The algorithm is correct AFAICS
I suppose you're using a small tree height =5 to showcase and test the functionality. For a height of 32, we're going to end up with a lot of boilerplate, I wonder what's the cleanest way to load all the canonical zeros to memory.
| # => [zero_array_ptr, mmr_root_ptr] | ||
|
|
||
| # store the zero for the height 0 | ||
| push.1676014350.378744630.4127735880.2512168523.3362732168.2839872470.2825030484.3656125737 |
There was a problem hiding this comment.
We should hardcode those constants at the top of the file
| # The offset of the MMR root. | ||
| # | ||
| # Q: do we need to store the root? It seems like we never return it | ||
| const MMR_ROOT_OFFSET = 4 | ||
|
|
||
| # The offset of the array of the zero hashes of respective heights. | ||
| const ZEROS_OFFSET = 12 # 6 double words, 48 felts in total | ||
|
|
||
| # The offset of the array of the frontier nodes of respective heights. | ||
| const FRONTIER_OFFSET = 60 # 6 double words, 48 felts in total |
There was a problem hiding this comment.
Would be great to add a description of the expected memory layout, like so:
[num_leaves, root, frontier_L[0], frontier_R[0], frontier_L[1], frontier_R[1], ... ]
There was a problem hiding this comment.
I would probably store the zeros separately from the above expected layout.
The reason is that this layout is expected to be provided by the caller of this procedure, whereas the zeros are internal, and not provided by the user.
So in practical terms, the canonical zeros could be stored at the very end.
| while.true | ||
| # => [iter_counter, num_leaves, mmr_frontier_ptr] | ||
|
|
||
| # get the pointer to the frontier node of the current height |
There was a problem hiding this comment.
what is the initial state of the frontier array? Is it zeros, or canonical zeros at tree height? Or something else?
|
|
||
| # load the current hash from the local memory back to the stack | ||
| padw loc_loadw.4 padw loc_loadw.0 swapdw | ||
| # => [FRONTIER[iter_counter]_LO, FRONTIER[iter_counter]_HI, CUR_HASH_LO, CUR_HASH_HI, iter_counter, num_leaves, mmr_frontier_ptr] |
There was a problem hiding this comment.
Following the algorithm description here, CUR_HASH has the value of NEW_LEAF in the first iteration - let's make this explicit in the inline comments, because it's not clear w/o knowing the algorithm in detail where CUR_HASH suddenly appears from
| # store the new leaf to the local memory | ||
| loc_storew.0 dropw | ||
| loc_storew.4 dropw |
There was a problem hiding this comment.
| # store the new leaf to the local memory | |
| loc_storew.0 dropw | |
| loc_storew.4 dropw | |
| # set CUR_HASH = NEW_LEAF and store to local memory | |
| loc_storew.0 dropw | |
| loc_storew.4 dropw |
(see a comment below about where CUR_HASH comes from
| # update the counter | ||
| push.1 add | ||
|
|
||
| # update the `num_leaves` (shift it right by 1 bit) |
There was a problem hiding this comment.
I would avoid using num_leaves as a mutable variable (the actual number of leaves doesn't change throughout the algorithm), and instead follow the convention to set:
let mut idx = num_leaves;at the beginning of the procedure, and just use idx later
| exec.mem_store_double_word movup.8 drop | ||
| # => [FIN_HASH_LO, FIN_HASH_HI, frontier[tree_height]_ptr] | ||
|
|
||
| # store the final hash to the frontier[tree_height]_ptr (frontier[5]) |
There was a problem hiding this comment.
Doesn't this step already happen as part of the loop (for the case when the last bit is zero)?
| # Q: do we need to store the root? It seems like we never return it | ||
| const MMR_ROOT_OFFSET = 4 |
There was a problem hiding this comment.
I suppose we don't have to store it as part of the append_and_update_frontier procedure. It will be the caller's responsibility to retrieve the result of append_and_update_frontier and update that root in its account storage.
| #! Inputs: [WORD_1, WORD_2, ptr] | ||
| #! Outputs: [WORD_1, WORD_2, ptr] | ||
| proc mem_store_double_word | ||
| dup.8 mem_storew_be swapw | ||
| # => [WORD_2, WORD_1, ptr] | ||
|
|
||
| dup.8 add.4 mem_storew_be swapw | ||
| # => [WORD_1, WORD_2, ptr] | ||
| end |
There was a problem hiding this comment.
a future note: these might be useful across the board, let's leave as-is for now but keep in mind that we might want to place these helpers in a more accessible location for other procedures to make use of.
|
Superseded by #2245 |
In this PR we primarily do two things:
agglayer::collectionsmoduleWiP
TODO:
Closes: #2105