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

Reject blocks that violate turnstile #3885

Merged
merged 12 commits into from Mar 19, 2019

Conversation

@ebfull
Copy link
Contributor

commented Mar 13, 2019

This is an implementation of a consensus rule which marks blocks as invalid if they would lead to a turnstile violation in the Sprout or Shielded value pools. The motivations and deployment details can be found in the accompanying ZIP draft.

This PR only introduces the rule for testnet at the moment.

We achieve the institution of this rule in three ways:

  1. Nodes prior to #2795 did not record the "delta" in the Sprout value pool balance as part of the on-disk block index. This was a long time ago, though, and all nodes that are consensus-compatible with the network today have been recording this value for newer blocks. However, the value is absent from older block indexes unless the node has reindexed or synchronized from scratch in the time since. We shouldn't need to require nodes to reindex in order to enforce this consensus rule. We avoid this problem by falling back on a hardcoded Sprout shielded value pool balance in a very recent block.
  2. If during ConnectBlock we observe that the resulting shielded value pool balance of Sprout or Sapling is negative, we reject the block.
  3. During the miner's block assembly process the miner will skip over transactions if adding them to the assembled block might violate the turnstile, since the resulting block would be invalid. This means that theoretical transactions violating the turnstile would still be relayed in the network (and made available in users' memory pools) and so a turnstile violation would have some visibility outside of block relay.

Smoke Testing

It's really tricky to test the behavior that automatically falls back to hardcoded shielded value pool balances in our architecture because it's very testnet-specific and node-version-specific. However, we can do some smoke tests to see that everything is working.

I modified the serialization of CDiskBlockIndex to serialize boost::none for nSproutValue

if ((s.GetType() & SER_DISK) && (nVersion >= SPROUT_VALUE_VERSION)) {
    boost::optional<CAmount> nSproutValueFake = boost::none;
    READWRITE(nSproutValueFake);
}

and then began a reindex of my node which I interruped around height 130k on testnet. I then restored the original serialization and resumed the reindex; I have thus roughly simulated a older node "upgrading" to a newer node that records the deltas when processing new blocks. My node showed pool monitoring was disabled, as expected, for Sprout. I confirmed that some blocks following the reindex had nonzero Sprout valueDelta from getblock, as expected. I finished the reindex, restarted the node, and confirmed that the serialization worked for newer blocks but not older blocks by querying getblock, simply as a reassurance.

Finally, I introduced the code in this PR and reloaded the node. The desired behavior (that the chain began to be "monitored" again) worked, and the values were consistent with the hardcoded constant. I then made a payment to a Sprout z-addr from the transparent pool and the pool value increased as expected, as reported by getblockchaininfo. I reindexed the node again to exercise the remaining logic and check for turnstile violations throughout the history of testnet; there were none.

@ebfull ebfull requested review from bitcartel and str4d Mar 13, 2019

Show resolved Hide resolved src/miner.cpp Outdated
Show resolved Hide resolved src/miner.cpp Outdated

@mms710 mms710 added this to Needs Prioritization in Arborist Team via automation Mar 14, 2019

@mms710 mms710 moved this from Needs Prioritization to R&D Backlog in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from R&D Backlog to Sapling Priorities in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from Sapling Priorities to Current Sprint in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from Current Sprint to In Review in Arborist Team Mar 14, 2019

@ebfull ebfull force-pushed the ebfull:turnstile branch from 509545b to 7729d73 Mar 14, 2019

@ebfull ebfull force-pushed the ebfull:turnstile branch from 7729d73 to 2b1252a Mar 14, 2019

Show resolved Hide resolved src/main.cpp
@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

I synced a testnet folder from around block 2xx,000 to tip 444686, and saw this error:

./zcash-cli getblocktemplate
zcashd: main.cpp:2472: bool ConnectBlock(const CBlock&, CValidationState&, CBlockIndex*, CCoinsViewCache&, bool): Assertion `pindex->nChainSaplingValue != boost::none' failed.
error: couldn't connect to server: EOF reached (code 1)
(make sure server is running and you are connecting to the correct RPC port)
@ebfull

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@bitcartel This is because the miner runs ConnectBlock on unpopulated block index values, so the assumption that the Sapling values have been populated is broken. So, I've removed that assertion now and made it work the same as in Sprout.

@bitcartel
Copy link
Contributor

left a comment

Could squash the recent review fix commits down, but not essential.

@str4d
Copy link
Contributor

left a comment

I'd like to see a unit test that exercises this by:

  • Creating a fake block index with a valid note.
  • Modifying the block's index to ignore some of the note's value (in order to simulate a violation).
  • Trying to spend said note in a subsequent block.

But if other reviewers are satisfied, I won't block on this.

// for nodes that have not reindexed since the introduction of monitoring
// in #2795.
nSproutValuePoolCheckpointHeight = 440329;
nSproutValuePoolCheckpointBalance = 40000029096803;

This comment has been minimized.

Copy link
@str4d

str4d Mar 19, 2019

Contributor

I have verified that this balance is accurate.

nSproutValuePoolCheckpointHeight = 440329;
nSproutValuePoolCheckpointBalance = 40000029096803;
fZIP209Enabled = true;
hashSproutValuePoolCheckpointBlock = uint256S("000a95d08ba5dcbabe881fc6471d11807bcca7df5f1795c99f3ec4580db4279b");

This comment has been minimized.

Copy link
@str4d

str4d Mar 19, 2019

Contributor

I have verified that this hash is accurate.

}

if (sproutValueDummy < 0) {
LogPrintf("CreateNewBlock(): tx %s appears to violate Sprout turnstile\n", tx.GetHash().ToString());

This comment has been minimized.

Copy link
@str4d

str4d Mar 19, 2019

Contributor

Non-blocking: We should log the txids of the transactions that are in the block at this time, because it's the set of transactions that causes the violation, not necessarily this specific transaction.

continue;
}
if (saplingValueDummy < 0) {
LogPrintf("CreateNewBlock(): tx %s appears to violate Sapling turnstile\n", tx.GetHash().ToString());

This comment has been minimized.

Copy link
@str4d

str4d Mar 19, 2019

Contributor

Non-blocking: ditto.

// so.
CAmount sproutValue = 0;
CAmount saplingValue = 0;
bool monitoring_pool_balances = true;

This comment has been minimized.

Copy link
@str4d

str4d Mar 19, 2019

Contributor

Non-blocking: Technically we can still enforce Sapling monitoring even if we don't have full coverage of Sprout. This is standard-rule territory though, so we can alter how we handle this later.

@ebfull

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

We've decided that writing a (perhaps reproducible) test of this behavior is not blocking for activation in testnet, but is definitely blocking for mainnet.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2019

📌 Commit 30a5d6f has been approved by ebfull

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2019

⌛️ Testing commit 30a5d6f with merge b81d2d4...

zkbot added a commit that referenced this pull request Mar 19, 2019

Auto merge of #3885 - ebfull:turnstile, r=ebfull
Reject blocks that violate turnstile

This is an implementation of a consensus rule which marks blocks as invalid if they would lead to a turnstile violation in the Sprout or Shielded value pools. The motivations and deployment details can be found in the [accompanying ZIP draft](zcash/zips#210).

**This PR only introduces the rule for testnet at the moment.**

We achieve the institution of this rule in three ways:

1. Nodes prior to #2795 did not record the "delta" in the Sprout value pool balance as part of the on-disk block index. This was a long time ago, though, and all nodes that are consensus-compatible with the network today have been recording this value for newer blocks. However, the value is absent from older block indexes unless the node has reindexed or synchronized from scratch in the time since. We shouldn't need to require nodes to reindex in order to enforce this consensus rule. We avoid this problem by falling back on a hardcoded Sprout shielded value pool balance in a very recent block.
2. If during `ConnectBlock` we observe that the resulting shielded value pool balance of Sprout or Sapling is negative, we reject the block.
3. During the miner's block assembly process the miner will skip over transactions if adding them to the assembled block might violate the turnstile, since the resulting block would be invalid. This means that theoretical transactions violating the turnstile would still be relayed in the network (and made available in users' memory pools) and so a turnstile violation would have some visibility outside of block relay.

## Smoke Testing

It's really tricky to test the behavior that automatically falls back to hardcoded shielded value pool balances in our architecture because it's very testnet-specific and node-version-specific. However, we can do some smoke tests to see that everything is working.

I modified the serialization of `CDiskBlockIndex` to serialize `boost::none` for `nSproutValue`

```
if ((s.GetType() & SER_DISK) && (nVersion >= SPROUT_VALUE_VERSION)) {
    boost::optional<CAmount> nSproutValueFake = boost::none;
    READWRITE(nSproutValueFake);
}
```

and then began a reindex of my node which I interruped around height 130k on testnet. I then restored the original serialization and resumed the reindex; I have thus _roughly_ simulated a older node "upgrading" to a newer node that records the deltas when processing new blocks. My node showed pool monitoring was disabled, as expected, for Sprout. I confirmed that some blocks following the reindex had nonzero Sprout `valueDelta` from `getblock`, as expected. I finished the reindex, restarted the node, and confirmed that the serialization worked for newer blocks but not older blocks by querying `getblock`, simply as a reassurance.

Finally, I introduced the code in this PR and reloaded the node. The desired behavior (that the chain began to be "monitored" again) worked, and the values were consistent with the hardcoded constant. I then made a payment to a Sprout z-addr from the transparent pool and the pool value increased as expected, as reported by `getblockchaininfo`. I reindexed the node again to exercise the remaining logic and check for turnstile violations throughout the history of testnet; there were none.
@str4d

str4d approved these changes Mar 19, 2019

@mms710 mms710 moved this from In Review to Merge Queue in Arborist Team Mar 19, 2019

@bitcartel bitcartel added this to the v2.0.4 milestone Mar 19, 2019

@defuse

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Could a block overflow the calculation of the total value with such a large negative amount that it becomes positive and the block is accepted anyway?

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2019

💔 Test failed - pr-merge

@daira

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Reminder that this is implementing a consensus change (albeit only on testnet, but eventually also on mainnet), so requires three ACKs according to our policy. I'm in the middle of reviewing it.

@daira

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

@zkbot r-

@ebfull

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

@defuse Blocks that overflow those fields would not be accepted at all because they would violate the MAX_MONEY limits on those fields.

@ebfull

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

I don't understand what the failure of the tests is. Is this a known transient failure?

@str4d

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

zcash-gtest is dying during the Validation.ReceivedBlockTransactions test. Probably hitting one of the asserts you added.

@ebfull

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Yes, some tests make their own genesis block and do some things that would violate the checkpointing. I've disabled ZIP209 on regtest so that this PR only enables it on testnet.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

ACK regtest change.

@str4d

str4d approved these changes Mar 19, 2019

Copy link
Contributor

left a comment

ACK 8d0e2be

// regtest.
nSproutValuePoolCheckpointHeight = 0;
nSproutValuePoolCheckpointBalance = 0;
fZIP209Enabled = true;

This comment has been minimized.

Copy link
@str4d

str4d Mar 19, 2019

Contributor

Nit: I'd prefer that we explicitly set fZIP209Enabled = false; here, as we do with other left-at-default flags.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2019

📌 Commit 8d0e2be has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2019

⌛️ Testing commit 8d0e2be with merge 008705d...

zkbot added a commit that referenced this pull request Mar 19, 2019

Auto merge of #3885 - ebfull:turnstile, r=bitcartel
Reject blocks that violate turnstile

This is an implementation of a consensus rule which marks blocks as invalid if they would lead to a turnstile violation in the Sprout or Shielded value pools. The motivations and deployment details can be found in the [accompanying ZIP draft](zcash/zips#210).

**This PR only introduces the rule for testnet at the moment.**

We achieve the institution of this rule in three ways:

1. Nodes prior to #2795 did not record the "delta" in the Sprout value pool balance as part of the on-disk block index. This was a long time ago, though, and all nodes that are consensus-compatible with the network today have been recording this value for newer blocks. However, the value is absent from older block indexes unless the node has reindexed or synchronized from scratch in the time since. We shouldn't need to require nodes to reindex in order to enforce this consensus rule. We avoid this problem by falling back on a hardcoded Sprout shielded value pool balance in a very recent block.
2. If during `ConnectBlock` we observe that the resulting shielded value pool balance of Sprout or Sapling is negative, we reject the block.
3. During the miner's block assembly process the miner will skip over transactions if adding them to the assembled block might violate the turnstile, since the resulting block would be invalid. This means that theoretical transactions violating the turnstile would still be relayed in the network (and made available in users' memory pools) and so a turnstile violation would have some visibility outside of block relay.

## Smoke Testing

It's really tricky to test the behavior that automatically falls back to hardcoded shielded value pool balances in our architecture because it's very testnet-specific and node-version-specific. However, we can do some smoke tests to see that everything is working.

I modified the serialization of `CDiskBlockIndex` to serialize `boost::none` for `nSproutValue`

```
if ((s.GetType() & SER_DISK) && (nVersion >= SPROUT_VALUE_VERSION)) {
    boost::optional<CAmount> nSproutValueFake = boost::none;
    READWRITE(nSproutValueFake);
}
```

and then began a reindex of my node which I interruped around height 130k on testnet. I then restored the original serialization and resumed the reindex; I have thus _roughly_ simulated a older node "upgrading" to a newer node that records the deltas when processing new blocks. My node showed pool monitoring was disabled, as expected, for Sprout. I confirmed that some blocks following the reindex had nonzero Sprout `valueDelta` from `getblock`, as expected. I finished the reindex, restarted the node, and confirmed that the serialization worked for newer blocks but not older blocks by querying `getblock`, simply as a reassurance.

Finally, I introduced the code in this PR and reloaded the node. The desired behavior (that the chain began to be "monitored" again) worked, and the values were consistent with the hardcoded constant. I then made a payment to a Sprout z-addr from the transparent pool and the pool value increased as expected, as reported by `getblockchaininfo`. I reindexed the node again to exercise the remaining logic and check for turnstile violations throughout the history of testnet; there were none.
// parents and can expect nChainSaplingValue not to be boost::none.
// However, the miner and mining RPCs may not have populated this
// value and will call `TestBlockValidity`. So, we act
// conditionally.

This comment has been minimized.

Copy link
@daira

daira Mar 19, 2019

Contributor

This implies that the built-in miner might try to mine a block that violates the turnstile rule. (In this situation the miner is not the attacker who submitted the transaction, and so we would care if they're DoS-penalised; I'm not sure whether they are.) The block will be seen as invalid by other nodes, but this still seems undesirable.

This comment also applies for Sprout.

This comment has been minimized.

Copy link
@ebfull

ebfull Mar 19, 2019

Author Contributor

There is logic in the miner for avoiding mining a block which violates the turnstile, it's just that the block index fields don't get populated before TestBlockValidity is called so we aren't relying on TestBlockValidity to defend us against wasted mining effort -- we're relying on the mining code to correctly avoid placing transactions that violate the turnstile in the block.

LogPrintf(
"FallbackSproutValuePoolBalance(): fallback block hash is incorrect, we got %s\n",
pindex->GetBlockHash().ToString()
);

This comment has been minimized.

Copy link
@daira

daira Mar 19, 2019

Contributor

If we're at the checkpoint height but on the wrong chain, this will fall through, leaving pindex->nChainSproutValue as none. Then the conditional on line R2461 will cause the turnstile rule not to be enforced.

It's important that we don't just assert that the block hash at the checkpoint height is as expected, because that would introduce a potential DoS during node startup if an attacker created an orphan block at the checkpoint height and sent it to the victim node at just the right time. On the other hand, it seems like we should be able to do better than failing open in this case: in particular if the node reaches 100 blocks past the checkpoint height and is still on the wrong chain, then it's never going to recover. Maybe it's too much complexity to detect that case (if it happens then we have other problems than the turnstile rule).

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2019

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 008705d to master...

@zkbot zkbot merged commit 8d0e2be into zcash:master Mar 19, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from Merge Queue to Released (Merged in Master) Mar 19, 2019

@daira

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Having thought for longer about the consequences of the points made in my comments, they seem non-blocking. Post-hoc utACK (thus satisfying the 3-ACK requirement for consensus rule changes, but please let's be strict about that in future).

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