From 2fd5fbbe0145f5105e8dbde6577175b6fe013e5f Mon Sep 17 00:00:00 2001 From: Daniel Lubarov Date: Fri, 19 Aug 2022 09:21:10 -0700 Subject: [PATCH] Change `from_biguint`'s behavior with extension fields These appear to be unused for extension fields, so we're free to change the mapping without breaking anything. As the TODO says, the mapping that's currently implemented doesn't seem natural or useful. It seems more natural to treat the `BigUint` as a base field element, potentially in a non-canonical form. --- ecdsa/src/curve/curve_msm.rs | 8 ++--- ecdsa/src/curve/curve_types.rs | 4 +-- ecdsa/src/curve/glv.rs | 8 ++--- ecdsa/src/curve/secp256k1.rs | 2 +- ecdsa/src/gadgets/curve_fixed_base.rs | 2 +- ecdsa/src/gadgets/curve_msm.rs | 2 +- ecdsa/src/gadgets/curve_windowed_mul.rs | 2 +- ecdsa/src/gadgets/glv.rs | 2 +- ecdsa/src/gadgets/nonnative.rs | 40 ++++++++++++++++++++----- field/src/extension/quadratic.rs | 6 ++-- field/src/extension/quartic.rs | 13 ++------ field/src/extension/quintic.rs | 4 +-- field/src/goldilocks_field.rs | 2 +- field/src/secp256k1_base.rs | 10 +++---- field/src/secp256k1_scalar.rs | 10 +++---- field/src/types.rs | 5 ++-- 16 files changed, 66 insertions(+), 54 deletions(-) diff --git a/ecdsa/src/curve/curve_msm.rs b/ecdsa/src/curve/curve_msm.rs index 6d07c097..f681deb2 100644 --- a/ecdsa/src/curve/curve_msm.rs +++ b/ecdsa/src/curve/curve_msm.rs @@ -207,7 +207,7 @@ mod tests { 0b00001111111111111111111111111111, 0b11111111111111111111111111111111, ]; - let x = Secp256K1Scalar::from_biguint(BigUint::from_slice(&x_canonical)); + let x = Secp256K1Scalar::from_noncanonical_biguint(BigUint::from_slice(&x_canonical)); assert_eq!(x.to_canonical_biguint().to_u32_digits(), x_canonical); assert_eq!( to_digits::(&x, 17), @@ -240,13 +240,13 @@ mod tests { let generator_2 = generator_1 + generator_1; let generator_3 = generator_1 + generator_2; - let scalar_1 = Secp256K1Scalar::from_biguint(BigUint::from_slice(&[ + let scalar_1 = Secp256K1Scalar::from_noncanonical_biguint(BigUint::from_slice(&[ 11111111, 22222222, 33333333, 44444444, ])); - let scalar_2 = Secp256K1Scalar::from_biguint(BigUint::from_slice(&[ + let scalar_2 = Secp256K1Scalar::from_noncanonical_biguint(BigUint::from_slice(&[ 22222222, 22222222, 33333333, 44444444, ])); - let scalar_3 = Secp256K1Scalar::from_biguint(BigUint::from_slice(&[ + let scalar_3 = Secp256K1Scalar::from_noncanonical_biguint(BigUint::from_slice(&[ 33333333, 22222222, 33333333, 44444444, ])); diff --git a/ecdsa/src/curve/curve_types.rs b/ecdsa/src/curve/curve_types.rs index bbf66d65..96821672 100644 --- a/ecdsa/src/curve/curve_types.rs +++ b/ecdsa/src/curve/curve_types.rs @@ -277,9 +277,9 @@ impl Neg for ProjectivePoint { } pub fn base_to_scalar(x: C::BaseField) -> C::ScalarField { - C::ScalarField::from_biguint(x.to_canonical_biguint()) + C::ScalarField::from_noncanonical_biguint(x.to_canonical_biguint()) } pub fn scalar_to_base(x: C::ScalarField) -> C::BaseField { - C::BaseField::from_biguint(x.to_canonical_biguint()) + C::BaseField::from_noncanonical_biguint(x.to_canonical_biguint()) } diff --git a/ecdsa/src/curve/glv.rs b/ecdsa/src/curve/glv.rs index 05ecea44..c58032ec 100644 --- a/ecdsa/src/curve/glv.rs +++ b/ecdsa/src/curve/glv.rs @@ -45,14 +45,14 @@ pub fn decompose_secp256k1_scalar( ) .round() .to_integer(); - let c1 = Secp256K1Scalar::from_biguint(c1_biguint); + let c1 = Secp256K1Scalar::from_noncanonical_biguint(c1_biguint); let c2_biguint = Ratio::new( MINUS_B1.to_canonical_biguint() * k.to_canonical_biguint(), p.clone(), ) .round() .to_integer(); - let c2 = Secp256K1Scalar::from_biguint(c2_biguint); + let c2 = Secp256K1Scalar::from_noncanonical_biguint(c2_biguint); let k1_raw = k - c1 * A1 - c2 * A2; let k2_raw = c1 * MINUS_B1 - c2 * B2; @@ -61,13 +61,13 @@ pub fn decompose_secp256k1_scalar( let two = BigUint::from_slice(&[2]); let k1_neg = k1_raw.to_canonical_biguint() > p.clone() / two.clone(); let k1 = if k1_neg { - Secp256K1Scalar::from_biguint(p.clone() - k1_raw.to_canonical_biguint()) + Secp256K1Scalar::from_noncanonical_biguint(p.clone() - k1_raw.to_canonical_biguint()) } else { k1_raw }; let k2_neg = k2_raw.to_canonical_biguint() > p.clone() / two; let k2 = if k2_neg { - Secp256K1Scalar::from_biguint(p - k2_raw.to_canonical_biguint()) + Secp256K1Scalar::from_noncanonical_biguint(p - k2_raw.to_canonical_biguint()) } else { k2_raw }; diff --git a/ecdsa/src/curve/secp256k1.rs b/ecdsa/src/curve/secp256k1.rs index e46fbb3d..8f7bccf3 100644 --- a/ecdsa/src/curve/secp256k1.rs +++ b/ecdsa/src/curve/secp256k1.rs @@ -71,7 +71,7 @@ mod tests { #[test] fn test_g1_multiplication() { - let lhs = Secp256K1Scalar::from_biguint(BigUint::from_slice(&[ + let lhs = Secp256K1Scalar::from_noncanonical_biguint(BigUint::from_slice(&[ 1111, 2222, 3333, 4444, 5555, 6666, 7777, 8888, ])); assert_eq!( diff --git a/ecdsa/src/gadgets/curve_fixed_base.rs b/ecdsa/src/gadgets/curve_fixed_base.rs index d99d5760..44dc9488 100644 --- a/ecdsa/src/gadgets/curve_fixed_base.rs +++ b/ecdsa/src/gadgets/curve_fixed_base.rs @@ -30,7 +30,7 @@ pub fn fixed_base_curve_mul_circuit, cons let limbs = builder.split_nonnative_to_4_bit_limbs(scalar); let hash_0 = KeccakHash::<32>::hash_no_pad(&[F::ZERO]); - let hash_0_scalar = C::ScalarField::from_biguint(BigUint::from_bytes_le( + let hash_0_scalar = C::ScalarField::from_noncanonical_biguint(BigUint::from_bytes_le( &GenericHashOut::::to_bytes(&hash_0), )); let rando = (CurveScalar(hash_0_scalar) * C::GENERATOR_PROJECTIVE).to_affine(); diff --git a/ecdsa/src/gadgets/curve_msm.rs b/ecdsa/src/gadgets/curve_msm.rs index 1265d399..e059638c 100644 --- a/ecdsa/src/gadgets/curve_msm.rs +++ b/ecdsa/src/gadgets/curve_msm.rs @@ -29,7 +29,7 @@ pub fn curve_msm_circuit, const D: usize> let num_limbs = limbs_n.len(); let hash_0 = KeccakHash::<32>::hash_no_pad(&[F::ZERO]); - let hash_0_scalar = C::ScalarField::from_biguint(BigUint::from_bytes_le( + let hash_0_scalar = C::ScalarField::from_noncanonical_biguint(BigUint::from_bytes_le( &GenericHashOut::::to_bytes(&hash_0), )); let rando = (CurveScalar(hash_0_scalar) * C::GENERATOR_PROJECTIVE).to_affine(); diff --git a/ecdsa/src/gadgets/curve_windowed_mul.rs b/ecdsa/src/gadgets/curve_windowed_mul.rs index d9dcc734..bc4e1caf 100644 --- a/ecdsa/src/gadgets/curve_windowed_mul.rs +++ b/ecdsa/src/gadgets/curve_windowed_mul.rs @@ -131,7 +131,7 @@ impl, const D: usize> CircuitBuilderWindowedMul, ) -> AffinePointTarget { let hash_0 = KeccakHash::<25>::hash_no_pad(&[F::ZERO]); - let hash_0_scalar = C::ScalarField::from_biguint(BigUint::from_bytes_le( + let hash_0_scalar = C::ScalarField::from_noncanonical_biguint(BigUint::from_bytes_le( &GenericHashOut::::to_bytes(&hash_0), )); let starting_point = CurveScalar(hash_0_scalar) * C::GENERATOR_PROJECTIVE; diff --git a/ecdsa/src/gadgets/glv.rs b/ecdsa/src/gadgets/glv.rs index 746d661f..8e62e906 100644 --- a/ecdsa/src/gadgets/glv.rs +++ b/ecdsa/src/gadgets/glv.rs @@ -116,7 +116,7 @@ impl, const D: usize> SimpleGenerator } fn run_once(&self, witness: &PartitionWitness, out_buffer: &mut GeneratedValues) { - let k = Secp256K1Scalar::from_biguint(witness_get_biguint_target( + let k = Secp256K1Scalar::from_noncanonical_biguint(witness_get_biguint_target( witness, self.k.value.clone(), )); diff --git a/ecdsa/src/gadgets/nonnative.rs b/ecdsa/src/gadgets/nonnative.rs index 3c2e2ed6..393aac75 100644 --- a/ecdsa/src/gadgets/nonnative.rs +++ b/ecdsa/src/gadgets/nonnative.rs @@ -467,8 +467,14 @@ impl, const D: usize, FF: PrimeField> SimpleGenerat } fn run_once(&self, witness: &PartitionWitness, out_buffer: &mut GeneratedValues) { - let a = FF::from_biguint(witness_get_biguint_target(witness, self.a.value.clone())); - let b = FF::from_biguint(witness_get_biguint_target(witness, self.b.value.clone())); + let a = FF::from_noncanonical_biguint(witness_get_biguint_target( + witness, + self.a.value.clone(), + )); + let b = FF::from_noncanonical_biguint(witness_get_biguint_target( + witness, + self.b.value.clone(), + )); let a_biguint = a.to_canonical_biguint(); let b_biguint = b.to_canonical_biguint(); let sum_biguint = a_biguint + b_biguint; @@ -508,7 +514,10 @@ impl, const D: usize, FF: PrimeField> SimpleGenerat .summands .iter() .map(|summand| { - FF::from_biguint(witness_get_biguint_target(witness, summand.value.clone())) + FF::from_noncanonical_biguint(witness_get_biguint_target( + witness, + summand.value.clone(), + )) }) .collect(); let summand_biguints: Vec<_> = summands @@ -553,8 +562,14 @@ impl, const D: usize, FF: PrimeField> SimpleGenerat } fn run_once(&self, witness: &PartitionWitness, out_buffer: &mut GeneratedValues) { - let a = FF::from_biguint(witness_get_biguint_target(witness, self.a.value.clone())); - let b = FF::from_biguint(witness_get_biguint_target(witness, self.b.value.clone())); + let a = FF::from_noncanonical_biguint(witness_get_biguint_target( + witness, + self.a.value.clone(), + )); + let b = FF::from_noncanonical_biguint(witness_get_biguint_target( + witness, + self.b.value.clone(), + )); let a_biguint = a.to_canonical_biguint(); let b_biguint = b.to_canonical_biguint(); @@ -594,8 +609,14 @@ impl, const D: usize, FF: PrimeField> SimpleGenerat } fn run_once(&self, witness: &PartitionWitness, out_buffer: &mut GeneratedValues) { - let a = FF::from_biguint(witness_get_biguint_target(witness, self.a.value.clone())); - let b = FF::from_biguint(witness_get_biguint_target(witness, self.b.value.clone())); + let a = FF::from_noncanonical_biguint(witness_get_biguint_target( + witness, + self.a.value.clone(), + )); + let b = FF::from_noncanonical_biguint(witness_get_biguint_target( + witness, + self.b.value.clone(), + )); let a_biguint = a.to_canonical_biguint(); let b_biguint = b.to_canonical_biguint(); @@ -625,7 +646,10 @@ impl, const D: usize, FF: PrimeField> SimpleGenerat } fn run_once(&self, witness: &PartitionWitness, out_buffer: &mut GeneratedValues) { - let x = FF::from_biguint(witness_get_biguint_target(witness, self.x.value.clone())); + let x = FF::from_noncanonical_biguint(witness_get_biguint_target( + witness, + self.x.value.clone(), + )); let inv = x.inverse(); let x_biguint = x.to_canonical_biguint(); diff --git a/field/src/extension/quadratic.rs b/field/src/extension/quadratic.rs index d68df42e..278abba9 100644 --- a/field/src/extension/quadratic.rs +++ b/field/src/extension/quadratic.rs @@ -3,7 +3,6 @@ use std::iter::{Product, Sum}; use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Neg, Sub, SubAssign}; use num::bigint::BigUint; -use num::Integer; use serde::{Deserialize, Serialize}; use crate::extension::{Extendable, FieldExtension, Frobenius, OEF}; @@ -89,9 +88,8 @@ impl> Field for QuadraticExtension { )) } - fn from_biguint(n: BigUint) -> Self { - let (high, low) = n.div_rem(&F::order()); - Self([F::from_biguint(low), F::from_biguint(high)]) + fn from_noncanonical_biguint(n: BigUint) -> Self { + F::from_noncanonical_biguint(n).into() } fn from_canonical_u64(n: u64) -> Self { diff --git a/field/src/extension/quartic.rs b/field/src/extension/quartic.rs index fc0cbcf8..6df39903 100644 --- a/field/src/extension/quartic.rs +++ b/field/src/extension/quartic.rs @@ -4,7 +4,6 @@ use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Neg, Sub, SubAssi use num::bigint::BigUint; use num::traits::Pow; -use num::Integer; use serde::{Deserialize, Serialize}; use crate::extension::{Extendable, FieldExtension, Frobenius, OEF}; @@ -94,16 +93,8 @@ impl> Field for QuarticExtension { )) } - fn from_biguint(n: BigUint) -> Self { - let (rest, first) = n.div_rem(&F::order()); - let (rest, second) = rest.div_rem(&F::order()); - let (rest, third) = rest.div_rem(&F::order()); - Self([ - F::from_biguint(first), - F::from_biguint(second), - F::from_biguint(third), - F::from_biguint(rest), - ]) + fn from_noncanonical_biguint(n: BigUint) -> Self { + F::from_noncanonical_biguint(n).into() } fn from_canonical_u64(n: u64) -> Self { diff --git a/field/src/extension/quintic.rs b/field/src/extension/quintic.rs index 564674c3..6680ebc7 100644 --- a/field/src/extension/quintic.rs +++ b/field/src/extension/quintic.rs @@ -99,8 +99,8 @@ impl> Field for QuinticExtension { Some(FieldExtension::<5>::scalar_mul(&f, g.inverse())) } - fn from_biguint(n: BigUint) -> Self { - Self([F::from_biguint(n), F::ZERO, F::ZERO, F::ZERO, F::ZERO]) + fn from_noncanonical_biguint(n: BigUint) -> Self { + F::from_noncanonical_biguint(n).into() } fn from_canonical_u64(n: u64) -> Self { diff --git a/field/src/goldilocks_field.rs b/field/src/goldilocks_field.rs index c5075b5d..c1bb60b0 100644 --- a/field/src/goldilocks_field.rs +++ b/field/src/goldilocks_field.rs @@ -90,7 +90,7 @@ impl Field for GoldilocksField { try_inverse_u64(self) } - fn from_biguint(n: BigUint) -> Self { + fn from_noncanonical_biguint(n: BigUint) -> Self { Self(n.mod_floor(&Self::order()).to_u64_digits()[0]) } diff --git a/field/src/secp256k1_base.rs b/field/src/secp256k1_base.rs index 9e39b982..504d63d7 100644 --- a/field/src/secp256k1_base.rs +++ b/field/src/secp256k1_base.rs @@ -106,7 +106,7 @@ impl Field for Secp256K1Base { Some(self.exp_biguint(&(Self::order() - BigUint::one() - BigUint::one()))) } - fn from_biguint(val: BigUint) -> Self { + fn from_noncanonical_biguint(val: BigUint) -> Self { Self( val.to_u64_digits() .into_iter() @@ -135,7 +135,7 @@ impl Field for Secp256K1Base { #[cfg(feature = "rand")] fn rand_from_rng(rng: &mut R) -> Self { use num::bigint::RandBigInt; - Self::from_biguint(rng.gen_biguint_below(&Self::order())) + Self::from_noncanonical_biguint(rng.gen_biguint_below(&Self::order())) } } @@ -157,7 +157,7 @@ impl Neg for Secp256K1Base { if self.is_zero() { Self::ZERO } else { - Self::from_biguint(Self::order() - self.to_canonical_biguint()) + Self::from_noncanonical_biguint(Self::order() - self.to_canonical_biguint()) } } } @@ -171,7 +171,7 @@ impl Add for Secp256K1Base { if result >= Self::order() { result -= Self::order(); } - Self::from_biguint(result) + Self::from_noncanonical_biguint(result) } } @@ -210,7 +210,7 @@ impl Mul for Secp256K1Base { #[inline] fn mul(self, rhs: Self) -> Self { - Self::from_biguint( + Self::from_noncanonical_biguint( (self.to_canonical_biguint() * rhs.to_canonical_biguint()).mod_floor(&Self::order()), ) } diff --git a/field/src/secp256k1_scalar.rs b/field/src/secp256k1_scalar.rs index eea67fab..e70b154d 100644 --- a/field/src/secp256k1_scalar.rs +++ b/field/src/secp256k1_scalar.rs @@ -115,7 +115,7 @@ impl Field for Secp256K1Scalar { Some(self.exp_biguint(&(Self::order() - BigUint::one() - BigUint::one()))) } - fn from_biguint(val: BigUint) -> Self { + fn from_noncanonical_biguint(val: BigUint) -> Self { Self( val.to_u64_digits() .into_iter() @@ -144,7 +144,7 @@ impl Field for Secp256K1Scalar { #[cfg(feature = "rand")] fn rand_from_rng(rng: &mut R) -> Self { use num::bigint::RandBigInt; - Self::from_biguint(rng.gen_biguint_below(&Self::order())) + Self::from_noncanonical_biguint(rng.gen_biguint_below(&Self::order())) } } @@ -166,7 +166,7 @@ impl Neg for Secp256K1Scalar { if self.is_zero() { Self::ZERO } else { - Self::from_biguint(Self::order() - self.to_canonical_biguint()) + Self::from_noncanonical_biguint(Self::order() - self.to_canonical_biguint()) } } } @@ -180,7 +180,7 @@ impl Add for Secp256K1Scalar { if result >= Self::order() { result -= Self::order(); } - Self::from_biguint(result) + Self::from_noncanonical_biguint(result) } } @@ -219,7 +219,7 @@ impl Mul for Secp256K1Scalar { #[inline] fn mul(self, rhs: Self) -> Self { - Self::from_biguint( + Self::from_noncanonical_biguint( (self.to_canonical_biguint() * rhs.to_canonical_biguint()).mod_floor(&Self::order()), ) } diff --git a/field/src/types.rs b/field/src/types.rs index 87fd8dd4..ac94bcfa 100644 --- a/field/src/types.rs +++ b/field/src/types.rs @@ -270,9 +270,8 @@ pub trait Field: subgroup.into_iter().map(|x| x * shift).collect() } - // TODO: The current behavior for composite fields doesn't seem natural or useful. - // Rename to `from_noncanonical_biguint` and have it return `n % Self::characteristic()`. - fn from_biguint(n: BigUint) -> Self; + /// Returns `n % Self::characteristic()`. + fn from_noncanonical_biguint(n: BigUint) -> Self; /// Returns `n`. Assumes that `n` is already in canonical form, i.e. `n < Self::order()`. // TODO: Should probably be unsafe.