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

WIP - Validate shielded coinbase #1190

Closed

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Oct 20, 2020

TODO

  • Replace Either with a custom type
  • Check we've implemented all the ZIP-213 rules
  • Add tests
  • Merge

Motivation

Shielded coinbase are available after Heartwood NU. Consensus rules for this are described at ZIP-213

Solution

Implement shielded_coinbase() function to the subsidy::general that makes rules validation.

Related Issues

.expect("heartwood activation height should be available");

if height < heartwood_height {
if !transaction.joinsplits() && transaction.shields().is_none() {
Copy link
Contributor

@dconnolly dconnolly Oct 24, 2020

Choose a reason for hiding this comment

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

We can answer this question without a shields() method, by destructuring the struct to get shielded_data, which is an Option(ShieldedData). If shielded_data == None, that means there are no Spend descriptions or Output descriptions. Some examples in https://github.com/ZcashFoundation/zebra/pull/1174/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should be destructuring inside subsidy. If we do it to get shielded_data we will have to do it for outputs() or joinsplits() for consistency. I might be missing something but i see the method as the best option here.

I did some cleaning to the method and renamed to shielded_data() in a4d2644

@@ -1,5 +1,7 @@
//! Transactions and transaction-related structures.

use futures::future::Either;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's replace Either<...> with a named and typed enum.

We could do something like:

enum JoinSplit<'_> {
    JoinSplitOnBctv14(&'_ JoinSplitData<Bctv14Proof>),
    JoinSplitOnGroth16(&'_ JoinSplitData<Groth16Proof>),
}

You'll need lifetimes here, the compiler should give you hints if I've got it wrong.

@teor2345
Copy link
Contributor

We decided to close this draft PR, and re-open it when we start working on ticket #608 again.

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.

4 participants