-
Notifications
You must be signed in to change notification settings - Fork 245
Implement GroupDigest for NistP256 and Secp256k1
#503
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
Conversation
|
I’d like to not have to copy osswu into k256. I’ve got it working locally with only those two changes |
|
I agree, we will need to fix this in elliptic-curve. EDIT: with the two changes you mean adding normalize inside the OSSWU implementation in elliptic-curve, yes? |
| const TEST_VECTORS: &[TestVector] = &[ | ||
| TestVector { | ||
| dst: b"HashToScalar-VOPRF08-\x00\x00\x03", | ||
| seed: &hex!("a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3"), | ||
| sk_sm: &hex!("c15d9e9ab36d495d9d62954db6aafe06d3edabf41600d58f9be0737af2719e97"), | ||
| }, | ||
| TestVector { | ||
| dst: b"HashToScalar-VOPRF08-\x01\x00\x03", | ||
| seed: &hex!("a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3"), | ||
| sk_sm: &hex!("7f62054fcd598b5e023c08ef0f04e05e26867438d5e355e846c9d8788d5c7a12"), | ||
| }, | ||
| ]; |
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 copied these test vectors from the VOPRF spec, I don't know any other spec providing test vectors for hash_to_scalar. The VOPRF spec also doesn't define any for k256.
82dd760 to
61551db
Compare
|
I'm happy to move my other PR which implements all the hash2curve to something similar to this unless you are planning to turn this into a regular PR. Thoughts? |
|
The plan is to turn this into a regular PR actually, I just can't until a new version of elliptic-curve is released. So as soon as that's done I'm going to update this. |
b343d4a to
4ee0034
Compare
|
I can't make sense of this error. |
60239c5 to
be9b41f
Compare
|
Thanks, this should be good to go now. |
k256/src/arithmetic/hash2curve.rs
Outdated
| // if e2, y = y1, else y = y2 | ||
| let mut y = Self::conditional_select(&y2, &y1, e2); | ||
|
|
||
| y.conditional_assign(&-y, self.normalize().sgn0() ^ y.normalize().sgn0()); |
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 noticed when removing the sgn0 method and internally using is_odd instead, it complained that it wasn't normalized. Does that look alright @mikelodder7?
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.
elliptic-curves/k256/src/arithmetic/field/field_impl.rs
Lines 91 to 94 in 8a44a3f
| pub fn is_odd(&self) -> Choice { | |
| debug_assert!(self.normalized); | |
| self.value.is_odd() | |
| } |
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.
@fjarri do you think is_odd should call normalize first?
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.
Anytime comparisons are used for osswu it has to be normalized otherwise it gets wrong results
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, I would assume you need to normalize, since the modulus is odd, so x and x+modulus (possible even if you have magnitude=1, but not normalized) will have different parity.
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.
Alright, will change is_odd to normalize first then.
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.
Co-Authored-By: Michael Lodder <[email protected]>
Co-Authored-By: Michael Lodder <[email protected]>
f6ac254 to
adc821c
Compare
|
Re-based because of #506. |
See RustCrypto/traits#865.
This replaces #495 where I copied some code from @mikelodder7. The reason I did that is to test changes, for example, for RustCrypto/traits#871.
Currently builds on top of RustCrypto/traits#876.