Merge #114

114: Fix BigInt::modpow behavior for negative base r=cuviper a=youknowone

For signed base, the sign was calculated in wrong way when exp is even

Co-authored-by: Jeong YunWon <jeong@youknowone.org>
Co-authored-by: Josh Stone <cuviper@gmail.com>

src/bigint.rs

@@ -2990,7 +2990,10 @@ impl BigInt {
         }
 
         // The sign of the result follows the modulus, like `mod_floor`.
-        let (sign, mag) = match (self.is_negative(), modulus.is_negative()) {
+        let (sign, mag) = match (
+            self.is_negative() && exponent.is_odd(),
+            modulus.is_negative(),
+        ) {
             (false, false) => (Plus, result),
             (true, false) => (Plus, &modulus.data - result),
             (false, true) => (Minus, &modulus.data - result),

tests/modpow.rs

@@ -115,7 +115,7 @@ mod biguint {
 mod bigint {
     use num_bigint::BigInt;
     use num_integer::Integer;
-    use num_traits::{Num, One, Signed, Zero};
+    use num_traits::{Num, One, Signed};
 
     fn check_modpow<T: Into<BigInt>>(b: T, e: T, m: T, r: T) {
         fn check(b: &BigInt, e: &BigInt, m: &BigInt, r: &BigInt) {
@@ -135,12 +135,18 @@ mod bigint {
         let m: BigInt = m.into();
         let r: BigInt = r.into();
 
-        let neg_r = if r.is_zero() { BigInt::zero() } else { &m - &r };
+        let neg_b_r = if e.is_odd() {
+            (-&r).mod_floor(&m)
+        } else {
+            r.clone()
+        };
+        let neg_m_r = r.mod_floor(&-&m);
+        let neg_bm_r = neg_b_r.mod_floor(&-&m);
 
         check(&b, &e, &m, &r);
-        check(&-&b, &e, &m, &neg_r);
-        check(&b, &e, &-&m, &-neg_r);
-        check(&-b, &e, &-m, &-r);
+        check(&-&b, &e, &m, &neg_b_r);
+        check(&b, &e, &-&m, &neg_m_r);
+        check(&-b, &e, &-&m, &neg_bm_r);
     }
 
     #[test]
@@ -153,6 +159,20 @@ mod bigint {
         check_modpow(-20, 1, 3, 1);
     }
 
+    #[test]
+    fn test_modpow_small() {
+        for b in -10i64..11 {
+            for e in 0i64..11 {
+                for m in -10..11 {
+                    if m == 0 {
+                        continue;
+                    }
+                    check_modpow(b, e, m, b.pow(e as u32).mod_floor(&m));
+                }
+            }
+        }
+    }
+
     #[test]
     fn test_modpow_big() {
         let b = BigInt::from_str_radix(super::BIG_B, 16).unwrap();

tests/quickcheck.rs

@@ -10,7 +10,8 @@ extern crate quickcheck;
 extern crate quickcheck_macros;
 
 use num_bigint::{BigInt, BigUint};
-use num_traits::{Num, One, Pow, Zero};
+use num_integer::Integer;
+use num_traits::{Num, One, Pow, Signed, Zero};
 use quickcheck::{QuickCheck, StdThreadGen, TestResult};
 
 #[quickcheck]
@@ -315,3 +316,46 @@ fn quicktest_shift() {
     qc.quickcheck(test_shl_unsigned as fn(u32, u8) -> TestResult);
     qc.quickcheck(test_shl_signed as fn(i32, u8) -> TestResult);
 }
+
+#[test]
+fn quickcheck_modpow() {
+    let gen = StdThreadGen::new(usize::max_value());
+    let mut qc = QuickCheck::with_gen(gen);
+
+    fn simple_modpow(base: &BigInt, exponent: &BigInt, modulus: &BigInt) -> BigInt {
+        assert!(!exponent.is_negative());
+        let mut result = BigInt::one().mod_floor(modulus);
+        let mut base = base.mod_floor(modulus);
+        let mut exponent = exponent.clone();
+        while !exponent.is_zero() {
+            if exponent.is_odd() {
+                result = (result * &base).mod_floor(modulus);
+            }
+            base = (&base * &base).mod_floor(modulus);
+            exponent >>= 1;
+        }
+        result
+    }
+
+    fn test_modpow(base: i128, exponent: u128, modulus: i128) -> TestResult {
+        if modulus.is_zero() {
+            TestResult::discard()
+        } else {
+            let base = BigInt::from(base);
+            let exponent = BigInt::from(exponent);
+            let modulus = BigInt::from(modulus);
+            let modpow = base.modpow(&exponent, &modulus);
+            let simple = simple_modpow(&base, &exponent, &modulus);
+            if modpow != simple {
+                eprintln!("{}.modpow({}, {})", base, exponent, modulus);
+                eprintln!("  expected {}", simple);
+                eprintln!("    actual {}", modpow);
+                TestResult::failed()
+            } else {
+                TestResult::passed()
+            }
+        }
+    }
+
+    qc.quickcheck(test_modpow as fn(i128, u128, i128) -> TestResult);
+}