-
Notifications
You must be signed in to change notification settings - Fork 267
Implement scalar arithmetic via Barrett reduction #56
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
| /// Parses the given byte array as hashed message. | ||
| /// | ||
| /// Subtracts the modulus when the byte array is larger than the modulus. | ||
| pub fn from_hash(bytes: [u8; 32]) -> 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.
I would expect a from_hash method to accept a Digest instance. How about naming it from_bytes_mod_order instead?
| pub fn from_hash(bytes: [u8; 32]) -> Self { | |
| pub fn from_bytes_mod_order(bytes: [u8; 32]) -> Self { |
You could then add a from_hash method that does accept a Digest instance (gated on a digest 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.
I'm not sure what the right name is. Thinking generically, this is tied to "Let z be the leftmost bits of
, where
is the bit length of the group order n.", which for p256 is just reduction but for other curves can involve right-shifts. So I'd like a method and name that captures this requirement. Ideas/prior art?
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 from_hash is fine if the argument is a Digest instance. There's a bit of a rationale for why e.g. signature::DigestSigner takes one (as opposed to a byte array) here:
https://docs.rs/signature/1.1.0/signature/trait.DigestSigner.html#notes
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.
Sure, I'll follow along if/when #76 is merged 👍
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 actually liking #78 better at this point
To make #54 more concrete.