Revert "Int wrapping multiplication improvements"

This reverts commit 75001b2 (#998)

We're noticing CI failures on cross which seem to be related to this
change, e.g.

https://github.com/RustCrypto/crypto-bigint/actions/runs/19647957432/job/56267760132

It looks like it's potentially getting stuck in an infinite loop.

Author: tarcieri

benches/int.rs

@@ -112,59 +112,6 @@ fn bench_concatenating_mul(c: &mut Criterion) {
     });
 }
 
-fn bench_wrapping_mul(c: &mut Criterion) {
-    let mut rng = ChaCha8Rng::from_seed([7u8; 32]);
-    let mut group = c.benchmark_group("wrapping ops");
-
-    group.bench_function("wrapping_mul, I128xI128", |b| {
-        b.iter_batched(
-            || (I256::random(&mut rng), I256::random(&mut rng)),
-            |(x, y)| black_box(x.wrapping_mul(&y)),
-            BatchSize::SmallInput,
-        )
-    });
-
-    group.bench_function("wrapping_mul, I256xI256", |b| {
-        b.iter_batched(
-            || (I256::random(&mut rng), I256::random(&mut rng)),
-            |(x, y)| black_box(x.wrapping_mul(&y)),
-            BatchSize::SmallInput,
-        )
-    });
-
-    group.bench_function("wrapping_mul, I512xI512", |b| {
-        b.iter_batched(
-            || (I512::random(&mut rng), I512::random(&mut rng)),
-            |(x, y)| black_box(x.wrapping_mul(&y)),
-            BatchSize::SmallInput,
-        )
-    });
-
-    group.bench_function("wrapping_mul, I1024xI1024", |b| {
-        b.iter_batched(
-            || (I1024::random(&mut rng), I1024::random(&mut rng)),
-            |(x, y)| black_box(x.wrapping_mul(&y)),
-            BatchSize::SmallInput,
-        )
-    });
-
-    group.bench_function("wrapping_mul, I2048xI2048", |b| {
-        b.iter_batched(
-            || (I2048::random(&mut rng), I2048::random(&mut rng)),
-            |(x, y)| black_box(x.wrapping_mul(&y)),
-            BatchSize::SmallInput,
-        )
-    });
-
-    group.bench_function("wrapping_mul, I4096xI4096", |b| {
-        b.iter_batched(
-            || (I4096::random(&mut rng), I4096::random(&mut rng)),
-            |(x, y)| black_box(x.wrapping_mul(&y)),
-            BatchSize::SmallInput,
-        )
-    });
-}
-
 fn bench_div(c: &mut Criterion) {
     let mut rng = ChaCha8Rng::from_seed([7u8; 32]);
     let mut group = c.benchmark_group("wrapping ops");
@@ -394,7 +341,6 @@ criterion_group!(
     benches,
     bench_mul,
     bench_concatenating_mul,
-    bench_wrapping_mul,
     bench_div,
     bench_add,
     bench_sub,

src/int.rs

@@ -56,17 +56,20 @@ impl<const LIMBS: usize> Int<LIMBS> {
     pub const ONE: Self = Self(Uint::ONE); // Bit sequence (be): 0000....0001
 
     /// The value `-1`
-    pub const MINUS_ONE: Self = Self(Uint::MAX); // Bit sequence (be): 1111....1111
+    pub const MINUS_ONE: Self = Self::FULL_MASK; // Bit sequence (be): 1111....1111
 
     /// Smallest value this [`Int`] can express.
-    pub const MIN: Self = Self::MAX.not(); // Bit sequence (be): 1000....0000
+    pub const MIN: Self = Self(Uint::MAX.bitxor(&Uint::MAX.shr(1u32))); // Bit sequence (be): 1000....0000
 
     /// Maximum value this [`Int`] can express.
-    pub const MAX: Self = Self(Uint::MAX.shr_vartime(1u32)); // Bit sequence (be): 0111....1111
+    pub const MAX: Self = Self(Uint::MAX.shr(1u32)); // Bit sequence (be): 0111....1111
 
     /// Bit mask for the sign bit of this [`Int`].
     pub const SIGN_MASK: Self = Self::MIN; // Bit sequence (be): 1000....0000
 
+    /// All-one bit mask.
+    pub const FULL_MASK: Self = Self(Uint::MAX); // Bit sequence (be): 1111...1111
+
     /// Total size of the represented integer in bits.
     pub const BITS: u32 = Uint::<LIMBS>::BITS;
 
@@ -110,7 +113,7 @@ impl<const LIMBS: usize> Int<LIMBS> {
     }
 
     /// Borrow the inner limbs as a mutable array of [`Word`]s.
-    pub const fn as_mut_words(&mut self) -> &mut [Word; LIMBS] {
+    pub fn as_mut_words(&mut self) -> &mut [Word; LIMBS] {
         self.0.as_mut_words()
     }
 
@@ -179,7 +182,7 @@ impl<const LIMBS: usize> Int<LIMBS> {
     }
 
     /// Whether this [`Int`] is equal to `Self::MAX`.
-    pub const fn is_max(&self) -> ConstChoice {
+    pub fn is_max(&self) -> ConstChoice {
         Self::eq(self, &Self::MAX)
     }
 

src/int/mul.rs

@@ -4,7 +4,7 @@ use core::ops::{Mul, MulAssign};
 use num_traits::WrappingMul;
 use subtle::CtOption;
 
-use crate::{Checked, CheckedMul, ConcatMixed, ConstChoice, ConstCtOption, Int, Uint};
+use crate::{Checked, CheckedMul, ConcatMixed, ConstChoice, ConstCtOption, Int, Uint, Zero};
 
 impl<const LIMBS: usize> Int<LIMBS> {
     /// Compute "wide" multiplication as a 3-tuple `(lo, hi, negate)`.
@@ -64,41 +64,12 @@ impl<const LIMBS: usize> Int<LIMBS> {
         Int::from_bits(product_abs.wrapping_neg_if(product_sign))
     }
 
-    /// Multiply `self` by `rhs`, returning a `ConstCtOption` which is `is_some` only if
-    /// overflow did not occur.
-    pub const fn checked_mul<const RHS_LIMBS: usize>(
-        &self,
-        rhs: &Int<RHS_LIMBS>,
-    ) -> ConstCtOption<Self> {
-        let (abs_lhs, lhs_sgn) = self.abs_sign();
-        let (abs_rhs, rhs_sgn) = rhs.abs_sign();
-        let maybe_res = abs_lhs.checked_mul(&abs_rhs);
-        let (lo, is_some) = maybe_res.components_ref();
-        Self::new_from_abs_sign(*lo, lhs_sgn.xor(rhs_sgn)).and_choice(is_some)
-    }
-
-    /// Multiply `self` by `rhs`, saturating at the numeric bounds instead of overflowing.
-    pub const fn saturating_mul<const RHS_LIMBS: usize>(&self, rhs: &Int<RHS_LIMBS>) -> Self {
-        let (abs_lhs, lhs_sgn) = self.abs_sign();
-        let (abs_rhs, rhs_sgn) = rhs.abs_sign();
-        let maybe_res = abs_lhs.checked_mul(&abs_rhs);
-        let (lo, is_some) = maybe_res.components_ref();
-        let is_neg = lhs_sgn.xor(rhs_sgn);
-        let bound = Self::select(&Self::MAX, &Self::MIN, is_neg);
-        Self::new_from_abs_sign(*lo, is_neg)
-            .and_choice(is_some)
-            .unwrap_or(bound)
-    }
-
     /// Multiply `self` by `rhs`, wrapping the result in case of overflow.
-    /// This is equivalent to `(self * rhs) % (Uint::<LIMBS>::MAX + 1)`.
     pub const fn wrapping_mul<const RHS_LIMBS: usize>(&self, rhs: &Int<RHS_LIMBS>) -> Self {
-        if RHS_LIMBS >= LIMBS {
-            Self(self.0.wrapping_mul(&rhs.0))
-        } else {
-            let (abs_rhs, rhs_sgn) = rhs.abs_sign();
-            Self(self.0.wrapping_mul(&abs_rhs).wrapping_neg_if(rhs_sgn))
-        }
+        let (abs_lhs, lhs_sgn) = self.abs_sign();
+        let (abs_rhs, rhs_sgn) = rhs.abs_sign();
+        let (lo, _) = abs_lhs.widening_mul(&abs_rhs);
+        *lo.wrapping_neg_if(lhs_sgn.xor(rhs_sgn)).as_int()
     }
 }
 
@@ -131,7 +102,9 @@ impl<const LIMBS: usize> Int<LIMBS> {
 impl<const LIMBS: usize, const RHS_LIMBS: usize> CheckedMul<Int<RHS_LIMBS>> for Int<LIMBS> {
     #[inline]
     fn checked_mul(&self, rhs: &Int<RHS_LIMBS>) -> CtOption<Self> {
-        self.checked_mul(rhs).into()
+        let (lo, hi, is_negative) = self.widening_mul(rhs);
+        let val = Self::new_from_abs_sign(lo, is_negative);
+        CtOption::from(val).and_then(|int| CtOption::new(int, hi.is_zero()))
     }
 }
 
@@ -200,7 +173,7 @@ impl<const LIMBS: usize> MulAssign<&Checked<Int<LIMBS>>> for Checked<Int<LIMBS>>
 
 #[cfg(test)]
 mod tests {
-    use crate::{ConstChoice, I64, I128, I256, Int, U64, U128, U256};
+    use crate::{CheckedMul, ConstChoice, I64, I128, I256, Int, U128, U256};
 
     #[test]
     #[allow(clippy::init_numbered_fields)]
@@ -298,26 +271,20 @@ mod tests {
     #[test]
     fn test_wrapping_mul() {
         // wrapping
-        let a = 0xFFFFFFFB7B63198EF870DF1F90D9BD9Eu128 as i128;
-        let b = 0xF20C29FA87B356AA3B4C05C4F9C24B4Au128 as i128;
-        let z = 0xAA700D354D6CF4EE881F8FF8093A19ACu128 as i128;
-        assert_eq!(a.wrapping_mul(b), z);
+        let a = I128::from_be_hex("FFFFFFFB7B63198EF870DF1F90D9BD9E");
+        let b = I128::from_be_hex("F20C29FA87B356AA3B4C05C4F9C24B4A");
         assert_eq!(
-            I128::from_i128(a).wrapping_mul(&I128::from_i128(b)),
-            I128::from_i128(z)
+            a.wrapping_mul(&b),
+            I128::from_be_hex("AA700D354D6CF4EE881F8FF8093A19AC")
         );
 
         // no wrapping
-        let c = -12345i64;
+        let c = I64::from_i64(-12345i64);
         assert_eq!(
-            I128::from_i128(a).wrapping_mul(&I128::from_i64(c)),
-            I128::from_i128(a.wrapping_mul(c as i128))
+            a.wrapping_mul(&c),
+            I128::from_be_hex("0000D9DEF2248095850866CFEBF727D2")
         );
 
-        // overflow into MSB
-        let a = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFu128 as i128;
-        assert!(!a.is_negative() && a.wrapping_mul(a).is_negative());
-
         // core case
         assert_eq!(i8::MAX.wrapping_mul(2), -2);
         assert_eq!(i64::MAX.wrapping_mul(2), -2);
@@ -345,88 +312,6 @@ mod tests {
         );
     }
 
-    #[test]
-    fn test_wrapping_mul_mixed() {
-        let a = U64::from_u64(0x0011223344556677);
-        let b = U128::from_u128(0x8899aabbccddeeff_8899aabbccddeeff);
-        let expected = a.as_int().concatenating_mul(b.as_int());
-        assert_eq!(a.as_int().wrapping_mul(b.as_int()), expected.resize());
-        assert_eq!(b.as_int().wrapping_mul(a.as_int()), expected.resize());
-        assert_eq!(
-            a.as_int().wrapping_neg().wrapping_mul(b.as_int()),
-            expected.wrapping_neg().resize()
-        );
-        assert_eq!(
-            a.as_int().wrapping_mul(&b.as_int().wrapping_neg()),
-            expected.wrapping_neg().resize()
-        );
-        assert_eq!(
-            b.as_int().wrapping_neg().wrapping_mul(a.as_int()),
-            expected.wrapping_neg().resize()
-        );
-        assert_eq!(
-            b.as_int().wrapping_mul(&a.as_int().wrapping_neg()),
-            expected.wrapping_neg().resize()
-        );
-        assert_eq!(
-            a.as_int()
-                .wrapping_neg()
-                .wrapping_mul(&b.as_int().wrapping_neg()),
-            expected.resize()
-        );
-        assert_eq!(
-            b.as_int()
-                .wrapping_neg()
-                .wrapping_mul(&a.as_int().wrapping_neg()),
-            expected.resize()
-        );
-    }
-
-    #[test]
-    fn test_saturating_mul() {
-        // wrapping
-        let a = 0xFFFFFFFB7B63198EF870DF1F90D9BD9Eu128 as i128;
-        let b = 0xF20C29FA87B356AA3B4C05C4F9C24B4Au128 as i128;
-        assert_eq!(a.saturating_mul(b), i128::MAX);
-        assert_eq!(
-            I128::from_i128(a).saturating_mul(&I128::from_i128(b)),
-            I128::MAX
-        );
-
-        // no wrapping
-        let c = -12345i64;
-        assert_eq!(
-            I128::from_i128(a).saturating_mul(&I128::from_i64(c)),
-            I128::from_i128(a.saturating_mul(c as i128))
-        );
-
-        // core case
-        assert_eq!(i8::MAX.saturating_mul(2), i8::MAX);
-        assert_eq!(i8::MAX.saturating_mul(-2), i8::MIN);
-        assert_eq!(i64::MAX.saturating_mul(2), i64::MAX);
-        assert_eq!(i64::MAX.saturating_mul(-2), i64::MIN);
-        assert_eq!(I128::MAX.saturating_mul(&I128::from_i64(2i64)), I128::MAX);
-        assert_eq!(I128::MAX.saturating_mul(&I128::from_i64(-2i64)), I128::MIN);
-
-        let x = -197044252290277702i64;
-        let y = -2631691865753118366;
-        assert_eq!(x.saturating_mul(y), i64::MAX);
-        assert_eq!(I64::from_i64(x).saturating_mul(&I64::from_i64(y)), I64::MAX);
-
-        let x = -86027672844719838068326470675019902915i128;
-        let y = 21188806580823612823777395451044967239i128;
-        assert_eq!(x.saturating_mul(y), i128::MIN);
-        assert_eq!(x.saturating_mul(-y), i128::MAX);
-        assert_eq!(
-            I128::from_i128(x).saturating_mul(&I128::from_i128(y)),
-            I128::MIN
-        );
-        assert_eq!(
-            I128::from_i128(x).saturating_mul(&I128::from_i128(-y)),
-            I128::MAX
-        );
-    }
-
     #[test]
     fn test_concatenating_mul() {
         assert_eq!(

src/int/mul_uint.rs

@@ -96,10 +96,8 @@ impl<const LIMBS: usize> Int<LIMBS> {
         &self,
         rhs: &Uint<RHS_LIMBS>,
     ) -> ConstCtOption<Int<LIMBS>> {
-        let (abs_lhs, lhs_sgn) = self.abs_sign();
-        let maybe_lo = abs_lhs.checked_mul(rhs);
-        let (lo, is_some) = maybe_lo.components_ref();
-        Self::new_from_abs_sign(*lo, lhs_sgn).and_choice(is_some)
+        let (lo, hi, is_negative) = self.widening_mul_uint(rhs);
+        Self::new_from_abs_sign(lo, is_negative).and_choice(hi.is_nonzero().not())
     }
 }
 
@@ -138,7 +136,7 @@ impl<const LIMBS: usize, const RHS_LIMBS: usize> Mul<&Uint<RHS_LIMBS>> for &Int<
     type Output = Int<LIMBS>;
 
     fn mul(self, rhs: &Uint<RHS_LIMBS>) -> Self::Output {
-        self.checked_mul_uint(rhs)
+        self.checked_mul(rhs)
             .expect("attempted to multiply with overflow")
     }
 }