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

Sponge API refactor #146

Merged
merged 29 commits into from
Dec 8, 2022
Merged

Sponge API refactor #146

merged 29 commits into from
Dec 8, 2022

Conversation

tessico
Copy link
Contributor

@tessico tessico commented Nov 24, 2022

Description

This is a WIP.

The refactor defines new explicit types for CRHF and PRF which are sponges that internally use the Rescue Permutation, and as such is more aligned with the CAPE spec.

By following the sponge API we are able to re-use code shared between the above two functionalities.

It should then be then easier to make the circuits more generic by also defining and implementing a trait like CryptographicSpongeVar, though we'd probably need to define our own due to a dependency on a R1CS ConstraintSystem.

closes: #15


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@tessico tessico marked this pull request as draft November 24, 2022 16:32
@tessico
Copy link
Contributor Author

tessico commented Nov 25, 2022

Btw, it goes a little further than #50, in that it also uses the new absorb/squeeze methods in the encapsulating types' implementation of full_state_keyed_sponge, sponge_no_padding etc.

@tessico tessico marked this pull request as ready for review November 29, 2022 14:51
@tessico
Copy link
Contributor Author

tessico commented Nov 29, 2022

@EspressoSystems/jellyfish

primitives/src/rescue/mod.rs Outdated Show resolved Hide resolved
primitives/src/rescue/mod.rs Outdated Show resolved Hide resolved
@tessico tessico mentioned this pull request Nov 29, 2022
11 tasks
Copy link
Contributor

@mrain mrain left a comment

Choose a reason for hiding this comment

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

Good work 👍 Rescue interface looks cleaner now.
There are a few questions left.

Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

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

I like the refactoring! being explicit about what we are using Rescue for is very good.

  • IMO, RescueSpongeCRHF should be RescueSpongeCRH or even simpler RescueCRH

primitives/src/rescue/sponge.rs Show resolved Hide resolved
primitives/src/rescue/sponge.rs Outdated Show resolved Hide resolved
primitives/src/prf.rs Outdated Show resolved Hide resolved
@tessico
Copy link
Contributor Author

tessico commented Dec 5, 2022

@EspressoSystems/jellyfish This should be ready for a (hopefully) final round of reviews!

Copy link
Contributor

@mrain mrain left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Could you add some lines in CHANGELOG.md under PENDING section?

mrain
mrain previously approved these changes Dec 6, 2022
Copy link
Contributor

@mrain mrain left a comment

Choose a reason for hiding this comment

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

LGTM!

chancharles92
chancharles92 previously approved these changes Dec 8, 2022
mrain
mrain previously approved these changes Dec 8, 2022
@tessico tessico dismissed stale reviews from mrain and chancharles92 via a25088e December 8, 2022 14:32
@tessico tessico merged commit 9b7beb2 into main Dec 8, 2022
@tessico tessico deleted the ark-sponge-traits branch December 8, 2022 14:33
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.

Ark-sponge compatibilty
4 participants