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

Redesign Transaction V5 serialization #88

Merged
merged 3 commits into from
Apr 12, 2021

Conversation

teor2345
Copy link

@teor2345 teor2345 commented Apr 12, 2021

Motivation

It's easy to accidentally serialize V5 transaction fields in the wrong order. So we should go back to the design stage for V5 transaction serialization.

Solution

  • Update the v5 transaction RFC with an explicit design for serialization
    • Only have serialize and deserialize impls on types that are serialized as continuous bytes in V4 and V5 transactions
    • To avoid accidental serialization of Outputs in the V4 format in V5 transactions, add a wrapper type, and move the serialization impls to that type
    • List the types that have serialization impls, noting new impls
    • Explain how to serialize types that don't have serialization impls, by splitting those types up into their parts
  • Implement the design for sapling spends and outputs, to make sure it works
  • Security: Implement trusted vector preallocation for spends and outputs

The code in this pull request has:

  • Documentation Comments
  • Property Tests

Review

@oxarbitrage should review, because it blocks ZcashFoundation#1996

@dconnolly might also want to have a look, because it's a redesign.

Follow Up Work

Add the serialization impls to the implementation section of ZcashFoundation#1829 and ZcashFoundation#1979 (done!)

@teor2345 teor2345 force-pushed the issue1829_2 branch 2 times, most recently from 3fd6944 to 3346bbc Compare April 12, 2021 05:49
Implement serialization for V4 and V5 spends and outputs, to make sure
that the design works.
Also add a few missing v4 tests.
Copy link
Owner

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

I think in general it looks good, i am going to merge here and we can discuss further in the zebra PR.

@oxarbitrage oxarbitrage merged commit 00f2222 into oxarbitrage:issue1829_2 Apr 12, 2021
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.

2 participants