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

ZIP 212: validate Sapling and Orchard output of coinbase transactions #3029

Merged
merged 11 commits into from
Nov 11, 2021
Merged

ZIP 212: validate Sapling and Orchard output of coinbase transactions #3029

merged 11 commits into from
Nov 11, 2021

Conversation

conradoplg
Copy link
Collaborator

Motivation

Coinbase transactions with Sapling or Orchard outputs must be decryptable with an all-zero outgoing viewing key and this needs to be checked by Zebra.

Specifications

[Heartwood onward] All Sapling and Orchard outputs in coinbase transactions MUST decrypt to a note
plaintext , i.e. the procedure in § 4.19.3 ‘Decryption using a Full Viewing Key ( Sapling and Orchard )’ on p. 67
does not return ⊥, using a sequence of 32 zero bytes as the outgoing viewing key . (This implies that before
Canopy activation, Sapling outputs of a coinbase transaction MUST have note plaintext lead byte equal to
0x01 .)

[Canopy onward] Any Sapling or Orchard output of a coinbase transaction decrypted to a note plaintext
according to the preceding rule MUST have note plaintext lead byte equal to 0x02 . (This applies even during
the “grace period” specified in [ZIP-212].)

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

Designs

Solution

  • Use zcash_note_encryption from librustzcash to decrypt and validate notes
    • Transactions are re-encoded and decoded with librustzcash for that
  • Test with transactions from test blocks.
    • The Orchard part is not tested since there are no Orchard transactions in the blockchain yet, but I'll check if there are generated test vectors that can be used for it.

Closes #2362

Review

Probably @teor2345 and/or @dconnolly can review this.

Reviewer Checklist

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

Follow Up Work

#3027 for a consensus check that we can't currently do with zcash_note_encryption

@mpguerra mpguerra requested review from dconnolly and upbqdn November 8, 2021 08:10
teor2345
teor2345 reviewed Nov 8, 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.

Not a review, just some things I thought might be helpful.

zebra-consensus/src/transaction/tests.rs Show resolved Hide resolved
@dconnolly dconnolly mentioned this pull request Nov 8, 2021
Closed
@conradoplg conradoplg force-pushed the zip-212-validate-coinbase-outputs branch from 314b604 to 74eee3c Compare November 9, 2021 16:25
conradoplg
conradoplg commented Nov 9, 2021
View reviewed changes
Copy link
Collaborator Author

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

Added a Orchard test

zebra-consensus/src/block/check.rs Outdated Show resolved Hide resolved
oxarbitrage
oxarbitrage reviewed Nov 11, 2021
View reviewed changes
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some minor doc suggestions.

zebra-chain/src/primitives/zcash_note_encryption.rs Outdated Show resolved Hide resolved
zebra-chain/src/primitives/zcash_note_encryption.rs Show resolved Hide resolved
zebra-consensus/src/transaction/check.rs Outdated Show resolved Hide resolved
oxarbitrage
oxarbitrage reviewed Nov 11, 2021
View reviewed changes
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.

A minor variable name change suggestion.

zebra-chain/src/primitives/zcash_note_encryption.rs Outdated Show resolved Hide resolved
zebra-chain/src/primitives/zcash_note_encryption.rs Outdated Show resolved Hide resolved
conradoplg and others added 3 commits November 11, 2021 13:35
@conradoplg @oxarbitrage
Apply suggestions from code review
681e85a 
Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>
@conradoplg
Use height from loop
52c128a
@conradoplg
Merge branch 'main' of https://github.com/ZcashFoundation/zebra into …
ddacf25 
…zip-212-validate-coinbase-outputs
@oxarbitrage
Copy link
Contributor

Additional test info:
@conradoplg tried a manual sync up to tip in the testnet from a previous state and no issues reported.
conrado

dconnolly
dconnolly reviewed Nov 11, 2021
View reviewed changes
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.

Made some mostly rustdoc/comments suggested changes

zebra-consensus/src/block.rs Show resolved Hide resolved
zebra-consensus/src/transaction/check.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Show resolved Hide resolved
conradoplg and others added 2 commits November 11, 2021 16:04
@conradoplg @dconnolly
Apply suggestions from code review
de4b16e 
Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>
@conradoplg
Fix formatting
90bb73d
dconnolly
dconnolly approved these changes Nov 11, 2021
View reviewed changes
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.

LGTM!

@dconnolly dconnolly added this to the 2021 Sprint 22 milestone Nov 11, 2021
@dconnolly dconnolly enabled auto-merge (rebase) November 11, 2021 21:40
@dconnolly dconnolly disabled auto-merge November 11, 2021 21:40
@dconnolly dconnolly enabled auto-merge (squash) November 11, 2021 21:40
@dconnolly dconnolly merged commit 6570ebe into main Nov 11, 2021
@dconnolly dconnolly deleted the zip-212-validate-coinbase-outputs branch November 11, 2021 22:18
@upbqdn
Copy link
Member

upbqdn commented Nov 11, 2021

This PR looks good to me too. 👍

@mpguerra mpguerra removed this from the 2021 Sprint 22 milestone Nov 15, 2021
@jvff jvff mentioned this pull request Mar 16, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 left review comments

@oxarbitrage oxarbitrage oxarbitrage left review comments

@dconnolly dconnolly dconnolly approved these changes

@upbqdn upbqdn Awaiting requested review from upbqdn

Assignees

@conradoplg conradoplg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Part of ZIP 212: validate Sapling and Orchard output of coinbase transactions
6 participants
@conradoplg @oxarbitrage @upbqdn @dconnolly @teor2345 @mpguerra