-
Notifications
You must be signed in to change notification settings - Fork 259
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
Add PrimeField::from_bytes_mod_order #164
Conversation
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 PR looks good. Especially the test is comprehensive.
We have access to the Size of the modulus generically, so we can use that to generate enough bytes, right? And for the independent source of truth we can use something like num-bigint as a dev-dependency |
Didn't realize there were pre-existing variable sized big-int libraries. Do you think we should go with a general approach for testing before merging this, or have a todo for using num-bigint as a dev-dependency? |
I think it would be fine to use |
Switched to using num-bigint for checking this. Is it fine to keep it with the test values being generated via python? |
ff/src/fields/mod.rs
Outdated
for i in num_bytes_to_directly_convert..bytes.len() { | ||
res *= window_size; | ||
res += Self::from(bytes[i]); | ||
} |
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.
for i in num_bytes_to_directly_convert..bytes.len() { | |
res *= window_size; | |
res += Self::from(bytes[i]); | |
} | |
for byte in bytes[num_bytes_to_directly_convert..] { | |
res *= window_size; | |
res += Self::from(byte); | |
} |
@@ -614,4 +650,100 @@ mod no_std_tests { | |||
); | |||
} | |||
} | |||
|
|||
#[test] | |||
fn test_from_be_bytes_mod_order() { |
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.
Should we also add tests for le
?
btw for the test, does it also make sense to test with random inputs? |
Now that we have a reference, it makes sense |
This LGTM modulo the nit and the comment about tests. |
Description
Adds a method
from_bytes_mod_order
, needed for hashing to the curve (#147).This also includes a python file I used for creating test vectors for the method. I don't know how to write useful tests for this function for a generic field (since we'd need to do arithmetic on integers of a length longer than the field). Hence this function is only tested on one field, but includes a script to add test vectors for any field.
ref: zkcrypto/ff#45
closes: #156
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pending
section inCHANGELOG.md
Files changed
in the Github PR explorer