Change elliptic curve new point function (#970)

* change edwards new * fix edwards, montgomery and weierstrass new * fix short weierstrass new * fix ate pairing test. change new point at infinity * change edwards new * change jacobian new short weierstrass * fix lint * change debug assert jacobian weierstrass * fix defining_equation_jacobian * fix operate_with for jacobian coordinates * fix babybear benches * solve p3 new version conflicts * fix edwards new: tiny jubjub working * Add safety comments for edwards * add safety docs * remove duplicated comments * fix rust doc formatting * Remove unchecked from wrap to compare performancesave work * Revert "Remove unchecked from wrap to compare performancesave work" This reverts commit 41f453b. * fix clippy * remove unchecked from unwrap. Save work * fix bug in bn254 bench * add Oppen comments * fix comments and remove redundant debug_assert * fix comment --------- Co-authored-by: Nicole <nicole@Nicoles-MacBook-Air.local> Co-authored-by: jotabulacios <jbulacios@fi.uba.ar> Co-authored-by: jotabulacios <45471455+jotabulacios@users.noreply.github.com>
lambdaclass · Feb 24, 2025 · 7e94a19 · 7e94a19
1 parent d78d18a
commit 7e94a19
Show file tree

Hide file tree

Showing 37 changed files with 619 additions and 175 deletions.
diff --git a/crypto/src/hash/monolith/mod.rs b/crypto/src/hash/monolith/mod.rs
@@ -120,8 +120,8 @@ impl<const WIDTH: usize, const NUM_FULL_ROUNDS: usize> MonolithMersenne31<WIDTH,
     // S-box lookups
     fn bars(&self, state: &mut [u32]) {
         for state in state.iter_mut().take(NUM_BARS) {
-            *state = (self.lookup2[(*state >> 16) as u16 as usize] as u32) << 16
-                | self.lookup1[*state as u16 as usize] as u32;
+            *state = ((self.lookup2[(*state >> 16) as u16 as usize] as u32) << 16)
+                | (self.lookup1[*state as u16 as usize] as u32);
         }
     }
 

diff --git a/math/benches/criterion_elliptic_curve.rs b/math/benches/criterion_elliptic_curve.rs
@@ -10,6 +10,6 @@ use elliptic_curves::{
 criterion_group!(
     name = elliptic_curve_benches;
     config = Criterion::default().with_profiler(PProfProfiler::new(100, Output::Flamegraph(None)));
-    targets = bn_254_elliptic_curve_benchmarks,bls12_381_elliptic_curve_benchmarks,bls12_377_elliptic_curve_benchmarks
+    targets = bn_254_elliptic_curve_benchmarks, bls12_377_elliptic_curve_benchmarks, bls12_381_elliptic_curve_benchmarks
 );
 criterion_main!(elliptic_curve_benches);
diff --git a/math/benches/elliptic_curves/bls12_377.rs b/math/benches/elliptic_curves/bls12_377.rs
@@ -15,7 +15,7 @@ pub fn bls12_377_elliptic_curve_benchmarks(c: &mut Criterion) {
     let a = BLS12377Curve::generator().operate_with_self(a_val);
     let b = BLS12377Curve::generator().operate_with_self(b_val);
 
-    let mut group = c.benchmark_group("BLS12-381 Ops");
+    let mut group = c.benchmark_group("BLS12-377 Ops");
     group.significance_level(0.1).sample_size(10000);
     group.throughput(criterion::Throughput::Elements(1));
 

diff --git a/math/benches/elliptic_curves/bn_254.rs b/math/benches/elliptic_curves/bn_254.rs
@@ -31,14 +31,14 @@ pub fn bn_254_elliptic_curve_benchmarks(c: &mut Criterion) {
     let a_g1 = BN254Curve::generator().operate_with_self(a_val);
     let b_g1 = BN254Curve::generator().operate_with_self(b_val);
 
-    let a_g2 = BN254TwistCurve::generator().operate_with_self(a_val);
+    let a_g2 = BN254TwistCurve::generator().operate_with_self(b_val);
     let b_g2 = BN254TwistCurve::generator().operate_with_self(b_val);
     let f_12 = Fp12E::from_coefficients(&[
         "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12",
     ]);
     let f_2 = Fp2E::new([FpE::from(a_val as u64), FpE::from(b_val as u64)]);
 
-    let miller_loop_output = miller_optimized(&a_g1, &a_g2);
+    let miller_loop_output = miller_optimized(&a_g1.to_affine(), &a_g2.to_affine());
 
     let mut group = c.benchmark_group("BN254 Ops");
 

diff --git a/math/benches/fields/baby_bear.rs b/math/benches/fields/baby_bear.rs
@@ -15,7 +15,7 @@ use lambdaworks_math::field::{
 
 use p3_baby_bear::BabyBear;
 use p3_field::extension::BinomialExtensionField;
-use p3_field::{Field, FieldAlgebra};
+use p3_field::{Field, PrimeCharacteristicRing};
 
 use rand::random;
 use rand::Rng;
@@ -83,18 +83,25 @@ pub fn rand_babybear_u32_fp4_elements(num: usize) -> Vec<(Fp4Eu32, Fp4Eu32)> {
     }
     result
 }
-
+fn random_baby_bear<R: Rng>(rng: &mut R) -> BabyBear {
+    BabyBear::new(rng.gen::<u32>())
+}
 fn rand_babybear_elements_p3(num: usize) -> Vec<(BabyBear, BabyBear)> {
     let mut rng = rand::thread_rng();
     (0..num)
-        .map(|_| (rng.gen::<BabyBear>(), rng.gen::<BabyBear>()))
+        .map(|_| (random_baby_bear(&mut rng), random_baby_bear(&mut rng)))
         .collect()
 }
 
 fn rand_babybear_fp4_elements_p3(num: usize) -> Vec<(EF4, EF4)> {
     let mut rng = rand::thread_rng();
     (0..num)
-        .map(|_| (rng.gen::<EF4>(), rng.gen::<EF4>()))
+        .map(|_| {
+            (
+                EF4::from(random_baby_bear(&mut rng)),
+                EF4::from(random_baby_bear(&mut rng)),
+            )
+        })
         .collect()
 }
 

diff --git a/math/benches/fields/mersenne31.rs b/math/benches/fields/mersenne31.rs
@@ -15,7 +15,6 @@ pub type Fp2E = FieldElement<Degree2ExtensionField>;
 pub type Fp4E = FieldElement<Degree4ExtensionField>;
 
 #[inline(never)]
-#[no_mangle]
 #[export_name = "util::rand_mersenne31_field_elements"]
 pub fn rand_field_elements(num: usize) -> Vec<(F, F)> {
     let mut result = Vec::with_capacity(num);

diff --git a/math/benches/fields/mersenne31_montgomery.rs b/math/benches/fields/mersenne31_montgomery.rs
@@ -22,7 +22,6 @@ pub type F = FieldElement<Mersenne31MontgomeryPrimeField>;
 const NUM_LIMBS: usize = 1;
 
 #[inline(never)]
-#[no_mangle]
 #[export_name = "util::rand_mersenne31_mont_field_elements"]
 pub fn rand_field_elements(num: usize) -> Vec<(F, F)> {
     let mut result = Vec::with_capacity(num);

diff --git a/math/benches/fields/stark252.rs b/math/benches/fields/stark252.rs
@@ -21,7 +21,6 @@ use rand::random;
 pub type F = FieldElement<Stark252PrimeField>;
 
 #[inline(never)]
-#[no_mangle]
 #[export_name = "util::rand_field_elements"]
 pub fn rand_field_elements(num: usize) -> Vec<(F, F)> {
     let mut result = Vec::with_capacity(num);

diff --git a/math/benches/utils/stark252_utils.rs b/math/benches/utils/stark252_utils.rs
@@ -14,14 +14,12 @@ pub type FE = FieldElement<F>;
 
 // NOTE: intentional duplicate to help IAI skip setup code
 #[inline(never)]
-#[no_mangle]
 #[export_name = "util::bitrev_permute"]
 pub fn bitrev_permute(input: &mut [FE]) {
     in_place_bit_reverse_permute(input);
 }
 
 #[inline(never)]
-#[no_mangle]
 #[export_name = "util::rand_field_elements"]
 pub fn rand_field_elements(order: u64) -> Vec<FE> {
     let mut result = Vec::with_capacity(1 << order);
@@ -33,14 +31,12 @@ pub fn rand_field_elements(order: u64) -> Vec<FE> {
 }
 
 #[inline(never)]
-#[no_mangle]
 #[export_name = "util::rand_poly"]
 pub fn rand_poly(order: u64) -> Polynomial<FE> {
     Polynomial::new(&rand_field_elements(order))
 }
 
 #[inline(never)]
-#[no_mangle]
 #[export_name = "util::get_twiddles"]
 pub fn twiddles(order: u64, config: RootsConfig) -> Vec<FE> {
     get_twiddles(order, config).unwrap()

diff --git a/math/src/elliptic_curve/edwards/curves/bandersnatch/curve.rs b/math/src/elliptic_curve/edwards/curves/bandersnatch/curve.rs
@@ -12,18 +12,35 @@ impl IsEllipticCurve for BandersnatchCurve {
     type BaseField = BaseBandersnatchFieldElement;
     type PointRepresentation = EdwardsProjectivePoint<Self>;
 
-    // Values are from https://github.com/arkworks-rs/curves/blob/5a41d7f27a703a7ea9c48512a4148443ec6c747e/ed_on_bls12_381_bandersnatch/src/curves/mod.rs#L120
-    // Converted to Hex
+    /// Returns the generator point of the Bandersnatch curve.
+    ///
+    /// The generator point is defined with coordinates `(x, y, 1)`, where `x` and `y`
+    /// are precomputed constants that belong to the curve.
+    ///
+    /// # Safety
+    ///
+    /// - The generator values are taken from the [Arkworks implementation](https://github.com/arkworks-rs/curves/blob/5a41d7f27a703a7ea9c48512a4148443ec6c747e/ed_on_bls12_381_bandersnatch/src/curves/mod.rs#L120)
+    ///   and have been converted to hexadecimal.
+    /// - `unwrap()` does not panic because:
+    ///   - The generator point is **known to be valid** on the curve.
+    ///   - The function only uses **hardcoded** and **verified** constants.
+    /// - This function should **never** be modified unless the new generator is fully verified.
     fn generator() -> Self::PointRepresentation {
-        Self::PointRepresentation::new([
+        // SAFETY:
+        // - The generator point coordinates (x, y) are taken from a well-tested,
+        //   verified implementation.
+        // - The constructor will only fail if the values are invalid, which is
+        //   impossible given that they are constants taken from a trusted source.
+        let point = Self::PointRepresentation::new([
             FieldElement::<Self::BaseField>::new_base(
                 "29C132CC2C0B34C5743711777BBE42F32B79C022AD998465E1E71866A252AE18",
             ),
             FieldElement::<Self::BaseField>::new_base(
                 "2A6C669EDA123E0F157D8B50BADCD586358CAD81EEE464605E3167B6CC974166",
             ),
             FieldElement::one(),
-        ])
+        ]);
+        point.unwrap()
     }
 }
 

diff --git a/math/src/elliptic_curve/edwards/curves/ed448_goldilocks.rs b/math/src/elliptic_curve/edwards/curves/ed448_goldilocks.rs
@@ -13,13 +13,27 @@ impl IsEllipticCurve for Ed448Goldilocks {
     type BaseField = P448GoldilocksPrimeField;
     type PointRepresentation = EdwardsProjectivePoint<Self>;
 
-    /// Taken from https://www.rfc-editor.org/rfc/rfc7748#page-6
+    /// Returns the generator point of the Ed448-Goldilocks curve.
+    ///
+    /// This generator is taken from [RFC 7748](https://www.rfc-editor.org/rfc/rfc7748#page-6).
+    ///
+    /// # Safety
+    ///
+    /// - The generator coordinates `(x, y, 1)` are well-known, predefined constants.
+    /// - `unwrap()` is used because the values are **known to be valid** points
+    ///   on the Ed448-Goldilocks curve.
+    /// - This function must **not** be modified unless new constants are mathematically verified.
     fn generator() -> Self::PointRepresentation {
-        Self::PointRepresentation::new([
+        // SAFETY:
+        // - These values are taken from RFC 7748 and are known to be valid.
+        // - `unwrap()` is safe because `new()` will only fail if the point is
+        //   invalid, which is **not possible** with hardcoded, verified values.
+        let point= Self::PointRepresentation::new([
             FieldElement::<Self::BaseField>::from_hex("4f1970c66bed0ded221d15a622bf36da9e146570470f1767ea6de324a3d3a46412ae1af72ab66511433b80e18b00938e2626a82bc70cc05e").unwrap(),
             FieldElement::<Self::BaseField>::from_hex("693f46716eb6bc248876203756c9c7624bea73736ca3984087789c1e05a0c2d73ad3ff1ce67c39c4fdbd132c4ed7c8ad9808795bf230fa14").unwrap(),
             FieldElement::one(),
-        ])
+        ]);
+        point.unwrap()
     }
 }
 

diff --git a/math/src/elliptic_curve/edwards/curves/tiny_jub_jub.rs b/math/src/elliptic_curve/edwards/curves/tiny_jub_jub.rs
@@ -14,12 +14,26 @@ impl IsEllipticCurve for TinyJubJubEdwards {
     type BaseField = U64PrimeField<13>;
     type PointRepresentation = EdwardsProjectivePoint<Self>;
 
+    /// Returns the generator point of the TinyJubJub Edwards curve.
+    ///
+    /// This generator is taken from **Moonmath Manual (page 97)**.
+    ///
+    /// # Safety
+    ///
+    /// - The generator coordinates `(8, 5, 1)` are **predefined** and belong to the TinyJubJub curve.
+    /// - `unwrap()` is used because the generator is a **verified valid point**,
+    ///   meaning there is **no risk** of runtime failure.
+    /// - This function must **not** be modified unless the new generator is mathematically verified.
     fn generator() -> Self::PointRepresentation {
-        Self::PointRepresentation::new([
+        // SAFETY:
+        // - The generator point `(8, 5, 1)` is **mathematically valid** on the curve.
+        // - `unwrap()` is safe because we **know** the point satisfies the curve equation.
+        let point = Self::PointRepresentation::new([
             FieldElement::from(8),
             FieldElement::from(5),
             FieldElement::one(),
-        ])
+        ]);
+        point.unwrap()
     }
 }
 

diff --git a/math/src/elliptic_curve/edwards/point.rs b/math/src/elliptic_curve/edwards/point.rs
@@ -12,10 +12,27 @@ use super::traits::IsEdwards;
 #[derive(Clone, Debug)]
 pub struct EdwardsProjectivePoint<E: IsEllipticCurve>(ProjectivePoint<E>);
 
-impl<E: IsEllipticCurve> EdwardsProjectivePoint<E> {
+impl<E: IsEllipticCurve + IsEdwards> EdwardsProjectivePoint<E> {
     /// Creates an elliptic curve point giving the projective [x: y: z] coordinates.
-    pub fn new(value: [FieldElement<E::BaseField>; 3]) -> Self {
-        Self(ProjectivePoint::new(value))
+    pub fn new(value: [FieldElement<E::BaseField>; 3]) -> Result<Self, EllipticCurveError> {
+        let (x, y, z) = (&value[0], &value[1], &value[2]);
+
+        // The point at infinity is (0, 1, 1).
+        // We convert every (0, y, y) into the infinity.
+        if x == &FieldElement::<E::BaseField>::zero() && z == y {
+            return Ok(Self(ProjectivePoint::new([
+                FieldElement::<E::BaseField>::zero(),
+                FieldElement::<E::BaseField>::one(),
+                FieldElement::<E::BaseField>::one(),
+            ])));
+        }
+        if z != &FieldElement::<E::BaseField>::zero()
+            && E::defining_equation_projective(x, y, z) == FieldElement::<E::BaseField>::zero()
+        {
+            Ok(Self(ProjectivePoint::new(value)))
+        } else {
+            Err(EllipticCurveError::InvalidPoint)
+        }
     }
 
     /// Returns the `x` coordinate of the point.
@@ -56,35 +73,50 @@ impl<E: IsEdwards> FromAffine<E::BaseField> for EdwardsProjectivePoint<E> {
     fn from_affine(
         x: FieldElement<E::BaseField>,
         y: FieldElement<E::BaseField>,
-    ) -> Result<Self, crate::elliptic_curve::traits::EllipticCurveError> {
-        if E::defining_equation(&x, &y) != FieldElement::zero() {
-            Err(EllipticCurveError::InvalidPoint)
-        } else {
-            let coordinates = [x, y, FieldElement::one()];
-            Ok(EdwardsProjectivePoint::new(coordinates))
-        }
+    ) -> Result<Self, EllipticCurveError> {
+        let coordinates = [x, y, FieldElement::one()];
+        EdwardsProjectivePoint::new(coordinates)
     }
 }
 
 impl<E: IsEllipticCurve> Eq for EdwardsProjectivePoint<E> {}
 
 impl<E: IsEdwards> IsGroup for EdwardsProjectivePoint<E> {
-    /// The point at infinity.
+    /// Returns the point at infinity (neutral element) in projective coordinates.
+    ///
+    /// # Safety
+    ///
+    /// - The values `[0, 1, 1]` are the **canonical representation** of the neutral element
+    ///   in the Edwards curve, meaning they are guaranteed to be a valid point.
+    /// - `unwrap()` is used because this point is **known** to be valid, so
+    ///   there is no need for additional runtime checks.
     fn neutral_element() -> Self {
-        Self::new([
+        // SAFETY:
+        // - `[0, 1, 1]` is a mathematically verified neutral element in Edwards curves.
+        // - `unwrap()` is safe because this point is **always valid**.
+        let point = Self::new([
             FieldElement::zero(),
             FieldElement::one(),
             FieldElement::one(),
-        ])
+        ]);
+        point.unwrap()
     }
 
     fn is_neutral_element(&self) -> bool {
         let [px, py, pz] = self.coordinates();
         px == &FieldElement::zero() && py == pz
     }
 
-    /// Computes the addition of `self` and `other`.
-    /// Taken from "Moonmath" (Eq 5.38, page 97)
+    /// Computes the addition of `self` and `other` using the Edwards curve addition formula.
+    ///
+    /// This implementation follows Equation (5.38) from "Moonmath" (page 97).
+    ///
+    /// # Safety
+    ///
+    /// - The function assumes both `self` and `other` are valid points on the curve.
+    /// - The resulting coordinates are computed using a well-defined formula that
+    ///   maintains the elliptic curve invariants.
+    /// - `unwrap()` is safe because the formula guarantees the result is valid.
     fn operate_with(&self, other: &Self) -> Self {
         // This avoids dropping, which in turn saves us from having to clone the coordinates.
         let (s_affine, o_affine) = (self.to_affine(), other.to_affine());
@@ -103,13 +135,24 @@ impl<E: IsEdwards> IsGroup for EdwardsProjectivePoint<E> {
         let num_s2 = &y1y2 - E::a() * &x1x2;
         let den_s2 = &one - &dx1x2y1y2;
 
-        Self::new([&num_s1 / &den_s1, &num_s2 / &den_s2, one])
+        // SAFETY: The creation of the result point is safe because the inputs are always points that belong to the curve.
+        let point = Self::new([&num_s1 / &den_s1, &num_s2 / &den_s2, one]);
+        point.unwrap()
     }
 
     /// Returns the additive inverse of the projective point `p`
+    ///  
+    /// # Safety
+    ///
+    /// - Negating the x-coordinate of a valid Edwards point results in another valid point.
+    /// - `unwrap()` is safe because negation does not break the curve equation.
     fn neg(&self) -> Self {
         let [px, py, pz] = self.coordinates();
-        Self::new([-px, py.clone(), pz.clone()])
+        // SAFETY:
+        // - The negation formula for Edwards curves is well-defined.
+        // - The result remains a valid curve point.
+        let point = Self::new([-px, py.clone(), pz.clone()]);
+        point.unwrap()
     }
 }
 
@@ -156,17 +199,20 @@ mod tests {
             FieldElement::from(5),
             FieldElement::from(5),
             FieldElement::from(1),
-        ]);
+        ])
+        .unwrap();
         let q = EdwardsProjectivePoint::<TinyJubJubEdwards>::new([
             FieldElement::from(8),
             FieldElement::from(5),
             FieldElement::from(1),
-        ]);
+        ])
+        .unwrap();
         let expected = EdwardsProjectivePoint::<TinyJubJubEdwards>::new([
             FieldElement::from(0),
             FieldElement::from(1),
             FieldElement::from(1),
-        ]);
+        ])
+        .unwrap();
         assert_eq!(p.operate_with(&q), expected);
     }