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

Update RPO's padding rule to use that in the xHash paper #318

Merged
merged 23 commits into from
Oct 18, 2024

Conversation

Al-Kindi-0
Copy link
Collaborator

Describe your changes

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 changed the base branch from main to next May 27, 2024 14:25
CHANGELOG.md Outdated
Comment on lines 1 to 20
## 0.10.0 (TBD)

* Added more `RpoDigest` and `RpxDigest` conversions (#311).
* Migrated to Winterfell v0.9.0 (#315)

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably rebase this PR from the latest next to make sure we carry over the latest updates.

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.

Great catch! I've added a few more comments to clarify the naming and potentially simplify things a bit.

src/hash/rescue/rpx/mod.rs Outdated Show resolved Hide resolved
src/hash/rescue/rpx/mod.rs Outdated Show resolved Hide resolved
src/hash/rescue/rpx/mod.rs Outdated Show resolved Hide resolved
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

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! Let's hold off on merging this until we propagate the changes to miden-base.

On potential thing left to consider: I believe currently hashing of empty list of elements and hashing of 8 ZERO elements will give us the same result - right? I wonder if there is a good way to fix this - just in case. Nothing immediately comes to mind though.

@Al-Kindi-0
Copy link
Collaborator Author

On potential thing left to consider: I believe currently hashing of empty list of elements and hashing of 8 ZERO elements will give us the same result - right? I wonder if there is a good way to fix this - just in case. Nothing immediately comes to mind though.

I think we had a similar discussion in RPO and I believe we settled on not accepting empty lists for hashing. I will think about it again to see if we can accommodate it differently.

@bobbinth
Copy link
Contributor

@Al-Kindi-0 - let's rebase this PR.

Also, should we implement the rule where hashing of an empty string gives us [ZERO; 4] output?

@Al-Kindi-0 Al-Kindi-0 force-pushed the al-rpo-new-padding-rule branch from 74a343c to 32c877e Compare September 12, 2024 16:22
@Al-Kindi-0
Copy link
Collaborator Author

@Al-Kindi-0 - let's rebase this PR.

Also, should we implement the rule where hashing of an empty string gives us [ZERO; 4] output?

Done!

@Al-Kindi-0 Al-Kindi-0 force-pushed the al-rpo-new-padding-rule branch from 32c877e to e6222dd Compare September 12, 2024 16:46
@Al-Kindi-0 Al-Kindi-0 marked this pull request as ready for review September 12, 2024 17:26
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 small comments inline.

Also, it seems like the "return default digest on empty input" as already working here, right?

Lastly, could you refresh 0xPolygonMiden/miden-vm#1343 when you get a chance?

src/hash/rescue/rpx/mod.rs Outdated Show resolved Hide resolved
src/hash/rescue/rpo/mod.rs Outdated Show resolved Hide resolved
src/hash/rescue/rpx/tests.rs Outdated Show resolved Hide resolved
src/hash/rescue/rpx/tests.rs Outdated Show resolved Hide resolved
@Al-Kindi-0
Copy link
Collaborator Author

Also, it seems like the "return default digest on empty input" as already working here, right?

Yes, had to check the bytes case, but even then it worked without complications

Lastly, could you refresh 0xPolygonMiden/miden-vm#1343 when you get a chance?

Sure thing

Copy link

Copy link

@bobbinth bobbinth merged commit a734dac into next Oct 18, 2024
12 checks passed
@bobbinth bobbinth deleted the al-rpo-new-padding-rule branch October 18, 2024 03:49
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.

4 participants