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

V5 transaction rfc #1886

Merged
merged 36 commits into from
Mar 25, 2021
Merged

V5 transaction rfc #1886

merged 36 commits into from
Mar 25, 2021

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Mar 11, 2021

Summary

Please copy the RFC summary over here.

Propose a design for V5 transactions. Open for discussion.

More information

Feature Name: my_feature

Start Date: YYYY-MM-DD

Design PR: ZcashFoundation/zebra#0000

Zebra Issue: ZcashFoundation/zebra#1863

Document

Rendered.

Zebra Team Approval

Everyone on the Zebra team should review design RFCs:

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.

Looks good for a first draft, I have a bunch of suggestions.

book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
}
```

Where `Action` is defined as [Action definition](https://github.com/ZcashFoundation/zebra/blob/68c12d045b63ed49dd1963dd2dc22eb991f3998c/zebra-chain/src/orchard/action.rs#L18-L41).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Link need to change after #1885 merges.

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 made a bunch of suggestions to complete the RFC, and also found a few bugs.

Let's get @dconnolly to check this, and then I think we're done.

book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved
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'm good with this RFC, I just need some cryptography API design advice from @dconnolly.

@teor2345 teor2345 marked this pull request as ready for review March 15, 2021 12:52
@mpguerra mpguerra requested a review from dconnolly March 16, 2021 09:28
@mpguerra

This comment has been minimized.

Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

So one thing we can do is instead of naming the structs or types as Sapling or V4/V5, we can namespace them in the module tree, so we could havezebra_chain::transaction::v5::ShieldedData We could represent ShieldedData as enum variants, alá Transaction::V4/V5, and it would use things like zebra_chain::orchard::Action.

I don't think we need OutputV4 and OutputV5, we can use the same sapling::Output type in both ShieldedData::V4 and ShieldedData::V5.

The Spends having the anchor moving around looks like yes we need a different one for v5. I wonder if we can have a sapling::Spend as an enum instead of a different type, similar to how we have Transaction as an enum with different variants, so we can have sapling::Spend::V4/V5.

@oxarbitrage @yaahc @teor2345 what do you think? Basically consolidates @oxarbitrage's design into enum variants vs separate types

@dconnolly dconnolly added the C-design Category: Software design work label Mar 16, 2021
@teor2345
Copy link
Contributor

@oxarbitrage @yaahc @teor2345 what do you think? Basically consolidates @oxarbitrage's design into enum variants vs separate types

I think enum variants are a good design. We can replace the trait part of the old design with methods on the enum.

@teor2345

This comment has been minimized.

@teor2345 teor2345 requested a review from dconnolly March 21, 2021 22:11
teor2345
teor2345 previously approved these changes Mar 21, 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 is fine with me

Copy link
Contributor Author

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

Looks great.

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.

Design alternative: distinguish shared and per spend anchors

book/src/dev/rfcs/0010-v5-transaction.md Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Show resolved Hide resolved
book/src/dev/rfcs/0010-v5-transaction.md Show resolved Hide resolved
@dconnolly
Copy link
Contributor

Design alternative: distinguish shared and per spend anchors

Since ShieldedData basically maps to transaction fields as described in the spec, I don't mind keeping them as anchors in our data structure

@dconnolly dconnolly requested a review from teor2345 March 24, 2021 17:25
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.

There are no blocking changes left, and we've all approved the RFC. Merging.

@teor2345 teor2345 merged commit 32beef2 into ZcashFoundation:main Mar 25, 2021
@teor2345 teor2345 added this to the 2021 Sprint 5 milestone Mar 25, 2021
@teor2345 teor2345 linked an issue Mar 25, 2021 that may be closed by this pull request
@teor2345 teor2345 mentioned this pull request Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-design Category: Software design work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Design: Transaction v5
5 participants