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

Update transaction::check::coinbase_tx_no_joinsplit_or_spend to validate V5 coinbase transactions with Orchard shielded data #2236

Merged

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Jun 1, 2021

Motivation

NU5 has the consensus rule that V5 coinbase transactions must not have the enableSpendsOrchard flag set.

Solution

Implement the consensus rule in the coinbase_tx_no_joinsplit_or_spend function, as described in #1980.

The code in this pull request has:

  • Documentation Comments
  • Unit Tests and Property Tests

Review

@teor2345 outlined the solution, and @oxarbitrage is working on similar Orchard issues.

Related Issues

Part of #1980.

Follow Up Work

  • What test vectors are needed?
  • What kind of property tests should be written?

@jvff jvff requested review from teor2345 and oxarbitrage June 1, 2021 20:53
@zfnd-bot zfnd-bot bot assigned jvff Jun 1, 2021
@jvff jvff changed the title Update coinbase tx no joinsplit or spend Update transaction::check::coinbase_tx_nojoinsplit_or_spend to validate V5 coinbase transactions with Orchard shielded data Jun 1, 2021
@jvff jvff changed the title Update transaction::check::coinbase_tx_nojoinsplit_or_spend to validate V5 coinbase transactions with Orchard shielded data Update transaction::check::coinbase_tx_no_joinsplit_or_spend to validate V5 coinbase transactions with Orchard shielded data Jun 1, 2021
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 great, just a suggestion for a code simplification

teor2345
teor2345 previously approved these changes Jun 3, 2021
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.

Actually let's just merge this, I'll submit the code simplification as a separate PR

jvff added 4 commits June 3, 2021 00:44
Allows accessing the Orchard shielded data if it is present in the
transaction, regardless of the transaction version.
Allows making the method more concise.
Used when the validation rule is not met.
The flag must not be set for the coinbase transaction.
@jvff jvff force-pushed the update-coinbase-tx-no-joinsplit-or-spend branch from 452ea30 to ffe09fa Compare June 3, 2021 00:55
Use the fact that `Option<T>` implements `Iterator<T>` to simplify the
code and remove the need for boxing the iterators.

Co-authored-by: teor <teor@riseup.net>
@jvff jvff force-pushed the update-coinbase-tx-no-joinsplit-or-spend branch from ffe09fa to bc29da7 Compare June 3, 2021 01:04
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, let's merge when the build passes

@teor2345 teor2345 enabled auto-merge (squash) June 3, 2021 01:10
@teor2345 teor2345 merged commit 9416b5d into ZcashFoundation:main Jun 3, 2021
@teor2345 teor2345 linked an issue Jun 6, 2021 that may be closed by this pull request
14 tasks
@jvff jvff deleted the update-coinbase-tx-no-joinsplit-or-spend branch June 10, 2021 13:18
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.

Implement transaction::check for Transaction Version 5 and Orchard
2 participants