Refactor bit iteration infrastructure:

* `to_bits` -> `to_bits_le`
* `BitIterator` -> `BitIteratorLE` + `BitIteratorBE`
* `found_one`/`seen_one` -> `BitIteratorBE::without_leading_zeros`

r1cs-std/src/bits/boolean.rs

@@ -1,4 +1,4 @@
-use algebra::{BitIterator, Field};
+use algebra::{BitIteratorBE, Field};
 
 use crate::{prelude::*, Assignment, Vec};
 use core::borrow::Borrow;
@@ -373,14 +373,15 @@ impl<F: Field> Boolean<F> {
         }
     }
 
-    /// Asserts that this bit_gadget representation is "in
-    /// the field" when interpreted in big endian.
-    pub fn enforce_in_field(bits: &[Self]) -> Result<(), SynthesisError> {
-        // b = char() - 1
+    /// Enforces that `bits`, when interpreted as a integer, is less than `F::characteristic()`,
+    /// That is, interpret bits as a little-endian integer, and enforce that this integer
+    /// is "in the field F".
+    pub fn enforce_in_field_le(bits: &[Self]) -> Result<(), SynthesisError> {
+        // `bits` < F::characteristic() <==> `bits` <= F::characteristic() -1
         let mut b = F::characteristic().to_vec();
         assert_eq!(b[0] % 2, 1);
-        b[0] -= 1;
-        let run = Self::enforce_smaller_or_equal_than(bits, b)?;
+        b[0] -= 1; // This works, because the LSB is one, so there's no borrows.
+        let run = Self::enforce_smaller_or_equal_than_le(bits, b)?;
 
         // We should always end in a "run" of zeros, because
         // the characteristic is an odd prime. So, this should
@@ -390,57 +391,35 @@ impl<F: Field> Boolean<F> {
         Ok(())
     }
 
-    /// Asserts that this bit_gadget representation is smaller
-    /// or equal than the provided element
-    pub fn enforce_smaller_or_equal_than(
+    /// Enforces that `bits` is less than or equal to `element`,
+    /// when both are interpreted as (little-endian) integers.
+    pub fn enforce_smaller_or_equal_than_le<'a>(
         bits: &[Self],
         element: impl AsRef<[u64]>,
     ) -> Result<Vec<Self>, SynthesisError> {
-        let mut bits_iter = bits.iter();
         let b: &[u64] = element.as_ref();
 
+        let mut bits_iter = bits.iter().rev(); // Iterate in big-endian
+
         // Runs of ones in r
         let mut last_run = Boolean::constant(true);
         let mut current_run = vec![];
 
-        let mut found_one = false;
+        let mut element_num_bits = 0;
+        for _ in BitIteratorBE::without_leading_zeros(b) {
+            element_num_bits += 1;
+        }
 
-        let char_num_bits = {
-            let mut leading_zeros = 0;
-            let mut total_bits = 0;
-            let mut found_one = false;
-            for b in BitIterator::new(b.clone()) {
-                total_bits += 1;
-                if !b && !found_one {
-                    leading_zeros += 1
-                }
-                if b {
-                    found_one = true;
-                }
-            }
-
-            total_bits - leading_zeros
-        };
-
-        if bits.len() > char_num_bits {
-            let num_extra_bits = bits.len() - char_num_bits;
+        if bits.len() > element_num_bits {
             let mut or_result = Boolean::constant(false);
-            for should_be_zero in &bits[0..num_extra_bits] {
+            for should_be_zero in &bits[element_num_bits..] {
                 or_result = or_result.or(should_be_zero)?;
                 let _ = bits_iter.next().unwrap();
             }
             or_result.enforce_equal(&Boolean::constant(false))?;
         }
 
-        for b in BitIterator::new(b) {
-            // Skip over unset bits at the beginning
-            found_one |= b;
-            if !found_one {
-                continue;
-            }
-
-            let a = bits_iter.next().unwrap();
-
+        for (b, a) in BitIteratorBE::without_leading_zeros(b).zip(bits_iter.by_ref()) {
             if b {
                 // This is part of a run of ones.
                 current_run.push(a.clone());
@@ -586,9 +565,8 @@ impl<F: Field> EqGadget<F> for Boolean<F> {
 impl<F: Field> ToBytesGadget<F> for Boolean<F> {
     /// Outputs `1u8` if `self` is true, and `0u8` otherwise.
     fn to_bytes(&self) -> Result<Vec<UInt8<F>>, SynthesisError> {
-        let mut bits = vec![Boolean::constant(false); 7];
-        bits.push(self.clone());
-        bits.reverse();
+        let mut bits = vec![self.clone()];
+        bits.extend(vec![Boolean::constant(false); 7]);
         let value = self.value().map(|val| val as u8).ok();
         let byte = UInt8 { bits, value };
         Ok(vec![byte])
@@ -655,7 +633,9 @@ impl<F: Field> CondSelectGadget<F> for Boolean<F> {
 mod test {
     use super::{AllocatedBit, Boolean};
     use crate::prelude::*;
-    use algebra::{bls12_381::Fr, BitIterator, Field, One, PrimeField, UniformRand, Zero};
+    use algebra::{
+        bls12_381::Fr, BitIteratorBE, BitIteratorLE, Field, One, PrimeField, UniformRand, Zero,
+    };
     use r1cs_core::{ConstraintSystem, Namespace, SynthesisError};
     use rand::SeedableRng;
     use rand_xorshift::XorShiftRng;
@@ -1331,17 +1311,58 @@ mod test {
         Ok(())
     }
 
+    #[test]
+    fn test_smaller_than_or_equal_to() -> Result<(), SynthesisError> {
+        let mut rng = XorShiftRng::seed_from_u64(1231275789u64);
+        for _ in 0..1000 {
+            let mut r = Fr::rand(&mut rng);
+            let mut s = Fr::rand(&mut rng);
+            if r > s {
+                core::mem::swap(&mut r, &mut s)
+            }
+
+            let cs = ConstraintSystem::<Fr>::new_ref();
+
+            let native_bits: Vec<_> = BitIteratorLE::new(r.into_repr()).collect();
+            let bits = Vec::new_witness(cs.clone(), || Ok(native_bits))?;
+            Boolean::enforce_smaller_or_equal_than_le(&bits, s.into_repr())?;
+
+            assert!(cs.is_satisfied().unwrap());
+        }
+
+        for _ in 0..1000 {
+            let r = Fr::rand(&mut rng);
+            if r == -Fr::one() {
+                continue;
+            }
+            let s = r + Fr::one();
+            let s2 = r.double();
+            let cs = ConstraintSystem::<Fr>::new_ref();
+
+            let native_bits: Vec<_> = BitIteratorLE::new(r.into_repr()).collect();
+            let bits = Vec::new_witness(cs.clone(), || Ok(native_bits))?;
+            Boolean::enforce_smaller_or_equal_than_le(&bits, s.into_repr())?;
+            if r < s2 {
+                Boolean::enforce_smaller_or_equal_than_le(&bits, s2.into_repr())?;
+            }
+
+            assert!(cs.is_satisfied().unwrap());
+        }
+        Ok(())
+    }
+
     #[test]
     fn test_enforce_in_field() -> Result<(), SynthesisError> {
         {
             let cs = ConstraintSystem::<Fr>::new_ref();
 
             let mut bits = vec![];
-            for b in BitIterator::new(Fr::characteristic()).skip(1) {
+            for b in BitIteratorBE::new(Fr::characteristic()).skip(1) {
                 bits.push(Boolean::new_witness(cs.clone(), || Ok(b))?);
             }
+            bits.reverse();
 
-            Boolean::enforce_in_field(&bits)?;
+            Boolean::enforce_in_field_le(&bits)?;
 
             assert!(!cs.is_satisfied().unwrap());
         }
@@ -1353,11 +1374,12 @@ mod test {
             let cs = ConstraintSystem::<Fr>::new_ref();
 
             let mut bits = vec![];
-            for b in BitIterator::new(r.into_repr()).skip(1) {
+            for b in BitIteratorBE::new(r.into_repr()).skip(1) {
                 bits.push(Boolean::new_witness(cs.clone(), || Ok(b))?);
             }
+            bits.reverse();
 
-            Boolean::enforce_in_field(&bits)?;
+            Boolean::enforce_in_field_le(&bits)?;
 
             assert!(cs.is_satisfied().unwrap());
         }

r1cs-std/src/bits/mod.rs

@@ -15,66 +15,63 @@ make_uint!(UInt32, 32, u32, uint32);
 make_uint!(UInt64, 64, u64, uint64);
 
 pub trait ToBitsGadget<F: Field> {
-    /// Outputs the canonical bit-wise representation of `self`.
+    /// Outputs the canonical little-endian bit-wise representation of `self`.
     ///
     /// This is the correct default for 99% of use cases.
-    fn to_bits(&self) -> Result<Vec<Boolean<F>>, SynthesisError>;
+    fn to_bits_le(&self) -> Result<Vec<Boolean<F>>, SynthesisError>;
 
     /// Outputs a possibly non-unique bit-wise representation of `self`.
     ///
     /// If you're not absolutely certain that your usecase can get away with a
     /// non-canonical representation, please use `self.to_bits(cs)` instead.
-    fn to_non_unique_bits(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
-        self.to_bits()
+    fn to_non_unique_bits_le(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
+        self.to_bits_le()
     }
 }
 
 impl<F: Field> ToBitsGadget<F> for Boolean<F> {
-    fn to_bits(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
+    fn to_bits_le(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
         Ok(vec![self.clone()])
     }
 }
 
 impl<F: Field> ToBitsGadget<F> for [Boolean<F>] {
-    fn to_bits(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
+    /// Outputs `self`.
+    fn to_bits_le(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
         Ok(self.to_vec())
     }
 }
 
-impl<F: Field> ToBitsGadget<F> for Vec<Boolean<F>> {
-    fn to_bits(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
-        Ok(self.clone())
-    }
-}
-
 impl<F: Field> ToBitsGadget<F> for UInt8<F> {
-    fn to_bits(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
-        Ok(self.into_bits_le())
+    fn to_bits_le(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
+        Ok(self.bits.to_vec())
     }
 }
 
 impl<F: Field> ToBitsGadget<F> for [UInt8<F>] {
-    fn to_bits(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
-        let mut result = Vec::with_capacity(&self.len() * 8);
-        for byte in self {
-            result.extend_from_slice(&byte.into_bits_le());
-        }
-        Ok(result)
+    /// Interprets `self` as an integer, and outputs the little-endian
+    /// bit-wise decomposition of that integer.
+    fn to_bits_le(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
+        let bits = self.iter().flat_map(|b| &b.bits).cloned().collect();
+        Ok(bits)
     }
 }
 
-impl<F: Field> ToBitsGadget<F> for Vec<UInt8<F>> {
-    fn to_bits(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
-        let mut result = Vec::with_capacity(&self.len() * 8);
-        for byte in self {
-            result.extend_from_slice(&byte.into_bits_le());
-        }
-        Ok(result)
+impl<F: Field, T> ToBitsGadget<F> for Vec<T>
+where
+    [T]: ToBitsGadget<F>,
+{
+    fn to_bits_le(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
+        self.as_slice().to_bits_le().map(|v| v.to_vec())
+    }
+
+    fn to_non_unique_bits_le(&self) -> Result<Vec<Boolean<F>>, SynthesisError> {
+        self.as_slice().to_non_unique_bits_le().map(|v| v.to_vec())
     }
 }
 
 pub trait ToBytesGadget<F: Field> {
-    /// Outputs a canonical byte-wise representation of `self`.
+    /// Outputs a canonical, little-endian, byte decomposition of `self`.
     ///
     /// This is the correct default for 99% of use cases.
     fn to_bytes(&self) -> Result<Vec<UInt8<F>>, SynthesisError>;

r1cs-std/src/bits/uint8.rs

@@ -98,11 +98,7 @@ impl<F: Field> UInt8<F> {
         let mut allocated_bits = Vec::new();
         for field_element in field_elements.into_iter() {
             let fe = AllocatedFp::new_input(cs.clone(), || Ok(field_element))?;
-            let mut fe_bits = fe.to_bits()?;
-            // FpGadget::to_bits outputs a big-endian binary representation of
-            // fe_gadget's value, so we have to reverse it to get the little-endian
-            // form.
-            fe_bits.reverse();
+            let fe_bits = fe.to_bits_le()?;
 
             // Remove the most significant bit, because we know it should be zero
             // because `values.to_field_elements()` only
@@ -113,19 +109,12 @@ impl<F: Field> UInt8<F> {
         }
 
         // Chunk up slices of 8 bit into bytes.
-        Ok(allocated_bits[0..8 * values_len]
+        Ok(allocated_bits[0..(8 * values_len)]
             .chunks(8)
             .map(Self::from_bits_le)
             .collect())
     }
 
-    /// Turns this `UInt8` into its little-endian byte order representation.
-    /// LSB-first means that we can easily get the corresponding field element
-    /// via double and add.
-    pub fn into_bits_le(&self) -> Vec<Boolean<F>> {
-        self.bits.to_vec()
-    }
-
     /// Converts a little-endian byte order representation of bits into a
     /// `UInt8`.
     pub fn from_bits_le(bits: &[Boolean<F>]) -> Self {
@@ -134,25 +123,10 @@ impl<F: Field> UInt8<F> {
         let bits = bits.to_vec();
 
         let mut value = Some(0u8);
-        for b in bits.iter().rev() {
-            value.as_mut().map(|v| *v <<= 1);
-
-            match *b {
-                Boolean::Constant(b) => {
-                    value.as_mut().map(|v| *v |= u8::from(b));
-                }
-                Boolean::Is(ref b) => match b.value() {
-                    Ok(b) => {
-                        value.as_mut().map(|v| *v |= u8::from(b));
-                    }
-                    Err(_) => value = None,
-                },
-                Boolean::Not(ref b) => match b.value() {
-                    Ok(b) => {
-                        value.as_mut().map(|v| *v |= u8::from(!b));
-                    }
-                    Err(_) => value = None,
-                },
+        for (i, b) in bits.iter().enumerate() {
+            value = match b.value().ok() {
+                Some(b) => value.map(|v| v + (u8::from(b) << i)),
+                None => None,
             }
         }
 
@@ -241,7 +215,7 @@ mod test {
         let cs = ConstraintSystem::<Fr>::new_ref();
         let byte_val = 0b01110001;
         let byte = UInt8::new_witness(cs.ns("alloc value"), || Ok(byte_val)).unwrap();
-        let bits = byte.into_bits_le();
+        let bits = byte.to_bits_le()?;
         for (i, bit) in bits.iter().enumerate() {
             assert_eq!(bit.value()?, (byte_val >> i) & 1 == 1)
         }
@@ -253,10 +227,17 @@ mod test {
         let cs = ConstraintSystem::<Fr>::new_ref();
         let byte_vals = (64u8..128u8).collect::<Vec<_>>();
         let bytes = UInt8::new_input_vec(cs.ns("alloc value"), &byte_vals).unwrap();
+        dbg!(bytes.value())?;
         for (native, variable) in byte_vals.into_iter().zip(bytes) {
-            let bits = variable.into_bits_le();
+            let bits = variable.to_bits_le()?;
             for (i, bit) in bits.iter().enumerate() {
-                assert_eq!(bit.value()?, (native >> i) & 1 == 1)
+                assert_eq!(
+                    bit.value()?,
+                    (native >> i) & 1 == 1,
+                    "native value {}: bit {:?}",
+                    native,
+                    i
+                )
             }
         }
         Ok(())
@@ -280,7 +261,7 @@ mod test {
                 }
             }
 
-            let expected_to_be_same = val.into_bits_le();
+            let expected_to_be_same = val.to_bits_le()?;
 
             for x in v.iter().zip(expected_to_be_same.iter()) {
                 match x {