-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite ec abstraction #17
Conversation
I made major changes to EC traits. currently implemented only for secp256k1. Unit tests are needed as well as implementation for curve25519. |
I changed the elliptic curves to be features. need to find a way to make the compilation of the crates optional such that we compile only the EC we need. |
4b8027e
to
596c8f3
Compare
The build seems to be failing: https://travis-ci.com/KZen-networks/cryptography-utils/jobs/139499279#L694:
|
@romanz this is because you must choose an elliptic curve feature for the build to be successful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly style and naming-related comments; didn't review the cryptography-related changes.
Cargo.toml
Outdated
@@ -8,9 +8,14 @@ authors = [ | |||
[lib] | |||
crate-type = ["lib"] | |||
|
|||
[features] | |||
#default = ["curve25519"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the commented line - or set the default explicitly.
|
||
for value in big_ints { | ||
let bytes: Vec<u8> = value.borrow().into(); | ||
digest.update(&bytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be written as digest.update(&value.to_vec())
?
If so - please change hash_sha256.rs
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed it. thanks!
//#[cfg(feature="curvesecp256k1")] | ||
//use secp256k1instance::{SK,PK,GE,FE}; | ||
//#[cfg(feature="curve25519-dalek")] | ||
//use curve25519instance::{SK,PK,GE,FE}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please delete the commented code blocks (above and in the rest of the changed code).
src/elliptic/curves/curve25519.rs
Outdated
// Secp256k1 elliptic curve utility functions (se: https://en.bitcoin.it/wiki/Secp256k1). | ||
// | ||
// In Cryptography utilities, we need to manipulate low level elliptic curve members as Point | ||
// in order to perform operation on them. As the library secp256k1 expose only SecretKey and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secp256k1
-> curve25519
src/elliptic/curves/curve25519.rs
Outdated
|
||
fn sub(&self, other: &SK) -> Curve25519Scalar { | ||
Curve25519Scalar { | ||
purpose: "mul", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mul
-> sub
src/elliptic/curves/traits.rs
Outdated
|
||
// TODO: add a fn is_point | ||
pub trait ECPoint<PK, SK> { | ||
fn new() -> Self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider naming it generator()
(instead of new()
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. good idea!!
src/lib.rs
Outdated
#[cfg(feature = "curvesecp256k1")] | ||
mod secp256k1instance { | ||
|
||
pub use elliptic::curves::secp256_k1::EC; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider removing the line (as "curve25519" doesn't export an EC
type).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments, but didn't look too closely at the crypto parts -- is that a review priority as well? anything that's worth focusing specifically on?
|
||
pub struct PedersenCommitment; | ||
|
||
impl Commitment<Secp256k1Point> for PedersenCommitment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't this fail when not using feature curvesecp256k1
? maybe find a way to use conditional compilation to leave this code out in those cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the reason was that not every elliptic curve that is used for signing can be used for Pedersen commitment as well so when new elliptic curves are introduced we would add pedersen commitment based on them. But your point is solid. @gbenattar @romanz do you know of a way to support Pederesen with fixed elliptic curve while condition compilation on all other elliptic curve related stuff? this might come in useful in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity, what determines whether a curve can be used for commitment or not?
for conditional compilation you can simply annotate the impl with #[cfg(feature = "...")]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the curve is prime order group or not. For example curve25519 is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sprayed #[cfg(feature = "curvesecp256k1")]
all over the place. It appears that we are using pedersen commitments in many places. @romanz @mortendahl @gbenattar maybe one of you have an idea on how to do this exclusion more elegant or at least how to minimize the times I use #[cfg(feature = "curvesecp256k1")]
?
pub struct HashCommitment; | ||
|
||
//TODO: using the function with BigInt's as input instead of string's makes it impossible to commit to empty message or use empty randomness | ||
impl Commitment for HashCommitment { | ||
impl Commitment<BigInt> for HashCommitment { | ||
fn create_commitment_with_user_defined_randomness( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create_commitment_with_user_defined_randomness
and create_commitment
share almost the same code; let one use the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. good catch.
mg.add_point(&rh.get_element()) | ||
} | ||
|
||
fn create_commitment(message: &BigInt) -> (Secp256k1Point, BigInt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both methods again share almost the same code; let one use the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
[features] | ||
default = ["curvesecp256k1"] | ||
curvesecp256k1 = [] | ||
curve25519 = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't get it to compile with this curve (blocked on serde errors)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbenattar probably some backward compatibility issue from your work on serialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default one? Just to make sure, do a cargo update
and also do a rustup update
, they changed nightly recently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbenattar not the default one, the build is failing for curve25519 because serde is not implemented for this.
*/ | ||
use BigInt; | ||
|
||
use super::ring::{digest, hmac}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the extern crate ring
is in the wrong place if super
is needed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
completely right, moved it to lib.rs
&generator_x, | ||
&pk.get_x_coor_as_big_int(), | ||
]); | ||
let challenge_fe: FE = ECScalar::from_big_int(&challenge); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some methods perhaps seem a bit more explicit in naming than typical rust code, eg from_big_int
instead of simply from
, get_element
instead of element
, and get_x_coor_as_big_int
instead of x_coor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the interface. my only problem is with get_element
- I need to differentiate it from set_element
so I cannot just change it to element
|
||
let base_point: GE = ECPoint::new(); | ||
//let sk_challenge_response : FE = ECScalar::from_big_int(&proof.challenge_response); | ||
let sk_challenge_response: FE = proof.challenge_response.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to clone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great!
use cryptographic_primitives::commitments::pedersen_commitment::PedersenCommitment; | ||
use cryptographic_primitives::commitments::traits::Commitment; | ||
|
||
use elliptic::curves::secp256_k1::Secp256k1Point; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conditional compilation to only be included when this curve is used?
let lhs = z1g.add_point(&z2h.get_element()); | ||
let rhs = proof.a1.add_point(&proof.a2.get_element()); | ||
let com_clone = proof.com.clone(); | ||
let ecom = com_clone.scalar_mul(&e.get_element()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is something slightly unpleasant about scalar_mul
taking mut self
while other operations don't. perhaps a Borrow
could work and still get the same reuse optimisation when needed? in any case the extra clones could be move into that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. scalar_mul takes self
with the intention to consume self
.
This was done to limit the interface. Is this a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a problem, only saying that the clone could be hidden if desired
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you give an example please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"hidden" as in moved into the function by letting the function take eg a Cow instead. does that make sense?
use cryptographic_primitives::twoparty::dh_key_exchange::*; | ||
|
||
#[test] | ||
fn test_full_key_gen() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if the interface could be changed to avoid this picking of fields, which might cause some confusion or worse down the line. What if eg verify_commitments_and_dlog_proof
simply took party_one_first_message
and party_one_second_message
as arguments?
* ECScalar and ECPoint and the EC specific implementation for Secp256k1Scalar and Secp256k1Point * Commitments: Pedersen, Sigma valid Pedersen blind, Two party coin toss, Key exchange PR: #17 Legacy branch created: https://github.com/KZen-networks/cryptography-utils/tree/two-party-v0
* ECScalar and ECPoint and the EC specific implementation for Secp256k1Scalar and Secp256k1Point * Commitments: Pedersen, Sigma valid Pedersen blind, Two party coin toss, Key exchange PR: ZenGo-X/curv#17 Legacy branch created: https://github.com/KZen-networks/cryptography-utils/tree/two-party-v0
No description provided.