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

Activate turnstile on mainnet #3968

Merged
merged 2 commits into from Apr 29, 2019

Conversation

@bitcartel
Copy link
Contributor

commented Apr 24, 2019

This PR enables ZIP209 support on mainnet, to mark blocks as invalid if they would lead to a turnstile violation in the Sprout or Sapling value pools.

To test this PR, I performed the following manual tests:

  1. Used RPC call getblock to verify the fallback Sprout value.
  2. Individually changed the fallback Sprout block hash, block height and chain value, recompiling and relaunching the node, verifying that each individual change resulted in an error. When the block hash and block height are incorrect, an error is logged to debug.log FallbackSproutValuePoolBalance(): fallback block hash is incorrect. An incorrect chain value results in node termination with error: void FallbackSproutValuePoolBalance(CBlockIndex*, const CChainParams&): Assertion '*pindex->nChainSproutValue == chainparams.SproutValuePoolCheckpointBalance()' failed.
  3. Ran the Smoke Testing described in PR #3885, on mainnet.
  4. Launched zcashd with experimental feature -developersetpoolsizezero to manually trigger a turnstile violation in both Sprout and Sapling shielded pools. The Sprout turnstile violation occurred after launch, due to chance, when the next incoming block 520786 contained a Sprout unshielding transaction. The Sapling turnstile violation was triggered after creating a Sapling unshielding transaction.

@bitcartel bitcartel added this to the v2.0.5 milestone Apr 24, 2019

@bitcartel bitcartel requested review from daira, ebfull and str4d Apr 24, 2019

@bitcartel bitcartel self-assigned this Apr 24, 2019

@bitcartel bitcartel added this to Needs Prioritization in Arborist Team via automation Apr 24, 2019

@bitcartel bitcartel moved this from Needs Prioritization to In Review in Arborist Team Apr 24, 2019

@LarryRuane LarryRuane self-requested a review Apr 24, 2019

@LarryRuane
Copy link
Contributor

left a comment

I did run most of the manual tests, except the smoke test because that required reindexing (I didn't want to wait that long). It all looks good to me!

nSproutValuePoolCheckpointHeight = 520633;
nSproutValuePoolCheckpointBalance = 22145062442933;
fZIP209Enabled = true;
hashSproutValuePoolCheckpointBlock = uint256S("0000000000c7b46b6bc04b4cbf87d8bb08722aebd51232619b214f7273f8460e");

This comment has been minimized.

Copy link
@LarryRuane

LarryRuane Apr 24, 2019

Contributor

Why do we not need similar for Sapling?

This comment has been minimized.

Copy link
@bitcartel

bitcartel Apr 24, 2019

Author Contributor

By the time Sapling activated, the non-deprecated releases of Zcashd at that time were tracking and recording value deltas in the block index. For the lifetime of Sprout, this wasn't the case. See point 1 here: #3885

@ebfull

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

ACK!

I tested this a little differently than @bitcartel; I didn't use -developersetpoolsizezero but instead modified the wallet serialization logic to mimic an older node. As expected, the correct balance of the value pool was established near the end of the reindex. It was also consistent with the balance shown when the block index was reloaded.

@ebfull

ebfull approved these changes Apr 27, 2019

@daira

daira approved these changes Apr 27, 2019

Copy link
Contributor

left a comment

utACK. Code review only; I have not tested the functionality nor verified the Sprout value pool checkpoint.

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

@daira The checkpoint details can be verified with zcash-cli getblock 520633.

@bitcartel bitcartel removed the request for review from str4d Apr 29, 2019

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

3 ACKs received: @zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2019

📌 Commit 867786d has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2019

⌛️ Testing commit 867786d with merge 63fff63...

zkbot added a commit that referenced this pull request Apr 29, 2019

Auto merge of #3968 - bitcartel:3768_enable_mainnet_turnstile, r=bitc…
…artel

Activate turnstile on mainnet

This PR enables [ZIP209](https://github.com/zcash/zips/blob/master/zip-0209.rst) support on mainnet, to mark blocks as invalid if they would lead to a turnstile violation in the Sprout or Sapling value pools.

To test this PR, I performed the following manual tests:

1. Used RPC call `getblock` to verify the fallback Sprout value.
2. Individually changed the fallback Sprout block hash, block height and chain value, recompiling and relaunching the node, verifying that each individual change resulted in an error.  When the block hash and block height are incorrect, an error is logged to debug.log `FallbackSproutValuePoolBalance(): fallback block hash is incorrect`.  An incorrect chain value results in node termination with error: `void FallbackSproutValuePoolBalance(CBlockIndex*, const CChainParams&): Assertion '*pindex->nChainSproutValue == chainparams.SproutValuePoolCheckpointBalance()' failed.`
3. Ran the `Smoke Testing` described in PR #3885, on mainnet.
4. Launched zcashd with experimental feature `-developersetpoolsizezero` to manually trigger a turnstile violation in both Sprout and Sapling shielded pools.  The Sprout turnstile violation occurred after launch, due to chance, when the next incoming block 520786 contained a Sprout unshielding transaction. The Sapling turnstile violation was triggered after creating a Sapling unshielding transaction.
@ebfull

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

(I also performed another reindex with no issues.)

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2019

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 63fff63 to master...

@zkbot zkbot merged commit 867786d into zcash:master Apr 29, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from In Review to Released (Merged in Master) Apr 29, 2019

@milesmanley milesmanley referenced this pull request May 21, 2019

Merged

Zcash v2.0.5 merge #45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.