remove extra P: SWParam bound

[alxiong]

Description

Major changes include:

• Remove unnecessary bound of P: SWParam on many APIs

The source of the "necessity" (afaiu) comes from conversion of E::G1Affine to struct Point, where previously since G1Affine: AffineCurve and there's no API from the trait AffineCurve to get x, y, thus we have to use struct SWGroupAffine<P> instead, and do From<SWGroupAffine<P>> for Point<F> instead, which is why you see so many API with <E, F, P> generic parameters.

I try to convert directly from G1Affine to Point by using ToConstraintField (see plonk/src/circuit/customized/ecc/mod.rs).

---

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.

• [x] Targeted PR against correct branch (main)
• [x] ~Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.~
• [x] ~Wrote unit tests~
• [x] ~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]

currently, code compiles, but there are two tests failing:

---- circuit::customized::transcript::tests::test_rescue_transcript_append_vk_and_input_circuit stdout ----
thread 'circuit::customized::transcript::tests::test_rescue_transcript_append_vk_and_input_circuit' panicked at 'assertion failed: `(left == right)`
  left: `Fp384(BigInteger384([15540749464332904421, 18331995328304195891, 13822085341868466267, 7299705786554684320, 13531873085501677006, 96083863222528029]))`,
 right: `Fp384(BigInteger384([16274841609147047203, 1106918205902200751, 17362253573908193814, 2527411524691038181, 18003024565380088277, 10811839989046662]))`', plonk/src/circuit/customized/transcript/mod.rs:412:13

---- circuit::customized::ultraplonk::plonk_verifier::test::test_partial_verification_circuit stdout ----
thread 'circuit::customized::ultraplonk::plonk_verifier::test::test_partial_verification_circuit' panicked at 'assertion failed: `(left == right)`
  left: `Point(Fp384(BigInteger384([6887586079592316126, 9174619448394306261, 5124305956569313306, 9923475876731582297, 8723329757350718491, 46742474901302855])), Fp384(BigInteger384([11863691049587468387, 9829724734104359374, 12337960522976505364, 8955731077705291582, 6177477606205049445, 53745691343707102])))`,
 right: `Point(Fp384(BigInteger384([13131123385999815323, 12231299757786153216, 9589356537784992119, 5239486690906781650, 1118711817424127218, 95434426850134010])), Fp384(BigInteger384([4615152520927982106, 5926318059491658012, 3495588473486362115, 13770630512343718549, 68641773362429922, 89875724587786704])))`', plonk/src/circuit/customized/ultraplonk/plonk_verifier/mod.rs:555:17

and I haven't figured out why yet.

Besides, if we can fix this err, we should be able to remove/simply a lot more generic parameters on our APIs.

need @zhenfeizhang help to give a preliminary review and feedback.

[zhenfeizhang]

most likely because of a different data serialization:

impl<M: Parameters, ConstraintF: Field> ToConstraintField<ConstraintF> for GroupAffine<M>
where
    M::BaseField: ToConstraintField<ConstraintF>,
{
    #[inline]
    fn to_field_elements(&self) -> Option<Vec<ConstraintF>> {
        let mut x_fe = self.x.to_field_elements()?;
        let y_fe = self.y.to_field_elements()?;
        let infinity_fe = self.infinity.to_field_elements()?;
        x_fe.extend_from_slice(&y_fe);
        x_fe.extend_from_slice(&infinity_fe);
        Some(x_fe)
    }
}

to_field_elements() actually generate 3 field elements where as we originally had 2 in the transcript IIUC

[alxiong]

to_field_elements() actually generate 3 field elements where as we originally had 2 in the transcript IIUC

no, I know that. thus Point(fe[0], fe[1]) <- I only use the first two fields.

[zhenfeizhang]

to_field_elements() actually generate 3 field elements where as we originally had 2 in the transcript IIUC

no, I know that. thus Point(fe[0], fe[1]) <- I only use the first two fields.

I see... Seems like the Into function is broken somehow... to_field_elements() may not do what we wanted....

Commitment(GroupAffine { x: Fp384(BigInteger384([17067432905348922317, 13885136919376023890, 11591349702061908255, 4290032807021416436, 8975329599356811165, 31614356757706562])), y: Fp384(BigInteger384([1181341231270418752, 7506790600429906048, 7088014141337319865, 1921548957019233669, 1054743409543516884, 110618278667390632])), infinity: false })
Point(Fp384(BigInteger384([1114476660426871599, 16448161201216081007, 1751262343116883085, 11014083834213638268, 4077590559061708943, 2817296961659828])), Fp384(BigInteger384([18356264769755708755, 8149015984265305309, 8083502019061564674, 17896413123343858469, 5300952347612358931, 89838961920149089])))

Commitment(GroupAffine { x: Fp384(BigInteger384([16083278869745470399, 14972536350892925329, 13819676513404681512, 4537174741316847330, 13142929348530964221, 84722756634852925])), y: Fp384(BigInteger384([15237545290724469338, 13287924847918513666, 15075925835875964902, 9015273236703937468, 7948293443996874751, 19818762986348615])), infinity: false })
Point(Fp384(BigInteger384([11785475210789032382, 9002734187434257232, 5782101414002785650, 16311523483941003655, 9202572497054910258, 59197513710489698])), Fp384(BigInteger384([8135173778949379298, 7336537345930851494, 850180275075417084, 233887553139950044, 6997267507789986075, 92643072557901273])))

[zhenfeizhang]

thought a bit further... I kind of like that point being associated with SWParameter as before, rather than a given field. This makes more logical sense to me -- for the same fields, you may have different curve parameters and therefore Point<F> may not bind to any specific logical point; where as Point<P> binds to a unique group of points.

[alxiong]

thought a bit further... I kind of like that point being associated with SWParameter as before, rather than a given field. This makes more logical sense to me -- for the same fields, you may have different curve parameters and therefore Point<F> may not bind to any specific logical point; where as Point<P> binds to a unique group of points.

hmm... interesting, you are suggesting a different API changes. Here's a possible down side: we will need different Point structs for TEParam and for SWParam, whereas right now we only need one, since our struct Point<F>.

also, my simplification attempt in this PR is not about changing struct Point, but about functions and API that are already generic over E: PairingEngine, and I want to obviate the need to be forced to generic over <E, F, P>.

[zhenfeizhang]

right. for that we may

• either have Point<Parameters> where both SWParam and TEParam derive from, or
• SWPoint<SWParam> and TEPoint<TEParam>

any way, it is kind of orthogonal to this PR

[alxiong]

Problem

I figured out why, because we have both From<SWGroupAffine<P>> for Point<F> and From<&SWGroupAffine<P>> for Point<F>, where the latter was reserved for F: SWToTEParam or for BLS12-377 in our case where we convert this point into TE form first before building the Point(F, F).

a short test for above is:

        let mut rng = ark_std::test_rng();
        let comm: Commitment<Bls12_377> =
            Commitment(<Bls12_377 as PairingEngine>::G1Projective::rand(&mut rng).into_affine());
        println!("comm: {:?}", comm);

        let p1: Point<ark_bls12_377::Fq> = comm.0.into();
        println!("p1: {:?}", p1);
        let p2: Point<ark_bls12_377::Fq> = comm.into();
        println!("p2: {:?}", p2);
        let p3: Point<ark_bls12_377::Fq> = (&comm.0).into();
        println!("p3: {:?}", p3);

which prints:

comm: Commitment(GroupAffine { x: Fp384(BigInteger384([364752289088487184, 7617106652781656504, 12292592146272044025, 7090012186315953455, 16002170982710123677, 110886730567121824])), y: Fp384(BigInteger384([14740173953584287443, 4073073183769772155, 5393057377041405219, 3591186941560730850, 6010672885359566684, 10603604873064755])), infinity: false })
SWGroupAffine -> Point
p1: Point(Fp384(BigInteger384([364752289088487184, 7617106652781656504, 12292592146272044025, 7090012186315953455, 16002170982710123677, 110886730567121824])), Fp384(BigInteger384([14740173953584287443, 4073073183769772155, 5393057377041405219, 3591186941560730850, 6010672885359566684, 10603604873064755])))
comm -> Point
p2: Point(Fp384(BigInteger384([364752289088487184, 7617106652781656504, 12292592146272044025, 7090012186315953455, 16002170982710123677, 110886730567121824])), Fp384(BigInteger384([14740173953584287443, 4073073183769772155, 5393057377041405219, 3591186941560730850, 6010672885359566684, 10603604873064755])))
p3: Point(Fp384(BigInteger384([11027956905854938165, 17810946948834525588, 4318096842554772639, 18413886287629693046, 13881553211729330338, 117272336727929061])), Fp384(BigInteger384([16751880156053325307, 8673637453566488844, 15456809315763668441, 594291235547216211, 12381613770485598007, 31080138179768168])))

Proposed Change

While resolving the test failure would be simple now that we identified the source, I think it's problematic API design that we even misuse them ourselves.

Two thing:

• we should remove the option to convert SWGroupAffine to Point without changing to TE form, there's just no usage for that iiuc. Since Point is something we use to generate PointVar in the constraint system, and our CS only deal with TE curve point arithmetics, there's simply no reason to keep the first From conversion above.
• modify my From<Commitment> for Point<F> logic

What's next

imo, the mistake I make illustrates the necessity of @zhenfeizhang's idea of struct Point<P: Parameter> instead of Point<F: Field>. but this is less urgent, because we don't yet to support any SW group arithmetic in our CS yet.

[alxiong]

okay, after some attempt, I can't avoid using <P> in the generic param, so I will close this issue now.

that said:

we should remove the option to convert SWGroupAffine to Point without changing to TE form,

and

illustrates the necessity of @zhenfeizhang's idea of struct Point<P: Parameter> instead of Point<F: Field>. but this is less urgent, because we don't yet to support any SW group arithmetic in our CS yet.

are still two valid points that we should address later.

[zhenfeizhang]

:-(

[zhenfeizhang]

Additional thoughts -- we never needs Point<SWParam> so it may be sufficient to rename current Point<F> to TEPoint<TEParam> and have a conversion (which we already have) from GroupAffine<SWParam> to TEPoint<TEParam>. This conversion is essential for recursion so we cannot avoid it.