From ad6a36ad02de357db6fb44605774ebabb4d4806f Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 9 Nov 2017 22:09:54 +0000 Subject: [PATCH 1/4] 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. --- src/chain.h | 21 +++++++++ src/gtest/test_validation.cpp | 86 +++++++++++++++++++++++++++++++++++ src/main.cpp | 25 ++++++++++ src/txdb.cpp | 1 + 4 files changed, 133 insertions(+) diff --git a/src/chain.h b/src/chain.h index b7e8a9176..5e5258339 100644 --- a/src/chain.h +++ b/src/chain.h @@ -16,6 +16,8 @@ #include +static const int SPROUT_VALUE_VERSION = 1001400; + struct CDiskBlockPos { int nFile; @@ -144,6 +146,15 @@ public: //! (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. + boost::optional nSproutValue; + + //! (memory only) Total value held by the Sprout circuit up to and including this block. + //! Will be boost::none for on old nodes until a reindex has taken place. + //! Will be boost::none if nChainTx is zero. + boost::optional nChainSproutValue; + //! block header int nVersion; uint256 hashMerkleRoot; @@ -172,6 +183,8 @@ public: hashAnchor = uint256(); hashAnchorEnd = uint256(); nSequenceId = 0; + nSproutValue = boost::none; + nChainSproutValue = boost::none; nVersion = 0; hashMerkleRoot = uint256(); @@ -339,6 +352,14 @@ public: READWRITE(nBits); READWRITE(nNonce); READWRITE(nSolution); + + // Only read/write nSproutValue if the client version used to create + // 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) { + READWRITE(nSproutValue); + } } uint256 GetBlockHash() const diff --git a/src/gtest/test_validation.cpp b/src/gtest/test_validation.cpp index 21ed20d84..6f603eaec 100644 --- a/src/gtest/test_validation.cpp +++ b/src/gtest/test_validation.cpp @@ -2,6 +2,18 @@ #include "consensus/validation.h" #include "main.h" +#include "utiltest.h" + +extern ZCJoinSplit* params; + +extern bool ReceivedBlockTransactions(const CBlock &block, CValidationState& state, CBlockIndex *pindexNew, const CDiskBlockPos& pos); + +void ExpectOptionalAmount(CAmount expected, boost::optional actual) { + EXPECT_TRUE((bool)actual); + if (actual) { + EXPECT_EQ(expected, *actual); + } +} // Fake an empty view class FakeCoinsViewDB : public CCoinsView { @@ -61,3 +73,77 @@ TEST(Validation, ContextualCheckInputsPassesWithCoinbase) { CValidationState state; EXPECT_TRUE(ContextualCheckInputs(tx, state, view, false, 0, false, Params(CBaseChainParams::MAIN).GetConsensus())); } + +TEST(Validation, ReceivedBlockTransactions) { + auto sk = libzcash::SpendingKey::random(); + + // Create a fake genesis block + CBlock block1; + block1.vtx.push_back(GetValidReceive(*params, sk, 5, true)); + block1.hashMerkleRoot = block1.BuildMerkleTree(); + CBlockIndex fakeIndex1 {block1}; + + // Create a fake child block + CBlock block2; + block2.hashPrevBlock = block1.GetHash(); + block2.vtx.push_back(GetValidReceive(*params, sk, 10, true)); + block2.hashMerkleRoot = block2.BuildMerkleTree(); + CBlockIndex fakeIndex2 {block2}; + fakeIndex2.pprev = &fakeIndex1; + + CDiskBlockPos pos1; + CDiskBlockPos pos2; + + // Set initial state of indices + ASSERT_TRUE(fakeIndex1.RaiseValidity(BLOCK_VALID_TREE)); + ASSERT_TRUE(fakeIndex2.RaiseValidity(BLOCK_VALID_TREE)); + EXPECT_TRUE(fakeIndex1.IsValid(BLOCK_VALID_TREE)); + EXPECT_TRUE(fakeIndex2.IsValid(BLOCK_VALID_TREE)); + EXPECT_FALSE(fakeIndex1.IsValid(BLOCK_VALID_TRANSACTIONS)); + EXPECT_FALSE(fakeIndex2.IsValid(BLOCK_VALID_TRANSACTIONS)); + + // Sprout pool values should not be set + EXPECT_FALSE((bool)fakeIndex1.nSproutValue); + EXPECT_FALSE((bool)fakeIndex1.nChainSproutValue); + EXPECT_FALSE((bool)fakeIndex2.nSproutValue); + EXPECT_FALSE((bool)fakeIndex2.nChainSproutValue); + + // Mark the second block's transactions as received first + CValidationState state; + EXPECT_TRUE(ReceivedBlockTransactions(block2, state, &fakeIndex2, pos2)); + EXPECT_FALSE(fakeIndex1.IsValid(BLOCK_VALID_TRANSACTIONS)); + EXPECT_TRUE(fakeIndex2.IsValid(BLOCK_VALID_TRANSACTIONS)); + + // Sprout pool value delta should now be set for the second block, + // but not any chain totals + EXPECT_FALSE((bool)fakeIndex1.nSproutValue); + EXPECT_FALSE((bool)fakeIndex1.nChainSproutValue); + { + SCOPED_TRACE("ExpectOptionalAmount call"); + ExpectOptionalAmount(20, fakeIndex2.nSproutValue); + } + EXPECT_FALSE((bool)fakeIndex2.nChainSproutValue); + + // Now mark the first block's transactions as received + EXPECT_TRUE(ReceivedBlockTransactions(block1, state, &fakeIndex1, pos1)); + EXPECT_TRUE(fakeIndex1.IsValid(BLOCK_VALID_TRANSACTIONS)); + EXPECT_TRUE(fakeIndex2.IsValid(BLOCK_VALID_TRANSACTIONS)); + + // Sprout pool values should now be set for both blocks + { + SCOPED_TRACE("ExpectOptionalAmount call"); + ExpectOptionalAmount(10, fakeIndex1.nSproutValue); + } + { + SCOPED_TRACE("ExpectOptionalAmount call"); + ExpectOptionalAmount(10, fakeIndex1.nChainSproutValue); + } + { + SCOPED_TRACE("ExpectOptionalAmount call"); + ExpectOptionalAmount(20, fakeIndex2.nSproutValue); + } + { + SCOPED_TRACE("ExpectOptionalAmount call"); + ExpectOptionalAmount(30, fakeIndex2.nChainSproutValue); + } +} diff --git a/src/main.cpp b/src/main.cpp index a8ec07403..d8e47caac 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2835,6 +2835,15 @@ bool ReceivedBlockTransactions(const CBlock &block, CValidationState& state, CBl { pindexNew->nTx = block.vtx.size(); pindexNew->nChainTx = 0; + CAmount sproutValue = 0; + for (auto tx : block.vtx) { + for (auto js : tx.vjoinsplit) { + sproutValue += js.vpub_old; + sproutValue -= js.vpub_new; + } + } + pindexNew->nSproutValue = sproutValue; + pindexNew->nChainSproutValue = boost::none; pindexNew->nFile = pos.nFile; pindexNew->nDataPos = pos.nPos; pindexNew->nUndoPos = 0; @@ -2852,6 +2861,15 @@ bool ReceivedBlockTransactions(const CBlock &block, CValidationState& state, CBl CBlockIndex *pindex = queue.front(); queue.pop_front(); pindex->nChainTx = (pindex->pprev ? pindex->pprev->nChainTx : 0) + pindex->nTx; + if (pindex->pprev) { + if (pindex->pprev->nChainSproutValue && pindex->nSproutValue) { + pindex->nChainSproutValue = *pindex->pprev->nChainSproutValue + *pindex->nSproutValue; + } else { + pindex->nChainSproutValue = boost::none; + } + } else { + pindex->nChainSproutValue = pindex->nSproutValue; + } { LOCK(cs_nBlockSequenceId); pindex->nSequenceId = nBlockSequenceId++; @@ -3522,12 +3540,19 @@ bool static LoadBlockIndexDB() if (pindex->pprev) { if (pindex->pprev->nChainTx) { pindex->nChainTx = pindex->pprev->nChainTx + pindex->nTx; + if (pindex->pprev->nChainSproutValue && pindex->nSproutValue) { + pindex->nChainSproutValue = *pindex->pprev->nChainSproutValue + *pindex->nSproutValue; + } else { + pindex->nChainSproutValue = boost::none; + } } else { pindex->nChainTx = 0; + pindex->nChainSproutValue = boost::none; mapBlocksUnlinked.insert(std::make_pair(pindex->pprev, pindex)); } } else { pindex->nChainTx = pindex->nTx; + pindex->nChainSproutValue = pindex->nSproutValue; } } if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && (pindex->nChainTx || pindex->pprev == NULL)) diff --git a/src/txdb.cpp b/src/txdb.cpp index 004b0be2c..e1e29d9ac 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -310,6 +310,7 @@ bool CBlockTreeDB::LoadBlockIndexGuts() pindexNew->nSolution = diskindex.nSolution; pindexNew->nStatus = diskindex.nStatus; pindexNew->nTx = diskindex.nTx; + pindexNew->nSproutValue = diskindex.nSproutValue; if (!CheckProofOfWork(pindexNew->GetBlockHash(), pindexNew->nBits, Params().GetConsensus())) return error("LoadBlockIndex(): CheckProofOfWork failed: %s", pindexNew->ToString()); From e319633435e3cd8c1726ebd3d6245200ae3f17c5 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 14 Dec 2017 15:18:08 +0000 Subject: [PATCH 2/4] Add Sprout value pool to getblock and getblockchaininfo --- qa/rpc-tests/wallet_protectcoinbase.py | 34 ++++++++++++++++++++++++-- src/rpcblockchain.cpp | 29 +++++++++++++++++++++- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/qa/rpc-tests/wallet_protectcoinbase.py b/qa/rpc-tests/wallet_protectcoinbase.py index e407f0914..5423dcd64 100755 --- a/qa/rpc-tests/wallet_protectcoinbase.py +++ b/qa/rpc-tests/wallet_protectcoinbase.py @@ -14,6 +14,16 @@ import time import timeit from decimal import Decimal +def check_value_pool(node, name, total): + value_pools = node.getblockchaininfo()['valuePools'] + found = False + for pool in value_pools: + if pool['id'] == name: + found = True + assert_equal(pool['monitored'], True) + assert_equal(pool['chainValue'], total) + assert(found) + class WalletProtectCoinbaseTest (BitcoinTestFramework): def setup_chain(self): @@ -76,6 +86,11 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): assert_equal(self.nodes[2].getbalance(), 0) assert_equal(self.nodes[3].getbalance(), 0) + check_value_pool(self.nodes[0], 'sprout', 0) + check_value_pool(self.nodes[1], 'sprout', 0) + check_value_pool(self.nodes[2], 'sprout', 0) + check_value_pool(self.nodes[3], 'sprout', 0) + # Send will fail because we are enforcing the consensus rule that # coinbase utxos can only be sent to a zaddr. errorString = "" @@ -141,8 +156,9 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): assert_equal("wallet does not allow any change" in errorString, True) # This send will succeed. We send two coinbase utxos totalling 20.0 less a fee of 0.00010000, with no change. + shieldvalue = Decimal('20.0') - Decimal('0.0001') recipients = [] - recipients.append({"address":myzaddr, "amount": Decimal('20.0') - Decimal('0.0001')}) + recipients.append({"address":myzaddr, "amount": shieldvalue}) myopid = self.nodes[0].z_sendmany(mytaddr, recipients) mytxid = self.wait_and_assert_operationid_status(myopid) self.sync_all() @@ -169,6 +185,10 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): assert_equal(Decimal(resp["private"]), Decimal('19.9999')) assert_equal(Decimal(resp["total"]), Decimal('39.9999')) + # The Sprout value pool should reflect the send + sproutvalue = shieldvalue + check_value_pool(self.nodes[0], 'sprout', sproutvalue) + # A custom fee of 0 is okay. Here the node will send the note value back to itself. recipients = [] recipients.append({"address":myzaddr, "amount": Decimal('19.9999')}) @@ -182,9 +202,13 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): assert_equal(Decimal(resp["private"]), Decimal('19.9999')) assert_equal(Decimal(resp["total"]), Decimal('39.9999')) + # The Sprout value pool should be unchanged + check_value_pool(self.nodes[0], 'sprout', sproutvalue) + # convert note to transparent funds + unshieldvalue = Decimal('10.0') recipients = [] - recipients.append({"address":mytaddr, "amount":Decimal('10.0')}) + recipients.append({"address":mytaddr, "amount": unshieldvalue}) myopid = self.nodes[0].z_sendmany(myzaddr, recipients) mytxid = self.wait_and_assert_operationid_status(myopid) assert(mytxid is not None) @@ -198,10 +222,12 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): self.sync_all() # check balances + sproutvalue -= unshieldvalue + Decimal('0.0001') resp = self.nodes[0].z_gettotalbalance() assert_equal(Decimal(resp["transparent"]), Decimal('30.0')) assert_equal(Decimal(resp["private"]), Decimal('9.9998')) assert_equal(Decimal(resp["total"]), Decimal('39.9998')) + check_value_pool(self.nodes[0], 'sprout', sproutvalue) # z_sendmany will return an error if there is transparent change output considered dust. # UTXO selection in z_sendmany sorts in ascending order, so smallest utxos are consumed first. @@ -277,7 +303,9 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): # check balance node2balance = amount_per_recipient * num_t_recipients + sproutvalue -= node2balance + Decimal('0.0001') assert_equal(self.nodes[2].getbalance(), node2balance) + check_value_pool(self.nodes[0], 'sprout', sproutvalue) # Send will fail because fee is negative try: @@ -336,6 +364,8 @@ class WalletProtectCoinbaseTest (BitcoinTestFramework): assert_equal(Decimal(resp["private"]), send_amount) resp = self.nodes[0].z_getbalance(myzaddr) assert_equal(Decimal(resp), zbalance - custom_fee - send_amount) + sproutvalue -= custom_fee + check_value_pool(self.nodes[0], 'sprout', sproutvalue) if __name__ == '__main__': WalletProtectCoinbaseTest().main() diff --git a/src/rpcblockchain.cpp b/src/rpcblockchain.cpp index cbc7109dd..363872ede 100644 --- a/src/rpcblockchain.cpp +++ b/src/rpcblockchain.cpp @@ -77,6 +77,25 @@ double GetNetworkDifficulty(const CBlockIndex* blockindex) return GetDifficultyINTERNAL(blockindex, true); } +static UniValue ValuePoolDesc( + const std::string &name, + const boost::optional chainValue, + const boost::optional valueDelta) +{ + UniValue rv(UniValue::VOBJ); + rv.push_back(Pair("id", name)); + rv.push_back(Pair("monitored", (bool)chainValue)); + if (chainValue) { + rv.push_back(Pair("chainValue", ValueFromAmount(*chainValue))); + rv.push_back(Pair("chainValueZat", *chainValue)); + } + if (valueDelta) { + rv.push_back(Pair("valueDelta", ValueFromAmount(*valueDelta))); + rv.push_back(Pair("valueDeltaZat", *valueDelta)); + } + return rv; +} + UniValue blockheaderToJSON(const CBlockIndex* blockindex) { UniValue result(UniValue::VOBJ); @@ -138,6 +157,10 @@ UniValue blockToJSON(const CBlock& block, const CBlockIndex* blockindex, bool tx result.push_back(Pair("chainwork", blockindex->nChainWork.GetHex())); result.push_back(Pair("anchor", blockindex->hashAnchorEnd.GetHex())); + UniValue valuePools(UniValue::VARR); + valuePools.push_back(ValuePoolDesc("sprout", blockindex->nChainSproutValue, blockindex->nSproutValue)); + result.push_back(Pair("valuePools", valuePools)); + if (blockindex->pprev) result.push_back(Pair("previousblockhash", blockindex->pprev->GetBlockHash().GetHex())); CBlockIndex *pnext = chainActive.Next(blockindex); @@ -688,8 +711,12 @@ UniValue getblockchaininfo(const UniValue& params, bool fHelp) pcoinsTip->GetAnchorAt(pcoinsTip->GetBestAnchor(), tree); obj.push_back(Pair("commitments", tree.size())); - const Consensus::Params& consensusParams = Params().GetConsensus(); CBlockIndex* tip = chainActive.Tip(); + UniValue valuePools(UniValue::VARR); + valuePools.push_back(ValuePoolDesc("sprout", tip->nChainSproutValue, boost::none)); + obj.push_back(Pair("valuePools", valuePools)); + + const Consensus::Params& consensusParams = Params().GetConsensus(); UniValue softforks(UniValue::VARR); softforks.push_back(SoftForkDesc("bip34", 2, tip, consensusParams)); softforks.push_back(SoftForkDesc("bip66", 3, tip, consensusParams)); From 9d0c70e9e7bec8fc90a533b6f8838807424be9f7 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 15 Dec 2017 18:36:05 +0000 Subject: [PATCH 3/4] Clarify operator precedence in serialization of nSproutValue --- src/chain.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/chain.h b/src/chain.h index 5e5258339..9d858a609 100644 --- a/src/chain.h +++ b/src/chain.h @@ -357,7 +357,7 @@ public: // 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) { + if ((nType & SER_DISK) && (nVersion >= SPROUT_VALUE_VERSION)) { READWRITE(nSproutValue); } } From e365ca1c53d996960dce64b75193fe7b809c86fb Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 16 Dec 2017 10:01:26 +0000 Subject: [PATCH 4/4] Remove nSproutValue TODO from CDiskBlockIndex 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. --- src/chain.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/chain.h b/src/chain.h index 9d858a609..a3b1b7ae9 100644 --- a/src/chain.h +++ b/src/chain.h @@ -355,8 +355,6 @@ public: // Only read/write nSproutValue if the client version used to create // 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)) { READWRITE(nSproutValue); }