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, impl trusted vector security, nullifier utility functions #1996

Merged
merged 12 commits into from
Apr 15, 2021

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Apr 10, 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

Design

  • 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

Code

  • Implement the design for sapling spends and outputs, to make sure it works
  • Implement transaction nullifier utility functions
  • Fix a bug in the overwinter flag in transaction v5 deserialization

Security

  • Implement trusted vector preallocation for spends and outputs

The code in this pull request has:

  • Documentation Comments
  • Property Tests

Review

Related Issues

Follow Up Work

Implement V5 shielded data serialization - #1829

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.

I think we need to go back to the design stage with transaction v5 serialisation, because the spends and outputs are split into multiple arrays during serialisation.

zebra-chain/src/transaction/serialize.rs Outdated Show resolved Hide resolved
zebra-chain/src/sapling/spend.rs Show resolved Hide resolved
teor2345 and others added 4 commits April 12, 2021 16:08
Implement serialization for V4 and V5 spends and outputs, to make sure
that the design works.
Also add a few missing v4 tests.
Redesign Transaction V5 serialization
@oxarbitrage oxarbitrage changed the title Include shielded data in V5 transaction Redesign Transaction V5 serialization Apr 12, 2021
@oxarbitrage oxarbitrage marked this pull request as ready for review April 12, 2021 19:21
@teor2345 teor2345 changed the title Redesign Transaction V5 serialization Redesign Transaction V5 serialization, add proptests, update nullifier utility functions Apr 12, 2021
@dconnolly dconnolly self-requested a review April 12, 2021 21:25
@teor2345 teor2345 changed the title Redesign Transaction V5 serialization, add proptests, update nullifier utility functions Redesign Transaction V5 serialization, impl trusted vector security, nullifier utility functions Apr 12, 2021
@@ -311,7 +325,7 @@ impl ZcashDeserialize for Transaction {
joinsplit_data,
})
}
(5, false) => {
(5, true) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch here by the way 👍

@teor2345 teor2345 dismissed their stale review April 12, 2021 23:04

I am happy with the current PR, waiting on @dconnolly to review

@teor2345 teor2345 added A-consensus Area: Consensus rule updates A-docs Area: Documentation A-rust Area: Updates to Rust code C-bug Category: This is a bug C-cleanup Category: This is a cleanup C-design Category: Software design work C-enhancement Category: This is an improvement C-security Category: Security issues labels Apr 12, 2021
@teor2345 teor2345 added I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data I-heavy Problems with excessive memory, disk, or CPU usage NU-5 Network Upgrade: NU5 specific tasks P-Medium labels Apr 12, 2021
@teor2345 teor2345 added this to the 2021 Sprint 7 milestone Apr 12, 2021
And add a missing serialized type.
Comment on lines 153 to 156
shared_anchor: AnchorV::Shared,
// The following fields are in a different order to the serialized data, see:
// https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus
first: Either<Spend<AnchorV>, Output>,
Copy link
Contributor

Choose a reason for hiding this comment

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

The anchor field is only present when nSpendsSapling > 0:

Suggested change
shared_anchor: AnchorV::Shared,
// The following fields are in a different order to the serialized data, see:
// https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus
first: Either<Spend<AnchorV>, Output>,
// The following fields are in a different order to the serialized data, see:
// https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus
first: Either<(Spend<AnchorV>, AnchorV::Shared), Output>,

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.

We've got 2 open PRs that depend on this PR, and we want to open 2 more, so we're just going to merge it.

@teor2345 teor2345 merged commit e42442d into ZcashFoundation:main Apr 15, 2021
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-docs Area: Documentation A-rust Area: Updates to Rust code C-bug Category: This is a bug C-cleanup Category: This is a cleanup C-design Category: Software design work C-enhancement Category: This is an improvement C-security Category: Security issues I-heavy Problems with excessive memory, disk, or CPU usage I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants