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

Implement transaction::check for Transaction Version 5 and Orchard #1980

Closed
4 of 14 tasks
teor2345 opened this issue Apr 6, 2021 · 4 comments · Fixed by #2229, #2234, #2236 or #2246
Closed
4 of 14 tasks

Implement transaction::check for Transaction Version 5 and Orchard #1980

teor2345 opened this issue Apr 6, 2021 · 4 comments · Fixed by #2229, #2234, #2236 or #2246
Assignees
Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks

Comments

@teor2345
Copy link
Contributor

teor2345 commented Apr 6, 2021

Is your feature request related to a problem? Please describe.

After we parse Sapling in #1829 and Orchard in #1979, we need to implement and test the transaction checks for transaction version 5.

Describe the solution you'd like

These checks depend on some of the Orchard action functions in #2185, we're splitting them out into a separate PR:
https://github.com/ZcashFoundation/zebra/pull/2185/files#r638307833

has_inputs_and_outputs

  • copy the orchard consensus rules into the function docs - https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus
  • implement for transaction version 5:
    • add an orchard_actions method to Transaction, structured like sapling_nullifiers, but using Actions from orchard_shielded_data.actions()
    • count the number of actions in has_inputs_and_outputs
    • add the number of actions to the total number of inputs, and the total number of outputs (they can contain either or both)
  • add Orchard tests (see details below)

shielded_balances_match

  • document that the value_balance field in sapling::ShieldedData is only present when there is at least one spend or output
    • copy the consensus rule here?
  • during V4 sapling deserialization, return an error if ShieldedData is None, and the value balance is non-zero
    • copy the consensus rule here?
  • during V4 sapling serialization, write a zero value balance if there is no ShieldedData
  • delete the shielded_balances_match function
  • create a byte test vector with a non-zero value_balance, but no spends or outputs

coinbase_tx_no_joinsplit_or_spend

  • copy the orchard consensus rules into the function docs - https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus
  • implement for transaction version 5:
    • create a new error variant CoinbaseHasEnableSpendsOrchard
    • if there is an orchard_shielded_data, and orchard_shielded_data.flags has ENABLE_SPENDS, return the error
    • feel free to add an orchard_shielded_data or orchard_shielded_data_flags method on Transaction
  • add Orchard tests (see details below)

Each function should have tests similar to #1979:

  • Transaction v5 test vectors from zcashd (or test vectors that we create and share with zcashd)
    • Orchard-only, Orchard/Sapling, Orchard/Transparent, and Orchard/Sapling/Transparent test vectors
  • Arbitrary transaction v5
  • an empty transaction v5, with no Orchard, Sapling, or Transparent data

We could create fake v5 transactions from our test vectors, but they are a lower priority, because they won't improve test coverage very much. (We want to focus on Orchard.)

We will verify the Orchard binding signature in a separate ticket:
#2103

Describe alternatives you've considered

These consensus rules are required for NU5.

@teor2345 teor2345 added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage NU-5 Network Upgrade: NU5 specific tasks P-Medium labels Apr 6, 2021
@teor2345 teor2345 added this to the 2021 Sprint 8 milestone Apr 6, 2021
@mpguerra mpguerra mentioned this issue Apr 7, 2021
53 tasks
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Apr 13, 2021
@teor2345 teor2345 changed the title Implement transaction::check for Transaction Version 5 Implement transaction::check for Transaction Version 5 and Orchard Apr 15, 2021
@mpguerra mpguerra linked a pull request Jun 1, 2021 that will close this issue
4 tasks
@teor2345 teor2345 reopened this Jun 2, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Jun 2, 2021

PR #2229 only implemented part of this ticket.

@mpguerra
Copy link
Contributor

mpguerra commented Jun 2, 2021

Oh, I didn't realise github was going to close it when #2229 merged, I was just adding the related PRs

@teor2345
Copy link
Contributor Author

teor2345 commented Jun 2, 2021

Oh, I didn't realise github was going to close it when #2229 merged, I was just adding the related PRs

Yeah it's weird, sometimes it closes them, sometimes it just leaves them alone.

I think maybe one is GitHub and the other is ZenHub?

Anyway, no big deal, we have to double-check things anyway :-)

@teor2345
Copy link
Contributor Author

teor2345 commented Jun 7, 2021

All 4 PRs for this ticket are merged, or set to auto-merge after minor fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment