-
Notifications
You must be signed in to change notification settings - Fork 106
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
Use prepared Groth16 keys for non-batch Sapling validation #3079
Conversation
ad10747
to
ec5757c
Compare
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.
This makes sense, the new values must be the same as the previous ones since they're computed the exact same way.
It would be good for @dconnolly to double check but it looks fine.
zebra/zebra-consensus/src/primitives/groth16/params.rs
Lines 72 to 81 in e5f5ac9
// Prepare verifying keys | |
let spend_prepared_verifying_key = groth16::prepare_verifying_key(&spend.vk); | |
let output_prepared_verifying_key = groth16::prepare_verifying_key(&output.vk); | |
Ok(Self { | |
spend, | |
spend_prepared_verifying_key, | |
output, | |
output_prepared_verifying_key, | |
}) |
ec5757c
to
4868349
Compare
CI failed with a listener conflict in a test with a race condition. If it happens again, we could:
We might also need to:
|
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.
LGTM!
I also reviewed the updated groth16/params changes in this context 💃
9d1853e
to
3ffb5c0
Compare
Oops, I auto-merged this into the wrong branch. I'll admin-merge it to |
Motivation
Zebra's Sapling proof validation prepares the validation key every time we do non-batch validation. But the parameter loader also creates a prepared copy of the key.
I think we can verify slightly faster if we re-use the prepared key.
This is related to #322, but it's not required.
Solution
This PR is based on PR #3091 to avoid unrelated test failures.
Review
I need a cryptographer to review this change.
@conradoplg if you're not sure, let's wait and ask Deirdre next week?
Reviewer Checklist