feat(pairing): BLS12-381 Subgroup Check #559

PatStiles · 2023-09-19T03:33:35Z

Add Naive Subgroup Check to pairing

Description

Description of the pull request changes and motivation.

Addresses part of #543 by adding a naive implementation of a pairing subgroup check leveraging the check_point_is_in_subgroup() from compression. To accomodate this check the following was added:

The trait implementation for IsPairing was
A new Error enum was added to src/math/elliptic_curve/short_weierstrauss/curves/error.rs
Made check_point_is_in_subgroup() accept ShortWeierstrassProjectivePoint generic over IsShortWeierstrauss.

@MauroToscano @diegokingston I wasn't sure if using a result was the best practice but it seemed simplest at the time. Let me know if you have any suggestions?

New feature

Checklist

Linked to Github Issue
Unit tests added

codecov-commenter · 2023-09-19T03:37:35Z

Codecov Report

Merging #559 (04c48cc) into main (314fafc) will decrease coverage by 0.01%.
The diff coverage is 95.04%.

@@            Coverage Diff             @@
##             main     #559      +/-   ##
==========================================
- Coverage   95.50%   95.49%   -0.01%     
==========================================
  Files         110      110              
  Lines       19002    19088      +86     
==========================================
+ Hits        18147    18229      +82     
- Misses        855      859       +4

Files	Coverage Δ
crypto/src/commitments/kzg.rs	`99.67% <100.00%> (ø)`
.../short_weierstrass/curves/bls12_381/compression.rs	`95.68% <100.00%> (-0.15%)`	⬇️
...urve/short_weierstrass/curves/bls12_381/pairing.rs	`99.66% <100.00%> (+0.13%)`	⬆️
math/src/errors.rs	`20.00% <0.00%> (-2.23%)`	⬇️
math/src/elliptic_curve/traits.rs	`53.84% <0.00%> (-16.16%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

MauroToscano · 2023-09-21T14:24:29Z

An error is how it's done in Rust, it's good

MauroToscano

It doesn't look right. KZG is defined for any G1, G2. But the check is working over the BLS12 381. It also doesn't look right that chek_point_is_in_subgroup works over any shortweierstrass point, but actually, it's only working over the bls 381. The compiler is being tricked into thinking that the function is more generic than what it is

MauroToscano · 2023-09-21T14:29:53Z

math/src/elliptic_curve/short_weierstrass/curves/bls12_381/compression.rs

+pub const SUBGROUP_ORDER: U256 =
    U256::from_hex_unchecked("73eda753299d7d483339d80809a1d80553bda402fffe5bfeffffffff00000001");

-pub fn check_point_is_in_subgroup(point: &G1Point) -> bool {
-    let inf = G1Point::neutral_element();
-    let aux_point = point.operate_with_self(MODULUS);
+pub fn check_point_is_in_subgroup<SW: IsShortWeierstrass>(
+    point: &ShortWeierstrassProjectivePoint<SW>,
+) -> bool {
+    let inf = ShortWeierstrassProjectivePoint::<SW>::neutral_element();
+    let aux_point = point.operate_with_self(SUBGROUP_ORDER);
    inf == aux_point
 }


It would be better for this to be in the pairing.rs file

MauroToscano · 2023-09-21T15:16:56Z

It doesn't look right. KZG is defined for any G1, G2. But the check is working over the BLS12 381. It also doesn't look right that chek_point_is_in_subgroup works over any shortweierstrass point, but actually, it's only working over the bls 381. The compiler is being tricked into thinking that the function is more generic than what it is

There's also an issue with the orders. G1 and G2 may not have the same order,or at least it's not obvious in the code*. Use the neutral_element of each for the checks as to avoid problems.

schouhy · 2023-09-21T20:54:46Z

math/src/elliptic_curve/short_weierstrass/curves/bls12_381/compression.rs

+    point: &ShortWeierstrassProjectivePoint<SW>,
+) -> bool {
+    let inf = ShortWeierstrassProjectivePoint::<SW>::neutral_element();
+    let aux_point = point.operate_with_self(SUBGROUP_ORDER);
    inf == aux_point


Here you can use a is a specific optimized method to check whether a point is the neutral element (here). In the case of Short Weierstrass, a point on the curve is the neutral element if and only if its Z coordinate is zero. So there's no need to compare all three coordinates.

PatStiles · 2023-09-21T22:11:22Z

@MauroToscano @schouhy Given your feedback I elected to add the subgroup check as a trait function for IsGroup. This may lead to deduplication if we have multiple coordinates system. Let me know your thoughts!

schouhy

Good work! Looks good. I left a suggestion and a request to remove a comment.

schouhy · 2023-09-22T15:26:20Z

math/src/cyclic_group.rs

+    fn is_in_subgroup<T: IsUnsignedInteger>(&self, order: T) -> bool {
+        let aux_point = self.operate_with_self(order);
+        aux_point.is_neutral_element()
+    }
+


Suggested change

fn is_in_subgroup<T: IsUnsignedInteger>(&self, order: T) -> bool {

let aux_point = self.operate_with_self(order);

aux_point.is_neutral_element()

}

/// This method checks whether the element belongs to the `order`-torsion subgroup.

/// That is, the subgroup formed by all elements `P` such that (in additive notation) `order * P` is the neutral element.

fn is_in_subgroup<T: IsUnsignedInteger>(&self, order: T) -> bool {

self.operate_with_self(order).is_neutral_element()

}

schouhy · 2023-09-22T15:28:49Z

math/src/elliptic_curve/short_weierstrass/curves/bls12_381/pairing.rs

+    //NOTE: Should we switch this to an result?
+    //Should we continue and skip elements if it doesn't work out


A Result is good. Let's remove this comment

PatStiles · 2023-09-25T22:16:02Z

@schouhy @MauroToscano should be good to go.

RajeshRk18 · 2023-09-25T23:16:22Z

@schouhy @MauroToscano I have already implemented subgroup checks using endomorphism #543 (comment)

Should I make a separate PR titled optimized subgroup checks?

PatStiles · 2023-09-27T04:45:13Z

@MauroToscano @schouhy I've started to work on adding Jacobian Coordinates so we can add the endomorphism used in gnark Should I continue with this pr or open a new one?

schouhy · 2023-09-27T14:35:17Z

@PatStiles is gnark's algorithm is strictly dependant on jacobian coordinates? That seems odd. I would expect it to be independent of the inner representation of the point. I'll check it out if I have time. But it would be useful if you could explain why we need jacobian coordinates for that.

MauroToscano · 2023-09-27T14:43:41Z

New PR, keep this one naive and small

PatStiles · 2023-09-27T14:54:10Z

@schouhy I haven't dived very deep into the implementation yet but will do so this afternoon. I noticed first when going over the gnark implementation the addition and mul operations differ. I investigating the alg reference https://hyperelliptic.org/EFD/g1p/auto-shortw-jacobian-3.html#addition-add-2007-bl are found they references these slides (pg 17) here which used jacobian coordinates.

One thing to note is this could be a semantic change only as gnark's implementation has both jacobian and extended jacobian coordinates which appear to be whats normally referenced as Jacobian.

Looking at there FromAffine() I think it is projective

Let me know your thoughts!

schouhy

Hi! Thanks for your work!
I left a few requests. Let me know what you think.

Also, I would move all the code relevant to subgrup membership to the curve.rs file instead of pairing.rs.

schouhy · 2023-10-24T11:25:17Z

math/src/elliptic_curve/short_weierstrass/curves/bls12_381/pairing.rs

+    "5f19672fdf76ce51ba69c6076a0f77eaddb3a93be6f89688de17d813620a00022e01fffffffefffe",
+);
+
+pub const ENDO_U: BLS12381TwistCurveFieldElement =


Please add docs about what these constant represent and add references to their sources. Also, prefer more declarative variable names for code readability: e.g. TWIST_ENDOMORPHISM_U

schouhy · 2023-10-24T11:28:01Z

math/src/elliptic_curve/short_weierstrass/curves/bls12_381/pairing.rs

+]);
+
+impl ShortWeierstrassProjectivePoint<BLS12381Curve> {
+    /// Returns phi(p) where `phi: (x,y)->(ux,y)` and `u` is the Cube Root of Unity in `G_1`


roots of unity live in the prime field, not in the group.

Suggested change

/// Returns phi(p) where `phi: (x,y)->(ux,y)` and `u` is the Cube Root of Unity in `G_1`

/// Returns phi(p) where `phi: (x,y)->(ux,y)` and `u` is the Cube Root of Unity in the base prime field

schouhy · 2023-10-24T11:33:59Z

math/src/elliptic_curve/short_weierstrass/curves/bls12_381/pairing.rs

+    fn is_on_curve(&self) -> bool {
+        let [x, y, z] = self.coordinates();
+        y.square() * z == (x.square() * x) + (z.square() * z * BLS12381TwistCurve::b())
+    }


An instance of ShortWeierstrassProjectivePoint<BLS12381TwistCurve> should always satisfy the curve equation. If not, the constructor is failing. No need to check this.
And in any case, the curve has already a definning_equation method that implements this

schouhy · 2023-10-24T11:43:26Z

math/src/elliptic_curve/short_weierstrass/curves/bls12_381/pairing.rs

+        y.square() * z == (x.square() * x) + (z.square() * z * BLS12381TwistCurve::b())
+    }
+
+    // psi(p) = u o frob o u**-1 where u:E'->E iso from the twist to E


Suggested change

// psi(p) = u o frob o u**-1 where u:E'->E iso from the twist to E

// psi(p) = u o frob o u**-1, where u is the isomorphism u:E'(𝔽ₚ₆) −> E(𝔽ₚ₁₂) from the twist to E

schouhy · 2023-10-24T11:44:53Z

math/src/elliptic_curve/short_weierstrass/curves/bls12_381/pairing.rs

+    fn psi(&self) -> Self {
+        let [x, y, z] = self.coordinates();
+        Self::new([
+            x.conjugate() * ENDO_U,
+            y.conjugate() * ENDO_V,
+            z.conjugate(),
+        ])
+    }


This morphism should have minimal polynomial X^2 - tX + q (https://eprint.iacr.org/2022/352.pdf 4.2 equation (7)). Please add a test that checks this in a few points.
Also, please add a reference to the source of the formula.

schouhy · 2023-10-24T11:45:59Z

math/src/elliptic_curve/short_weierstrass/curves/bls12_381/pairing.rs

+    // https://eprint.iacr.org/2019/814.pdf, 3.1
+    // z*psi^3 - psi^2 + 1 = 0 <==> -z*(psi o phi) + phi + 1 = 0
+    pub fn is_in_subgroup(&self) -> bool {
+        let res = self
+            .psi()
+            .operate_with(&self.operate_with_self(MILLER_LOOP_CONSTANT));
+        res.is_on_curve() && res.is_neutral_element()
+    }
+}


The code doesn't match the description and reference of the comment. You prolly mean https://eprint.iacr.org/2022/352.pdf 4.2. Also, please make it a doc string and explain what z is.

schouhy · 2023-10-24T11:49:09Z

math/src/elliptic_curve/short_weierstrass/curves/bls12_381/pairing.rs

+impl FieldElement<Degree2ExtensionField> {
+    //TODO: Expose this function in extensions/qaudratic.rs
+    pub fn conjugate(&self) -> Self {
+        let [a, b] = self.value();
+        Self::new([a.clone(), -b])


Degree2ExtensionField is a custom implementation that doesn't use the generic one in extensions/quadratic.rs.
Please move this implementation to field_extension.rs where Degree2ExtensionField is defined

schouhy · 2023-10-24T11:52:48Z

math/src/elliptic_curve/short_weierstrass/curves/bls12_381/pairing.rs

+    fn generator_g2_is_on_curve() {
+        let g = BLS12381TwistCurve::generator();
+        assert!(g.is_on_curve())
+    }
+
+    #[test]
+    fn arbitrary_g2_point_is_on_curve() {
+        let p = BLS12381TwistCurve::generator().operate_with_self(20_u64);
+        assert!(p.is_on_curve())
+    }


These shouldn't be unit tests of pairing. Please move them to curve.rs and use the defining_equation method instead of is_on_curve

schouhy · 2023-10-24T11:55:33Z

math/src/elliptic_curve/short_weierstrass/curves/bls12_381/pairing.rs

+    fn generator_g1_is_in_subgroup() {
+        let g = BLS12381Curve::generator();
+        assert!(g.is_in_subgroup())
+    }
+
+    #[test]
+    fn arbitrary_g1_point_is_in_subgroup() {
+        let p = BLS12381Curve::generator().operate_with_self(20_u64);
+        assert!(p.is_in_subgroup())
+    }
+
+    #[test]
+    fn generator_g2_is_in_subgroup() {
+        let g = BLS12381TwistCurve::generator();
+        assert!(g.is_in_subgroup())
+    }
+
+    #[test]
+    fn arbitrary_g2_point_is_in_subgroup() {
+        let p = BLS12381TwistCurve::generator().operate_with_self(20_u64);
+        assert!(p.is_in_subgroup())
+    }


Please add tests of points that are in the curve but not in the subgroup, where the p.is_in_subgroup returns false.

PatStiles requested review from schouhy, ajgara and a team as code owners September 19, 2023 03:33

MauroToscano requested changes Sep 21, 2023

View reviewed changes

schouhy suggested changes Sep 21, 2023

View reviewed changes

schouhy suggested changes Sep 22, 2023

View reviewed changes

schouhy reviewed Sep 22, 2023

View reviewed changes

RajeshRk18 mentioned this pull request Sep 27, 2023

Optimized subgroup checks #580

Closed

9 tasks

PatStiles changed the title ~~feat(pairing): add naive subgroup check to pairing~~ feat(pairing): BLS12-381 Subgroup Check Oct 9, 2023

MauroToscano approved these changes Oct 23, 2023

View reviewed changes

schouhy suggested changes Oct 24, 2023

View reviewed changes

PatStiles closed this Oct 31, 2023

PatStiles force-pushed the feat/pairing_subgroup_check branch from 47f60be to dff5f2d Compare October 31, 2023 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pairing): BLS12-381 Subgroup Check #559

feat(pairing): BLS12-381 Subgroup Check #559

PatStiles commented Sep 19, 2023 •

edited

Loading

codecov-commenter commented Sep 19, 2023 •

edited

Loading

MauroToscano commented Sep 21, 2023

MauroToscano left a comment •

edited

Loading

MauroToscano Sep 21, 2023

MauroToscano commented Sep 21, 2023 •

edited

Loading

schouhy Sep 21, 2023

PatStiles Sep 21, 2023

PatStiles commented Sep 21, 2023

schouhy left a comment

schouhy Sep 22, 2023

schouhy Sep 22, 2023

PatStiles commented Sep 25, 2023

RajeshRk18 commented Sep 25, 2023 •

edited

Loading

PatStiles commented Sep 27, 2023

schouhy commented Sep 27, 2023

MauroToscano commented Sep 27, 2023

PatStiles commented Sep 27, 2023 •

edited

Loading

schouhy left a comment

schouhy Oct 24, 2023

schouhy Oct 24, 2023

schouhy Oct 24, 2023

schouhy Oct 24, 2023

schouhy Oct 24, 2023

schouhy Oct 24, 2023 •

edited

Loading

schouhy Oct 24, 2023

schouhy Oct 24, 2023

schouhy Oct 24, 2023

		//NOTE: Should we switch this to an result?
		//Should we continue and skip elements if it doesn't work out

	/// Returns phi(p) where `phi: (x,y)->(ux,y)` and `u` is the Cube Root of Unity in `G_1`
	/// Returns phi(p) where `phi: (x,y)->(ux,y)` and `u` is the Cube Root of Unity in the base prime field

	// psi(p) = u o frob o u**-1 where u:E'->E iso from the twist to E
	// psi(p) = u o frob o u**-1, where u is the isomorphism u:E'(𝔽ₚ₆) −> E(𝔽ₚ₁₂) from the twist to E

feat(pairing): BLS12-381 Subgroup Check #559

feat(pairing): BLS12-381 Subgroup Check #559

Conversation

PatStiles commented Sep 19, 2023 • edited Loading

Add Naive Subgroup Check to pairing

Description

Checklist

codecov-commenter commented Sep 19, 2023 • edited Loading

Codecov Report

MauroToscano commented Sep 21, 2023

MauroToscano left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MauroToscano commented Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatStiles commented Sep 21, 2023

schouhy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatStiles commented Sep 25, 2023

RajeshRk18 commented Sep 25, 2023 • edited Loading

PatStiles commented Sep 27, 2023

schouhy commented Sep 27, 2023

MauroToscano commented Sep 27, 2023

PatStiles commented Sep 27, 2023 • edited Loading

schouhy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schouhy Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PatStiles commented Sep 19, 2023 •

edited

Loading

codecov-commenter commented Sep 19, 2023 •

edited

Loading

MauroToscano left a comment •

edited

Loading

MauroToscano commented Sep 21, 2023 •

edited

Loading

RajeshRk18 commented Sep 25, 2023 •

edited

Loading

PatStiles commented Sep 27, 2023 •

edited

Loading

schouhy Oct 24, 2023 •

edited

Loading