EC math functions #4696
EC math functions #4696
Conversation
|
P.S.: at least these work fine in secretstore (PR is coming) |
rphmeier
added
A0-pleasereview 🤓
ethkey/src/math.rs
Outdated
| use secp256k1::key; | ||
|
|
||
| /// Inplace multiply public key by secret key (EC point * scalar) | ||
| pub fn public_mul_secret(public: &mut Public, secret: &Secret) -> Result<(), Error> { |
There was a problem hiding this comment.
why not to make this methods on corresponding structs?
There was a problem hiding this comment.
I considered this, but these are very specific (i.e. won't be used by most of users) so I decided that it is better to place math functions in separate module (to avoid garbage in IntelliSense && etc). I could move, if you think it is better
There was a problem hiding this comment.
i already use them, see extended.rs
There was a problem hiding this comment.
rather than as methods, it should use std::ops::{Add, Mul} and maybe their _Assign variants. I assume the failure condition is just that the context wasn't created with the correct capabilities. Since our context is global and always created with full capabilities, it should be safe to assert that no errors occur.
There was a problem hiding this comment.
Failure conditions like invalid scalar/curve point also shouldn't be possible; if they are then it's a bug in the type allowing us to create invalid secrets.
There was a problem hiding this comment.
in that case, then it's probably best not to use ops, just methods on the type which can error.
There was a problem hiding this comment.
still can move some of functions to Secret:: methods, though
There was a problem hiding this comment.
yeah
and as far as i remember @debris wanted to wrap H512 to Public the same as H256 wrapped to Secret, so maybe leave it until then
There was a problem hiding this comment.
moved appropriate functions to Secret methods
ethkey/src/math.rs
Outdated
|
|
||
| /// Compute power of secret key inplace (secret ^ pow). | ||
| /// This function is not intended to be used with large powers. | ||
| pub fn secret_pow(secret: &mut Secret, pow: usize) -> Result<(), Error> { |
There was a problem hiding this comment.
the code of this function could be improved with match:
match pow {
0 => {}
1 => {}
pow => {}
}
Ok(())
| } | ||
|
|
||
| /// Return base point of secp256k1 | ||
| pub fn generation_point() -> Public { |
There was a problem hiding this comment.
isn't it the same as taking GENERATOR_X, GENERATOR_Y from here using less cpu cycles?
https://github.com/ethcore/rust-secp256k1/blob/master/src/constants.rs#L53
There was a problem hiding this comment.
cool, but idk how to combine these :) could you point the proper function?
There was a problem hiding this comment.
something like this
let mut public_sec_raw = [0u8; 65];
public_sec_raw[0] = 4;
public_sec_raw[1..33].copy_from_slice(&GENERATOR_X);
public_sec_raw[33..65].copy_from_slice(&GENERATOR_Y);
let public_sec = PublicKey::from_slice(&SECP256K1, &public_sec_raw).map_err(|_| Error::InvalidPoint)?;
ethkey/src/math.rs
Outdated
| Ok(key::PublicKey::from_slice(&SECP256K1, &public_data)?) | ||
| } | ||
|
|
||
| fn to_secp256k1_secret(secret: &Secret) -> Result<key::SecretKey, Error> { |
There was a problem hiding this comment.
could be a consuming method on Secret struct as well
There was a problem hiding this comment.
or a From implementation (which may already exist?)
There was a problem hiding this comment.
cool, will add these :)
There was a problem hiding this comment.
These are inplace methods => consuming does not fits here.
From must not fail. This could fail.
ethkey/src/lib.rs
Outdated
| @@ -28,6 +28,7 @@ mod brain; | |||
| mod error; | |||
| mod keypair; | |||
| mod keccak; | |||
| pub mod math; | |||
There was a problem hiding this comment.
style: public modules should be separated from private.
|
Any tests against well-known vectors? |
NikVolf
added
A5-grumble 🔥
|
@NikVolf see first comment. If you have any 'well-known' vector, please share - I'll add |
svyatonik
added
A0-pleasereview 🤓
|
Fair enough, we can hardcode some samples later in case to be confident with later changes |
NikVolf
added
A8-looksgood 🦄
Will need this for secret store.
There are no tests as these will be very artificial. Like - I do not know the answer, but my functions shows result A, so let it be A. I could add such tests, if you think these would be helpful.