From fb31d09218b94bcfb807cc40ecf2a4b0b5ed775f Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@z.cash>
Date: Sat, 2 May 2020 15:48:51 +1200
Subject: [PATCH] ff: Remove Ord bound from PrimeField

ff_derive still implements Ord and PartialOrd for the fields it
implements, because pairing::bls12_381 internally assumes that those are
implemented. Once we delete that implementation, we will remove the Ord
and PartialOrd implementations from ff_derive.
---
 bellman/src/groth16/tests/dummy_engine.rs | 13 -------------
 ff/src/lib.rs                             |  2 +-
 zcash_primitives/src/jubjub/fs.rs         | 20 --------------------
 zcash_primitives/src/jubjub/tests.rs      | 23 ++++++++++++++++++-----
 4 files changed, 19 insertions(+), 39 deletions(-)

diff --git a/bellman/src/groth16/tests/dummy_engine.rs b/bellman/src/groth16/tests/dummy_engine.rs
index 3cf426a..b738faf 100644
--- a/bellman/src/groth16/tests/dummy_engine.rs
+++ b/bellman/src/groth16/tests/dummy_engine.rs
@@ -3,7 +3,6 @@ use group::{CurveAffine, CurveProjective, EncodedPoint, GroupDecodingError};
 use pairing::{Engine, PairingCurveAffine};
 
 use rand_core::RngCore;
-use std::cmp::Ordering;
 use std::fmt;
 use std::num::Wrapping;
 use std::ops::{Add, AddAssign, BitAnd, Mul, MulAssign, Neg, Shr, Sub, SubAssign};
@@ -48,18 +47,6 @@ impl ConditionallySelectable for Fr {
     }
 }
 
-impl Ord for Fr {
-    fn cmp(&self, other: &Fr) -> Ordering {
-        (self.0).0.cmp(&(other.0).0)
-    }
-}
-
-impl PartialOrd for Fr {
-    fn partial_cmp(&self, other: &Fr) -> Option<Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
 impl Neg for Fr {
     type Output = Self;
 
diff --git a/ff/src/lib.rs b/ff/src/lib.rs
index 92a6079..d59ebfe 100644
--- a/ff/src/lib.rs
+++ b/ff/src/lib.rs
@@ -147,7 +147,7 @@ impl Endianness for byteorder::LittleEndian {
 }
 
 /// This represents an element of a prime field.
-pub trait PrimeField: Field + Ord + From<u64> {
+pub trait PrimeField: Field + From<u64> {
     /// The prime field can be converted back and forth into this binary
     /// representation.
     type Repr: Default + AsRef<[u8]> + AsMut<[u8]> + From<Self> + for<'r> From<&'r Self>;
diff --git a/zcash_primitives/src/jubjub/fs.rs b/zcash_primitives/src/jubjub/fs.rs
index 6a48f8f..9f7c1b8 100644
--- a/zcash_primitives/src/jubjub/fs.rs
+++ b/zcash_primitives/src/jubjub/fs.rs
@@ -120,26 +120,6 @@ impl ConstantTimeEq for Fs {
     }
 }
 
-impl Ord for Fs {
-    #[inline(always)]
-    fn cmp(&self, other: &Fs) -> ::std::cmp::Ordering {
-        let mut a = *self;
-        a.mont_reduce(self.0[0], self.0[1], self.0[2], self.0[3], 0, 0, 0, 0);
-
-        let mut b = *other;
-        b.mont_reduce(other.0[0], other.0[1], other.0[2], other.0[3], 0, 0, 0, 0);
-
-        a.cmp_native(&b)
-    }
-}
-
-impl PartialOrd for Fs {
-    #[inline(always)]
-    fn partial_cmp(&self, other: &Fs) -> Option<::std::cmp::Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
 impl ::std::fmt::Display for Fs {
     fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> ::std::fmt::Result {
         write!(f, "Fs({})", self.into_repr())
diff --git a/zcash_primitives/src/jubjub/tests.rs b/zcash_primitives/src/jubjub/tests.rs
index a2f7fc7..6eeb1f5 100644
--- a/zcash_primitives/src/jubjub/tests.rs
+++ b/zcash_primitives/src/jubjub/tests.rs
@@ -385,9 +385,8 @@ fn test_jubjub_params<E: JubjubEngine>(params: &E::Params) {
                 borrow = new_borrow;
             }
 
-            // Convert back to a field element.
-            <E::Fs as PrimeField>::ReprEndianness::toggle_little_endian(&mut tmp);
-            E::Fs::from_repr(tmp).unwrap()
+            // Turns out we want this in little endian!
+            tmp
         };
 
         let mut pacc = E::Fs::zero();
@@ -400,8 +399,22 @@ fn test_jubjub_params<E: JubjubEngine>(params: &E::Params) {
             pacc += &tmp;
             nacc -= &tmp; // The first subtraction wraps intentionally.
 
-            assert!(pacc < max);
-            assert!(pacc < nacc);
+            let mut pacc_repr = pacc.into_repr();
+            let mut nacc_repr = nacc.into_repr();
+            <E::Fs as PrimeField>::ReprEndianness::toggle_little_endian(&mut pacc_repr);
+            <E::Fs as PrimeField>::ReprEndianness::toggle_little_endian(&mut nacc_repr);
+
+            fn less_than(val: &[u8], bound: &[u8]) -> bool {
+                for (a, b) in val.iter().rev().zip(bound.iter().rev()) {
+                    if a < b {
+                        return true;
+                    }
+                }
+
+                false
+            }
+            assert!(less_than(pacc_repr.as_ref(), max.as_ref()));
+            assert!(less_than(pacc_repr.as_ref(), nacc_repr.as_ref()));
 
             // cur = cur * 16
             for _ in 0..4 {