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

Implement ff traits for bls12_381 and jubjub crates #227

Merged
merged 3 commits into from
Jun 17, 2020

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Apr 30, 2020

Closes #160 and #166.

@str4d str4d requested a review from ebfull April 30, 2020 21:15
@ebfull ebfull mentioned this pull request Apr 30, 2020
18 tasks
@str4d str4d added this to the Core Sprint 2020-17 milestone Apr 30, 2020
@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #227 into master will decrease coverage by 30.34%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #227       +/-   ##
===========================================
- Coverage   65.43%   35.09%   -30.35%     
===========================================
  Files         105       94       -11     
  Lines       14936    11325     -3611     
===========================================
- Hits         9774     3974     -5800     
- Misses       5162     7351     +2189     
Impacted Files Coverage Δ
bls12_381/src/fp.rs 0.00% <ø> (-47.91%) ⬇️
bls12_381/src/scalar.rs 0.00% <0.00%> (-51.44%) ⬇️
jubjub/src/fr.rs 0.00% <0.00%> (-49.14%) ⬇️
bellman/src/groth16/generator.rs 0.00% <0.00%> (-89.23%) ⬇️
bellman/src/gadgets/sha256.rs 0.00% <0.00%> (-86.67%) ⬇️
bellman/tests/mimc.rs 0.00% <0.00%> (-85.19%) ⬇️
bellman/src/groth16/prover.rs 0.00% <0.00%> (-85.13%) ⬇️
bellman/src/multiexp.rs 0.00% <0.00%> (-81.82%) ⬇️
zcash_history/src/node_data.rs 0.00% <0.00%> (-77.07%) ⬇️
bls12_381/src/g1.rs 0.00% <0.00%> (-75.41%) ⬇️
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ff8562...b0542dd. Read the comment docs.

bls12_381/Cargo.toml Show resolved Hide resolved
bls12_381/src/fp.rs Outdated Show resolved Hide resolved
bls12_381/src/fp.rs Outdated Show resolved Hide resolved
bls12_381/src/fp.rs Outdated Show resolved Hide resolved
bls12_381/src/fp.rs Outdated Show resolved Hide resolved
bls12_381/src/fp.rs Outdated Show resolved Hide resolved
@str4d str4d mentioned this pull request May 2, 2020
@str4d
Copy link
Contributor Author

str4d commented May 2, 2020

I've addressed most comments in #228. Once that PR has been reviewed and merged, I'll rebase this PR to remove the now-unnecessary parts, and implement Field::random.

@str4d
Copy link
Contributor Author

str4d commented May 12, 2020

Rebased on master after merging #228.

@str4d str4d mentioned this pull request May 14, 2020
@str4d
Copy link
Contributor Author

str4d commented May 14, 2020

The first commit in this PR is also in #230; I'll rebase whichever PR is not merged first.

@str4d
Copy link
Contributor Author

str4d commented May 18, 2020

Rebased on master now that #230 has been merged.

@daira daira self-requested a review May 19, 2020 06:18
@str4d
Copy link
Contributor Author

str4d commented May 29, 2020

Rebased the PR; it now only implements the ff traits for the structs that are used as scalars in the group implementations, because the Base associated types are being removed from the group traits.

@@ -70,6 +78,22 @@ const MODULUS: Scalar = Scalar([
0x73ed_a753_299d_7d48,
]);

const MODULUS_BYTES: [u8; 32] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Say what byte order this is in (maybe include it in the name).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also consider renaming Scalar::from_bytes to from_le_bytes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this value is only used later in the PrimeField impl, where the constant is defined to be little endian by the implementation, so this property is documented and accounted for.

@@ -162,7 +162,7 @@ impl Fp {
self.ct_eq(&Fp::zero())
}

/// Attempts to convert a little-endian byte representation of
/// Attempts to convert a big-endian byte representation of
/// a scalar into an `Fp`, failing if the input is not canonical.
pub fn from_bytes(bytes: &[u8; 48]) -> CtOption<Fp> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider naming this from_be_bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd personally rather not; it implies a little-endian scalar encoding would also be correct and supported (rather than big-endian being canonical for Fp), and that way lies madness.

@@ -70,6 +78,33 @@ pub const MODULUS: Fr = Fr([
0x0e7d_b4ea_6533_afa9,
]);

const MODULUS_BYTES: [u8; 32] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Say what byte order this is in (maybe include it in the name).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also consider renaming Fr::from_bytes to from_le_bytes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See the other comment; the same holds here.

@str4d str4d merged commit 9f0ee56 into zcash:master Jun 17, 2020
@str4d str4d deleted the impl-ff-traits branch June 17, 2020 22:13
@str4d str4d mentioned this pull request Jun 26, 2020
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.

impl ff::* for jubjub
4 participants