-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mmr packing #841
Mmr packing #841
Conversation
9338943
to
79ba9e1
Compare
stdlib/asm/collections/mmr.masm
Outdated
padw padw padw | ||
exec.native::hash_even_memory_addresses | ||
exec.native::digest_output | ||
# => [HASH, peaks_end, peaks_end, ...] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm modifying the code as follows:
- [done] Also push this to the advice map, that way pack works better as an inverse operation of unpack.
- My initial idea was to have another procedure, I guess we can still have it if anyone finds that useful, but from the naming pack/unpack should be the inverse of each other, so one read and the other writes to the advice map
- [done] I'm going to change the padding to make the length a word
- My initial idea was to just push padding as necessary to make use of
adv.mem
, but that would make the format of the input and output different, which can be super confusing. I think it is better to make them consistent. Edit: It would be even more confusing than I thought, I assumed I could just push data to the end, but that is not howadv.mem
works, it reads a memory range, so the padding would appear on the position I added in this PR.
- My initial idea was to just push padding as necessary to make use of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
humm, adv.mem
only accepts immediate parameters, I have to fix that before continuing here.
done: #844
8e42ad4
to
4420b91
Compare
marking this as a draft, first have to fix |
550b151
to
76da1b8
Compare
stdlib/asm/collections/mmr.masm
Outdated
padw padw padw | ||
exec.native::hash_even_memory_addresses | ||
exec.native::digest_output | ||
# => [HASH, peaks_end, peaks_end, peaks_start, num_leaves, ...] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If adv.mem
was defined with start_address
and end_address
instead of number_of_words
, I could reuse peaks_end
and maybe save a few cycles.
I haven't changed it because I'm not sure if that would be best in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the end it saved just a single cycle.
note: moving the "control word" with swapw
uses the same number of cycles.
76da1b8
to
84519a8
Compare
84519a8
to
045d790
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left a few minor comments inline.
stdlib/asm/crypto/hashes/native.masm
Outdated
@@ -9,7 +26,7 @@ | |||
#! Cycles: 4 + 3 * words, where `words` is the `start_addr - end_addr - 1` | |||
#! | |||
#! Where `A` is the capacity word that will be used by the hashing function, and `B'` the hash output. | |||
proc.hash_even_memory_addresses | |||
export.hash_even_memory_addresses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the addresses don't have to be even - right? (it's the number of words that needs to be even?) Maybe rename this to something like hash_memory_even
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, that is right. changed the name!
16c61cb
to
fba711a
Compare
fba711a
to
a1f17d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good! Thank you!
Describe your changes
Review/Merge after
#834#844This adds the procedure to pack a Mmr.
Checklist before requesting a review
next
according to naming convention.