-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Track net value entering and exiting the Sprout circuit #2795
Conversation
c351fca
to
66be57e
Compare
Currently this just tracks the values internally, but doesn't expose them anywhere or enforce their ranges. If we want to expose this in an RPC method, we should figure out which one (maybe |
Yes the value should be returned via RPC so that we can write a QA test. |
src/chain.h
Outdated
// TODO: See if we can get away with not serializing the boost::optional | ||
// one-byte header, without requiring users to reindex on upgrade. | ||
// TODO: Do we want this included for SER_GETHASH, or will that cause problems? | ||
if (nType & SER_DISK && nVersion >= SPROUT_VALUE_VERSION) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment on what this condition exactly means?
@@ -144,6 +146,15 @@ class CBlockIndex | |||
//! (memory only) The anchor for the tree state up to the end of this block | |||
uint256 hashAnchorEnd; | |||
|
|||
//! Change in value held by the Sprout circuit over this block. | |||
//! Will be boost::none for older blocks on old nodes until a reindex has taken place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we're changing the blocks retroactively and adding this value to all of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are changing the block indices retroactively, not the blocks themselves. For older indices, if/when they are rewritten this value will be boost::none
until a full reindex is done.
src/rpcblockchain.cpp
Outdated
@@ -77,6 +77,25 @@ double GetNetworkDifficulty(const CBlockIndex* blockindex) | |||
return GetDifficultyINTERNAL(blockindex, true); | |||
} | |||
|
|||
static UniValue ValuePoolDesc( | |||
const std::string &name, | |||
boost::optional<CAmount> chainValue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be const
infront of chainValue,valueDelta
as they are just being read here? (I think I managed to use const
with boost::optional
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, probably - I just copied this from another existing function, where this parameter was an int
, so const didn't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, if you could change this and answer my first comment I'll ack.
8a41ca0
to
6b452dd
Compare
Delta values will be stored for new blocks; old blocks can be filled in by re-indexing. The net value currently in the Sprout circuit is only calculated when delta values for all previous blocks are present.
6b452dd
to
e319633
Compare
@arielgabizon comments addressed. |
src/chain.h
Outdated
// this index was storing them. | ||
// TODO: See if we can get away with not serializing the boost::optional | ||
// one-byte header, without requiring users to reindex on upgrade. | ||
if (nType & SER_DISK && nVersion >= SPROUT_VALUE_VERSION) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still don't understand the logic of this line. In particular, what do this variables on the LHS have to do with the version the client is running?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an artifact of the way Bitcoin does serialization. nType
can be one of SER_NETWORK
, SER_DISK
or SER_GETHASH
(the latter used in very specific cases), By convention, if nType == SER_NETWORK
then nVersion
is set to the protocol version (because that serialization is for communicating with peers), while if nType == SER_DISK
then nVersion
is set to the client version (because that serialization is only read by the local client, often across local version upgrades).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I kind of get it. Is there a reason for the choice 1001400
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's worth adding parentheses for each of the two conditions; initially I parsed it as one condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1001400
is what CLIENT_VERSION
will be set to from 1.0.14-rc1, which will be the first tagged "release" to have this code enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @arielgabizon parentheses would make it clearer, even though operator precedence is fine. re: http://en.cppreference.com/w/cpp/language/operator_precedence
Added a commit to clarify the operator precedence. Still trying to figure out if I can solve the TODO (reduce the size of the index); if I can't figure it out tomorrow I will merge this PR as-is. |
Block indices are flushed to disk when they are marked as dirty, and this happens via enough distinct pathways that it would be sufficiently complex to update nSproutValue via all of them. Thus it is necessary to be able to serialize "no value" for writes by upgraded clients.
Decided it would significantly complicate the code if we tried to ensure @zkbot r+ |
📌 Commit e365ca1 has been approved by |
Track net value entering and exiting the Sprout circuit Delta values will be stored for new blocks; old blocks can be filled in by re-indexing. The net value currently in the Sprout circuit is only calculated when delta values for all previous blocks are present. Part of #2351.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
post-hoc utACK
if pool['id'] == name: | ||
found = True | ||
assert_equal(pool['monitored'], True) | ||
assert_equal(pool['chainValue'], total) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also check chainValueZat
.
Check chainValueZat when checking value pool monitoring Addresses #2795 (comment)
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.
Delta values will be stored for new blocks; old blocks can be filled in by
re-indexing. The net value currently in the Sprout circuit is only calculated
when delta values for all previous blocks are present.
Part of #2351.