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

Add orchard binding_verification_key #2441

Merged
merged 12 commits into from
Aug 16, 2021

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Jul 3, 2021

Motivation

Binding verification key must be implemented.

Closes #2102
Closes #2103

Specifications

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

Solution

Implement simular to sapling. This is a draft.

Review

I will like @dconnolly to take an initial view. I made some comments where i have some doubts.

Reviewer Checklist

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

Follow Up Work


This change is Reviewable

@oxarbitrage oxarbitrage mentioned this pull request Jul 4, 2021
3 tasks
@mpguerra mpguerra linked an issue Jul 5, 2021 that may be closed by this pull request
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.

It looks like we need to do two things before we can make progress on this PR:

  1. Fix the sapling value balance errors in Fix sapling binding signature errors #1939
  2. Work with a cryptographer to understand the Orchard value balance consensus rules, and how they are different from Sapling

@teor2345

This comment has been minimized.

@teor2345

This comment has been minimized.

@dconnolly
Copy link
Contributor

We need to merge PR #2472 before merging this PR, so we're sure that the common Sapling/Orchard binding signature code works.

#2472 and #2463 have been merged so we can move forward with this PR i think

@teor2345
Copy link
Contributor

We need to merge PR #2472 before merging this PR, so we're sure that the common Sapling/Orchard binding signature code works.

#2472 and #2463 have been merged so we can move forward with this PR i think

Also our cached state tests actually test this code now!

@dconnolly dconnolly marked this pull request as ready for review July 21, 2021 04:30
@dconnolly
Copy link
Contributor

@oxarbitrage I moved this from draft to not-draft if that's ok with you

@teor2345
Copy link
Contributor

@oxarbitrage I moved this from draft to not-draft if that's ok with you

Can we focus on state changes for this sprint?

We can do this validation next sprint, or delay it until we have time.
But we need a bunch of the state changes sorted to move forward with the mempool.

@dconnolly
Copy link
Contributor

@oxarbitrage I moved this from draft to not-draft if that's ok with you

Can we focus on state changes for this sprint?

We can do this validation next sprint, or delay it until we have time.
But we need a bunch of the state changes sorted to move forward with the mempool.

Oh sure yes 👍

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.

Code looks good, some suggestions on the doc comments 📜

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.

Code looks good, some suggestions on the doc comments 📜

@mpguerra
Copy link
Contributor

Does this no longer close #2103 also?

Co-authored-by: Deirdre Connolly <durumcrustulum@gmail.com>
@oxarbitrage
Copy link
Contributor Author

Does this no longer close #2103 also?

Thank you for the question. I removed more code than i should in one of the rebases, the validation code was here so i will readd. With this addition this PR will close both tickets as initially intended.

@oxarbitrage oxarbitrage requested a review from dconnolly August 16, 2021 15:11
@oxarbitrage oxarbitrage dismissed teor2345’s stale review August 16, 2021 15:14

was not requested

Copy link
Contributor Author

@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.

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @dconnolly and @teor2345)


zebra-chain/src/orchard/shielded_data.rs, line 61 at r4 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…
    /// Getting the binding signature validating key from the Action description 
    /// value commitments and the balancing value implicitly checks that the 
    /// balancing value is consistent with the value transferred in the
    /// Action descriptions, but also proves that the signer knew the
    /// randomness used for the Action value commitments, which
    /// prevents replays of Action descriptions that perform an output.
    /// In Orchard, all Action descriptions have a spend authorization signature,
    /// therefore the proof of knowledge of the value commitment randomness
    /// is less important, but stills provides defense in depth, and reduces the 
    /// differences between Orchard and Sapling.

Done.


zebra-chain/src/orchard/shielded_data.rs, line 65 at r4 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…
    /// The net value of Orchard spends minus outputs in a transaction
    /// is called the balancing value, measured in zatoshi as a signed integer
    /// cv_balance.

Done.


zebra-chain/src/orchard/shielded_data.rs, line 69 at r4 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…
    /// Consistency of cv_balance with the value commitments in Action 
    /// descriptions is enforced by the binding signature.

Done.


zebra-chain/src/orchard/shielded_data.rs, line 73 at r4 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…
    /// Instead of generating a key pair at random, we generate it as a function
    /// of the value commitments in the Action descriptions of the transaction, and 
    /// the balancing value.

Done.


zebra-chain/src/orchard/shielded_data.rs, line 82 at r4 (raw file):

Previously, dconnolly (Deirdre Connolly) wrote…

Looks good 👍

Done.


zebra-consensus/src/transaction.rs, line 544 at r1 (raw file):

Previously, teor2345 (teor) wrote…

Please add TODO comments about the missing Orchard checks, with a ticket number for each check.

Done.

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.

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate bindingSigOrchard for Transaction::V5 Implement Orchard binding verification key derivation
4 participants