Rename BigInteger::add_nocarry and sub_noborrow and add tests (#383)

Co-authored-by: Pratyush Mishra <[email protected]>

CHANGELOG.md

@@ -26,6 +26,8 @@
     - Rename `Fp*Parameters` to `Fp*Config`.
     - Add `From<u32>`, `From<u16>`, and `From<u8>` `impl`s for `BigInt<N>`.
     - Remove `FftConfig`; move its contents to `FftField`.
+- [\#383](https://github.com/arkworks-rs/algebra/pull/383) (arc-ff) Rename `BigInteger::add_nocarry` to `add_with_carry` and `sub_noborrow` to `sub_with_borrow`.
+
 
 ### Features
 

ff/src/biginteger/mod.rs

@@ -85,7 +85,7 @@ macro_rules! const_modulo {
             remainder = remainder.const_mul2();
             remainder.0[0] |= $a.get_bit(i as usize) as u64;
             if remainder.const_geq($divisor) {
-                let (r, borrow) = remainder.const_sub_noborrow($divisor);
+                let (r, borrow) = remainder.const_sub_with_borrow($divisor);
                 remainder = r;
                 assert!(!borrow);
             }
@@ -183,7 +183,7 @@ impl<const N: usize> BigInt<N> {
 
     #[inline]
     #[ark_ff_asm::unroll_for_loops]
-    pub(crate) const fn const_sub_noborrow(mut self, other: &Self) -> (Self, bool) {
+    pub(crate) const fn const_sub_with_borrow(mut self, other: &Self) -> (Self, bool) {
         let mut borrow = 0;
 
         const_for!((i in 0..N) {
@@ -235,7 +235,7 @@ impl<const N: usize> BigInteger for BigInt<N> {
 
     #[inline]
     #[ark_ff_asm::unroll_for_loops]
-    fn add_nocarry(&mut self, other: &Self) -> bool {
+    fn add_with_carry(&mut self, other: &Self) -> bool {
         let mut carry = 0;
 
         for i in 0..N {
@@ -257,7 +257,7 @@ impl<const N: usize> BigInteger for BigInt<N> {
 
     #[inline]
     #[ark_ff_asm::unroll_for_loops]
-    fn sub_noborrow(&mut self, other: &Self) -> bool {
+    fn sub_with_borrow(&mut self, other: &Self) -> bool {
         let mut borrow = 0;
 
         for i in 0..N {
@@ -701,42 +701,47 @@ pub trait BigInteger:
     /// Number of 64-bit limbs representing `Self`.
     const NUM_LIMBS: usize;
 
-    /// Add another representation to this one, returning the carry bit.
+    /// Add another [`BigInteger`] to `self`. This method stores the result in `self`,
+    /// and returns a carry bit.
     /// # Example
     ///
     /// ```
     /// use ark_ff::{biginteger::BigInteger64 as B, BigInteger as _};
     ///
     /// // Basic
-    /// let (mut one, mut two_add) = (B::from(1u64), B::from(2u64));
-    /// two_add.add_nocarry(&one);
-    /// assert_eq!(two_add, B::from(3u64));
+    /// let (mut one, mut x) = (B::from(1u64), B::from(2u64));
+    /// let carry = x.add_with_carry(&one);
+    /// assert_eq!(x, B::from(3u64));
+    /// assert_eq!(carry, false);
     ///
     /// // Edge-Case
-    /// let mut max_one = B::from(u64::MAX);
-    /// let carry = max_one.add_nocarry(&one);
-    /// assert_eq!(max_one, B::from(0u64));
+    /// let mut x = B::from(u64::MAX);
+    /// let carry = x.add_with_carry(&one);
+    /// assert_eq!(x, B::from(0u64));
     /// assert_eq!(carry, true)
     /// ```
-    fn add_nocarry(&mut self, other: &Self) -> bool;
+    fn add_with_carry(&mut self, other: &Self) -> bool;
 
-    /// Subtract another representation from this one, returning the borrow bit.
+    /// Subtract another [`BigInteger`] from this one. This method stores the result in
+    /// `self`, and returns a borrow.
+    /// the borrow bit.
     /// # Example
     ///
     /// ```
     /// use ark_ff::{biginteger::BigInteger64 as B, BigInteger as _};
     ///
     /// // Basic
-    /// let (mut one, mut two, mut three_sub) = (B::from(1u64), B::from(2u64), B::from(3u64));
-    /// three_sub.sub_noborrow(&two);
-    /// assert_eq!(three_sub, one);
+    /// let (mut one_sub, two, mut three_sub) = (B::from(1u64), B::from(2u64), B::from(3u64));
+    /// let borrow = three_sub.sub_with_borrow(&two);
+    /// assert_eq!(three_sub, one_sub);
+    /// assert_eq!(borrow, false);
     ///
     /// // Edge-Case
-    /// let borrow = one.sub_noborrow(&two);
+    /// let borrow = one_sub.sub_with_borrow(&two);
+    /// assert_eq!(one_sub, B::from(u64::MAX));
     /// assert_eq!(borrow, true);
-    /// assert_eq!(one, B::from(u64::MAX));
     /// ```
-    fn sub_noborrow(&mut self, other: &Self) -> bool;
+    fn sub_with_borrow(&mut self, other: &Self) -> bool;
 
     /// Performs a leftwise bitshift of this number, effectively multiplying
     /// it by 2. Overflow is ignored.
@@ -763,7 +768,8 @@ pub trait BigInteger:
     /// ```
     fn mul2(&mut self);
 
-    /// Performs a leftwise bitshift of this number by some amount.
+    /// Performs a leftwise bitshift of this number by n bits, effectively multiplying
+    /// it by 2^n. Overflow is ignored.
     /// # Example
     ///
     /// ```
@@ -996,9 +1002,9 @@ pub trait BigInteger:
                 if e.is_odd() {
                     z = signed_mod_reduction(e.as_ref()[0], 1 << w);
                     if z >= 0 {
-                        e.sub_noborrow(&Self::from(z as u64));
+                        e.sub_with_borrow(&Self::from(z as u64));
                     } else {
-                        e.add_nocarry(&Self::from((-z) as u64));
+                        e.add_with_carry(&Self::from((-z) as u64));
                     }
                 } else {
                     z = 0;

ff/src/biginteger/tests.rs

@@ -14,25 +14,41 @@ fn biginteger_arithmetic_test<B: BigInteger>(a: B, b: B, zero: B) {
 
     // a + 0 = a
     let mut a0_add = a.clone();
-    a0_add.add_nocarry(&zero);
+    let carry = a0_add.add_with_carry(&zero);
     assert_eq!(a0_add, a);
+    assert_eq!(carry, false);
 
     // a - 0 = a
     let mut a0_sub = a.clone();
-    a0_sub.sub_noborrow(&zero);
+    let borrow = a0_sub.sub_with_borrow(&zero);
     assert_eq!(a0_sub, a);
+    assert_eq!(borrow, false);
 
     // a - a = 0
     let mut aa_sub = a.clone();
-    aa_sub.sub_noborrow(&a);
+    let borrow = aa_sub.sub_with_borrow(&a);
     assert_eq!(aa_sub, zero);
+    assert_eq!(borrow, false);
 
     // a + b = b + a
     let mut ab_add = a.clone();
-    ab_add.add_nocarry(&b);
+    let ab_carry = ab_add.add_with_carry(&b);
     let mut ba_add = b.clone();
-    ba_add.add_nocarry(&a);
+    let ba_carry = ba_add.add_with_carry(&a);
     assert_eq!(ab_add, ba_add);
+    assert_eq!(ab_carry, ba_carry);
+
+    // a * 1 = a
+    let mut a_mul1 = a.clone();
+    a_mul1.muln(0);
+    assert_eq!(a_mul1, a);
+
+    // a * 2 = a + a
+    let mut a_mul2 = a.clone();
+    a_mul2.mul2();
+    let mut a_plus_a = a.clone();
+    a_plus_a.add_with_carry(&a); // Won't assert anything about carry bit.
+    assert_eq!(a_mul2, a_plus_a);
 }
 
 // Test correctness of BigInteger's bit values

ff/src/fields/models/fp/mod.rs

@@ -198,7 +198,7 @@ impl<P: FpConfig<N>, const N: usize> Fp<P, N> {
     #[inline]
     fn subtract_modulus(&mut self) {
         if !self.is_less_than_modulus() {
-            self.0.sub_noborrow(&Self::MODULUS);
+            self.0.sub_with_borrow(&Self::MODULUS);
         }
     }
 
@@ -733,7 +733,7 @@ impl<P: FpConfig<N>, const N: usize> Neg for Fp<P, N> {
     fn neg(self) -> Self {
         if !self.is_zero() {
             let mut tmp = P::MODULUS;
-            tmp.sub_noborrow(&self.0);
+            tmp.sub_with_borrow(&self.0);
             Fp::new(tmp)
         } else {
             self

ff/src/fields/models/fp/montgomery_backend.rs

@@ -52,17 +52,17 @@ pub trait MontConfig<const N: usize>: 'static + Sync + Send + Sized {
     /// Set a += b;
     fn add_assign(a: &mut Fp<MontBackend<Self, N>, N>, b: &Fp<MontBackend<Self, N>, N>) {
         // This cannot exceed the backing capacity.
-        a.0.add_nocarry(&b.0);
+        a.0.add_with_carry(&b.0);
         // However, it may need to be reduced
         a.subtract_modulus();
     }
 
     fn sub_assign(a: &mut Fp<MontBackend<Self, N>, N>, b: &Fp<MontBackend<Self, N>, N>) {
         // If `other` is larger than `self`, add the modulus to self first.
         if b.0 > a.0 {
-            a.0.add_nocarry(&Self::MODULUS);
+            a.0.add_with_carry(&Self::MODULUS);
         }
-        a.0.sub_noborrow(&b.0);
+        a.0.sub_with_borrow(&b.0);
     }
 
     fn double_in_place(a: &mut Fp<MontBackend<Self, N>, N>) {
@@ -220,7 +220,7 @@ pub trait MontConfig<const N: usize>: 'static + Sync + Send + Sized {
                     if b.0.is_even() {
                         b.0.div2();
                     } else {
-                        b.0.add_nocarry(&Self::MODULUS);
+                        b.0.add_with_carry(&Self::MODULUS);
                         b.0.div2();
                     }
                 }
@@ -231,16 +231,16 @@ pub trait MontConfig<const N: usize>: 'static + Sync + Send + Sized {
                     if c.0.is_even() {
                         c.0.div2();
                     } else {
-                        c.0.add_nocarry(&Self::MODULUS);
+                        c.0.add_with_carry(&Self::MODULUS);
                         c.0.div2();
                     }
                 }
 
                 if v < u {
-                    u.sub_noborrow(&v);
+                    u.sub_with_borrow(&v);
                     b -= &c;
                 } else {
-                    v.sub_noborrow(&u);
+                    v.sub_with_borrow(&u);
                     c -= &b;
                 }
             }
@@ -428,7 +428,7 @@ impl<T, const N: usize> Fp<MontBackend<T, N>, N> {
 
     const fn const_neg(self, modulus: BigInt<N>) -> Self {
         if !self.const_is_zero() {
-            Self::new(Self::sub_noborrow(&modulus, &self.0))
+            Self::new(Self::sub_with_borrow(&modulus, &self.0))
         } else {
             self
         }
@@ -539,13 +539,13 @@ impl<T, const N: usize> Fp<MontBackend<T, N>, N> {
     #[inline]
     const fn const_reduce(mut self, modulus: BigInt<N>) -> Self {
         if !self.const_is_valid(modulus) {
-            self.0 = Self::sub_noborrow(&self.0, &modulus);
+            self.0 = Self::sub_with_borrow(&self.0, &modulus);
         }
         self
     }
 
-    const fn sub_noborrow(a: &BigInt<N>, b: &BigInt<N>) -> BigInt<N> {
-        a.const_sub_noborrow(b).0
+    const fn sub_with_borrow(a: &BigInt<N>, b: &BigInt<N>) -> BigInt<N> {
+        a.const_sub_with_borrow(b).0
     }
 }
 

ff/src/fields/models/quadratic_extension.rs

@@ -432,7 +432,7 @@ where
         // This is cheaper than `P::BaseField::one().double().inverse()`
         let mut two_inv = P::BasePrimeField::MODULUS;
 
-        two_inv.add_nocarry(&1u64.into());
+        two_inv.add_with_carry(&1u64.into());
         two_inv.div2();
 
         let two_inv = P::BasePrimeField::from(two_inv);