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

RedPallas async verifier service #2318

Merged
merged 7 commits into from
Jul 2, 2021
Merged

RedPallas async verifier service #2318

merged 7 commits into from
Jul 2, 2021

Conversation

dconnolly
Copy link
Contributor

@dconnolly dconnolly commented Jun 15, 2021

Motivation

We want to match the async processing patterns that we've been using for Sapling verification, including validating SpendAuth and Binding signatures via an async batch verifier Tower service.

Solution

This PR provides the Tower service that validates RedPallas signatures, and tests.

It depends on the changes in #2288.

This is an extremely boilerplate version of the RedJubjub verifier service. We have spitballed creating a derive-able proc macro for these batch verifier services to cut down on the boilerplate, but that may be out of scope for this issue.

This PR does not actually invoke RedPallas verification in the TransactionVerifier service for V5 transcations yet, so it is technically not doing the validation of any consensus rules yet.

Resolves #2287

Review

@teor2345 @jvff @conradoplg @oxarbitrage 🙏

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

Actually invoke the service as part of transaction verification: #2317

Reduce boilerplate:
#1951

@dconnolly dconnolly added the A-rust Area: Updates to Rust code label Jun 15, 2021
@dconnolly dconnolly added this to the 2021 Sprint 12 milestone Jun 15, 2021
@dconnolly dconnolly force-pushed the redpallas-verifier branch from 63c4476 to 03e4252 Compare June 15, 2021 22:50
@teor2345 teor2345 changed the base branch from main to redpallas-vartime-multiscalar June 15, 2021 23:05
@dconnolly dconnolly force-pushed the redpallas-vartime-multiscalar branch from 2ef1525 to ca4de96 Compare June 16, 2021 00:33
@dconnolly dconnolly force-pushed the redpallas-verifier branch from 03e4252 to d30a304 Compare June 16, 2021 00:34
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This looks good!

I still want to to a diff between the redjubjub and redpallas verifier and tests, to see if there are any important differences.

zebra-consensus/src/error.rs Outdated Show resolved Hide resolved
zebra-consensus/src/error.rs Show resolved Hide resolved
zebra-consensus/src/primitives/redpallas/tests.rs Outdated Show resolved Hide resolved
zebra-consensus/src/primitives/redpallas/tests.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

There are no significant differences between the RedJubjub and RedPallas batch verifier and test code.

@mpguerra mpguerra removed this from the 2021 Sprint 12 milestone Jun 17, 2021
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

I'm not yet familiar with async code to provide any specific feedback (and I couldn't find any minor issues) but from I could gather it looks good! It was also very useful to get more familiar with it.

I'll leave approval to @teor2345 but I can help reviewing any improvements if needed.

jvff
jvff previously approved these changes Jun 22, 2021
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Looks, great! I did experiment with a few suggestions, but they're all optional ideas 😄

zebra-consensus/src/error.rs Show resolved Hide resolved
zebra-consensus/src/primitives/redpallas.rs Outdated Show resolved Hide resolved
zebra-consensus/src/primitives/redpallas/tests.rs Outdated Show resolved Hide resolved
Base automatically changed from redpallas-vartime-multiscalar to main June 30, 2021 20:26
@dconnolly dconnolly force-pushed the redpallas-verifier branch from 735dbdd to 4571776 Compare July 2, 2021 03:32
@dconnolly dconnolly requested review from teor2345, jvff and conradoplg July 2, 2021 03:35
@dconnolly dconnolly added this to the 2021 Sprint 12 milestone Jul 2, 2021
@dconnolly dconnolly enabled auto-merge (squash) July 2, 2021 03:36
@dconnolly dconnolly disabled auto-merge July 2, 2021 06:49
@dconnolly dconnolly merged commit ff29978 into main Jul 2, 2021
@dconnolly dconnolly deleted the redpallas-verifier branch July 2, 2021 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement RedPallasVerifier async tower service
5 participants