Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix field division by handling division by zero #969

Merged
merged 12 commits into from
Mar 5, 2025
6 changes: 5 additions & 1 deletion crypto/src/hash/monolith/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,11 @@ impl<const WIDTH: usize, const NUM_FULL_ROUNDS: usize> MonolithMersenne31<WIDTH,

for (i, x_i) in x.iter().enumerate() {
for (j, yj) in y.iter().enumerate() {
output[i] = F::add(&output[i], &F::div(&to_multiply[j], &F::add(x_i, yj)));
output[i] = F::add(
&output[i],
// We are using that x_i + yj != 0 because they are both much smaller than the modulus.
&F::div(&to_multiply[j], &F::add(x_i, yj)).unwrap(),
);
}
}

Expand Down
8 changes: 4 additions & 4 deletions math/benches/fields/baby_bear.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ pub fn babybear_u32_ops_benchmarks(c: &mut Criterion) {
group.bench_with_input(format!("Division {:?}", &i.len()), &i, |bench, i| {
bench.iter(|| {
for (x, y) in i {
black_box(black_box(x) / black_box(y));
black_box(black_box(x) / black_box(y)).unwrap();
}
});
});
Expand Down Expand Up @@ -207,7 +207,7 @@ pub fn babybear_u64_ops_benchmarks(c: &mut Criterion) {
group.bench_with_input(format!("Division {:?}", &i.len()), &i, |bench, i| {
bench.iter(|| {
for (x, y) in i {
black_box(black_box(x) / black_box(y));
black_box(black_box(x) / black_box(y)).unwrap();
}
});
});
Expand Down Expand Up @@ -259,7 +259,7 @@ pub fn babybear_u32_extension_ops_benchmarks(c: &mut Criterion) {
group.bench_with_input(format!("Inverse of Fp4 {:?}", &i.len()), &i, |bench, i| {
bench.iter(|| {
for (x, y) in i {
black_box(black_box(x) / black_box(y));
black_box(black_box(x) / black_box(y)).unwrap();
}
});
});
Expand Down Expand Up @@ -321,7 +321,7 @@ pub fn babybear_u64_extension_ops_benchmarks(c: &mut Criterion) {
group.bench_with_input(format!("Inverse of Fp4 {:?}", &i.len()), &i, |bench, i| {
bench.iter(|| {
for (x, y) in i {
black_box(black_box(x) / black_box(y));
black_box(black_box(x) / black_box(y)).unwrap();
}
});
});
Expand Down
2 changes: 1 addition & 1 deletion math/benches/fields/mersenne31.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ pub fn mersenne31_ops_benchmarks(c: &mut Criterion) {
group.bench_with_input(format!("div {:?}", &i.len()), &i, |bench, i| {
bench.iter(|| {
for (x, y) in i {
black_box(black_box(x) / black_box(y));
black_box(black_box(x) / black_box(y)).unwrap();
}
});
});
Expand Down
2 changes: 1 addition & 1 deletion math/benches/fields/mersenne31_montgomery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ pub fn mersenne31_mont_ops_benchmarks(c: &mut Criterion) {
group.bench_with_input(format!("div {:?}", &i.len()), &i, |bench, i| {
bench.iter(|| {
for (x, y) in i {
black_box(black_box(x) / black_box(y));
black_box(black_box(x) / black_box(y)).unwrap();
}
});
});
Expand Down
2 changes: 1 addition & 1 deletion math/benches/fields/stark252.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ pub fn starkfield_ops_benchmarks(c: &mut Criterion) {
group.bench_with_input(format!("div {:?}", &i.len()), &i, |bench, i| {
bench.iter(|| {
for (x, y) in i {
black_box(black_box(x) / black_box(y));
black_box(black_box(x) / black_box(y)).unwrap();
}
});
});
Expand Down
2 changes: 1 addition & 1 deletion math/benches/fields/u64_goldilocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ pub fn u64_goldilocks_ops_benchmarks(c: &mut Criterion) {
group.bench_with_input(format!("div {:?}", &i.len()), &i, |bench, i| {
bench.iter(|| {
for (x, y) in i {
black_box(black_box(x) / black_box(y));
black_box(black_box(x) / black_box(y)).unwrap();
}
});
});
Expand Down
2 changes: 1 addition & 1 deletion math/benches/fields/u64_goldilocks_montgomery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ pub fn u64_goldilocks_montgomery_ops_benchmarks(c: &mut Criterion) {
group.bench_with_input(format!("div {:?}", &i.len()), &i, |bench, i| {
bench.iter(|| {
for (x, y) in i {
black_box(black_box(x) / black_box(y));
black_box(black_box(x) / black_box(y)).unwrap();
}
});
});
Expand Down
8 changes: 7 additions & 1 deletion math/src/elliptic_curve/edwards/point.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,13 @@ 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])
// We are using that den_s1 and den_s2 aren't zero.
// See Theorem 3.3 from https://eprint.iacr.org/2007/286.pdf.
Self::new([
unsafe { (&num_s1 / &den_s1).unwrap_unchecked() },
unsafe { (&num_s2 / &den_s2).unwrap_unchecked() },
one,
])
}

/// Returns the additive inverse of the projective point `p`
Expand Down
9 changes: 7 additions & 2 deletions math/src/elliptic_curve/montgomery/point.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,11 @@ impl<E: IsMontgomery> IsGroup for MontgomeryProjectivePoint<E> {
let x1_square = &x1 * &x1;
let num = &x1_square + &x1_square + x1_square + &x1a + x1a + &one;
let den = (&b + &b) * &y1;
let div = num / den;

// We are using that den != 0 because b and y1 aren't zero.
// b != 0 because the cofficient b of a montgomery elliptic curve has to be different from zero.
// y1 != 0 because if not, it woould be the case from above: x2 = x1 and y2 + y1 = 0.
let div = unsafe { (num / den).unwrap_unchecked() };

let new_x = &div * &div * &b - (&x1 + x2) - a;
let new_y = div * (x1 - &new_x) - y1;
Expand All @@ -120,7 +124,8 @@ impl<E: IsMontgomery> IsGroup for MontgomeryProjectivePoint<E> {
} else {
let num = &y2 - &y1;
let den = &x2 - &x1;
let div = num / den;

let div = unsafe { (num / den).unwrap_unchecked() };

let new_x = &div * &div * E::b() - (&x1 + &x2) - E::a();
let new_y = div * (x1 - &new_x) - y1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,9 @@ impl IsField for Degree2ExtensionField {
}

/// Returns the division of `a` and `b`
fn div(a: &Self::BaseType, b: &Self::BaseType) -> Self::BaseType {
<Self as IsField>::mul(a, &Self::inv(b).unwrap())
fn div(a: &Self::BaseType, b: &Self::BaseType) -> Result<Self::BaseType, FieldError> {
let b_inv = &Self::inv(b).map_err(|_| FieldError::DivisionByZero)?;
Ok(<Self as IsField>::mul(a, b_inv))
}

/// Returns a boolean indicating whether `a` and `b` are equal component wise.
Expand Down Expand Up @@ -124,9 +125,11 @@ impl IsSubFieldOf<Degree2ExtensionField> for BLS12377PrimeField {
fn div(
a: &Self::BaseType,
b: &<Degree2ExtensionField as IsField>::BaseType,
) -> <Degree2ExtensionField as IsField>::BaseType {
let b_inv = Degree2ExtensionField::inv(b).unwrap();
<Self as IsSubFieldOf<Degree2ExtensionField>>::mul(a, &b_inv)
) -> Result<<Degree2ExtensionField as IsField>::BaseType, FieldError> {
let b_inv = Degree2ExtensionField::inv(b)?;
Ok(<Self as IsSubFieldOf<Degree2ExtensionField>>::mul(
a, &b_inv,
))
}

fn sub(
Expand Down Expand Up @@ -378,7 +381,7 @@ mod tests {
let a = Fp6E::from(3);
let a_extension = Fp12E::from(3);
let b = Fp12E::from(2);
assert_eq!(a / &b, a_extension / b);
assert_eq!((a / &b).unwrap(), (a_extension / b).unwrap());
}

#[test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,9 @@ impl IsField for Degree2ExtensionField {
}

/// Returns the division of `a` and `b`
fn div(a: &Self::BaseType, b: &Self::BaseType) -> Self::BaseType {
<Self as IsField>::mul(a, &Self::inv(b).unwrap())
fn div(a: &Self::BaseType, b: &Self::BaseType) -> Result<Self::BaseType, FieldError> {
let b_inv = &Self::inv(b).map_err(|_| FieldError::DivisionByZero)?;
Ok(<Self as IsField>::mul(a, b_inv))
}

/// Returns a boolean indicating whether `a` and `b` are equal component wise.
Expand Down Expand Up @@ -126,9 +127,11 @@ impl IsSubFieldOf<Degree2ExtensionField> for BLS12381PrimeField {
fn div(
a: &Self::BaseType,
b: &<Degree2ExtensionField as IsField>::BaseType,
) -> <Degree2ExtensionField as IsField>::BaseType {
let b_inv = Degree2ExtensionField::inv(b).unwrap();
<Self as IsSubFieldOf<Degree2ExtensionField>>::mul(a, &b_inv)
) -> Result<<Degree2ExtensionField as IsField>::BaseType, FieldError> {
let b_inv = Degree2ExtensionField::inv(b)?;
Ok(<Self as IsSubFieldOf<Degree2ExtensionField>>::mul(
a, &b_inv,
))
}

fn sub(
Expand Down Expand Up @@ -429,7 +432,7 @@ mod tests {
let a = FieldElement::<BLS12381PrimeField>::from(3);
let a_extension = FieldElement::<Degree2ExtensionField>::from(3);
let b = FieldElement::<Degree2ExtensionField>::from(2);
assert_eq!(a / &b, a_extension / b);
assert_eq!((a / &b).unwrap(), (a_extension / b).unwrap());
}

#[test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,9 @@ impl IsField for Degree2ExtensionField {
}

/// Returns the division of `a` and `b`
fn div(a: &Self::BaseType, b: &Self::BaseType) -> Self::BaseType {
<Self as IsField>::mul(a, &Self::inv(b).unwrap())
fn div(a: &Self::BaseType, b: &Self::BaseType) -> Result<Self::BaseType, FieldError> {
let b_inv = &Self::inv(b).map_err(|_| FieldError::DivisionByZero)?;
Ok(<Self as IsField>::mul(a, b_inv))
}

/// Returns a boolean indicating whether `a` and `b` are equal component wise.
Expand Down Expand Up @@ -128,9 +129,11 @@ impl IsSubFieldOf<Degree2ExtensionField> for BN254PrimeField {
fn div(
a: &Self::BaseType,
b: &<Degree2ExtensionField as IsField>::BaseType,
) -> <Degree2ExtensionField as IsField>::BaseType {
let b_inv = Degree2ExtensionField::inv(b).unwrap();
<Self as IsSubFieldOf<Degree2ExtensionField>>::mul(a, &b_inv)
) -> Result<<Degree2ExtensionField as IsField>::BaseType, FieldError> {
let b_inv = Degree2ExtensionField::inv(b)?;
Ok(<Self as IsSubFieldOf<Degree2ExtensionField>>::mul(
a, &b_inv,
))
}

fn sub(
Expand Down Expand Up @@ -397,7 +400,7 @@ mod tests {
let a = Fp6E::from(3);
let a_extension = Fp12E::from(3);
let b = Fp12E::from(2);
assert_eq!(a / &b, a_extension / b);
assert_eq!((a / &b).unwrap(), (a_extension / b).unwrap());
}

#[test]
Expand Down
5 changes: 4 additions & 1 deletion math/src/elliptic_curve/short_weierstrass/point.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,10 @@ where
} else {
Err(DeserializationError::FieldFromBytesError)
}
} else if E::defining_equation(&(&x / &z), &(&y / &z)) == FieldElement::zero() {
} else if E::defining_equation(unsafe { &(&x / &z).unwrap_unchecked() }, unsafe {
&(&y / &z).unwrap_unchecked()
}) == FieldElement::zero()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use regular unwrap here.
It may be useful to compute the inverse of z only once (not necessary to do now) and multiply later as well.

{
Ok(Self::new([x, y, z]))
} else {
Err(DeserializationError::FieldFromBytesError)
Expand Down
13 changes: 6 additions & 7 deletions math/src/field/element.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,12 +329,11 @@ where
F: IsSubFieldOf<L>,
L: IsField,
{
type Output = FieldElement<L>;
type Output = Result<FieldElement<L>, FieldError>;

fn div(self, rhs: &FieldElement<L>) -> Self::Output {
Self::Output {
value: <F as IsSubFieldOf<L>>::div(&self.value, &rhs.value),
}
let value = <F as IsSubFieldOf<L>>::div(&self.value, &rhs.value)?;
Ok(FieldElement::<L> { value })
}
}

Expand All @@ -343,7 +342,7 @@ where
F: IsSubFieldOf<L>,
L: IsField,
{
type Output = FieldElement<L>;
type Output = Result<FieldElement<L>, FieldError>;

fn div(self, rhs: FieldElement<L>) -> Self::Output {
&self / &rhs
Expand All @@ -355,7 +354,7 @@ where
F: IsSubFieldOf<L>,
L: IsField,
{
type Output = FieldElement<L>;
type Output = Result<FieldElement<L>, FieldError>;

fn div(self, rhs: &FieldElement<L>) -> Self::Output {
&self / rhs
Expand All @@ -367,7 +366,7 @@ where
F: IsSubFieldOf<L>,
L: IsField,
{
type Output = FieldElement<L>;
type Output = Result<FieldElement<L>, FieldError>;

fn div(self, rhs: FieldElement<L>) -> Self::Output {
self / &rhs
Expand Down
24 changes: 15 additions & 9 deletions math/src/field/extensions/cubic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,12 @@ where
}

/// Returns the division of `a` and `b`
fn div(a: &[FieldElement<F>; 3], b: &[FieldElement<F>; 3]) -> [FieldElement<F>; 3] {
<Self as IsField>::mul(a, &Self::inv(b).unwrap())
fn div(
a: &[FieldElement<F>; 3],
b: &[FieldElement<F>; 3],
) -> Result<[FieldElement<F>; 3], FieldError> {
let b_inv = &Self::inv(b).map_err(|_| FieldError::DivisionByZero)?;
Ok(<Self as IsField>::mul(a, b_inv))
}

/// Returns a boolean indicating whether `a` and `b` are equal component wise.
Expand Down Expand Up @@ -180,9 +184,11 @@ where
fn div(
a: &Self::BaseType,
b: &<CubicExtensionField<F, Q> as IsField>::BaseType,
) -> <CubicExtensionField<F, Q> as IsField>::BaseType {
let b_inv = <CubicExtensionField<F, Q> as IsField>::inv(b).unwrap();
<Self as IsSubFieldOf<CubicExtensionField<F, Q>>>::mul(a, &b_inv)
) -> Result<<CubicExtensionField<F, Q> as IsField>::BaseType, FieldError> {
let b_inv = <CubicExtensionField<F, Q> as IsField>::inv(b)?;
Ok(<Self as IsSubFieldOf<CubicExtensionField<F, Q>>>::mul(
a, &b_inv,
))
}

fn sub(
Expand Down Expand Up @@ -285,15 +291,15 @@ mod tests {
let a = FEE::new([FE::new(0), FE::new(3), FE::new(2)]);
let b = FEE::new([-FE::new(2), FE::new(8), FE::new(5)]);
let expected_result = FEE::new([FE::new(12), FE::new(6), FE::new(1)]);
assert_eq!(a / b, expected_result);
assert_eq!((a / b).unwrap(), expected_result);
}

#[test]
fn test_div_2() {
let a = FEE::new([FE::new(12), FE::new(5), FE::new(4)]);
let b = FEE::new([-FE::new(4), FE::new(2), FE::new(2)]);
let expected_result = FEE::new([FE::new(3), FE::new(8), FE::new(11)]);
assert_eq!(a / b, expected_result);
assert_eq!((a / b).unwrap(), expected_result);
}

#[test]
Expand Down Expand Up @@ -379,14 +385,14 @@ mod tests {
let a = FE::new(2);
let b = FEE::new([-FE::new(2), FE::new(8), FE::new(5)]);
let expected_result = FEE::new([FE::new(8), FE::new(4), FE::new(10)]);
assert_eq!(a / b, expected_result);
assert_eq!((a / b).unwrap(), expected_result);
}

#[test]
fn test_div_as_subfield_2() {
let a = FE::new(4);
let b = FEE::new([-FE::new(4), FE::new(2), FE::new(2)]);
let expected_result = FEE::new([FE::new(3), FE::new(6), FE::new(11)]);
assert_eq!(a / b, expected_result);
assert_eq!((a / b).unwrap(), expected_result);
}
}
Loading
Loading