ZIP 209: Prohibit Negative Shielded Value Pool #210
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
str4d
previously requested changes
Mar 6, 2019
daira
changed the title
zkbot
added a commit
to zcash/zcash
that referenced
this pull request
Mar 19, 2019
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.
Eirik0
suggested changes
Mar 26, 2019
|
|
||
| It is possible for nodes to monitor the total amount of coins that are shielded or unshielded in each of the Sprout or Sapling shielded transaction protocols. If the amount of coins that are unshielded exceed the amount that are shielded, a balance violation has occurred in the corresponding shielded transaction protocol. | ||
|
|
||
| It would be preferable for the network to reject blocks that result in the aforementioned balance violation. However, nodes do not currently react to such an event. Remediation may therefore require chain rollbacks and other disruption. |
There was a problem hiding this comment.
This is a little bit clunky, here is a suggested rewording:
Rather than allow blocks which result in such a balance violation, it would be preferable for the network to reject them. As nodes do not currently react to this, remediation to such an event may require chain rollbacks and other disruptions.
There was a problem hiding this comment.
🤷♂️ I am okay with the current wording. This rewording uses "them" and "this" when there are many objects (blocks, balance violations, the network, etc.)
Co-Authored-By: ebfull <ewillbefull@gmail.com>
Co-Authored-By: ebfull <ewillbefull@gmail.com>
Eirik0
approved these changes
Mar 26, 2019
defuse
approved these changes
Mar 26, 2019
Co-Authored-By: ebfull <ewillbefull@gmail.com>
Co-Authored-By: ebfull <ewillbefull@gmail.com>
This was referenced May 20, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rendered.