Merge remote-tracking branch 'hdevalence/feature/refactor-scalar-api' into develop

Makefile

@@ -1,3 +1,7 @@
 
 doc:
 	cargo rustdoc --features "nightly yolocrypto" -- --html-in-header rustdoc-include-katex-header.html
+
+doc-internal:
+	cargo rustdoc --features "nightly yolocrypto" -- --html-in-header rustdoc-include-katex-header.html --no-defaults --passes "collapse-docs" --passes "unindent-comments"
+

src/constants.rs

@@ -64,28 +64,34 @@ pub const RISTRETTO_BASEPOINT_POINT: RistrettoPoint = RistrettoPoint(ED25519_BAS
 
 /// `BASEPOINT_ORDER` is the order of base point, i.e. `l = 2^252 +
 /// 27742317777372353535851937790883648493`, in little-endian bytes.
-pub const BASEPOINT_ORDER: Scalar = Scalar([
+pub const BASEPOINT_ORDER: Scalar = Scalar{
-    0xed, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58,
+    bytes: [ 
-    0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14,
+        0xed, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58,
-    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+        0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14,
-    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10,
+        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-]);
+        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10,
+    ],
+};
 
 /// `BASEPOINT_ORDER_MINUS_1` is the order of base point minus one, i.e. `l-1`, in little-endian bytes.
-pub const BASEPOINT_ORDER_MINUS_1: Scalar = Scalar([
+pub const BASEPOINT_ORDER_MINUS_1: Scalar = Scalar{
-    0xec, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58,
+    bytes: [
-    0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14,
+        0xec, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58,
-    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+        0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14,
-    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10,
+        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-]);
+        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10,
+    ],
+};
 
 /// `BASEPOINT_ORDER_MINUS_2` is the order of base point minus two, i.e. `l-2`, in little-endian bytes.
-pub const BASEPOINT_ORDER_MINUS_2: Scalar = Scalar([
+pub const BASEPOINT_ORDER_MINUS_2: Scalar = Scalar{
+    bytes: [
     0xeb, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58,
     0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14,
     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10,
-]);
+    ],
+};
 
 // Precomputed basepoint table is generated into a file by build.rs
 

src/edwards.rs

@@ -878,18 +878,24 @@ mod test {
                             0x72, 0xc3, 0x7f, 0x82, 0xf2, 0x96, 0x96, 0x70]);
 
     /// 4493907448824000747700850167940867464579944529806937181821189941592931634714
-    pub static A_SCALAR: Scalar = Scalar([
+    pub static A_SCALAR: Scalar = Scalar{
-        0x1a, 0x0e, 0x97, 0x8a, 0x90, 0xf6, 0x62, 0x2d,
+        bytes: [
-        0x37, 0x47, 0x02, 0x3f, 0x8a, 0xd8, 0x26, 0x4d,
+            0x1a, 0x0e, 0x97, 0x8a, 0x90, 0xf6, 0x62, 0x2d,
-        0xa7, 0x58, 0xaa, 0x1b, 0x88, 0xe0, 0x40, 0xd1,
+            0x37, 0x47, 0x02, 0x3f, 0x8a, 0xd8, 0x26, 0x4d,
-        0x58, 0x9e, 0x7b, 0x7f, 0x23, 0x76, 0xef, 0x09]);
+            0xa7, 0x58, 0xaa, 0x1b, 0x88, 0xe0, 0x40, 0xd1,
+            0x58, 0x9e, 0x7b, 0x7f, 0x23, 0x76, 0xef, 0x09,
+        ],
+    };
 
     /// 2506056684125797857694181776241676200180934651973138769173342316833279714961
-    pub static B_SCALAR: Scalar = Scalar([
+    pub static B_SCALAR: Scalar = Scalar{
-        0x91, 0x26, 0x7a, 0xcf, 0x25, 0xc2, 0x09, 0x1b,
+        bytes: [
-        0xa2, 0x17, 0x74, 0x7b, 0x66, 0xf0, 0xb3, 0x2e,
+            0x91, 0x26, 0x7a, 0xcf, 0x25, 0xc2, 0x09, 0x1b,
-        0x9d, 0xf2, 0xa5, 0x67, 0x41, 0xcf, 0xda, 0xc4,
+            0xa2, 0x17, 0x74, 0x7b, 0x66, 0xf0, 0xb3, 0x2e,
-        0x56, 0xa7, 0xd4, 0xaa, 0xb8, 0x60, 0x8a, 0x05]);
+            0x9d, 0xf2, 0xa5, 0x67, 0x41, 0xcf, 0xda, 0xc4,
+            0x56, 0xa7, 0xd4, 0xaa, 0xb8, 0x60, 0x8a, 0x05,
+        ],
+    };
 
     /// A_SCALAR * basepoint, computed with ed25519.py
     pub static A_TIMES_BASEPOINT: CompressedEdwardsY = CompressedEdwardsY([
@@ -1050,8 +1056,8 @@ mod test {
     #[test]
     #[cfg(feature="precomputed_tables")]
     fn basepoint_mult_two_vs_basepoint2() {
-        let mut two_bytes = [0u8; 32]; two_bytes[0] = 2;
+        let two = Scalar::from_u64(2);
-        let bp2 = &constants::ED25519_BASEPOINT_TABLE * &Scalar(two_bytes);
+        let bp2 = &constants::ED25519_BASEPOINT_TABLE * &two;
         assert_eq!(bp2.compress(), BASE2_CMPRSSD);
     }
 

src/montgomery.rs

@@ -549,20 +549,6 @@ mod test {
 
         assert_eq!(result.compress(), expected.to_montgomery().compress());
     }
-
-    #[test]
-    #[should_panic(expected = "assertion failed: self[31] <= 127")]
-    #[cfg(feature="precomputed_tables")]
-    fn ladder_matches_scalarmult_with_scalar_high_bit_set() {
-        let mut s: Scalar = Scalar::one();
-
-        s[31] = 255;
-
-        let result: MontgomeryPoint = &BASE_COMPRESSED_MONTGOMERY.decompress() * &s;
-        let expected: ExtendedPoint = &constants::ED25519_BASEPOINT_TABLE * &s;
-
-        assert_eq!(result.compress(), expected.to_montgomery().compress())
-    }
 }
 
 #[cfg(all(test, feature = "bench"))]

src/scalar.rs

@@ -12,28 +12,34 @@
 
 //! Arithmetic for scalar multiplication.
 //!
-//! The Ed25519 basepoint P has prime order
+//! Both the Ristretto group and the Ed25519 basepoint have prime order
+//! \\( \ell = 2\^{252} + 27742317777372353535851937790883648493 \\).
 //!
-//! l = 2^252 + 27742317777372353535851937790883648493.
+//! The `Scalar` struct holds an integer \\(s < 2\^{255} \\) which
+//! represents an element of \\(\mathbb Z / \ell\\).
 //!
-//! Thus a multiple `aP` of the basepoint (with a ∈ ℤ) depends only
+//! The code is intended to be useful with both the Ristretto group
-//! on the value of `a (mod l)`, or equivalently, the image of `a` in
+//! (where everything is done modulo \\( \ell \\), and the X/Ed25519
-//! the quotient ℤ/lℤ.
+//! setting, which mandates specific bit-twiddles that are not
+//! well-defined modulo \\( \ell \\).
 //!
-//! The `Scalar` struct represents an element in ℤ/lℤ.
+//! To create a `Scalar` from a supposedly canonical encoding, use
+//! `Scalar::from_canonical_bytes`.
 //!
-//! In contrast to `FieldElement`s, `Scalar`s are stored in
+//! To create a `Scalar` by reducing a 256-bit integer mod \\( \ell \\),
-//! memory as bytes, allowing easy access to the bits of the `Scalar`
+//! use `Scalar::from_bytes_mod_order`.
-//! when multiplying a point by a scalar.  For efficient arithmetic
+//!
-//! between two scalars, the `UnpackedScalar` struct (internally
+//! To create a `Scalar` with a specific bit-pattern (e.g., for
-//! either `Scalar32` or `Scalar64`) is stored as limbs.
+//! compatibility with X25519 "clamping"), use `Scalar::from_bits`.
+//!
+//! All arithmetic on `Scalars` is done modulo \\( \ell \\).
 
 use core::fmt::Debug;
 use core::ops::Neg;
 use core::ops::{Add, AddAssign};
 use core::ops::{Sub, SubAssign};
 use core::ops::{Mul, MulAssign};
-use core::ops::{Index, IndexMut};
+use core::ops::{Index};
 use core::cmp::{Eq, PartialEq};
 
 #[cfg(feature = "std")]
@@ -47,12 +53,19 @@ use subtle::ConditionallyAssignable;
 use subtle::Equal;
 
 use backend;
+use constants;
 
 /// An `UnpackedScalar` represents an element of the field GF(l), optimized for speed.
+/// 
+/// This is a type alias for one of the scalar types in the `backend`
+/// module.
 #[cfg(feature="radix_51")]
 type UnpackedScalar = backend::u64::scalar::Scalar64;
 
 /// An `UnpackedScalar` represents an element of the field GF(l), optimized for speed.
+/// 
+/// This is a type alias for one of the scalar types in the `backend`
+/// module.
 #[cfg(not(feature="radix_51"))]
 type UnpackedScalar = backend::u32::scalar::Scalar32;
 
@@ -63,11 +76,66 @@ type UnpackedScalar = backend::u32::scalar::Scalar32;
 ///
 /// is the order of the basepoint.  The `Scalar` is stored as bytes.
 #[derive(Copy, Clone)]
-pub struct Scalar(pub [u8; 32]);
+pub struct Scalar {
+    /// `bytes` is a little-endian byte encoding of an integer representing a scalar modulo the group order.
+    /// 
+    /// # Invariant
+    /// 
+    /// The integer representing this scalar must be bounded above by 2^255, or equivalently the high bit of `bytes[31]` must be zero.
+    /// 
+    // XXX This is pub(crate) so we can write literal constants.  If const fns were stable, we could make the Scalar constructors const fns and use those instead.
+    pub(crate) bytes: [u8; 32],
+}
+
+impl Scalar {
+    /// Construct a `Scalar` by reducing a 256-bit integer modulo the group order.
+    pub fn from_bytes_mod_order(bytes: [u8;32]) -> Scalar {
+        // Temporarily allow s_unreduced.bytes > 2^255 ...
+        let s_unreduced = Scalar{bytes: bytes};
+
+        // Then reduce mod the group order and return the reduced representative.
+        let s = s_unreduced.reduce();
+        debug_assert_eq!(0u8, s[31] >> 7);
+
+        s
+    }
+
+    /// Attempt to construct a `Scalar` from a canonical byte representation.
+    ///
+    /// # Return
+    ///
+    /// - `Some(s)`, where `s` is the `Scalar` corresponding to `bytes`,
+    ///   if `bytes` is a canonical byte representation;
+    /// - `None` if `bytes` is not a canonical byte representation.
+    pub fn from_canonical_bytes(bytes: [u8; 32]) -> Option<Scalar> {
+        // Check that the high bit is not set
+        if (bytes[31] >> 7) != 0u8 { return None; }
+        let candidate = Scalar::from_bits(bytes);
+
+        if candidate.is_canonical() {
+            Some(candidate)
+        } else {
+            None
+        }
+    }
+
+    /// Construct a `Scalar` from the low 255 bits of a 256-bit integer.
+    /// 
+    /// This function is intended for applications like X25519 which
+    /// require specific bit-patterns when performing scalar
+    /// multiplication.
+    pub fn from_bits(bytes: [u8; 32]) -> Scalar {
+        let mut s = Scalar{bytes: bytes};
+        // Ensure that s < 2^255 by masking the high bit
+        s.bytes[31] &= 0b0111_1111;
+
+        s
+    }
+}
 
 impl Debug for Scalar {
     fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
-        write!(f, "Scalar({:?})", &self.0[..])
+        write!(f, "Scalar{{\n\tbytes: {:?},\n}}", &self.bytes)
     }
 }
 
@@ -84,7 +152,7 @@ impl PartialEq for Scalar {
     ///
     /// True if they are equal, and false otherwise.
     fn eq(&self, other: &Self) -> bool {
-        slices_equal(&self.0, &other.0) == 1u8
+        slices_equal(&self.bytes, &other.bytes) == 1u8
     }
 }
 
@@ -95,67 +163,62 @@ impl Equal for Scalar {
     ///
     /// `1u8` if they are equal, and `0u8` otherwise.
     fn ct_eq(&self, other: &Self) -> u8 {
-        slices_equal(&self.0, &other.0)
+        slices_equal(&self.bytes, &other.bytes)
     }
 }
 
 impl Index<usize> for Scalar {
     type Output = u8;
 
+    /// Index the bytes of the representative for this `Scalar`.  Mutation is not permitted.
     fn index(&self, _index: usize) -> &u8 {
-        &(self.0[_index])
+        &(self.bytes[_index])
-    }
-}
-
-impl IndexMut<usize> for Scalar {
-    fn index_mut(&mut self, _index: usize) -> &mut u8 {
-        &mut (self.0[_index])
     }
 }
 
 impl<'b> MulAssign<&'b Scalar> for Scalar {
     fn mul_assign(&mut self, _rhs: &'b Scalar) {
-        *self = Scalar::mul(self, _rhs)
+        *self = UnpackedScalar::mul(&self.unpack(), &_rhs.unpack()).pack();
     }
 }
 
 impl<'a, 'b> Mul<&'b Scalar> for &'a Scalar {
     type Output = Scalar;
     fn mul(self, _rhs: &'b Scalar) -> Scalar {
-        Scalar::mul(self, _rhs)
+        UnpackedScalar::mul(&self.unpack(), &_rhs.unpack()).pack()
     }
 }
 
 impl<'b> AddAssign<&'b Scalar> for Scalar {
     fn add_assign(&mut self, _rhs: &'b Scalar) {
-        *self = Scalar::add(self, _rhs);
+        *self = UnpackedScalar::add(&self.unpack(), &_rhs.unpack()).pack();
     }
 }
 
 impl<'a, 'b> Add<&'b Scalar> for &'a Scalar {
     type Output = Scalar;
     fn add(self, _rhs: &'b Scalar) -> Scalar {
-        Scalar::add(self, _rhs)
+        UnpackedScalar::add(&self.unpack(), &_rhs.unpack()).pack()
     }
 }
 
 impl<'b> SubAssign<&'b Scalar> for Scalar {
     fn sub_assign(&mut self, _rhs: &'b Scalar) {
-        *self = Scalar::sub(self, _rhs);
+        *self = UnpackedScalar::sub(&self.unpack(), &_rhs.unpack()).pack();
     }
 }
 
 impl<'a, 'b> Sub<&'b Scalar> for &'a Scalar {
     type Output = Scalar;
     fn sub(self, _rhs: &'b Scalar) -> Scalar {
-        Scalar::sub(self, _rhs)
+        UnpackedScalar::sub(&self.unpack(), &_rhs.unpack()).pack()
     }
 }
 
 impl<'a> Neg for &'a Scalar {
     type Output = Scalar;
     fn neg(self) -> Scalar {
-        Scalar::sub(&Scalar::zero(), self)
+        &Scalar::zero() - self
     }
 }
 
@@ -168,10 +231,8 @@ impl ConditionallyAssignable for Scalar {
     /// # use curve25519_dalek::scalar::Scalar;
     /// # use subtle::ConditionallyAssignable;
     /// # fn main() {
-    /// let a = Scalar([0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    /// let a = Scalar::from_bits([0u8;32]);
-    ///                 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]);
+    /// let b = Scalar::from_bits([1u8;32]);
-    /// let b = Scalar([1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
-    ///                 1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1]);
     /// let mut t = a;
     /// t.conditional_assign(&b, 0u8);
     /// assert!(t[0] == a[0]);
@@ -189,7 +250,7 @@ impl ConditionallyAssignable for Scalar {
         // if choice = 1u8, mask = (-1i8) as u8 = 11111111
         let mask = -(choice as i8) as u8;
         for i in 0..32 {
-            self[i] ^= mask & (self[i] ^ other[i]);
+            self.bytes[i] ^= mask & (self.bytes[i] ^ other.bytes[i]);
         }
     }
 }
@@ -204,7 +265,7 @@ impl Serialize for Scalar {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
         where S: Serializer
     {
-        serializer.serialize_bytes(self.as_bytes())
+        serializer.serialize_bytes(self.reduce().as_bytes())
     }
 }
 
@@ -219,17 +280,25 @@ impl<'de> Deserialize<'de> for Scalar {
             type Value = Scalar;
 
             fn expecting(&self, formatter: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
-                formatter.write_str("a 32-byte scalar value")
+                formatter.write_str("a canonically-encoded 32-byte scalar value")
             }
 
             fn visit_bytes<E>(self, v: &[u8]) -> Result<Scalar, E>
                 where E: serde::de::Error
             {
                 if v.len() == 32 {
-                    // array_ref turns &[u8] into &[u8;32]
                     let mut bytes = [0u8;32];
                     bytes.copy_from_slice(v);
-                    Ok(Scalar(bytes))
+
+                    static ERRMSG: &'static str = "encoding was not canonical";
+
+                    Scalar::from_canonical_bytes(bytes)
+                        .ok_or(
+                            serde::de::Error::invalid_value(
+                                serde::de::Unexpected::Bytes(v),
+                                &ERRMSG,
+                            )
+                        )
                 } else {
                     Err(serde::de::Error::invalid_length(v.len(), &self))
                 }
@@ -254,7 +323,7 @@ impl Scalar {
     pub fn random<T: Rng>(rng: &mut T) -> Self {
         let mut scalar_bytes = [0u8; 64];
         rng.fill_bytes(&mut scalar_bytes);
-        Scalar::reduce(&scalar_bytes)
+        Scalar::reduce_wide(&scalar_bytes)
     }
 
     /// Hash a slice of bytes into a scalar.
@@ -299,7 +368,7 @@ impl Scalar {
         // XXX this seems clumsy
         let mut output = [0u8; 64];
         output.copy_from_slice(hash.result().as_slice());
-        Scalar::reduce(&output)
+        Scalar::reduce_wide(&output)
     }
 
     /// Convert this `Scalar` to its underlying sequence of bytes.
@@ -309,27 +378,31 @@ impl Scalar {
 
     /// View this `Scalar` as a sequence of bytes.
     pub fn as_bytes(&self) -> &[u8; 32] {
-        &self.0
+        &self.bytes
     }
 
     /// Construct the additive identity
     pub fn zero() -> Self {
-        Scalar([0u8; 32])
+        Scalar { bytes: [0u8; 32]}
     }
 
     /// Construct the multiplicative identity
     pub fn one() -> Self {
-        Scalar([ 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+        Scalar {
-                 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 ])
+            bytes: [
+                1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+            ],
+        }
     }
 
     /// Construct a scalar from the given `u64`.
     pub fn from_u64(x: u64) -> Scalar {
-        let mut s = Scalar::zero();
+        let mut s_bytes = [0u8; 32];
         for i in 0..8 {
-            s[i] = (x >> (i*8)) as u8;
+            s_bytes[i] = (x >> (i*8)) as u8;
         }
-        s
+        Scalar{ bytes: s_bytes }
     }
 
     /// Compute the multiplicative inverse of this scalar.
@@ -338,12 +411,12 @@ impl Scalar {
     }
 
     /// Get the bits of the scalar.
-    pub fn bits(&self) -> [i8; 256] {
+    pub(crate) fn bits(&self) -> [i8; 256] {
         let mut bits = [0i8; 256];
         for i in 0..256 {
             // As i runs from 0..256, the bottom 3 bits index the bit,
             // while the upper bits index the byte.
-            bits[i] = ((self.0[i>>3] >> (i&7)) & 1u8) as i8;
+            bits[i] = ((self.bytes[i>>3] >> (i&7)) & 1u8) as i8;
         }
         bits
     }
@@ -359,7 +432,7 @@ impl Scalar {
     /// Intuitively, this is like a binary expansion, except that we
     /// allow some coefficients to grow up to `2^(w-1)` so that the
     /// nonzero coefficients are as sparse as possible.
-    pub fn non_adjacent_form(&self) -> [i8; 256] {
+    pub(crate) fn non_adjacent_form(&self) -> [i8; 256] {
         // Step 1: write out bits of the scalar
         let mut naf = self.bits();
 
@@ -404,7 +477,7 @@ impl Scalar {
     ///
     /// Precondition: self[31] <= 127.  This is the case whenever
     /// `self` is reduced.
-    pub fn to_radix_16(&self) -> [i8; 64] {
+    pub(crate) fn to_radix_16(&self) -> [i8; 64] {
         debug_assert!(self[31] <= 127);
         let mut output = [0i8; 64];
 
@@ -435,22 +508,7 @@ impl Scalar {
 
     /// Unpack this `Scalar` to an `UnpackedScalar`
     pub(crate) fn unpack(&self) -> UnpackedScalar {
-        UnpackedScalar::from_bytes(&self.0)
+        UnpackedScalar::from_bytes(&self.bytes)
-    }
-
-    /// Compute `a + b` (mod l)
-    pub fn add(a: &Scalar, b: &Scalar) -> Scalar {
-        UnpackedScalar::add(&a.unpack(), &b.unpack()).pack()
-    }
-
-    /// Compute `a - b` (mod l).
-    pub fn sub(a: &Scalar, b: &Scalar) -> Scalar {
-        UnpackedScalar::sub(&a.unpack(), &b.unpack()).pack()
-    }
-
-    /// Compute `a * b` (mod l).
-    pub fn mul(a: &Scalar, b: &Scalar) -> Scalar {
-        UnpackedScalar::mul(&a.unpack(), &b.unpack()).pack()
     }
 
     /// Compute `(a * b) + c` (mod l).
@@ -458,8 +516,38 @@ impl Scalar {
         UnpackedScalar::add(&UnpackedScalar::mul(&a.unpack(), &b.unpack()), &c.unpack()).pack()
     }
 
+    /// Reduce this `Scalar` mod l.
+    pub fn reduce(&self) -> Scalar {
+        let x = self.unpack();
+        let xR = UnpackedScalar::mul_internal(&x, &constants::R);
+        let x_mod_l = UnpackedScalar::montgomery_reduce(&xR);
+        x_mod_l.pack()
+    }
+
+    /// Check whether this `Scalar` is the canonical representative mod \\(\ell\\). 
+    ///
+    /// This is intended for uses like input validation, where variable-time code is acceptable.
+    ///
+    /// ```
+    /// # extern crate curve25519_dalek;
+    /// # extern crate subtle;
+    /// # use curve25519_dalek::scalar::Scalar;
+    /// # use subtle::ConditionallyAssignable;
+    /// # fn main() {
+    /// // 2^255 - 1, since `from_bits` clears the high bit
+    /// let _2_255_minus_1 = Scalar::from_bits([0xff;32]);
+    /// assert!(!_2_255_minus_1.is_canonical());
+    ///
+    /// let reduced = _2_255_minus_1.reduce();
+    /// assert!(reduced.is_canonical());
+    /// # }
+    /// ```
+    pub fn is_canonical(&self) -> bool {
+        *self == self.reduce()
+    }
+
     /// Reduce a 512-bit little endian number mod l
-    pub fn reduce(input: &[u8; 64]) -> Scalar {
+    pub fn reduce_wide(input: &[u8; 64]) -> Scalar {
         UnpackedScalar::from_bytes_wide(input).pack()
     }
 }
@@ -467,7 +555,7 @@ impl Scalar {
 impl UnpackedScalar {
     /// Pack the limbs of this `UnpackedScalar` into a `Scalar`.
     fn pack(&self) -> Scalar {
-        Scalar(self.to_bytes())
+        Scalar{ bytes: self.to_bytes() }
     }
 
     /// Compute the multiplicative inverse of this scalar.
@@ -535,48 +623,81 @@ mod test {
     use constants;
 
     /// x = 2238329342913194256032495932344128051776374960164957527413114840482143558222
-    pub static X: Scalar = Scalar(
+    pub static X: Scalar = Scalar{
-        [0x4e, 0x5a, 0xb4, 0x34, 0x5d, 0x47, 0x08, 0x84,
+        bytes: [
-         0x59, 0x13, 0xb4, 0x64, 0x1b, 0xc2, 0x7d, 0x52,
+            0x4e, 0x5a, 0xb4, 0x34, 0x5d, 0x47, 0x08, 0x84,
-         0x52, 0xa5, 0x85, 0x10, 0x1b, 0xcc, 0x42, 0x44,
+            0x59, 0x13, 0xb4, 0x64, 0x1b, 0xc2, 0x7d, 0x52,
-         0xd4, 0x49, 0xf4, 0xa8, 0x79, 0xd9, 0xf2, 0x04]);
+            0x52, 0xa5, 0x85, 0x10, 0x1b, 0xcc, 0x42, 0x44,
+            0xd4, 0x49, 0xf4, 0xa8, 0x79, 0xd9, 0xf2, 0x04,
+        ],
+    };
     /// 1/x = 6859937278830797291664592131120606308688036382723378951768035303146619657244
-    pub static XINV: Scalar = Scalar(
+    pub static XINV: Scalar = Scalar{
-        [0x1c, 0xdc, 0x17, 0xfc, 0xe0, 0xe9, 0xa5, 0xbb,
+        bytes: [
-         0xd9, 0x24, 0x7e, 0x56, 0xbb, 0x01, 0x63, 0x47,
+            0x1c, 0xdc, 0x17, 0xfc, 0xe0, 0xe9, 0xa5, 0xbb,
-         0xbb, 0xba, 0x31, 0xed, 0xd5, 0xa9, 0xbb, 0x96,
+            0xd9, 0x24, 0x7e, 0x56, 0xbb, 0x01, 0x63, 0x47,
-         0xd5, 0x0b, 0xcd, 0x7a, 0x3f, 0x96, 0x2a, 0x0f]);
+            0xbb, 0xba, 0x31, 0xed, 0xd5, 0xa9, 0xbb, 0x96,
+            0xd5, 0x0b, 0xcd, 0x7a, 0x3f, 0x96, 0x2a, 0x0f,
+        ],
+    };
     /// y = 2592331292931086675770238855846338635550719849568364935475441891787804997264
-    pub static Y: Scalar = Scalar(
+    pub static Y: Scalar = Scalar{
-        [0x90, 0x76, 0x33, 0xfe, 0x1c, 0x4b, 0x66, 0xa4,
+        bytes: [
-         0xa2, 0x8d, 0x2d, 0xd7, 0x67, 0x83, 0x86, 0xc3,
+            0x90, 0x76, 0x33, 0xfe, 0x1c, 0x4b, 0x66, 0xa4,
-         0x53, 0xd0, 0xde, 0x54, 0x55, 0xd4, 0xfc, 0x9d,
+            0xa2, 0x8d, 0x2d, 0xd7, 0x67, 0x83, 0x86, 0xc3,
-         0xe8, 0xef, 0x7a, 0xc3, 0x1f, 0x35, 0xbb, 0x05]);
+            0x53, 0xd0, 0xde, 0x54, 0x55, 0xd4, 0xfc, 0x9d,
+            0xe8, 0xef, 0x7a, 0xc3, 0x1f, 0x35, 0xbb, 0x05,
+        ],
+    };
     /// z = 5033871415930814945849241457262266927579821285980625165479289807629491019013
-    pub static Z: Scalar = Scalar(
+    pub static Z: Scalar = Scalar{
-        [0x05, 0x9d, 0x3e, 0x0b, 0x09, 0x26, 0x50, 0x3d,
+        bytes: [
-         0xa3, 0x84, 0xa1, 0x3c, 0x92, 0x7a, 0xc2, 0x06,
+            0x05, 0x9d, 0x3e, 0x0b, 0x09, 0x26, 0x50, 0x3d,
-         0x41, 0x98, 0xcf, 0x34, 0x3a, 0x24, 0xd5, 0xb7,
+            0xa3, 0x84, 0xa1, 0x3c, 0x92, 0x7a, 0xc2, 0x06,
-         0xeb, 0x33, 0x6a, 0x2d, 0xfc, 0x11, 0x21, 0x0b]);
+            0x41, 0x98, 0xcf, 0x34, 0x3a, 0x24, 0xd5, 0xb7,
+            0xeb, 0x33, 0x6a, 0x2d, 0xfc, 0x11, 0x21, 0x0b,
+        ],
+    };
     /// w = 3486911242272497535104403593250518247409663771668155364040899665266216860804
-    static W: Scalar = Scalar(
+    static W: Scalar = Scalar{
-        [0x84, 0xfc, 0xbc, 0x4f, 0x78, 0x12, 0xa0, 0x06,
+        bytes: [
-         0xd7, 0x91, 0xd9, 0x7a, 0x3a, 0x27, 0xdd, 0x1e,
+            0x84, 0xfc, 0xbc, 0x4f, 0x78, 0x12, 0xa0, 0x06,
-         0x21, 0x43, 0x45, 0xf7, 0xb1, 0xb9, 0x56, 0x7a,
+            0xd7, 0x91, 0xd9, 0x7a, 0x3a, 0x27, 0xdd, 0x1e,
-         0x81, 0x30, 0x73, 0x44, 0x96, 0x85, 0xb5, 0x07]);
+            0x21, 0x43, 0x45, 0xf7, 0xb1, 0xb9, 0x56, 0x7a,
+            0x81, 0x30, 0x73, 0x44, 0x96, 0x85, 0xb5, 0x07,
+        ],
+    };
 
     /// x*y = 5690045403673944803228348699031245560686958845067437804563560795922180092780
-    static X_TIMES_Y: Scalar = Scalar(
+    static X_TIMES_Y: Scalar = Scalar{
-        [0x6c, 0x33, 0x74, 0xa1, 0x89, 0x4f, 0x62, 0x21,
+        bytes: [
-         0x0a, 0xaa, 0x2f, 0xe1, 0x86, 0xa6, 0xf9, 0x2c,
+            0x6c, 0x33, 0x74, 0xa1, 0x89, 0x4f, 0x62, 0x21,
-         0xe0, 0xaa, 0x75, 0xc2, 0x77, 0x95, 0x81, 0xc2,
+            0x0a, 0xaa, 0x2f, 0xe1, 0x86, 0xa6, 0xf9, 0x2c,
-         0x95, 0xfc, 0x08, 0x17, 0x9a, 0x73, 0x94, 0x0c]);
+            0xe0, 0xaa, 0x75, 0xc2, 0x77, 0x95, 0x81, 0xc2,
+            0x95, 0xfc, 0x08, 0x17, 0x9a, 0x73, 0x94, 0x0c,
+        ],
+    };
 
-    static A_SCALAR: Scalar = Scalar([
+    /// sage: l = 2^252 + 27742317777372353535851937790883648493
-        0x1a, 0x0e, 0x97, 0x8a, 0x90, 0xf6, 0x62, 0x2d,
+    /// sage: big = 2^256 - 1
-        0x37, 0x47, 0x02, 0x3f, 0x8a, 0xd8, 0x26, 0x4d,
+    /// sage: repr((big % l).digits(256))
-        0xa7, 0x58, 0xaa, 0x1b, 0x88, 0xe0, 0x40, 0xd1,
+    static CANONICAL_2_256_MINUS_1: Scalar = Scalar{
-        0x58, 0x9e, 0x7b, 0x7f, 0x23, 0x76, 0xef, 0x09]);
+        bytes: [
+              28, 149, 152, 141, 116,  49, 236, 214,
+             112, 207, 125, 115, 244,  91, 239, 198,
+             254, 255, 255, 255, 255, 255, 255, 255,
+             255, 255, 255, 255, 255, 255, 255,  15,
+        ],
+    };
+
+    static A_SCALAR: Scalar = Scalar{
+        bytes: [
+            0x1a, 0x0e, 0x97, 0x8a, 0x90, 0xf6, 0x62, 0x2d,
+            0x37, 0x47, 0x02, 0x3f, 0x8a, 0xd8, 0x26, 0x4d,
+            0xa7, 0x58, 0xaa, 0x1b, 0x88, 0xe0, 0x40, 0xd1,
+            0x58, 0x9e, 0x7b, 0x7f, 0x23, 0x76, 0xef, 0x09,
+        ],
+    };
 
     static A_NAF: [i8; 256] =
         [0,13,0,0,0,0,0,0,0,7,0,0,0,0,0,0,-9,0,0,0,0,-11,0,0,0,0,3,0,0,0,0,1,
@@ -597,19 +718,19 @@ mod test {
         // LE bytes of 6432735165214683820902750800207468552549813371247423777071615116673864412038
         let c_bytes = [134, 171, 119, 216, 180, 128, 178, 62, 171, 132, 32, 62, 34, 119, 104, 193, 47, 215, 181, 250, 14, 207, 172, 93, 75, 207, 211, 103, 144, 204, 56, 14];
 
-        let a = Scalar(a_bytes);
+        let a = Scalar::from_bytes_mod_order(a_bytes);
-        let b = Scalar(b_bytes);
+        let b = Scalar::from_bytes_mod_order(b_bytes);
-        let c = Scalar(c_bytes);
+        let c = Scalar::from_bytes_mod_order(c_bytes);
 
         let mut tmp = [0u8; 64];
 
         // also_a = (a mod l)
         tmp[0..32].copy_from_slice(&a_bytes[..]);
-        let also_a = Scalar::reduce(&tmp);
+        let also_a = Scalar::reduce_wide(&tmp);
 
         // also_b = (b mod l)
         tmp[0..32].copy_from_slice(&b_bytes[..]);
-        let also_b = Scalar::reduce(&tmp);
+        let also_b = Scalar::reduce_wide(&tmp);
 
         let expected_c = &a * &b;
         let also_expected_c = &also_a * &also_b;
@@ -652,8 +773,7 @@ mod test {
 
     #[test]
     fn impl_add() {
-        let mut two = Scalar::zero(); two[0] = 2;
+        let two = Scalar::from_u64(2);
-        let two = two;
         let one = Scalar::one();
         let should_be_two = &one + &one;
         assert_eq!(should_be_two, two);
@@ -690,7 +810,13 @@ mod test {
     }
 
     #[test]
-    fn scalar_reduce() {
+    fn reduce() {
+        let biggest = Scalar::from_bytes_mod_order([0xff; 32]);
+        assert_eq!(biggest, CANONICAL_2_256_MINUS_1);
+    }
+
+    #[test]
+    fn reduce_wide() {
         let mut bignum = [0u8; 64];
         // set bignum = x + 2^256x
         for i in 0..32 {
@@ -699,11 +825,15 @@ mod test {
         }
         // 3958878930004874126169954872055634648693766179881526445624823978500314864344
         // = x + 2^256x (mod l)
-        let reduced = Scalar([216, 154, 179, 139, 210, 121,   2,  71,
+        let reduced = Scalar{
-                               69,  99, 158, 216,  23, 173,  63, 100,
+            bytes: [
-                              204,   0,  91,  50, 219, 153,  57, 249,
+                216, 154, 179, 139, 210, 121,   2,  71,
-                               28,  82,  31, 197, 100, 165, 192,   8]);
+                 69,  99, 158, 216,  23, 173,  63, 100,
-        let test_red = Scalar::reduce(&bignum);
+                204,   0,  91,  50, 219, 153,  57, 249,
+                 28,  82,  31, 197, 100, 165, 192,   8,
+            ],
+        };
+        let test_red = Scalar::reduce_wide(&bignum);
         for i in 0..32 {
             assert!(test_red[i] == reduced[i]);
         }
@@ -738,7 +868,7 @@ mod test {
     }
 
     #[test]
-    fn montgomery_reduce_matches_reduce() {
+    fn montgomery_reduce_matches_reduce_wide() {
         let mut bignum = [0u8; 64];
 
         // set bignum = x + 2^256x
@@ -748,14 +878,18 @@ mod test {
         }
         // x + 2^256x (mod l)
         //         = 3958878930004874126169954872055634648693766179881526445624823978500314864344
-        let expected = Scalar([216, 154, 179, 139, 210, 121,   2,  71,
+        let expected = Scalar{
-                                69,  99, 158, 216,  23, 173,  63, 100,
+            bytes: [
-                               204,   0,  91,  50, 219, 153,  57, 249,
+                216, 154, 179, 139, 210, 121,   2,  71,
-                                28,  82,  31, 197, 100, 165, 192,   8]);
+                 69,  99, 158, 216,  23, 173,  63, 100,
-        let reduced = Scalar::reduce(&bignum);
+                204,   0,  91,  50, 219, 153,  57, 249,
+                 28,  82,  31, 197, 100, 165, 192,   8
+            ],
+        };
+        let reduced = Scalar::reduce_wide(&bignum);
 
         // The reduced scalar should match the expected
-        assert_eq!(reduced.0, expected.0);
+        assert_eq!(reduced.bytes, expected.bytes);
 
         //  (x + 2^256x) * R
         let interim = UnpackedScalar::mul_internal(&UnpackedScalar::from_bytes_wide(&bignum),
@@ -768,12 +902,30 @@ mod test {
         assert_eq!(montgomery_reduced.0, expected.unpack().0)
     }
 
-    #[cfg(feature = "serde")]
+    #[test]
-    use serde_cbor;
+    fn canonical_decoding() {
+        // canonical encoding of 1667457891
+        let canonical_bytes = [99, 99, 99, 99, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,];
+
+        // encoding of
+        //   7265385991361016183439748078976496179028704920197054998554201349516117938192
+        // = 28380414028753969466561515933501938171588560817147392552250411230663687203 (mod l)
+        // non_canonical because unreduced mod l
+        let non_canonical_bytes_because_unreduced = [16; 32];
+
+        // encoding with high bit set, to check that the parser isn't pre-masking the high bit
+        let non_canonical_bytes_because_highbit = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 128];
+
+        assert!( Scalar::from_canonical_bytes(canonical_bytes).is_some() );
+        assert!( Scalar::from_canonical_bytes(non_canonical_bytes_because_unreduced).is_none() );
+        assert!( Scalar::from_canonical_bytes(non_canonical_bytes_because_highbit).is_none() );
+    }
 
     #[test]
     #[cfg(feature = "serde")]
     fn serde_cbor_scalar_roundtrip() {
+        // XXX remove serde_cbor
+        use serde_cbor;
         let output = serde_cbor::to_vec(&X).unwrap();
         let parsed: Scalar = serde_cbor::from_slice(&output).unwrap();
         assert_eq!(parsed, X);
@@ -788,6 +940,13 @@ mod bench {
     use super::*;
     use super::test::{X};
 
+    #[bench]
+    fn reduce(b: &mut Bencher) {
+        let unreduced = Scalar::from_bits([0xff; 32]);
+
+        b.iter(|| unreduced.reduce());
+    }
+
     #[bench]
     fn scalar_random(b: &mut Bencher) {
         let mut csprng: OsRng = OsRng::new().unwrap();