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

Parse Sapling data in Transaction Version 5 #1829

Closed
25 tasks done
teor2345 opened this issue Mar 1, 2021 · 5 comments
Closed
25 tasks done

Parse Sapling data in Transaction Version 5 #1829

teor2345 opened this issue Mar 1, 2021 · 5 comments
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
Milestone

Comments

@teor2345
Copy link
Contributor

teor2345 commented Mar 1, 2021

Is your feature request related to a problem?

In #1824, we parse the transaction header and transparent data for transaction version 5.

Transaction version 5 also contains Sapling data, which we need to parse:

Describe the solution you'd like

Implementation

Spend

Implement serialization and deserialization for:

  • Spend<PerSpendAnchor> (moved from the pre-RFC Spend)
  • SpendPrefixInTransactionV5 (new)
  • redjubjub::Signature<redjubjub::SpendAuth> (new - for v5 spend auth sig arrays)

Test Arbitrary Spend (both variants):

  • round-trip serialization and deserialization
  • trusted vector deserialization

Output

Implement serialization and trusted vector deserialization for:

  • OutputInTransactionV4 (moved from Output)
  • OutputPrefixInTransactionV5 (new)

Test Arbitrary Output (both transaction versions):

  • round-trip serialization and deserialization
  • trusted vector deserialization

ShieldedData

Documentation

  • read the relevant consensus rules in the spec
  • read the relevant RFC section
  • summarise them in each struct and function's docs

Implement serialization and deserialization for:

TestArbitrary Option<sapling::ShieldedData<SharedAnchor>>

  • round-trip serialization and deserialization

Test manual implementations:

Transaction

  • Parse the Sapling section of V5 transactions using ShieldedData
  • return an empty iterator for transaction version 5 in sprout_nullifiers
  • implement transaction version 5 in sapling_nullifiers

TestArbitrary Transaction::V5

  • round-trip serialization and deserialization

Cleanups and Refactoring

Testing

Test round-trip serialization and deserialization for transaction v4 and v5 (without orchard)

Unit tests

  • an empty transaction v5, with no Sapling or Transparent data
    • empty transaction are invalid, but Zebra only checks this rule in zebra_consensus::transaction::Verifier
  • v5 with empty sapling spends, some outputs
    • covers the shared anchor serialization condition
  • an empty transaction v4, with no Sapling, Sprout, or Transparent data
    • empty transaction are invalid, but Zebra only checks this rule in zebra_consensus::transaction::Verifier

Test vectors

  • "Fake" Sapling-only and Sapling/Transparent v5 transactions based on zebra_test::vectors::block::BLOCKS

    • provides Sapling only, Transparent only, and Sapling/Transparent test vectors
    • we can write a test utility function to "convert" transactions from V4 to V5 format
    • some transactions won't convert because they have multiple sapling anchors
  • work out why these tests fail on blocks, but not transactions (fake_v5_round_trip test fails on blocks but not in transactions #2023)

  • transaction v5 sapling-only test vectors from zcashd (or created by us and shared with zcashd)

Describe alternatives you've considered

This is a required change for NU5.

Follow Up Tasks

"Fake" Sapling-only and Sapling/Transparent v5 transactions based on zebra_test::vectors::block::BLOCKS

Test vectors

@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 Mar 1, 2021
@teor2345 teor2345 mentioned this issue Mar 1, 2021
2 tasks
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Mar 4, 2021
@mpguerra mpguerra added this to the 2021 Sprint 5 milestone Mar 4, 2021
@mpguerra mpguerra modified the milestones: 2021 Sprint 5, 2021 Sprint 4 Mar 8, 2021
@oxarbitrage
Copy link
Contributor

Blocked on #1863

@oxarbitrage oxarbitrage added the S-blocked Status: Blocked on other tasks label Mar 9, 2021
@mpguerra
Copy link
Contributor

I just wanted to highlight this item from the spec which may be relevant here also:

For a v5 transaction, vbalanceSapling is implicitly zero if the transaction has no Spend descriptions
or Output descriptions.

@mpguerra mpguerra mentioned this issue Mar 16, 2021
53 tasks
@teor2345
Copy link
Contributor Author

I just wanted to highlight this item from the spec which may be relevant here also:

For a v5 transaction, vbalanceSapling is implicitly zero if the transaction has no Spend descriptions
or Output descriptions.

Thanks Pili, that was a bug in the draft PR, I've added a comment at https://github.com/ZcashFoundation/zebra/pull/1860/files#r595643213

@teor2345
Copy link
Contributor Author

I just wanted to highlight this item from the spec which may be relevant here also:

For a v5 transaction, vbalanceSapling is implicitly zero if the transaction has no Spend descriptions
or Output descriptions.

Thanks Pili, that was a bug in the draft PR, I've added a comment at https://github.com/ZcashFoundation/zebra/pull/1860/files#r595643213

These was actually also a missing related Orchard spec rule as well:
Screen Shot 2021-03-18 at 08 41 45

I've opened #1917 to implement those rules, and I'll also update the transaction RFC.

@teor2345
Copy link
Contributor Author

I opened #2023 and #2047 for follow-up, so we can close this ticket now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants