Add `weierstrass::FixedBaseScalarMul` trait #49

tarcieri · 2020-06-19T17:52:29Z

Adds a trait for fixed-base scalar multiplication which accepts &ScalarBytes as input and returns an associated point type, whose bounds allow for a From conversion to either the CompressedCurvePoint or UncompressedCurvePoint for a given curve.

Using this trait, a weierstrass::PublicKey<C>::from_secret_key method is conditionally implemented when the curve C impls the FixedBaseScalarMul trait, allowing generic computation of a public key from a secret key, with optional point compression (selected via a compress argument).

tarcieri · 2020-06-19T17:54:23Z

@str4d @tuxxy pushing this up early for a bit of design review. I implemented FixedBaseScalarMul for k256 so far (but not for p256 yet), which is interesting because it demonstrates not all curves are obliged to implement it which is nice.

~~TODO: test with test vectors before merging~~

I added some test vectors for secp256k1, which is both what I'm immediately interested in and also provides a PoC that the trait is working at a conceptual level.

For now I left a TODO in the p256 crate's arithmetic.rs to add some test vectors, however I also feel like it's a bit redundant as it's ultimately testing that scalar mult is working.

codecov-commenter · 2020-06-19T17:55:06Z

Codecov Report

Merging #49 into master will decrease coverage by 0.74%.
The diff coverage is 62.90%.

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
- Coverage   82.02%   81.27%   -0.75%     
==========================================
  Files          12       12              
  Lines        1296     1346      +50     
==========================================
+ Hits         1063     1094      +31     
- Misses        233      252      +19

Impacted Files	Coverage Δ
elliptic-curve-crate/src/weierstrass/public_key.rs	`40.54% <0.00%> (-11.19%)`	⬇️
k256/src/arithmetic/field.rs	`85.54% <ø> (ø)`
p256/src/arithmetic/field.rs	`82.11% <ø> (ø)`
p256/src/arithmetic/scalar.rs	`75.00% <0.00%> (-18.75%)`	⬇️
p256/src/arithmetic.rs	`84.69% <73.33%> (-0.93%)`	⬇️
k256/src/arithmetic/scalar.rs	`60.60% <75.00%> (+4.60%)`	⬆️
k256/src/arithmetic.rs	`86.68% <95.65%> (+1.18%)`	⬆️
elliptic-curve-crate/src/weierstrass/point.rs	`48.14% <0.00%> (+7.40%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7faa68f...46c9a3c. Read the comment docs.

elliptic-curve-crate/src/weierstrass.rs

k256/src/arithmetic.rs

tarcieri · 2020-06-19T19:42:29Z

Calling this effectively complete. I'm going to open another PR (which incorporates this one) to add an AffineCoordinates trait and see if I can use that to further DRY out some of the copy-and-pasted code between the k256 and p256 crates.

PTAL @str4d and @tuxxy !

Adds a trait for fixed-base scalar multiplication which accepts `&ScalarBytes` as input and returns an associated point type, whose bounds allow for a `From` conversion to either the `CompressedCurvePoint` or `UncompressedCurvePoint` for a given curve. Using this trait, a `weierstrass::PublicKey<C>::from_secret_key` method is conditionally implemented when the curve `C` impls the `FixedBaseScalarMul` trait, allowing generic computation of a public key from a secret key, with optional point compression (selected via a `compress` argument).

tarcieri · 2020-06-20T16:56:37Z

elliptic-curve-crate/src/weierstrass/public_key.rs

+    pub fn from_secret_key(secret_key: &SecretKey<C::ScalarSize>, compress: bool) -> Option<Self> {
+        let ct_option = C::mul_base(secret_key.secret_scalar());
+
+        if ct_option.is_some().into() {
+            let affine_point = ct_option.unwrap();
+
+            if compress {
+                Some(PublicKey::Compressed(affine_point.into()))
+            } else {
+                Some(PublicKey::Uncompressed(affine_point.into()))
+            }
+        } else {
+            None
+        }


I was able to eliminate the Default bound on AffinePoint by having this return an Option rather than a CtOption as soon as scalar multiplication is complete (i.e. unwrapping the CtOption containing the public key).

Since we're dealing with a public key after scalar multiplication is complete, this seems ok to me.

tarcieri · 2020-06-20T17:44:47Z

Going to go ahead and merge this as I think it's a pretty basic and important feature.

There are some potential alternative designs to explore/consider here, but I think this is a reasonable start.

tarcieri requested review from tuxxy and str4d June 19, 2020 17:52

tarcieri marked this pull request as draft June 19, 2020 17:52