Skip to content
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

Migrate to new padding rule #1343

Merged
merged 22 commits into from
Oct 24, 2024
Merged

Migrate to new padding rule #1343

merged 22 commits into from
Oct 24, 2024

Conversation

Al-Kindi-0
Copy link
Collaborator

Describe your changes

Change the padding rule for RPO so that it uses the one in the xHash paper.

Checklist before requesting a review

  • Repo forked and branch created from next according to naming convention.
  • Commit messages and codestyle follow conventions.
  • Relevant issues are linked in the PR description.
  • Tests added for new functionality.
  • Documentation/comments updated according to changes.

@Al-Kindi-0 Al-Kindi-0 requested a review from bobbinth May 28, 2024 10:08
Copy link
Contributor

@bobbinth bobbinth 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! Thank you! I left some comments inline - but all of them are pretty minor.

I was actually expecting a bit more changes - but nice to see that the impact is not that big.

assembly/src/assembler/instruction/crypto_ops.rs Outdated Show resolved Hide resolved
stdlib/asm/crypto/hashes/native.masm Outdated Show resolved Hide resolved
stdlib/asm/mem.masm Outdated Show resolved Hide resolved
stdlib/asm/crypto/hashes/native.masm Outdated Show resolved Hide resolved
stdlib/asm/mem.masm Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth 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! Thank you! I left a few more small comments inline.

Overall, let's hold off on merging this until we propagate the changes to miden-base (we can do that once we update miden-base to work with the current VM version).

stdlib/asm/crypto/hashes/rpo.masm Show resolved Hide resolved
stdlib/asm/mem.masm Outdated Show resolved Hide resolved
stdlib/tests/mem/mod.rs Outdated Show resolved Hide resolved
stdlib/tests/crypto/native.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a 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!

#! even words: 48 cycles + 3 * words
#! odd words: 60 cycles + 3 * words
#! even words: 49 cycles + 3 * words
#! odd words: 61 cycles + 3 * words
export.hash_memory
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that I realized: I'm not sure the behavior of this procedure is entirely consistent with its naming/docs. Specifically, we assume that the number of elements to be hashed is a multiple of 4 - so, for example, it cannot be used to hash 9 elements.

Maybe a better approach would be to split it into 2 procedures:

#! Input:  [C, B, A, start_addr, num_words, ...]
#! Output: [C', B', A', end_addr, ...]
export.absorb_words_from_memory

The above procedure would be similar to absorb_double_words_from_memory but would handle the case of the odd number of words. (separately, I think we could modify the absorb_double_words_from_memory procedure to also take num_words instead of end_addr).

The second procedure would actually be for computing hash of memory:

#! Input:  [start_addr, num_elements, ...]
#! Output: [HASH, end_addr, ...]
export.hash_memory

One open question is whether we should assume that the last word is padded with zeros (in case the number of elements is not divisible by 4) or if we need to handle this in the procedure.

@Al-Kindi-0 Al-Kindi-0 force-pushed the al-migrate-new-padding-rule branch from 5b93c94 to 18ea698 Compare September 30, 2024 08:06
@bobbinth
Copy link
Contributor

@Fumuran - could you merge the latest next into this branch?

@bobbinth
Copy link
Contributor

@Fumuran could you update this branch to use miden-crypto v0.11.0 (I just published it to crates.io) and also merge next into this branch to make sure we don't have any merge conflicts?

@Fumuran Fumuran force-pushed the al-migrate-new-padding-rule branch from d24a429 to 816e28c Compare October 18, 2024 10:11
core/Cargo.toml Outdated Show resolved Hide resolved
@bobbinth
Copy link
Contributor

@Fumuran - this is basically done, right?

If so, before merging this, let's create a parallel PR in miden-base to migrate to the to this branch. I'm expecting that PR to be quite a bit of work because of the padding rule changes.

@Fumuran
Copy link
Contributor

Fumuran commented Oct 18, 2024

@bobbinth Should I prioritize migrating miden-base to the new padding rule over this issue (0xPolygonMiden/miden-base#836)?

@bobbinth bobbinth merged commit ae34c56 into next Oct 24, 2024
9 checks passed
@bobbinth bobbinth deleted the al-migrate-new-padding-rule branch October 24, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants