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

Migrate to bls12_381 and jubjub crates #272

Merged
merged 10 commits into from
Aug 22, 2020

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Aug 19, 2020

Closes #170. Closes #171.

Includes tests to ensure that the new generator constants match the
current zcash_primitives::JUBJUB generators.
Includes tests to ensure that the new generator constants match the
current zcash_primitives::JUBJUB generators.
We now use the jubjub crate for this.
It is replaced by the bls12_381 crate.
@codecov
Copy link

codecov bot commented Aug 19, 2020

Codecov Report

Merging #272 into master will increase coverage by 1.12%.
The diff coverage is 86.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #272      +/-   ##
==========================================
+ Coverage   64.95%   66.07%   +1.12%     
==========================================
  Files         117      101      -16     
  Lines       16632    13215    -3417     
==========================================
- Hits        10803     8732    -2071     
+ Misses       5829     4483    -1346     
Impacted Files Coverage Δ
bellman/src/domain.rs 64.10% <ø> (-1.88%) ⬇️
bellman/src/groth16/mod.rs 64.86% <ø> (-2.70%) ⬇️
bellman/src/lib.rs 32.60% <ø> (ø)
jubjub/src/lib.rs 56.62% <0.00%> (+14.66%) ⬆️
zcash_client_sqlite/src/address.rs 16.66% <0.00%> (ø)
zcash_client_sqlite/src/scan.rs 95.93% <ø> (-0.03%) ⬇️
...imitives/src/test_vectors/pedersen_hash_vectors.rs 100.00% <ø> (ø)
zcash_proofs/src/circuit/sprout/mod.rs 0.00% <ø> (ø)
zcash_proofs/src/lib.rs 69.69% <ø> (ø)
zcash_proofs/src/sapling/verifier.rs 0.00% <ø> (ø)
... and 99 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 544d593...24c61f9. Read the comment docs.

@r3ld3v r3ld3v added this to the Core Sprint 2020-33 milestone Aug 20, 2020
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK. This is leaving the circuit code with a confusing mixture of (x, y) and (u, v) conventions, though. Is the plan to fix that immediately?

/// the curve or in the prime-order subgroup.
///
/// This should only be used for hard-coding constants (e.g. fixed generators); in all
/// other cases, use [`SubgroupPoint::from_bytes`] instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

😿 that we can't use constant evaluation with reasonable compile times. Hopefully that will get better.

Failing that, is the initialization-time overhead of using from_bytes significant enough to worry about? I would be inclined to just use from_bytes.

zcash_client_backend/src/decrypt.rs Show resolved Hide resolved
zcash_primitives/src/keys.rs Show resolved Hide resolved
zcash_primitives/src/keys.rs Outdated Show resolved Hide resolved
zcash_primitives/src/keys.rs Show resolved Hide resolved
zcash_proofs/src/circuit/ecc.rs Show resolved Hide resolved
zcash_proofs/src/circuit/ecc.rs Show resolved Hide resolved
zcash_proofs/src/circuit/ecc.rs Show resolved Hide resolved
zcash_proofs/src/circuit/ecc.rs Outdated Show resolved Hide resolved
@str4d str4d force-pushed the migrate-to-bls12_381-jubjub-crates branch from 1280b3e to d15acf8 Compare August 21, 2020 17:35
Likely left over from the Sapling audit.
@str4d
Copy link
Contributor Author

str4d commented Aug 21, 2020

Addressed all @daira's comments except the (x, y) vs (u, v) comments. I agree we should fix the conventions, but that should be an entirely internal change, and finding all the places that need fixing is out of scope for this PR. I've opened #275 for that.

@daira daira mentioned this pull request Aug 22, 2020
@str4d str4d merged commit 984d31d into zcash:master Aug 22, 2020
@str4d str4d deleted the migrate-to-bls12_381-jubjub-crates branch August 22, 2020 00:17
@daira
Copy link
Contributor

daira commented Aug 22, 2020

Post-hoc ACK of the last 5 commits. The (x, y) -> (u, v) renames (and some missing renames to cmu) were done in #276.

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.

Replace pairing::bls12_381 with the bls12_381 crate Replace zcash_primitives::jubjub with the jubjub crate
4 participants