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

fix(verify): Only verify halo2 proofs once per transaction #4752

Merged
merged 3 commits into from
Jul 6, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 5, 2022

Motivation

There is only one aggregated halo2 proof per transaction, but Zebra is verifying the same proof for each action.

This uses a lot of RAM and CPU, and stops Zebra syncing to the tip.

Solution

Only verify the halo2 proof once per transaction.

Review

This bug is blocking almost all other work, it's urgent.

Reviewer Checklist

  • All Halo2 proofs and Orchard Actions are being verified
  • CI passes
  • Full sync passes

@teor2345 teor2345 added C-bug Category: This is a bug P-Critical 🚑 C-security Category: Security issues I-heavy Problems with excessive memory, disk, or CPU usage I-slow Problems with performance or responsiveness I-integration-fail Continuous integration fails, including build and test failures A-cryptography Area: Cryptography related labels Jul 5, 2022
@teor2345 teor2345 self-assigned this Jul 5, 2022
@teor2345 teor2345 requested a review from a team as a code owner July 5, 2022 21:21
@teor2345 teor2345 requested review from upbqdn and removed request for a team July 5, 2022 21:21
@teor2345
Copy link
Contributor Author

teor2345 commented Jul 5, 2022

I'm running a full sync here:
https://github.com/ZcashFoundation/zebra/actions/runs/2618944705

It should finish in around 6 hours.

oxarbitrage
oxarbitrage previously approved these changes Jul 5, 2022
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Definitely good, no need to do this inside the loop. Nice find.

@oxarbitrage
Copy link
Contributor

It should finish in around 6 hours.

It seems this should be merged anyway even if it does not make the difference in time right now, it is a performance fix.

Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

That's a great find!

zebra-consensus/src/transaction.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #4752 (63e0877) into main (42ef884) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #4752      +/-   ##
==========================================
- Coverage   78.87%   78.86%   -0.02%     
==========================================
  Files         306      306              
  Lines       37557    37545      -12     
==========================================
- Hits        29624    29608      -16     
- Misses       7933     7937       +4     

upbqdn
upbqdn previously approved these changes Jul 5, 2022
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

All looks good.

dconnolly
dconnolly previously approved these changes Jul 5, 2022
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

Double-checked the spec and the existing Halo2 verifier service, looks good!

When we push a new halo2::Item to the batch verifier, it produces orchard::circuit::Instances from each Action in the transacton, and then the whole set of Instances is passed as input to verify() a single Halo2 aggregate proof, so this change is correct.

@gustavovalverde
Copy link
Member

Admin merged, as the full sync got stuck and we need the fix anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cryptography Area: Cryptography related C-bug Category: This is a bug C-security Category: Security issues I-heavy Problems with excessive memory, disk, or CPU usage I-integration-fail Continuous integration fails, including build and test failures I-slow Problems with performance or responsiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants