-
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
Ed25519 async batch verification for JoinSplit signatures #1952
Conversation
cf193a5
to
626462c
Compare
We've been verifying JoinSplitSigs one-by-one pre-ZIP-215. Now as we're post-ZIP-215, we can take advantage of the batch math to validate this signatures. I would have pumped all the joinsplits in our MAINNET_BLOCKS test vectors but these signatures are over the sighash, which needs the NU code to compute, and once we're doing all that set up, we're basically doing transaction validation, so. Resolves #1944
626462c
to
ce81c01
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.
I am not an expert on async code but i think this looks good.
I found a few minors that we can fix before merging:
- remove some not needed code and commented code.
- add "ed25519" string to some tracing messages that are too generic and can be confused with other similar ones.
Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>
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.
Looks great!
This PR could merge as-is, but let's do some tweaks and open some follow-ups first.
I'm testing some full mainnet and testnet syncs locally with this branch and #1950. I don't want to block this PR on those tests, it's not a high risk PR. |
Seems to work fine after 2 full mainnet syncs and 2 full testnet syncs, and after syncing to tip. |
Seeing 2 mainnet instances at around 180% CPU, spread across all cores. (The CPU might even be a bit elevated due to all the "Invalid binding signature" logs.) |
Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>
Co-authored-by: teor <teor@riseup.net>
Co-authored-by: teor <teor@riseup.net>
Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>
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.
Looks great, let's do more batching!
I'm going to admin-merge because @oxarbitrage's changes were addressed. |
Motivation
We've been verifying JoinSplitSigs one-by-one pre-ZIP-215. Now as we're post-ZIP-215,
we can take advantage of the batch math to validate this signatures.
Solution
This adds another
tower::Service
that allows async batch validation of Ed25519 signatures. This is very similar boilerplate toredjubjub::Verifier
and similar tests.I removed the single check and replaced its invocation with a call to this communal
ed25519::Verifier
instance.I would have pumped all the joinsplits in our MAINNET_BLOCKS test vectors but these
signatures are over the sighash, which needs the NU code to compute, and once we're
doing all that set up, we're basically doing transaction validation, so.
The code in this pull request has:
Review
Not urgent, a quick chunk of work we've recently been unblocked on doing.
Related Issues
Resolves #1944
Follow Up Work
This is the third async batch verifier
tower::Service
, and won't be the last (Orchard will bringhalo2::Verifier
andredpallas::Verifier
eventually). There is a lot of boilerplate being copy-pasted around, which may be ok for easy deletion, but when it's duplicated amongst ~5 variants, it's a signal to revisit where we can DRY this up or make a more generic async batch Verifier, which can then be instantiated with differentItem
s/ItemWrapper
s and somecall
method/function. Tracked in #1951.These batch verifiers are returning their own errors from their underlying batchers, we should figure out the best way to turn those into
TransactionError
s, either inzebra-consensus::transaction
or in the async service layer.