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

zcash_primitives: Replace sapling::redjubjub with redjubjub crate #1056

Merged
merged 3 commits into from Dec 4, 2023

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Nov 30, 2023

As a side-effect, we remove the ability to verify individual transactions with pre-ZIP 216 rules (which we already removed from zcashd consensus nodes in zcash/zcash#6000 and zcash/zcash#6399, as all pre-ZIP 216 transactions on mainnet and testnet are also valid under ZIP 216).

As a side-effect, we remove the ability to verify individual
transactions with pre-ZIP 216 rules (which we already removed from
`zcashd` consensus nodes in zcash/zcash#6000 and zcash/zcash#6399, as
all pre-ZIP 216 transactions on mainnet are also valid under ZIP 216).
Comment on lines -755 to +745
binding_signature: auth.sigs.bsk.sign(
&sighash,
&mut rng,
VALUE_COMMITMENT_RANDOMNESS_GENERATOR,
),
binding_signature: auth.sigs.bsk.sign(&mut rng, &sighash),
Copy link
Contributor Author

@str4d str4d Nov 30, 2023

Choose a reason for hiding this comment

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

There was a bug here previously in that this didn't include key prefixing, because we never implemented that inside our redjubjub module (as the original RedDSA draft spec lacked it). I should have used the equivalent of the sapling::spend_sig method here when I added this in #1023 (not that exact method because it also handled re-randomization), but as we haven't cut a release including this API yet, the bug is not in deployed code. The redjubjub crate includes key prefixing.

@str4d str4d requested review from nuttycom and daira November 30, 2023 18:02
@str4d str4d added this to the Extract sapling-crypto milestone Nov 30, 2023
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (764127f) 70.62% compared to head (ded09f9) 70.79%.

Files Patch % Lines
zcash_primitives/src/sapling/verifier/batch.rs 0.00% 7 Missing ⚠️
zcash_primitives/src/sapling/builder.rs 64.28% 5 Missing ⚠️
zcash_primitives/src/sapling/keys.rs 93.93% 4 Missing ⚠️
zcash_primitives/src/sapling/verifier.rs 0.00% 4 Missing ⚠️
zcash_primitives/src/sapling/verifier/single.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1056      +/-   ##
==========================================
+ Coverage   70.62%   70.79%   +0.16%     
==========================================
  Files         140      138       -2     
  Lines       13786    13698      -88     
==========================================
- Hits         9737     9698      -39     
+ Misses       4049     4000      -49     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nuttycom
nuttycom previously approved these changes Nov 30, 2023
Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK de1ed21

zcash_primitives/src/transaction/builder.rs Outdated Show resolved Hide resolved
The Sapling key components specification places more constraints on the
values of `ask` and `ak` than general RedJubjub signing and verification
keys.
@str4d
Copy link
Contributor Author

str4d commented Dec 1, 2023

There's a somewhat concerning test vector failure in zcashd (zcash/zcash#6784 (review)) that I need to debug. EDIT: Nope, it was just a bug in the test vectors.

Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK ded09f9

#[derive(Clone, Debug)]
pub struct SpendValidatingKey(redjubjub::VerificationKey<SpendAuth>);

impl From<&SpendAuthorizingKey> for SpendValidatingKey {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change this to an explicitly named method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #1056 .

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

@daira daira merged commit acf1462 into main Dec 4, 2023
31 checks passed
@daira daira deleted the 581-use-redjubjub-crate branch December 4, 2023 18:27
@str4d str4d added the C-tech-debt Category: Technical debt that needs to be paid off label Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tech-debt Category: Technical debt that needs to be paid off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants