From 1e74cb3e56c1b2dba7d72b5bf8ed69c094417e0b Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Tue, 17 Jul 2018 10:53:02 -0700 Subject: [PATCH] Replace Scalar::from_u64 with From impls Unfortunately, Rust selects `i32` as the type for an integer literal when the literal has no other type constraints. This means that someone cannot write `Scalar::from(1)`, as Rust will choose `i32` as the type for `1`, and we don't `impl From for Scalar`. We could implement `From` conversions for signed integers, but since `Scalar` operations should be constant-time by default, this would require us to extract the sign bit of the integer and use it to conditionally select between the positive and negative of Scalar constructed from the value bits. This is more expensive than the unsigned operation, and I don't think it's what anyone really wants. Making API consumers specify that their literals are unsigned is slightly annoying, but better than the above alternative. It would also be nice to change `Scalar::from_hash` to be `impl> From for Scalar`, but this isn't currently allowed by Rust (since that `impl` "could" conflict with the `impl From` if someone decided that `u8` should `impl Digest`). --- Cargo.toml | 4 +- benches/dalek_benchmarks.rs | 12 ++--- src/backend/avx2/edwards.rs | 4 +- src/edwards.rs | 8 +-- src/ristretto.rs | 10 ++-- src/scalar.rs | 103 ++++++++++++++++++++++++------------ src/traits.rs | 12 ++--- 7 files changed, 94 insertions(+), 59 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 052b9cc..32d5783 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,7 +42,7 @@ harness = false [dependencies] rand = { version = "0.5", default-features = false } -byteorder = { version = "1", default-features = false } +byteorder = { version = "1", default-features = false, features = ["i128"] } digest = "0.7" generic-array = "0.9" clear_on_drop = "=0.2.3" @@ -51,7 +51,7 @@ serde = { version = "1.0", optional = true } [build-dependencies] rand = { version = "0.5", default-features = false } -byteorder = { version = "1", default-features = false } +byteorder = { version = "1", default-features = false, features = ["i128"] } digest = "0.7" generic-array = "0.9" clear_on_drop = "=0.2.3" diff --git a/benches/dalek_benchmarks.rs b/benches/dalek_benchmarks.rs index 75017cf..71792a3 100644 --- a/benches/dalek_benchmarks.rs +++ b/benches/dalek_benchmarks.rs @@ -39,7 +39,7 @@ mod edwards_benches { fn consttime_fixed_base_scalar_mul(c: &mut Criterion) { let B = &constants::ED25519_BASEPOINT_TABLE; - let s = Scalar::from_u64(897987897).invert(); + let s = Scalar::from(897987897u64).invert(); c.bench_function("Constant-time fixed-base scalar mul", move |b| { b.iter(|| B * &s) }); @@ -47,7 +47,7 @@ mod edwards_benches { fn consttime_variable_base_scalar_mul(c: &mut Criterion) { let B = &constants::ED25519_BASEPOINT_POINT; - let s = Scalar::from_u64(897987897).invert(); + let s = Scalar::from(897987897u64).invert(); c.bench_function("Constant-time variable-base scalar mul", move |b| { b.iter(|| B * &s) }); @@ -56,8 +56,8 @@ mod edwards_benches { fn vartime_double_base_scalar_mul(c: &mut Criterion) { c.bench_function("Variable-time aA+bB, A variable, B fixed", |bench| { let B = &constants::ED25519_BASEPOINT_POINT; - let a = Scalar::from_u64(298374928).invert(); - let b = Scalar::from_u64(897987897).invert(); + let a = Scalar::from(298374928u64).invert(); + let b = Scalar::from(897987897u64).invert(); let A = B * (b * a); bench.iter(|| EdwardsPoint::vartime_double_scalar_mul_basepoint(&a, &A, &b)); }); @@ -157,7 +157,7 @@ mod montgomery_benches { fn montgomery_ladder(c: &mut Criterion) { c.bench_function("Montgomery pseudomultiplication", |b| { let B = constants::X25519_BASEPOINT; - let s = Scalar::from_u64(897987897).invert(); + let s = Scalar::from(897987897u64).invert(); b.iter(|| B * s); }); } @@ -174,7 +174,7 @@ mod scalar_benches { fn scalar_inversion(c: &mut Criterion) { c.bench_function("Scalar inversion", |b| { - let s = Scalar::from_u64(897987897).invert(); + let s = Scalar::from(897987897u64).invert(); b.iter(|| s.invert()); }); } diff --git a/src/backend/avx2/edwards.rs b/src/backend/avx2/edwards.rs index d9f0b01..1510bda 100644 --- a/src/backend/avx2/edwards.rs +++ b/src/backend/avx2/edwards.rs @@ -431,7 +431,7 @@ mod test { println!("Testing B +- kB"); let P = constants::ED25519_BASEPOINT_POINT; - let Q = &constants::ED25519_BASEPOINT_TABLE * &Scalar::from_u64(8475983829); + let Q = &constants::ED25519_BASEPOINT_TABLE * &Scalar::from(8475983829u64); addition_test_helper(P, Q); } @@ -510,7 +510,7 @@ mod test { doubling_test_helper(P); println!("Testing [2]([k]B)"); - let P = &constants::ED25519_BASEPOINT_TABLE * &Scalar::from_u64(8475983829); + let P = &constants::ED25519_BASEPOINT_TABLE * &Scalar::from(8475983829u64); doubling_test_helper(P); } } diff --git a/src/edwards.rs b/src/edwards.rs index 7e15fce..2246d6f 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -1004,7 +1004,7 @@ mod test { /// Test that computing 2*basepoint is the same as basepoint.double() #[test] fn basepoint_mult_two_vs_basepoint2() { - let two = Scalar::from_u64(2); + let two = Scalar::from(2u64); let bp2 = &constants::ED25519_BASEPOINT_TABLE * &two; assert_eq!(bp2.compress(), BASE2_CMPRSSD); } @@ -1030,10 +1030,10 @@ mod test { // Test that sum works for non-empty iterators let BASE = constants::ED25519_BASEPOINT_POINT; - let s1 = Scalar::from_u64(999); + let s1 = Scalar::from(999u64); let P1 = &BASE * &s1; - let s2 = Scalar::from_u64(333); + let s2 = Scalar::from(333u64); let P2 = &BASE * &s2; let vec = vec![P1.clone(), P2.clone()]; @@ -1048,7 +1048,7 @@ mod test { assert_eq!(sum, EdwardsPoint::identity()); // Test that sum works on owning iterators - let s = Scalar::from_u64(2); + let s = Scalar::from(2u64); let mapped = vec.iter().map(|x| x * &s); let sum: EdwardsPoint = mapped.sum(); diff --git a/src/ristretto.rs b/src/ristretto.rs index 5581a19..6124f92 100644 --- a/src/ristretto.rs +++ b/src/ristretto.rs @@ -839,7 +839,7 @@ impl VartimeMultiscalarMul for RistrettoPoint { /// use curve25519_dalek::constants; /// use curve25519_dalek::scalar::Scalar; /// -/// let a = Scalar::from_u64(87329482); +/// let a = Scalar::from(87329482u64); /// let P = &a * &constants::RISTRETTO_BASEPOINT_TABLE; /// ``` #[derive(Clone)] @@ -959,7 +959,7 @@ mod test { #[test] fn scalarmult_ristrettopoint_works_both_ways() { let P = constants::RISTRETTO_BASEPOINT_POINT; - let s = Scalar::from_u64(999); + let s = Scalar::from(999u64); let P1 = &P * &s; let P2 = &s * &P; @@ -973,10 +973,10 @@ mod test { // Test that sum works for non-empty iterators let BASE = constants::RISTRETTO_BASEPOINT_POINT; - let s1 = Scalar::from_u64(999); + let s1 = Scalar::from(999u64); let P1 = &BASE * &s1; - let s2 = Scalar::from_u64(333); + let s2 = Scalar::from(333u64); let P2 = &BASE * &s2; let vec = vec![P1.clone(), P2.clone()]; @@ -991,7 +991,7 @@ mod test { assert_eq!(sum, RistrettoPoint::identity()); // Test that sum works on owning iterators - let s = Scalar::from_u64(2); + let s = Scalar::from(2u64); let mapped = vec.iter().map(|x| x * &s); let sum: RistrettoPoint = mapped.sum(); diff --git a/src/scalar.rs b/src/scalar.rs index 945691c..36045b0 100644 --- a/src/scalar.rs +++ b/src/scalar.rs @@ -426,6 +426,50 @@ where } } +impl From for Scalar { + fn from(x: u8) -> Scalar { + let mut s_bytes = [0u8; 32]; + s_bytes[0] = x; + Scalar{ bytes: s_bytes } + } +} + +impl From for Scalar { + fn from(x: u16) -> Scalar { + use byteorder::{ByteOrder, LittleEndian}; + let mut s_bytes = [0u8; 32]; + LittleEndian::write_u16(&mut s_bytes, x); + Scalar{ bytes: s_bytes } + } +} + +impl From for Scalar { + fn from(x: u32) -> Scalar { + use byteorder::{ByteOrder, LittleEndian}; + let mut s_bytes = [0u8; 32]; + LittleEndian::write_u32(&mut s_bytes, x); + Scalar{ bytes: s_bytes } + } +} + +impl From for Scalar { + fn from(x: u64) -> Scalar { + use byteorder::{ByteOrder, LittleEndian}; + let mut s_bytes = [0u8; 32]; + LittleEndian::write_u64(&mut s_bytes, x); + Scalar{ bytes: s_bytes } + } +} + +impl From for Scalar { + fn from(x: u128) -> Scalar { + use byteorder::{ByteOrder, LittleEndian}; + let mut s_bytes = [0u8; 32]; + LittleEndian::write_u128(&mut s_bytes, x); + Scalar{ bytes: s_bytes } + } +} + impl Scalar { /// Return a `Scalar` chosen uniformly at random using a user-provided RNG. /// @@ -513,15 +557,6 @@ impl Scalar { } } - /// Construct a scalar from the given `u64`. - pub fn from_u64(x: u64) -> Scalar { - let mut s_bytes = [0u8; 32]; - for i in 0..8 { - s_bytes[i] = (x >> (i*8)) as u8; - } - Scalar{ bytes: s_bytes } - } - /// Given a nonzero `Scalar`, compute its multiplicative inverse. /// /// # Warning @@ -585,19 +620,19 @@ impl Scalar { /// # use curve25519_dalek::scalar::Scalar; /// # fn main() { /// let mut scalars = [ - /// Scalar::from_u64(3), - /// Scalar::from_u64(5), - /// Scalar::from_u64(7), - /// Scalar::from_u64(11), + /// Scalar::from(3u64), + /// Scalar::from(5u64), + /// Scalar::from(7u64), + /// Scalar::from(11u64), /// ]; /// /// let allinv = Scalar::batch_invert(&mut scalars); /// - /// assert_eq!(allinv, Scalar::from_u64(3*5*7*11).invert()); - /// assert_eq!(scalars[0], Scalar::from_u64(3).invert()); - /// assert_eq!(scalars[1], Scalar::from_u64(5).invert()); - /// assert_eq!(scalars[2], Scalar::from_u64(7).invert()); - /// assert_eq!(scalars[3], Scalar::from_u64(11).invert()); + /// assert_eq!(allinv, Scalar::from(3*5*7*11u64).invert()); + /// assert_eq!(scalars[0], Scalar::from(3u64).invert()); + /// assert_eq!(scalars[1], Scalar::from(5u64).invert()); + /// assert_eq!(scalars[2], Scalar::from(7u64).invert()); + /// assert_eq!(scalars[3], Scalar::from(11u64).invert()); /// # } /// ``` #[cfg(any(feature = "alloc", feature = "std"))] @@ -1050,9 +1085,9 @@ mod test { } #[test] - fn from_unsigned() { - let val = 0xdeadbeefdeadbeef; - let s = Scalar::from_u64(val); + fn from_u64() { + let val: u64 = 0xdeadbeefdeadbeef; + let s = Scalar::from(val); assert_eq!(s[7], 0xde); assert_eq!(s[6], 0xad); assert_eq!(s[5], 0xbe); @@ -1073,7 +1108,7 @@ mod test { #[test] fn impl_add() { - let two = Scalar::from_u64(2); + let two = Scalar::from(2u64); let one = Scalar::one(); let should_be_two = &one + &one; assert_eq!(should_be_two, two); @@ -1101,8 +1136,8 @@ mod test { assert_eq!(should_be_one, one); // Test that product works for iterators where Item = Scalar - let xs = [Scalar::from_u64(2); 10]; - let ys = [Scalar::from_u64(3); 10]; + let xs = [Scalar::from(2u64); 10]; + let ys = [Scalar::from(3u64); 10]; // now zs is an iterator with Item = Scalar let zs = xs.iter().zip(ys.iter()).map(|(x,y)| x * y); @@ -1110,9 +1145,9 @@ mod test { let y_prod: Scalar = ys.iter().product(); let z_prod: Scalar = zs.product(); - assert_eq!(x_prod, Scalar::from_u64(1024)); - assert_eq!(y_prod, Scalar::from_u64(59049)); - assert_eq!(z_prod, Scalar::from_u64(60466176)); + assert_eq!(x_prod, Scalar::from(1024u64)); + assert_eq!(y_prod, Scalar::from(59049u64)); + assert_eq!(z_prod, Scalar::from(60466176u64)); assert_eq!(x_prod * y_prod, z_prod); } @@ -1121,7 +1156,7 @@ mod test { fn impl_sum() { // Test that sum works for non-empty iterators - let two = Scalar::from_u64(2); + let two = Scalar::from(2u64); let one_vector = vec![Scalar::one(), Scalar::one()]; let should_be_two: Scalar = one_vector.iter().sum(); assert_eq!(should_be_two, two); @@ -1133,8 +1168,8 @@ mod test { assert_eq!(should_be_zero, zero); // Test that sum works for owned types - let xs = [Scalar::from_u64(1); 10]; - let ys = [Scalar::from_u64(2); 10]; + let xs = [Scalar::from(1u64); 10]; + let ys = [Scalar::from(2u64); 10]; // now zs is an iterator with Item = Scalar let zs = xs.iter().zip(ys.iter()).map(|(x,y)| x + y); @@ -1142,9 +1177,9 @@ mod test { let y_sum: Scalar = ys.iter().sum(); let z_sum: Scalar = zs.sum(); - assert_eq!(x_sum, Scalar::from_u64(10)); - assert_eq!(y_sum, Scalar::from_u64(20)); - assert_eq!(z_sum, Scalar::from_u64(30)); + assert_eq!(x_sum, Scalar::from(10u64)); + assert_eq!(y_sum, Scalar::from(20u64)); + assert_eq!(z_sum, Scalar::from(30u64)); assert_eq!(x_sum + y_sum, z_sum); } @@ -1296,7 +1331,7 @@ mod test { #[test] fn batch_invert_consistency() { - let mut x = Scalar::from_u64(1); + let mut x = Scalar::from(1u64); let mut v1: Vec<_> = (0..16).map(|_| {let tmp = x; x = x + x; tmp}).collect(); let v2 = v1.clone(); diff --git a/src/traits.rs b/src/traits.rs index aac84d0..5e8ae05 100644 --- a/src/traits.rs +++ b/src/traits.rs @@ -72,9 +72,9 @@ pub trait MultiscalarMul { /// use curve25519_dalek::scalar::Scalar; /// /// // Some scalars - /// let a = Scalar::from_u64(87329482); - /// let b = Scalar::from_u64(37264829); - /// let c = Scalar::from_u64(98098098); + /// let a = Scalar::from(87329482u64); + /// let b = Scalar::from(37264829u64); + /// let c = Scalar::from(98098098u64); /// /// // Some points /// let P = constants::RISTRETTO_BASEPOINT_POINT; @@ -128,9 +128,9 @@ pub trait VartimeMultiscalarMul { /// use curve25519_dalek::scalar::Scalar; /// /// // Some scalars - /// let a = Scalar::from_u64(87329482); - /// let b = Scalar::from_u64(37264829); - /// let c = Scalar::from_u64(98098098); + /// let a = Scalar::from(87329482u64); + /// let b = Scalar::from(37264829u64); + /// let c = Scalar::from(98098098u64); /// /// // Some points /// let P = constants::RISTRETTO_BASEPOINT_POINT;