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.

Sign up for GitHub

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

Jump to bottom

Verify orchard spend auth #2442

Merged
merged 1 commit into from
Jul 8, 2021
Merged

Verify orchard spend auth #2442

merged 1 commit into from
Jul 8, 2021

Conversation

oxarbitrage
Copy link
Contributor

Motivation

#2317

Specifications

https://zips.z.cash/protocol/protocol.pdf#actiondesc

Solution

Verify orchard spend auth very similar to sapling. No tests are currently included.

Review

This is just a draft and it is based on #2441 as the code was added into verify_orchard_shielded_data(). The relevant commit is 8cbe813

Reviewer Checklist

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

Follow Up Work

@mpguerra mpguerra linked an issue Jul 5, 2021 that may be closed by this pull request
Validate Orchard SpendAuth signatures for Transaction::V5 #2317
Closed
1 task
teor2345
teor2345 previously requested changes Jul 6, 2021
View reviewed changes
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.

Can you please split this PR from #2441?
#2441 is blocked on bug #1939, but this PR isn't.

We might also need to do some additional checks before we can validate Orchard spend authorizations. This is something we should check with the spec and a cryptographer.

@oxarbitrage
Copy link
Contributor Author

Can you please split this PR from #2441?

Force pushed to do this.

@oxarbitrage oxarbitrage marked this pull request as ready for review July 6, 2021 16:16
@teor2345 teor2345 dismissed their stale review July 7, 2021 00:37

The PR has been split.

@teor2345 teor2345 requested review from jvff and conradoplg July 7, 2021 00:38
@teor2345
Copy link
Contributor

teor2345 commented Jul 7, 2021

@conradoplg can you review the cryptography in this PR, and make sure we've covered all the Orchard Spend Auth consensus rules?

@jvff can you check that the new validation function fits the refactor you've just done?

jvff
jvff reviewed Jul 7, 2021
View reviewed changes
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 good to me, but I did not check the cryptography rules.

conradoplg
conradoplg approved these changes Jul 7, 2021
View reviewed changes
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.

Looks good!

@jvff jvff mentioned this pull request Jul 7, 2021
Closed
@conradoplg conradoplg merged commit 01ea87a into ZcashFoundation:main Jul 8, 2021
@conradoplg conradoplg mentioned this pull request Jul 8, 2021
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvff jvff jvff left review comments

@conradoplg conradoplg conradoplg approved these changes

@teor2345 teor2345 teor2345 left review comments

Assignees

@oxarbitrage oxarbitrage

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate Orchard SpendAuth signatures for Transaction::V5
4 participants
@oxarbitrage @teor2345 @conradoplg @jvff