-
Notifications
You must be signed in to change notification settings - Fork 198
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
Implement scalar arithmetic via Barrett reduction #56
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.