Skip to content
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

Merged
merged 17 commits into from
Jan 9, 2021
Merged

Conversation

ValarDragon
Copy link
Member

@ValarDragon ValarDragon commented Dec 31, 2020

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.

  • Targeted PR against correct branch (main)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

Copy link
Member

@weikengchen weikengchen left a 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.

ff/src/fields/macros.rs Outdated Show resolved Hide resolved
@Pratyush
Copy link
Member

Pratyush commented Jan 2, 2021

I don't know how to write useful tests for this function for a generic field

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

@ValarDragon
Copy link
Member Author

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?

@weikengchen
Copy link
Member

I think it would be fine to use num-bigint. Indeed, nonnative already uses it (https://github.com/arkworks-rs/nonnative/blob/master/Cargo.toml)

@ValarDragon
Copy link
Member Author

ValarDragon commented Jan 7, 2021

Switched to using num-bigint for checking this. Is it fine to keep it with the test values being generated via python?
(So the test works for any fields, but the values tested on aren't particularly interesting for modulii of different byte lengths)

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 367 to 370
for i in num_bytes_to_directly_convert..bytes.len() {
res *= window_size;
res += Self::from(bytes[i]);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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() {
Copy link
Member

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?

@Pratyush
Copy link
Member

Pratyush commented Jan 8, 2021

btw for the test, does it also make sense to test with random inputs?

@ValarDragon
Copy link
Member Author

Now that we have a reference, it makes sense

@Pratyush
Copy link
Member

Pratyush commented Jan 8, 2021

This LGTM modulo the nit and the comment about tests.

@ValarDragon ValarDragon merged commit 12afad6 into master Jan 9, 2021
@ValarDragon ValarDragon deleted the from_bytes_mod_order branch January 9, 2021 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Field::from_bytes method that reduces the input
3 participants