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

ZIP-211: Validate Disabling Addition of New Value to the Sprout Value Pool #2399

Merged
merged 10 commits into from
Jul 1, 2021

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We need to add the disabled sprout pool consensus rule. Resolves #1564 when merged.

Specifications

https://zips.z.cash/zip-0211
https://zips.z.cash/protocol/protocol.pdf#joinsplitdesc

Solution

Implement the rule, add a test. The test can be improved.

Review

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

@teor2345 teor2345 changed the title Add disabled sprout pool check ZIP-211: Validate Disabling Addition of New Value to the Sprout Value Pool Jun 28, 2021
@teor2345
Copy link
Contributor

I updated the title of this PR so it says "Disabling Addition of New Value to the Sprout Value Pool".

It is still possible to remove value from the sprout value pool.

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 there's a bit of confusion in this PR about what ZIP-211 actually does.

Users can still remove value from the sprout pool. Only adding new value to the pool is disabled.

Screen Shot 2021-06-28 at 11 10 19

I also made some suggestions about code structure.

zebra-chain/src/transaction.rs Outdated Show resolved Hide resolved
zebra-chain/src/parameters/network_upgrade.rs Outdated Show resolved Hide resolved
zebra-consensus/src/error.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/check.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/check.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/check.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction.rs 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.

We still need to:

  • cover coinbase transactions in the tests, or skip them in the verifier
  • make sure we're testing transactions with and without JoinSplits
  • move the sprout check into the sprout verifier function

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.

  • cover coinbase transactions in the tests, or skip them in the verifier

  • make sure we're testing transactions with and without JoinSplits

This is done, just waiting on the refactor.

@teor2345 teor2345 dismissed their stale review July 1, 2021 01:01

Some requested changes have been made, @jvff is reviewing the rest

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 good enough for now.

@teor2345 teor2345 merged commit e4ab01d into ZcashFoundation:main Jul 1, 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.

ZIP-211: Disable Addition of New Value to the Sprout Value Pool
3 participants