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

@@ -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,

src/gadgets/ecc.rs

@@ -101,18 +101,19 @@ impl<G: Group> AllocatedPoint<G> {
   }
 
   /// Allocates a default point on the curve, set to the identity point.
-  pub fn default<CS: ConstraintSystem<G::Base>>(mut cs: CS) -> Result<Self, SynthesisError> {
+  pub fn default<CS: ConstraintSystem<G::Base>>(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<G: Group> AllocatedPoint<G> {
     // 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<G: Group> AllocatedPoint<G> {
 
     // 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<G: Group> AllocatedPoint<G> {
       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<G: Group> {
 
 impl<G: Group> AllocatedPointNonInfinity<G> {
   /// Creates a new `AllocatedPointNonInfinity` from the specified coordinates
+  #[allow(unused)]
   pub const fn new(x: AllocatedNum<G::Base>, y: AllocatedNum<G::Base>) -> Self {
     Self { x, y }
   }
 
   /// Allocates a new point on the curve using coordinates provided by `coords`.
+  #[allow(unused)]
   pub fn alloc<CS: ConstraintSystem<G::Base>>(
     mut cs: CS,
     coords: Option<(G::Base, G::Base)>,
@@ -624,18 +627,16 @@ impl<G: Group> AllocatedPointNonInfinity<G> {
   }
 
   /// Returns an `AllocatedPoint` from an `AllocatedPointNonInfinity`
-  pub fn to_allocated_point(
-    &self,
-    is_infinity: &AllocatedNum<G::Base>,
-  ) -> Result<AllocatedPoint<G>, SynthesisError> {
-    Ok(AllocatedPoint {
+  pub fn to_allocated_point(&self, is_infinity: &AllocatedNum<G::Base>) -> AllocatedPoint<G> {
+    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<G::Base>, &AllocatedNum<G::Base>) {
     (&self.x, &self.y)
   }

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,
+};

src/gadgets/nonnative/util.rs

@@ -158,9 +158,9 @@ impl<Scalar: PrimeField> Num<Scalar> {
   /// Computes the natural number represented by an array of bits.
   /// Checks if the natural number equals `self`
   pub fn is_equal<CS: ConstraintSystem<Scalar>>(&self, mut cs: CS, other: &Bitvector<Scalar>) {
-    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<Scalar: PrimeField> Num<Scalar> {
     let sum_lc = LinearCombination::zero() + &self.num - &sum;
     cs.enforce(|| "sum", |lc| lc + &sum_lc, |lc| lc + CS::one(), |lc| lc);
     let bits: Vec<LinearCombination<Scalar>> = allocations
-      .clone()
-      .into_iter()
+      .iter()
       .map(|a| LinearCombination::zero() + &a.bit)
       .collect();
     Ok(Bitvector {
@@ -245,7 +244,7 @@ fn write_be<F: PrimeField, W: Write>(f: &F, mut writer: W) -> io::Result<()> {
 /// Convert a field element to a natural number
 pub fn f_to_nat<Scalar: PrimeField>(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<Scalar: PrimeField>(f: &Scalar) -> BigInt {
 pub fn nat_to_f<Scalar: PrimeField>(n: &BigInt) -> Option<Scalar> {
   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<F: PrimeFieldBits>() {
+    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::<u64>();
+
+      // 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::<pasta_curves::pallas::Scalar>();
+    test_repr_is_le_with::<pasta_curves::pallas::Base>();
+    test_repr_is_le_with::<crate::provider::bn256_grumpkin::bn256::Scalar>();
+    test_repr_is_le_with::<crate::provider::bn256_grumpkin::bn256::Base>();
+    test_repr_is_le_with::<crate::provider::secp_secq::secp256k1::Scalar>();
+    test_repr_is_le_with::<crate::provider::secp_secq::secp256k1::Base>();
+  }
+}

src/gadgets/r1cs.rs

@@ -124,7 +124,7 @@ impl<E: Engine, const N: usize> AllocatedRelaxedR1CSInstance<E, N> {
     limb_width: usize,
     n_limbs: usize,
   ) -> Result<Self, SynthesisError> {
-    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<E: Engine, const N: usize> AllocatedRelaxedR1CSInstance<E, N> {
     limb_width: usize,
     n_limbs: usize,
   ) -> Result<Self, SynthesisError> {
-    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"));
 

src/gadgets/utils.rs

@@ -80,15 +80,7 @@ where
   CS: ConstraintSystem<<E as Engine>::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::<E>(input.unwrap_or(E::Scalar::ZERO));
     Ok(val)
   })
 }

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,

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;

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,
   },

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,

src/supernova/test.rs

@@ -1,12 +1,12 @@
-use crate::gadgets::utils::alloc_zero;
+use crate::gadgets::alloc_zero;
 use crate::provider::poseidon::PoseidonConstantsCircuit;
 use crate::provider::Bn256EngineIPA;
 use crate::provider::PallasEngine;
 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};

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,
 };