From bb2b23391ba159056e593c27f62b5d316ee2abd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Garillot?= <4142+huitseeker@users.noreply.github.com> Date: Thu, 29 Feb 2024 13:56:28 -0700 Subject: [PATCH] Small Gadgets refactor (#353) * refactor: restrict visibility of nonnative * refactor: restrict visibility of ECC * refactor: restrict visibility of gadgets::r1cs * refactor: restrict visibility of gadgets::utils * refactor: simple cleanup * test: check field representations in used libraries * chore: minor cleanups * refactor: simplify AllocatedPoint::default * refactor: simplify AllocatedPointNonInfinity::to_allocated_point --- src/circuit.rs | 9 +++---- src/gadgets/ecc.rs | 25 ++++++++++--------- src/gadgets/mod.rs | 23 ++++++++++++++--- src/gadgets/nonnative/util.rs | 47 +++++++++++++++++++++++++++++++---- src/gadgets/r1cs.rs | 4 +-- src/gadgets/utils.rs | 10 +------- src/lib.rs | 2 +- src/provider/poseidon.rs | 2 +- src/r1cs/mod.rs | 5 +--- src/supernova/circuit.rs | 14 +++-------- src/supernova/test.rs | 4 +-- src/supernova/utils.rs | 2 +- 12 files changed, 90 insertions(+), 57 deletions(-) diff --git a/src/circuit.rs b/src/circuit.rs index ef7d2bbbe..cfeb027f1 100644 --- a/src/circuit.rs +++ b/src/circuit.rs @@ -7,11 +7,8 @@ use crate::{ constants::{NIO_NOVA_FOLD, NUM_FE_WITHOUT_IO_FOR_CRHF, NUM_HASH_BITS}, gadgets::{ - ecc::AllocatedPoint, - r1cs::{AllocatedR1CSInstance, AllocatedRelaxedR1CSInstance}, - utils::{ - alloc_num_equals, alloc_scalar_as_base, alloc_zero, conditionally_select_vec, le_bits_to_num, - }, + alloc_num_equals, alloc_scalar_as_base, alloc_zero, conditionally_select_vec, le_bits_to_num, + AllocatedPoint, AllocatedR1CSInstance, AllocatedRelaxedR1CSInstance, }, r1cs::{R1CSInstance, RelaxedR1CSInstance}, traits::{ @@ -370,7 +367,7 @@ mod tests { test_shape_cs::TestShapeCS, }, constants::{BN_LIMB_WIDTH, BN_N_LIMBS}, - gadgets::utils::scalar_as_base, + gadgets::scalar_as_base, provider::{ poseidon::PoseidonConstantsCircuit, Bn256EngineKZG, GrumpkinEngine, PallasEngine, Secp256k1Engine, Secq256k1Engine, VestaEngine, diff --git a/src/gadgets/ecc.rs b/src/gadgets/ecc.rs index cc282b6e1..0ebef4c4b 100644 --- a/src/gadgets/ecc.rs +++ b/src/gadgets/ecc.rs @@ -101,18 +101,19 @@ impl AllocatedPoint { } /// Allocates a default point on the curve, set to the identity point. - pub fn default>(mut cs: CS) -> Result { + pub fn default>(mut cs: CS) -> Self { let zero = alloc_zero(cs.namespace(|| "zero")); let one = alloc_one(cs.namespace(|| "one")); - Ok(Self { + Self { x: zero.clone(), y: zero, is_infinity: one, - }) + } } /// Returns coordinates associated with the point. + #[allow(unused)] pub const fn get_coordinates( &self, ) -> ( @@ -490,7 +491,7 @@ impl AllocatedPoint { // convert back to AllocatedPoint let res = { // we set acc.is_infinity = self.is_infinity - let acc = acc.to_allocated_point(&self.is_infinity)?; + let acc = acc.to_allocated_point(&self.is_infinity); // we remove the initial slack if bits[0] is as not as assumed (i.e., it is not 1) let acc_minus_initial = { @@ -508,7 +509,7 @@ impl AllocatedPoint { // when self.is_infinity = 1, return the default point, else return res // we already set res.is_infinity to be self.is_infinity, so we do not need to set it here - let default = Self::default(cs.namespace(|| "default"))?; + let default = Self::default(cs.namespace(|| "default")); let x = conditionally_select2( cs.namespace(|| "check if self.is_infinity is zero (x)"), &default.x, @@ -529,7 +530,7 @@ impl AllocatedPoint { y, is_infinity: res.is_infinity, }; - let mut p_complete = p.to_allocated_point(&self.is_infinity)?; + let mut p_complete = p.to_allocated_point(&self.is_infinity); for (i, bit) in complete_bits.iter().enumerate() { let temp = acc.add(cs.namespace(|| format!("add_complete {i}")), &p_complete)?; @@ -596,11 +597,13 @@ pub struct AllocatedPointNonInfinity { impl AllocatedPointNonInfinity { /// Creates a new `AllocatedPointNonInfinity` from the specified coordinates + #[allow(unused)] pub const fn new(x: AllocatedNum, y: AllocatedNum) -> Self { Self { x, y } } /// Allocates a new point on the curve using coordinates provided by `coords`. + #[allow(unused)] pub fn alloc>( mut cs: CS, coords: Option<(G::Base, G::Base)>, @@ -624,18 +627,16 @@ impl AllocatedPointNonInfinity { } /// Returns an `AllocatedPoint` from an `AllocatedPointNonInfinity` - pub fn to_allocated_point( - &self, - is_infinity: &AllocatedNum, - ) -> Result, SynthesisError> { - Ok(AllocatedPoint { + pub fn to_allocated_point(&self, is_infinity: &AllocatedNum) -> AllocatedPoint { + AllocatedPoint { x: self.x.clone(), y: self.y.clone(), is_infinity: is_infinity.clone(), - }) + } } /// Returns coordinates associated with the point. + #[allow(unused)] pub const fn get_coordinates(&self) -> (&AllocatedNum, &AllocatedNum) { (&self.x, &self.y) } diff --git a/src/gadgets/mod.rs b/src/gadgets/mod.rs index d42474626..926d1ba46 100644 --- a/src/gadgets/mod.rs +++ b/src/gadgets/mod.rs @@ -1,5 +1,20 @@ //! This module implements various gadgets necessary for Nova and applications built with Nova. -pub mod ecc; -pub(crate) mod nonnative; -pub(crate) mod r1cs; -pub(crate) mod utils; +mod ecc; +pub(crate) use ecc::AllocatedPoint; + +mod nonnative; +pub(crate) use nonnative::{bignat::nat_to_limbs, util::f_to_nat}; + +mod r1cs; +pub(crate) use r1cs::{ + conditionally_select_alloc_relaxed_r1cs, conditionally_select_vec_allocated_relaxed_r1cs_instance, +}; +pub(crate) use r1cs::{AllocatedR1CSInstance, AllocatedRelaxedR1CSInstance}; + +mod utils; +#[cfg(test)] +pub(crate) use utils::alloc_one; +pub(crate) use utils::{ + alloc_num_equals, alloc_scalar_as_base, alloc_zero, conditionally_select_vec, le_bits_to_num, + scalar_as_base, +}; diff --git a/src/gadgets/nonnative/util.rs b/src/gadgets/nonnative/util.rs index 61a60263c..3a4f30aa8 100644 --- a/src/gadgets/nonnative/util.rs +++ b/src/gadgets/nonnative/util.rs @@ -158,9 +158,9 @@ impl Num { /// Computes the natural number represented by an array of bits. /// Checks if the natural number equals `self` pub fn is_equal>(&self, mut cs: CS, other: &Bitvector) { - let allocations = other.allocations.clone(); let mut f = Scalar::ONE; - let sum = allocations + let sum = other + .allocations .iter() .fold(LinearCombination::zero(), |lc, bit| { let l = lc + (f, &bit.bit); @@ -202,8 +202,7 @@ impl Num { let sum_lc = LinearCombination::zero() + &self.num - ∑ cs.enforce(|| "sum", |lc| lc + &sum_lc, |lc| lc + CS::one(), |lc| lc); let bits: Vec> = allocations - .clone() - .into_iter() + .iter() .map(|a| LinearCombination::zero() + &a.bit) .collect(); Ok(Bitvector { @@ -245,7 +244,7 @@ fn write_be(f: &F, mut writer: W) -> io::Result<()> { /// Convert a field element to a natural number pub fn f_to_nat(f: &Scalar) -> BigInt { let mut s = Vec::new(); - write_be(f, &mut s).unwrap(); // f.to_repr().write_be(&mut s).unwrap(); + write_be(f, &mut s).unwrap(); BigInt::from_bytes_le(Sign::Plus, f.to_repr().as_ref()) } @@ -254,3 +253,41 @@ pub fn f_to_nat(f: &Scalar) -> BigInt { pub fn nat_to_f(n: &BigInt) -> Option { Scalar::from_str_vartime(&format!("{n}")) } + +#[cfg(test)] +mod tests { + use bitvec::field::BitField as _; + use ff::PrimeFieldBits; + use rand::SeedableRng; + use rand_chacha::ChaCha20Rng; + + // the write_be function above assumes Field::to_repr() outputs a representation that's an instance + // of `AsRef<[u8]>` in lower endian. We test that here, as this is not what the I2OSP standard recommends + // and may change in some implementations. + fn test_repr_is_le_with() { + let mut rng = ChaCha20Rng::from_seed([0u8; 32]); + for _i in 0..50 { + let f = F::random(&mut rng); + // This is guaranteed to be in LE + let le_bits = f.to_le_bits(); + let leftmost_u64 = le_bits[..64].load_le::(); + + // This is not + let f_repr = f.to_repr(); + let bytes: [u8; 8] = f_repr.as_ref()[..8].try_into().unwrap(); + let u64_from_repr = u64::from_le_bytes(bytes); + + assert_eq!(leftmost_u64, u64_from_repr); + } + } + + #[test] + fn test_repr_is_le() { + test_repr_is_le_with::(); + test_repr_is_le_with::(); + test_repr_is_le_with::(); + test_repr_is_le_with::(); + test_repr_is_le_with::(); + test_repr_is_le_with::(); + } +} diff --git a/src/gadgets/r1cs.rs b/src/gadgets/r1cs.rs index ea8c8ec91..a93a6e474 100644 --- a/src/gadgets/r1cs.rs +++ b/src/gadgets/r1cs.rs @@ -124,7 +124,7 @@ impl AllocatedRelaxedR1CSInstance { limb_width: usize, n_limbs: usize, ) -> Result { - let W = AllocatedPoint::default(cs.namespace(|| "allocate W"))?; + let W = AllocatedPoint::default(cs.namespace(|| "allocate W")); let E = W.clone(); let u = W.x.clone(); // In the default case, W.x = u = 0 @@ -159,7 +159,7 @@ impl AllocatedRelaxedR1CSInstance { limb_width: usize, n_limbs: usize, ) -> Result { - let E = AllocatedPoint::default(cs.namespace(|| "allocate default E"))?; + let E = AllocatedPoint::default(cs.namespace(|| "allocate default E")); let u = alloc_one(cs.namespace(|| "one")); diff --git a/src/gadgets/utils.rs b/src/gadgets/utils.rs index 7a43e5a33..b768a8dd9 100644 --- a/src/gadgets/utils.rs +++ b/src/gadgets/utils.rs @@ -80,15 +80,7 @@ where CS: ConstraintSystem<::Base>, { AllocatedNum::alloc(cs.namespace(|| "allocate scalar as base"), || { - let input_bits = input.unwrap_or(E::Scalar::ZERO).clone().to_le_bits(); - let mut mult = E::Base::ONE; - let mut val = E::Base::ZERO; - for bit in input_bits { - if bit { - val += mult; - } - mult = mult + mult; - } + let val = scalar_as_base::(input.unwrap_or(E::Scalar::ZERO)); Ok(val) }) } diff --git a/src/lib.rs b/src/lib.rs index 9c3a877f5..1b53e0552 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -46,7 +46,7 @@ use circuit::{NovaAugmentedCircuit, NovaAugmentedCircuitInputs, NovaAugmentedCir use constants::{BN_LIMB_WIDTH, BN_N_LIMBS, NUM_FE_WITHOUT_IO_FOR_CRHF, NUM_HASH_BITS}; use errors::NovaError; use ff::{Field, PrimeField}; -use gadgets::utils::scalar_as_base; +use gadgets::scalar_as_base; use nifs::NIFS; use r1cs::{ CommitmentKeyHint, R1CSInstance, R1CSShape, R1CSWitness, RelaxedR1CSInstance, RelaxedR1CSWitness, diff --git a/src/provider/poseidon.rs b/src/provider/poseidon.rs index 9c2b4609e..3a42bf4a9 100644 --- a/src/provider/poseidon.rs +++ b/src/provider/poseidon.rs @@ -208,7 +208,7 @@ mod tests { }; use crate::{ bellpepper::solver::SatisfyingAssignment, constants::NUM_CHALLENGE_BITS, - gadgets::utils::le_bits_to_num, traits::Engine, + gadgets::le_bits_to_num, traits::Engine, }; use ff::Field; use rand::rngs::OsRng; diff --git a/src/r1cs/mod.rs b/src/r1cs/mod.rs index aab1022ea..63c7f5dc2 100644 --- a/src/r1cs/mod.rs +++ b/src/r1cs/mod.rs @@ -6,10 +6,7 @@ use crate::{ constants::{BN_LIMB_WIDTH, BN_N_LIMBS}, digest::{DigestComputer, SimpleDigestible}, errors::NovaError, - gadgets::{ - nonnative::{bignat::nat_to_limbs, util::f_to_nat}, - utils::scalar_as_base, - }, + gadgets::{f_to_nat, nat_to_limbs, scalar_as_base}, traits::{ commitment::CommitmentEngineTrait, AbsorbInROTrait, Engine, ROTrait, TranscriptReprTrait, }, diff --git a/src/supernova/circuit.rs b/src/supernova/circuit.rs index b9a33cda6..d287bf3d2 100644 --- a/src/supernova/circuit.rs +++ b/src/supernova/circuit.rs @@ -14,15 +14,9 @@ use crate::{ constants::{NIO_NOVA_FOLD, NUM_HASH_BITS}, gadgets::{ - ecc::AllocatedPoint, - r1cs::{ - conditionally_select_alloc_relaxed_r1cs, - conditionally_select_vec_allocated_relaxed_r1cs_instance, AllocatedR1CSInstance, - AllocatedRelaxedR1CSInstance, - }, - utils::{ - alloc_num_equals, alloc_scalar_as_base, alloc_zero, conditionally_select_vec, le_bits_to_num, - }, + alloc_num_equals, alloc_scalar_as_base, alloc_zero, conditionally_select_alloc_relaxed_r1cs, + conditionally_select_vec, conditionally_select_vec_allocated_relaxed_r1cs_instance, + le_bits_to_num, AllocatedPoint, AllocatedR1CSInstance, AllocatedRelaxedR1CSInstance, }, r1cs::{R1CSInstance, RelaxedR1CSInstance}, traits::{commitment::CommitmentTrait, Engine, ROCircuitTrait, ROConstantsCircuit}, @@ -711,7 +705,7 @@ mod tests { test_shape_cs::TestShapeCS, }, constants::{BN_LIMB_WIDTH, BN_N_LIMBS}, - gadgets::utils::scalar_as_base, + gadgets::scalar_as_base, provider::{ poseidon::PoseidonConstantsCircuit, Bn256EngineIPA, GrumpkinEngine, PallasEngine, Secp256k1Engine, Secq256k1Engine, VestaEngine, diff --git a/src/supernova/test.rs b/src/supernova/test.rs index d499b1dc8..9a7da120c 100644 --- a/src/supernova/test.rs +++ b/src/supernova/test.rs @@ -1,4 +1,4 @@ -use crate::gadgets::utils::alloc_zero; +use crate::gadgets::alloc_zero; use crate::provider::poseidon::PoseidonConstantsCircuit; use crate::provider::Bn256EngineIPA; use crate::provider::PallasEngine; @@ -6,7 +6,7 @@ use crate::provider::Secp256k1Engine; use crate::provider::VestaEngine; use crate::supernova::circuit::{StepCircuit, TrivialSecondaryCircuit, TrivialTestCircuit}; use crate::traits::snark::default_ck_hint; -use crate::{bellpepper::test_shape_cs::TestShapeCS, gadgets::utils::alloc_one}; +use crate::{bellpepper::test_shape_cs::TestShapeCS, gadgets::alloc_one}; use abomonation::Abomonation; use bellpepper_core::num::AllocatedNum; use bellpepper_core::{ConstraintSystem, SynthesisError}; diff --git a/src/supernova/utils.rs b/src/supernova/utils.rs index ec36281ea..1180d1e7c 100644 --- a/src/supernova/utils.rs +++ b/src/supernova/utils.rs @@ -8,7 +8,7 @@ use itertools::Itertools as _; use crate::{ constants::NIO_NOVA_FOLD, - gadgets::r1cs::{conditionally_select_alloc_relaxed_r1cs, AllocatedRelaxedR1CSInstance}, + gadgets::{conditionally_select_alloc_relaxed_r1cs, AllocatedRelaxedR1CSInstance}, traits::Engine, };