Refactor sqrt_ratio to return either sqrt(u/v) or sqrt(iu/v)

Also removes the chi function since Ristretto elligator merges it with the square root.

src/constants.rs

@@ -129,12 +129,13 @@ mod test {
         }
     }
 
+    /// Test that SQRT_M1 is the positive square root of -1
     #[test]
-    /// Test that SQRT_M1 is a square root of -1
     fn test_sqrt_minus_one() {
         let minus_one = FieldElement::minus_one();
         let sqrt_m1_sq = &constants::SQRT_M1 * &constants::SQRT_M1;
         assert_eq!(minus_one,  sqrt_m1_sq);
+        assert_eq!(constants::SQRT_M1.is_negative().unwrap_u8(), 0);
     }
 
     #[test]
@@ -143,8 +144,6 @@ mod test {
         let (was_nonzero_square, invsqrt_m1) = minus_one.invsqrt();
         assert_eq!(was_nonzero_square.unwrap_u8(), 1u8);
         let sign_test_sqrt  = &invsqrt_m1 * &constants::SQRT_M1;
-        // XXX it seems we have flipped the sign relative to
-        // the invsqrt function?
         assert_eq!(sign_test_sqrt, minus_one);
     }
 

src/edwards.rs

@@ -169,9 +169,9 @@ impl CompressedEdwardsY {
         let YY = Y.square();
         let u = &YY - &Z;                            // u =  y²-1
         let v = &(&YY * &constants::EDWARDS_D) + &Z; // v = dy²+1
-        let (is_nonzero_square, mut X) = FieldElement::sqrt_ratio(&u, &v);
+        let (is_valid_y_coord, mut X) = FieldElement::sqrt_ratio_i(&u, &v);
 
-        if is_nonzero_square.unwrap_u8() != 1u8 { return None; }
+        if is_valid_y_coord.unwrap_u8() != 1u8 { return None; }
 
         // Flip the sign of X if it's not correct
         let compressed_sign_bit = Choice::from(self.as_bytes()[31] >> 7);

src/field.rs

@@ -195,40 +195,19 @@ impl FieldElement {
         t21
     }
 
-    /// Given `FieldElements` `u` and `v`, attempt to compute
+    /// Given `FieldElements` `u` and `v`, compute either `sqrt(u/v)`
-    /// `sqrt(u/v)` in constant time.
+    /// or `sqrt(i*u/v)` in constant time.
     ///
-    /// This function always returns the nonnegative square root, if it exists.
+    /// This function always returns the nonnegative square root.
-    ///
-    /// It would be much better to use an `Option` type here, but
-    /// doing so forces the caller to branch, which we don't want to
-    /// do.  This seems like the least bad solution.
     ///
     /// # Return
     ///
-    /// - `(1u8, sqrt(u/v))` if `v` is nonzero and `u/v` is square;
+    /// - `(Choice(1), +sqrt(u/v))  ` if `v` is nonzero and `u/v` is square;
-    /// - `(0u8, zero)`      if `v` is zero;
+    /// - `(Choice(1), zero)        ` if `u` is zero;
-    /// - `(0u8, garbage)`   if `u/v` is nonsquare.
+    /// - `(Choice(0), zero)        ` if `v` is zero and `u` is nonzero;
+    /// - `(Choice(0), +sqrt(i*u/v))` if `u/v` is nonsquare (so `i*u/v` is square).
     ///
-    /// # Example
+    pub fn sqrt_ratio_i(u: &FieldElement, v: &FieldElement) -> (Choice, FieldElement) {
-    ///
-    /// ```ignore
-    /// let one = FieldElement::one();
-    /// let two = &one + &one;
-    /// let four = &two * &two;
-    ///
-    /// // two is nonsquare mod p
-    /// let (two_is_square, two_sqrt) = FieldElement::sqrt_ratio(&two, &one);
-    /// assert_eq!(two_is_square.unwrap_u8(), 0u8);
-    ///
-    /// // four is square mod p
-    /// let (four_is_square, four_sqrt) = FieldElement::sqrt_ratio(&four, &one);
-    ///
-    /// assert_eq!(four_is_square.unwrap_u8(), 1u8);
-    /// assert_eq!(four_sqrt.is_negative().unwrap_u8
-    /// ```
-    ///
-    pub fn sqrt_ratio(u: &FieldElement, v: &FieldElement) -> (Choice, FieldElement) {
         // Using the same trick as in ed25519 decoding, we merge the
         // inversion, the square root, and the square test as follows.
         //
@@ -258,11 +237,14 @@ impl FieldElement {
         let mut r = &(u * &v3) * &(u * &v7).pow_p58();
         let check = v * &r.square();
 
-        let correct_sign_sqrt = check.ct_eq(   u);
+        let i = &constants::SQRT_M1;
-        let flipped_sign_sqrt = check.ct_eq(&(-u));
+
+        let correct_sign_sqrt   = check.ct_eq(        u);
+        let flipped_sign_sqrt   = check.ct_eq(     &(-u));
+        let flipped_sign_sqrt_i = check.ct_eq(&(&(-u)*i));
 
         let r_prime = &constants::SQRT_M1 * &r;
-        r.conditional_assign(&r_prime, flipped_sign_sqrt);
+        r.conditional_assign(&r_prime, flipped_sign_sqrt | flipped_sign_sqrt_i);
 
         // Choose the nonnegative square root.
         let r_is_negative = r.is_negative();
@@ -273,42 +255,20 @@ impl FieldElement {
         (was_nonzero_square, r)
     }
 
-    /// For `self` a nonzero square, compute 1/sqrt(self) in
+    /// Attempt to compute `1/sqrt(self)` in constant time.
-    /// constant time.
     ///
-    /// It would be much better to use an `Option` type here, but
+    /// Convenience wrapper around `sqrt_ratio_i`.
-    /// doing so forces the caller to branch, which we don't want to
+    ///
-    /// do.  This seems like the least bad solution.
+    /// This function always returns the nonnegative square root.
     ///
     /// # Return
     ///
-    /// - `(1u8, 1/sqrt(self))` if `self` is a nonzero square;
+    /// - `(Choice(1), +sqrt(1/self))  ` if `self` is a nonzero square;
-    /// - `(0u8, zero)`         if `self` is zero;
+    /// - `(Choice(0), zero)           ` if `self` is zero;
-    /// - `(0u8, garbage)`      if `self` is nonsquare.
+    /// - `(Choice(0), +sqrt(i*u/v))   ` if `self` is a nonzero nonsquare;
     ///
     pub fn invsqrt(&self) -> (Choice, FieldElement) {
-        FieldElement::sqrt_ratio(&FieldElement::one(), self)
+        FieldElement::sqrt_ratio_i(&FieldElement::one(), self)
-    }
-
-    /// chi calculates `self^((p-1)/2)`.
-    ///
-    /// # Return
-    ///
-    /// * If this element is a non-zero square, returns `1`.
-    /// * If it is zero, returns `0`.
-    /// * If it is non-square, returns `-1`.
-    pub fn chi(&self) -> FieldElement {  // extra25519.chi
-        // The bits of (p-1)/2 = 2^254 -10 are 0110111111...11.
-        //
-        //                                 nonzero bits of exponent
-        let (t19, _) = self.pow22501();    // 249..0
-        let t20 = t19.pow2k(4);            // 253..4
-        let t21 = self.square();           // 1
-        let t22 = t21.square();            // 2
-        let t23 = &t22 * &t21;             // 2,1
-        let t24 = &t20 * &t23;             // 253..4,2,1
-
-        t24
     }
 }
 
@@ -392,6 +352,45 @@ mod test {
         }
     }
 
+    #[test]
+    fn sqrt_ratio_behavior() {
+        let zero = FieldElement::zero();
+        let one = FieldElement::one();
+        let i = constants::SQRT_M1;
+        let two = &one + &one; // 2 is nonsquare mod p.
+        let four = &two + &two; // 4 is square mod p.
+
+        // 0/0 should return (1, 0) since u is 0
+        let (choice, sqrt) = FieldElement::sqrt_ratio_i(&zero, &zero);
+        assert_eq!(choice.unwrap_u8(), 1);
+        assert_eq!(sqrt, zero);
+        assert_eq!(sqrt.is_negative().unwrap_u8(), 0);
+
+        // 1/0 should return (0, 0) since v is 0, u is nonzero
+        let (choice, sqrt) = FieldElement::sqrt_ratio_i(&one, &zero);
+        assert_eq!(choice.unwrap_u8(), 0);
+        assert_eq!(sqrt, zero);
+        assert_eq!(sqrt.is_negative().unwrap_u8(), 0);
+
+        // 2/1 is nonsquare, so we expect (0, sqrt(i*2))
+        let (choice, sqrt) = FieldElement::sqrt_ratio_i(&two, &one);
+        assert_eq!(choice.unwrap_u8(), 0);
+        assert_eq!(sqrt.square(), &two * &i);
+        assert_eq!(sqrt.is_negative().unwrap_u8(), 0);
+
+        // 4/1 is square, so we expect (1, sqrt(4))
+        let (choice, sqrt) = FieldElement::sqrt_ratio_i(&four, &one);
+        assert_eq!(choice.unwrap_u8(), 1);
+        assert_eq!(sqrt.square(), four);
+        assert_eq!(sqrt.is_negative().unwrap_u8(), 0);
+
+        // 1/4 is square, so we expect (1, 1/sqrt(4))
+        let (choice, sqrt) = FieldElement::sqrt_ratio_i(&one, &four);
+        assert_eq!(choice.unwrap_u8(), 1);
+        assert_eq!(&sqrt.square() * &four, one);
+        assert_eq!(sqrt.is_negative().unwrap_u8(), 0);
+    }
+
     #[test]
     fn a_p58_vs_ap58_constant() {
         let a    = FieldElement::from_bytes(&A_BYTES);
@@ -399,17 +398,6 @@ mod test {
         assert_eq!(ap58, a.pow_p58());
     }
 
-    #[test]
-    fn chi_on_square_and_nonsquare() {
-        let a = FieldElement::from_bytes(&A_BYTES);
-        // a is square
-        assert_eq!(a.chi(), FieldElement::one());
-        let mut two_bytes = [0u8; 32]; two_bytes[0] = 2;
-        let two = FieldElement::from_bytes(&two_bytes);
-        // 2 is nonsquare
-        assert_eq!(two.chi(), FieldElement::minus_one());
-    }
-
     #[test]
     fn equality() {
         let a    = FieldElement::from_bytes(&A_BYTES);

src/ristretto.rs

@@ -586,21 +586,14 @@ impl RistrettoPoint {
         let d_sq = d.square();
         let N = -&( &(&d_sq - &one) * &(&r + &one) );
 
-        let mut s = FieldElement::zero();
         let mut c = -&one;
+        let (N_over_D_is_square, mut s) = FieldElement::sqrt_ratio_i(&N, &D);
+        let mut s_prime = &s * r_0;
+        let s_prime_is_pos = !s_prime.is_negative();
+        s_prime.conditional_negate(s_prime_is_pos);
 
-        let (N_over_D_is_square, maybe_s) = FieldElement::sqrt_ratio(&N, &D);
+        s.conditional_assign(&s_prime, !N_over_D_is_square);
-        // s = sqrt(N/D) if N/D is square
+        c.conditional_assign(&r, !N_over_D_is_square);
-        s.conditional_assign(&maybe_s, N_over_D_is_square);
-
-        // XXX how exactly do we reuse the computation of sqrt(N/D) to find sqrt(rN/D) ?
-        let (rN_over_D_is_square, mut maybe_s) = FieldElement::sqrt_ratio(&(&r*&N), &D);
-        maybe_s.negate();
-
-        // s = -sqrt(rN/D) if rN/D is square (should happen exactly when N/D is nonsquare)
-        debug_assert_eq!((N_over_D_is_square ^ rN_over_D_is_square).unwrap_u8(), 1u8);
-        s.conditional_assign(&maybe_s, rN_over_D_is_square);
-        c.conditional_assign(&r, rN_over_D_is_square);
 
         // T = (c * (r - one) * (d-one).square()) - D;
         let T = &(&c * &(&(&r - &one) * &((d - &one).square()))) - &D;