Optimized subgroup checks #580

RajeshRk18 · 2023-09-27T14:59:33Z

Optimized subgroup checks using Endomorphism

Description

Existing subgroup check involves mutliplying a point $P$ by $r$ where $r$ is the prime order subgroup. This PR improves the performance of subgroup checks for $G_1$ and $G_2$ by using endomorphism. Theoritically, we will have 2x speedup for $G_1$ and 4x speedup for $G_2$.

#559 implements Jacobian coordinates just for subgroup check which is not necessary.

Type of change

New feature
Bug fix
Optimization

Checklist

Linked to Github Issue
Unit tests added
This change requires new documentation.
- Documentation has been added/updated.
This change is an Optimization
- Benchmarks added/run

codecov-commenter · 2023-09-27T15:08:37Z

Codecov Report

Merging #580 (7904e42) into main (9825074) will increase coverage by 0.03%.
Report is 4 commits behind head on main.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##             main     #580      +/-   ##
==========================================
+ Coverage   95.48%   95.52%   +0.03%     
==========================================
  Files         112      110       -2     
  Lines       19008    19267     +259     
==========================================
+ Hits        18149    18404     +255     
- Misses        859      863       +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%)`	⬇️
..._curve/short_weierstrass/curves/bls12_381/curve.rs	`99.00% <100.00%> (+0.32%)`	⬆️
...rt_weierstrass/curves/bls12_381/field_extension.rs	`89.44% <100.00%> (+0.14%)`	⬆️
...c_curve/short_weierstrass/curves/bls12_381/sqrt.rs	`88.77% <ø> (ø)`
math/src/field/extensions/quadratic.rs	`96.79% <ø> (ø)`
math/src/unsigned_integer/montgomery.rs	`100.00% <ø> (ø)`
...urve/short_weierstrass/curves/bls12_381/pairing.rs	`99.11% <95.00%> (-0.43%)`	⬇️
math/src/errors.rs	`20.00% <0.00%> (-2.23%)`	⬇️
..._curve/short_weierstrass/curves/bls12_381/twist.rs	`97.16% <96.00%> (-0.64%)`	⬇️
... and 1 more

... and 10 files with indirect coverage changes

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

schouhy

Thanks for the PR! We'll be reviewing it in the next few days. Left some minor comments of some things I saw.

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

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

schouhy

Hi! I'm still going through it when I have time. I left some comments based on what I've reviewed so far. Let me know what you think.

schouhy · 2023-09-29T22:20:43Z

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

+        let fe: FieldElement<QuadraticExtensionField<MyQuadraticNonResidue>> =
+            QuadraticExtensionFieldElement::new([elem[0].clone(), elem[1].clone()]);
+        let frob_map = fe.conjugate();
+
+        [frob_map.value()[0].clone(), frob_map.value()[1].clone()]


Let's keep this simple and avoid all those clones.

Suggested change

let fe: FieldElement<QuadraticExtensionField<MyQuadraticNonResidue>> =

QuadraticExtensionFieldElement::new([elem[0].clone(), elem[1].clone()]);

let frob_map = fe.conjugate();

[frob_map.value()[0].clone(), frob_map.value()[1].clone()]

[elem[0].clone(), -&elem[1]]

I saw FieldElement<QuadraticExtensionField> has conjugate fn. But it constraints to type that implement HasQuadraticNonResidue. So, I implemented it (Named it so badly). If we can conjugate like you suggested, then why the FieldElement<QuadraticExtensionField> requires HasQuadraticNonResidue trait to be implemented to access conjugate fn?

QuadraticExtensionField is a generic implementation of a quadratic extension for general non quadratic residues. The struct Degree2Extension is an independent custom impementation just for BLS12 381 to leverage some optimizations coming from the fact that the quadratic non residue is -1. For example, multiplication formulas for quadratic extensions in general involve multiplying by the quadratic non residue. When the residue is -1, there are aimpler formulas and one can avoid multiplying by 1 or -1. That's why for bls 12 381 we don't use the generic quadratic extension implementation.

This should be explained in the docs of Degree2Extension. Sorry for that. I bet its confusing

QuadraticExtensionField is a generic implementation of a quadratic extension for general non quadratic residues. The struct Degree2Extension is an independent custom impementation just for BLS12 381 to leverage some optimizations coming from the fact that the quadratic non residue is -1. For example, multiplication formulas for quadratic extensions in general involve multiplying by the quadratic non residue. When the residue is -1, there are aimpler formulas and one can avoid multiplying by 1 or -1. That's why for bls 12 381 we don't use the generic quadratic extension implementation.

Thanks a lot for this explanation! Makes a lot more sense now. This is the true learning.

schouhy · 2023-09-29T22:21:27Z

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

+        FieldElement::<BLS12381PrimeField>::from_hex_unchecked("0"),
+    ]);
+
+/// Seed value for BLS-12 381 curve.


Suggested change

/// Seed value for BLS-12 381 curve.

/// Modulus of seed value for BLS-12 381 curve.

I got the value from here. There it is mentioned as seed.

Ah I see. For the sake of clarity, let's cite Construction 6.6 of the taxonomy of elliptic curves paper and mention that SEED is $-x$ where $x$ is the parameter of the Construction 6.6 for $k=12$. And add the formulas for the subgroup order and the field size in terms of $x$.
Check out this site to convert latex to ut8 and produce more legible formulas in code comments, (like x⁴ − x² + 1 instead of x^4 - x^2 + 1)

schouhy · 2023-09-29T22:28:29Z

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

+    /// Returns the boolean indicating whether the given field element is zero.
+    pub fn is_zero(elem: &<Self as IsField>::BaseType) -> bool {
+        elem[0] == FieldElement::<BLS12381PrimeField>::zero()
+            && elem[1] == FieldElement::<BLS12381PrimeField>::zero()
+    }
+}


There's no need to implement this. We already have this behavior with zero elements and eq.

Suggested change

/// Returns the boolean indicating whether the given field element is zero.

pub fn is_zero(elem: &<Self as IsField>::BaseType) -> bool {

elem[0] == FieldElement::<BLS12381PrimeField>::zero()

&& elem[1] == FieldElement::<BLS12381PrimeField>::zero()

}

}

schouhy · 2023-09-29T22:29:07Z

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

+        Degree2ExtensionField::is_zero(psi_plus_seed_times_p.z().value())
+            && BLS12381TwistCurve::is_on_curve(&psi_plus_seed_times_p)


Suggested change

Degree2ExtensionField::is_zero(psi_plus_seed_times_p.z().value())

&& BLS12381TwistCurve::is_on_curve(&psi_plus_seed_times_p)

&FieldElement::zero() == psi_plus_seed_times_p.z()

&& BLS12381TwistCurve::is_on_curve(&psi_plus_seed_times_p)

schouhy · 2023-09-29T22:55:42Z

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

+        let psi_plus_seed_times_p = self.psi().operate_with(&seed_times_p);
+
+        Degree2ExtensionField::is_zero(psi_plus_seed_times_p.z().value())
+            && BLS12381TwistCurve::is_on_curve(&psi_plus_seed_times_p)


If z is zero then the point is already the point at infinity and does belong to the curve. There's no need to check that. Did you mean BLS12381TwistCurve::is_on_curve(self) here?
Even in that case, self is already of type ShortWeierstrassProjectivePoint<BLS12381TwistCurve>, that shouldn't be necessary.

I saw that's how its implemented in Gnark. I should have listened to it that both checks for same thing.

Actually it doesn't really have to do with z equal to zero implying being equal to the point at infinity (since that already assumes that the point is on the curve). We should be able to rely on the fact that points of type ShortWeierstrassProjectivePoint<BLS12381TwistCurve> are on the curve. So a method such as is_on_curve should be redundant for instances of that type.

Maybe, ShortWeierstrassProjectivePoint can be made public at the crate level and add a builder fn that checks whether the point is on curve. Will that work?

Yes. Prolly we only need the two builder functions we already have: create_point_from_affine and deserialize . Both are checking the defining equation already.
But the attribute of the projective point is set to public. So probably one could instantiate a mutable point from outside this crate and later change its coordinates. That would brake the "is on curve" invariant. That's a problem.

You can assume in this PR that any instance of type ShortWeierstrassProjectivePoint<E> is a point that's on the curve E. Let's discuss the other issue somewhere else. Feel free to create an issue for it (if you can provide an example and a candidate solution, even better!).

schouhy · 2023-09-29T23:03:07Z

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

+    pub fn is_on_curve(point: &<Self as IsEllipticCurve>::PointRepresentation) -> bool {
+        let [x, y, z] = point.coordinates();
+
+        let lhs = {
+            let y_sq = <Self as IsEllipticCurve>::BaseField::square(y.value());
+            <Self as IsEllipticCurve>::BaseField::mul(&y_sq, z.value())
+        };
+
+        let rhs = {
+            let x_sq = <Self as IsEllipticCurve>::BaseField::square(x.value());
+            let x_cubed = <Self as IsEllipticCurve>::BaseField::mul(&x_sq, x.value());
+
+            let z_sq = <Self as IsEllipticCurve>::BaseField::square(z.value());
+            let z_cubed = <Self as IsEllipticCurve>::BaseField::mul(&z_sq, z.value());
+
+            let z_cubed_with_b =
+                <Self as IsEllipticCurve>::BaseField::mul(&z_cubed, Self::b().value());
+
+            <Self as IsEllipticCurve>::BaseField::add(&x_cubed, &z_cubed_with_b)
+        };
+
+        lhs == rhs
+    }
+}


Something along these lines should do the work. Slower performancewise probably. But I wouldn't worry about that, since we didn't tackle optimizations of elliptic curve operations yet.

Suggested change

pub fn is_on_curve(point: &<Self as IsEllipticCurve>::PointRepresentation) -> bool {

let [x, y, z] = point.coordinates();

let lhs = {

let y_sq = <Self as IsEllipticCurve>::BaseField::square(y.value());

<Self as IsEllipticCurve>::BaseField::mul(&y_sq, z.value())

};

let rhs = {

let x_sq = <Self as IsEllipticCurve>::BaseField::square(x.value());

let x_cubed = <Self as IsEllipticCurve>::BaseField::mul(&x_sq, x.value());

let z_sq = <Self as IsEllipticCurve>::BaseField::square(z.value());

let z_cubed = <Self as IsEllipticCurve>::BaseField::mul(&z_sq, z.value());

let z_cubed_with_b =

<Self as IsEllipticCurve>::BaseField::mul(&z_cubed, Self::b().value());

<Self as IsEllipticCurve>::BaseField::add(&x_cubed, &z_cubed_with_b)

};

lhs == rhs

}

}

pub fn is_on_curve(point: &<Self as IsEllipticCurve>::PointRepresentation) -> bool {

if point.z() == &FieldElement::zero() {

true

} else {

let point = point.to_affine();

Self::defining_equation(&point.x(), &point.y()) == FieldElement::zero()

}

}

On the other hand. I saw that you use this method on a point that's already of type ShortWeierstrassProjectivePoint<BLS12381TwistCurve>. That shouldn't be necessary. If the current state of the code allows one to instantiate an element of that type that's not on the curve, then that's a bigger problem. If you suspect that's the case, please feel free to create an issue.

Can you explain it more?

ShortWeierstrassProjectivePoint<BLS12381TwistCurve> is supposed to model a point on the twist. If there is an instance of it for which the defining equation of the twist does not hold, then there's a constructor failing to raise a creation error.

schouhy · 2023-09-29T23:22:35Z

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

+    fn conjugate_works() {
+        let a: QuadraticExtensionFieldElement<MyQuadraticNonResidue> =
+            QuadraticExtensionFieldElement::zero();
+        let mut expected = a.conjugate();
+        expected = expected.conjugate();
+
+        assert_eq!(a, expected);
+    }


This test is not testing that conjugate is properly implemented. It only tests that its square is the identity, which is also true for the identity function. I mean. conjugate could be implemented as the identity and this test would pass.
But besides that. Why are you adding this test? This is functionality of generic quadratic extension fields and its already tested here.
Please remove it and also remove MyQuadraticNonResidue that's only used for this I guess.

schouhy · 2023-09-29T23:23:33Z

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

+#[derive(Debug, Clone)]
+pub struct MyQuadraticNonResidue;
+
+impl HasQuadraticNonResidue for MyQuadraticNonResidue {
+    type BaseField = BLS12381PrimeField;
+
+    fn residue() -> FieldElement<Self::BaseField> {
+        -FieldElement::one()
+    }
+}


Code for testing purposes should go in the tests module

Suggested change

#[derive(Debug, Clone)]

pub struct MyQuadraticNonResidue;

impl HasQuadraticNonResidue for MyQuadraticNonResidue {

type BaseField = BLS12381PrimeField;

fn residue() -> FieldElement<Self::BaseField> {

-FieldElement::one()

}

}

Oh I see now that you were using it for the conjugate function. Not only for testing

schouhy · 2023-09-30T10:07:42Z

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

@@ -109,8 +95,7 @@ impl ShortWeierstrassProjectivePoint<BLS12381TwistCurve> {
        let seed_times_p = self.operate_with_self(SEED);
        let psi_plus_seed_times_p = self.psi().operate_with(&seed_times_p);

-        Degree2ExtensionField::is_zero(psi_plus_seed_times_p.z().value())
-            && BLS12381TwistCurve::is_on_curve(&psi_plus_seed_times_p)
+        psi_plus_seed_times_p.z() == &FieldElement::zero()


Short Weierstrass points have a is_neutral_element already implemented with this logic. You can use that here too.

schouhy

Hi, thanks for incorporating all the previous changes!

I found an error on the subgroup check for G2. You prolly removed the wrong line in the previous version. Anyway, I left a few requests that aim to catch that kind of mistakes.

schouhy · 2023-10-13T13:59:18Z

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

@@ -112,13 +104,13 @@ mod tests {
    fn test_zero_point() {
        let g1 = BLS12381Curve::generator();

-        assert!(super::check_point_is_in_subgroup(&g1));
+        assert!(g1.is_in_subgroup());
        let new_x = BLS12381FieldElement::zero();
        let new_y = BLS12381FieldElement::one() + BLS12381FieldElement::one();

        let false_point2 = G1Point::from_affine(new_x, new_y).unwrap();


Please rename the variable. There's no such thing as a false point. Add please a test that asserts that the point is not in the subgroup by other means. For example asserting that it's order does not divide the order of the subgroup being checked.

Suggested change

let false_point2 = G1Point::from_affine(new_x, new_y).unwrap();

let point_not_in_subgroup = G1Point::from_affine(new_x, new_y).unwrap();

schouhy · 2023-10-13T14:00:16Z

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

@@ -112,13 +104,13 @@ mod tests {
    fn test_zero_point() {


Please rename this test

Suggested change

fn test_zero_point() {

fn test_point_not_in_subgroup() {

schouhy · 2023-10-13T14:02:48Z

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

+        let seed_times_p = self.operate_with_self(SEED);
+        let psi_plus_seed_times_p = self.psi().operate_with(&seed_times_p);
+
+        BLS12381TwistCurve::is_on_curve(&psi_plus_seed_times_p)


This is not checking that $\psi(P) - uP$ is the neutral element. You probably mean something different here.

Suggested change

BLS12381TwistCurve::is_on_curve(&psi_plus_seed_times_p)

psi_plus_seed_times_p.is_neutral_element()

schouhy · 2023-10-13T14:03:50Z

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

+    #[test]
+    fn check_generator_g2_in_subgroup() {
+        let gen = BLS12381TwistCurve::generator();
+        assert!(gen.is_in_subgroup());
+    }
+
+    #[test]
+    fn check_arbitrary_g2_point_in_subgroup() {
+        let arb_point = BLS12381TwistCurve::generator().operate_with_self(420_u32);
+        assert!(arb_point.is_in_subgroup());
+    }


Please add a test that checks that is_in_subgroup returns false for a point not in $G_2$.

schouhy · 2023-10-13T14:06:35Z

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

 impl ShortWeierstrassProjectivePoint<BLS12381TwistCurve> {
+    /// Returns the "Untwist-Frobenius-Twist" endomorphism
+    pub fn psi(&self) -> Self {


Please add a test that checks that this endomorphism has minimal polynomial $X^2 - tX + q$. Maybe checking that on a few random points.

MauroToscano · 2023-11-23T17:24:15Z

This is stale

RajeshRk18 added 3 commits September 21, 2023 22:04

Add: Subgroup check

934b433

Add: Pairing error

e98ad66

Merge branch 'lambdaclass:main' into add/subgroup-checks

d7505a9

RajeshRk18 requested review from schouhy, ajgara and a team as code owners September 27, 2023 14:59

schouhy reviewed Sep 27, 2023

View reviewed changes

RajeshRk18 added 2 commits September 28, 2023 03:43

Merge branch 'lambdaclass:main' into add/subgroup-checks

76659ba

Fixes

17edd37

schouhy reviewed Sep 29, 2023

View reviewed changes

Refactor

c3374b0

schouhy reviewed Sep 30, 2023

View reviewed changes

Added Reference

7904e42

schouhy suggested changes Oct 13, 2023

View reviewed changes

MauroToscano closed this Nov 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimized subgroup checks #580

Optimized subgroup checks #580

RajeshRk18 commented Sep 27, 2023

codecov-commenter commented Sep 27, 2023 •

edited

Loading

schouhy left a comment •

edited

Loading

schouhy left a comment

schouhy Sep 29, 2023

RajeshRk18 Sep 30, 2023 •

edited

Loading

schouhy Sep 30, 2023 •

edited

Loading

schouhy Sep 30, 2023

RajeshRk18 Sep 30, 2023

schouhy Sep 29, 2023

RajeshRk18 Sep 30, 2023

schouhy Sep 30, 2023

schouhy Sep 29, 2023

schouhy Sep 29, 2023

schouhy Sep 29, 2023

RajeshRk18 Sep 30, 2023

schouhy Sep 30, 2023

RajeshRk18 Sep 30, 2023

schouhy Sep 30, 2023

schouhy Sep 29, 2023 •

edited

Loading

RajeshRk18 Sep 30, 2023

schouhy Sep 30, 2023

schouhy Sep 29, 2023 •

edited

Loading

schouhy Sep 29, 2023

schouhy Sep 30, 2023

schouhy Sep 30, 2023 •

edited

Loading

schouhy left a comment

schouhy Oct 13, 2023 •

edited

Loading

schouhy Oct 13, 2023

schouhy Oct 13, 2023

schouhy Oct 13, 2023

schouhy Oct 13, 2023

MauroToscano commented Nov 23, 2023

	/// Seed value for BLS-12 381 curve.
	/// Modulus of seed value for BLS-12 381 curve.

		Degree2ExtensionField::is_zero(psi_plus_seed_times_p.z().value())
		&& BLS12381TwistCurve::is_on_curve(&psi_plus_seed_times_p)

	let false_point2 = G1Point::from_affine(new_x, new_y).unwrap();
	let point_not_in_subgroup = G1Point::from_affine(new_x, new_y).unwrap();

	BLS12381TwistCurve::is_on_curve(&psi_plus_seed_times_p)
	psi_plus_seed_times_p.is_neutral_element()

Optimized subgroup checks #580

Optimized subgroup checks #580

Conversation

RajeshRk18 commented Sep 27, 2023

Optimized subgroup checks using Endomorphism

Description

Type of change

Checklist

codecov-commenter commented Sep 27, 2023 • edited Loading

Codecov Report

schouhy left a comment • edited Loading

Choose a reason for hiding this comment

schouhy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RajeshRk18 Sep 30, 2023 • edited Loading

Choose a reason for hiding this comment

schouhy Sep 30, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schouhy Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schouhy Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schouhy Sep 30, 2023 • edited Loading

Choose a reason for hiding this comment

schouhy left a comment

Choose a reason for hiding this comment

schouhy Oct 13, 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

Choose a reason for hiding this comment

MauroToscano commented Nov 23, 2023

codecov-commenter commented Sep 27, 2023 •

edited

Loading

schouhy left a comment •

edited

Loading

RajeshRk18 Sep 30, 2023 •

edited

Loading

schouhy Sep 30, 2023 •

edited

Loading

schouhy Sep 29, 2023 •

edited

Loading

schouhy Sep 29, 2023 •

edited

Loading

schouhy Sep 30, 2023 •

edited

Loading

schouhy Oct 13, 2023 •

edited

Loading