Traits for Default Poseidon Parameters + Poseidon cleanup #22

weikengchen · 2021-07-06T09:00:19Z

Description

This PR is from a branch named add-rescue, although this PR did not add Rescue.

It consists of two parts:

Cleanup of the Poseidon implementation to better structure the Parameters and the Sponge struct.
Add necessary traits and derivations for default Poseidon parameters for each field, which includes Grain LFSR and an aligned parameter generation for Poseidon.

This marks the first step toward the handling of Poseidon parameter generation.

More changes may show up in separate PRs. This one is long enough and should be concluded.

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 (master)
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

weikengchen · 2021-07-06T09:24:05Z

Note that ideally, the interface should use optimization_goal instead of optimized_for_constraints: bool. This is intentionally avoided because OptimizationGoal is inside R1CS and requires "std", so the implementation can be "no-std".

weikengchen · 2021-07-07T02:19:57Z

I will make more cleanup to this repo---the reference implementation puts the capacity at the beginning, while we put it at the end. Since it is better to avoid such differences, I will make a PR to change so, as well as adding some test numbers.

tsunrise

Thanks for the PR! This PR is quite useful. Left a few comments

src/poseidon/traits.rs

src/poseidon/grain_lfsr.rs

src/poseidon/mod.rs

src/poseidon/traits.rs

Pratyush · 2021-07-07T18:37:02Z

src/poseidon/traits.rs

+    impl Fp256Parameters for TestFrParameters {}
+    impl FftParameters for TestFrParameters {
+        type BigInt = <FrParameters as FftParameters>::BigInt;
+        const TWO_ADICITY: u32 = FrParameters::TWO_ADICITY;
+        const TWO_ADIC_ROOT_OF_UNITY: Self::BigInt = FrParameters::TWO_ADIC_ROOT_OF_UNITY;
+    }
+
+    // This TestFrParameters is the same as the BLS12-381's Fr.
+    // MODULUS = 52435875175126190479447740508185965837690552500527637822603658699938581184513
+    impl FpParameters for TestFrParameters {
+        const MODULUS: BigInteger256 = FrParameters::MODULUS;
+        const MODULUS_BITS: u32 = FrParameters::MODULUS_BITS;
+        const CAPACITY: u32 = FrParameters::CAPACITY;
+        const REPR_SHAVE_BITS: u32 = FrParameters::REPR_SHAVE_BITS;
+        const R: BigInteger256 = FrParameters::R;
+        const R2: BigInteger256 = FrParameters::R2;
+        const INV: u64 = FrParameters::INV;
+        const GENERATOR: BigInteger256 = FrParameters::GENERATOR;
+        const MODULUS_MINUS_ONE_DIV_TWO: BigInteger256 = FrParameters::MODULUS_MINUS_ONE_DIV_TWO;
+        const T: BigInteger256 = FrParameters::T;
+        const T_MINUS_ONE_DIV_TWO: BigInteger256 = FrParameters::T_MINUS_ONE_DIV_TWO;
+    }


Why not just set TestFrParameters to be bls12_381::FrParameters?

Since this TestFrParameters would be slightly different in that it has:
impl PoseidonDefaultParameters for TestFrParameters

and I cannot add this to the existing struct bls12_381::FrParameters that is in another crate (as this is the test mod).

Pratyush · 2021-07-07T18:48:43Z

src/poseidon/traits.rs

+}
+
+/// A field with Poseidon parameters associated
+pub trait PoseidonDefaultParametersField: PrimeField {


Hmm the issue with this design is that we can only add an impl for PoseidonDefaultParametersField in two places: the ark-curves crate corresponding to that field, or in this crate. Do you think there's a better way to do this?

Also, what does it mean for Parameters to be "default" here?

I would consider this for a future PR. This PR probably will just provide a "default parameter retriever".

I think we need to make sure that a user can freely choose a different set of Poseidon parameters for, e.g., Marlin.

Pratyush · 2021-07-07T18:52:53Z

Thanks for the PR @weikengchen

It might also be worth it to look at zcash/orchard#41 (or the more recent version of the code here) to see if we can port anything from their design (and if there are any incompatibilities)

Co-authored-by: Pratyush Mishra <[email protected]>

…escue

Co-authored-by: Pratyush Mishra <[email protected]>

weikengchen · 2021-07-07T23:20:18Z

The PR is ready for a follow-up review. And I would consider this to be the preliminary API which we will make further improvements over.

For example, we may need more optimization goals, as there can be a "balanced" version which takes into account that (1) one may use this same sponge in both Groth16 and Marlin, (2) Marlin has a different metrics, and (3) Groth16 is faster than Marlin.

weikengchen added 8 commits July 2, 2021 00:17

half way adding Rescue

da9436f

add the grain lfsr

8f1718a

add Grain LFSR

f6222a6

update changelog

feaa160

minor

5a1acc2

minor

cd8ac5c

This PR masks away 21

7f79b2a

remove dependency on bigint

8c2b195

weikengchen changed the title ~~Trais for Default Poseidon Parameters + Poseidon cleanup~~ Traits for Default Poseidon Parameters + Poseidon cleanup Jul 6, 2021

no-std

689530e

weikengchen requested a review from tsunrise July 6, 2021 09:58

make param fields public

06f83db

tsunrise reviewed Jul 7, 2021

View reviewed changes

src/poseidon/traits.rs Outdated Show resolved Hide resolved

src/poseidon/traits.rs Outdated Show resolved Hide resolved

src/poseidon/traits.rs Show resolved Hide resolved