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

Created CRHF trait and conform RescueCRH impl #169

Merged
merged 8 commits into from
Dec 22, 2022
Merged

Created CRHF trait and conform RescueCRH impl #169

merged 8 commits into from
Dec 22, 2022

Conversation

alxiong
Copy link
Contributor

@alxiong alxiong commented Dec 20, 2022

Description

Part of: #168

Changes include:

  • add trait CRHF
  • Provide struct FixedLengthRescueCRHF and struct VariableLengthRescueCRHF conforming to the trait.
  • add trait CommitmentScheme
  • Provide struct FixedLengthRescueCRHF

NOTE:
to hack around some nightly-only feature on using const generic in an expression, we have a temporarily excessive generic parameter on FixedLengthRescueCRHF<F, INPUT_LEN, INPUT_LEN_PLUS_ONE> which will be fixed in the future.


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

@alxiong alxiong requested a review from a team December 20, 2022 09:16
@alxiong alxiong self-assigned this Dec 20, 2022
Copy link
Contributor

@tessico tessico left a comment

Choose a reason for hiding this comment

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

What about replacing all the current calls to RescueCRHF with (Fixed/Variable)LengthRescueCRHF?

primitives/src/crhf.rs Show resolved Hide resolved
primitives/src/crhf.rs Outdated Show resolved Hide resolved
@alxiong
Copy link
Contributor Author

alxiong commented Dec 20, 2022

What about replacing all the current calls to RescueCRHF with (Fixed/Variable)LengthRescueCRHF?

Yes I have considered this, the problem is, currently RescueCRHF::sponge_with_padding() -> Vec<F> always succeed whereas sponge_no_padding() -> Result<> could fail.
Some functions that internally call padded version API also have a function signature that won't fail. If we switch to VariableLengthRescueCRHF::evaluate() -> Result<Self::Output>, since we need to conform with trait CRH then we will have to return a Result and do unwrap().

A possible middle ground is we declare another API: VariableLengthRescueCRHF::evaluate_safe() -> Self::Output which internal call evaluate() and unwrap() once cuz caller know it won't panic.

@@ -30,7 +30,7 @@ use ark_std::{vec, vec::Vec};
/// The state size of rescue hash.
pub const STATE_SIZE: usize = 4;
/// The rate of the sponge used in RescueCRHF.
pub const CRHF_RATE: usize = 3;
pub(crate) const CRHF_RATE: usize = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do the same thing to STATE_SIZE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently still usage in transcript in jf-plonk that utilize this constant here, I think we might be able make it consistent after #159 ?

I will think more meanwhile

Copy link
Contributor

Choose a reason for hiding this comment

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

Suddenly I feel like we should export these 2 constants. Perhaps we could make them trait/struct dependent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to disagree. I think it's intuitive that CRHF_RATE is a internal detail of CRHF, thus try to hide from public is sensible. whereas STATE_SIZE is a parameter of our Rescue permutation instance, which like A and MDS should be public.

I think the current visibility makes sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

But what if the user need to know the "rate of compression"?

Copy link
Contributor Author

@alxiong alxiong Dec 21, 2022

Choose a reason for hiding this comment

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

why would the user want to know that? I couldn't think of a use case for now.

alright, maybe I will just expose them. I don't see much harm either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed back in 7b77e79

@alxiong
Copy link
Contributor Author

alxiong commented Dec 21, 2022

Update: during refactoring of @tessico's suggestion

What about replacing all the current calls to RescueCRHF with (Fixed/Variable)LengthRescueCRHF?

I need to use FixedLengthRescueCRHF::<F, INPUT_LEN, 1>::evaluate() inside our Commitment, but currently the length is just a field (variable) instead of a constant value (meaning not knowable at compile time), thus compiler complains.

Therefore this forces me try the following:

pub struct FixedLengthRescueCommitment<F: RescueParameter, const INPUT_LEN: usize>(PhantomData<F>);

meanwhile, I might as well just declare a trait CommitmentScheme in this PR.

@alxiong alxiong requested a review from tessico December 21, 2022 14:51
Copy link
Contributor

@tessico tessico 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, I left some final minor comments.

primitives/src/circuit/merkle_tree/mod.rs Show resolved Hide resolved
primitives/src/rescue/sponge.rs Show resolved Hide resolved
primitives/src/signatures/schnorr.rs Outdated Show resolved Hide resolved
@alxiong alxiong requested a review from tessico December 22, 2022 09:50
@alxiong alxiong merged commit f88c714 into main Dec 22, 2022
@alxiong alxiong deleted the crhf-trait branch December 22, 2022 11:59
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.

None yet

3 participants