From 01cdea54ee41d0e676916a320fce71638ec36f8f Mon Sep 17 00:00:00 2001 From: therealyingtong Date: Tue, 12 May 2020 10:43:18 +0800 Subject: [PATCH 1/6] Add contextual check to main.cpp Reject transactions with nonzero vpub_old after NU4 --- src/main.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/main.cpp b/src/main.cpp index d11ca5bae..aba02d9fd 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -792,6 +792,7 @@ bool ContextualCheckTransaction( bool saplingActive = chainparams.GetConsensus().NetworkUpgradeActive(nHeight, Consensus::UPGRADE_SAPLING); bool isSprout = !overwinterActive; bool heartwoodActive = chainparams.GetConsensus().NetworkUpgradeActive(nHeight, Consensus::UPGRADE_HEARTWOOD); + bool canopyActive = chainparams.GetConsensus().NetworkUpgradeActive(nHeight, Consensus::UPGRADE_CANOPY); // If Sprout rules apply, reject transactions which are intended for Overwinter and beyond if (isSprout && tx.fOverwintered) { @@ -936,6 +937,15 @@ bool ContextualCheckTransaction( } } + // Rules that apply to Canopy or later: + if (canopyActive) { + for (const JSDescription& joinsplit : tx.vJoinSplit) { + if (joinsplit.vpub_old > 0) { + return state.DoS(DOS_LEVEL_BLOCK, error("ContextualCheckTransaction(): joinsplit.vpub_old nonzero"), REJECT_INVALID, "bad-txns-vpub_old-nonzero"); + } + } + } + auto consensusBranchId = CurrentEpochBranchId(nHeight, chainparams.GetConsensus()); auto prevConsensusBranchId = PrevEpochBranchId(consensusBranchId, chainparams.GetConsensus()); uint256 dataToBeSigned; From 2a2fc2a16f5d9f394e4f3fa42f6bad3dc8be0fcb Mon Sep 17 00:00:00 2001 From: therealyingtong Date: Tue, 12 May 2020 10:45:51 +0800 Subject: [PATCH 2/6] Add gtests Should accept Sprout shielding before NU4 but reject it afterwards --- src/gtest/test_checktransaction.cpp | 63 +++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/src/gtest/test_checktransaction.cpp b/src/gtest/test_checktransaction.cpp index 9b0ff0ae6..0e0e639ff 100644 --- a/src/gtest/test_checktransaction.cpp +++ b/src/gtest/test_checktransaction.cpp @@ -1097,7 +1097,7 @@ TEST(ChecktransactionTests, BadTxReceivedOverNetwork) } } -TEST(CheckTransaction, InvalidShieldedCoinbase) { +TEST(ChecktransactionTests, InvalidShieldedCoinbase) { RegtestActivateSapling(); CMutableTransaction mtx = GetValidTransaction(); @@ -1128,7 +1128,7 @@ TEST(CheckTransaction, InvalidShieldedCoinbase) { RegtestDeactivateHeartwood(); } -TEST(CheckTransaction, HeartwoodAcceptsShieldedCoinbase) { +TEST(ChecktransactionTests, HeartwoodAcceptsShieldedCoinbase) { RegtestActivateHeartwood(false, Consensus::NetworkUpgrade::ALWAYS_ACTIVE); auto chainparams = Params(); @@ -1211,7 +1211,7 @@ TEST(CheckTransaction, HeartwoodAcceptsShieldedCoinbase) { // Check that the consensus rules relevant to valueBalance, vShieldedOutput, and // bindingSig from https://zips.z.cash/protocol/protocol.pdf#txnencoding are // applied to coinbase transactions. -TEST(CheckTransaction, HeartwoodEnforcesSaplingRulesOnShieldedCoinbase) { +TEST(ChecktransactionTests, HeartwoodEnforcesSaplingRulesOnShieldedCoinbase) { RegtestActivateHeartwood(false, Consensus::NetworkUpgrade::ALWAYS_ACTIVE); auto chainparams = Params(); @@ -1284,3 +1284,60 @@ TEST(CheckTransaction, HeartwoodEnforcesSaplingRulesOnShieldedCoinbase) { RegtestDeactivateHeartwood(); } + + +TEST(ChecktransactionTests, CanopyRejectsNonzeroVPubOld) { + + RegtestActivateSapling(); + + CMutableTransaction mtx = GetValidTransaction(NetworkUpgradeInfo[Consensus::UPGRADE_SAPLING].nBranchId); + + // Make a JoinSplit with nonzero vpub_old + mtx.vJoinSplit.resize(1); + mtx.vJoinSplit[0].vpub_old = 1; + mtx.vJoinSplit[0].vpub_new = 0; + mtx.vJoinSplit[0].proof = libzcash::GrothProof(); + CreateJoinSplitSignature(mtx, NetworkUpgradeInfo[Consensus::UPGRADE_SAPLING].nBranchId); + + CTransaction tx(mtx); + + // Before Canopy, nonzero vpub_old is accepted in both non-contextual and contextual checks + MockCValidationState state; + EXPECT_TRUE(CheckTransactionWithoutProofVerification(tx, state)); + EXPECT_TRUE(ContextualCheckTransaction(tx, state, Params(), 1, true)); + + RegtestActivateCanopy(false, Consensus::NetworkUpgrade::ALWAYS_ACTIVE); + + // After Canopy, nonzero vpub_old is accepted in non-contextual checks but rejected in contextual checks + EXPECT_TRUE(CheckTransactionWithoutProofVerification(tx, state)); + EXPECT_CALL(state, DoS(100, false, REJECT_INVALID, "bad-txns-vpub_old-nonzero", false)).Times(1); + EXPECT_FALSE(ContextualCheckTransaction(tx, state, Params(), 10, true)); + + RegtestDeactivateCanopy(); + +} + +TEST(ChecktransactionTests, CanopyAcceptsZeroVPubOld) { + + CMutableTransaction mtx = GetValidTransaction(NetworkUpgradeInfo[Consensus::UPGRADE_SAPLING].nBranchId); + + // Make a JoinSplit with zero vpub_old + mtx.vJoinSplit.resize(1); + mtx.vJoinSplit[0].vpub_old = 0; + mtx.vJoinSplit[0].vpub_new = 1; + mtx.vJoinSplit[0].proof = libzcash::GrothProof(); + CreateJoinSplitSignature(mtx, NetworkUpgradeInfo[Consensus::UPGRADE_CANOPY].nBranchId); + + CTransaction tx(mtx); + + // After Canopy, zero value vpub_old (i.e. unshielding) is accepted in both non-contextual and contextual checks + MockCValidationState state; + + RegtestActivateCanopy(false, Consensus::NetworkUpgrade::ALWAYS_ACTIVE); + + EXPECT_TRUE(CheckTransactionWithoutProofVerification(tx, state)); + EXPECT_TRUE(ContextualCheckTransaction(tx, state, Params(), 10, true)); + + RegtestDeactivateCanopy(); + +} From 1c59f06df0fb9733b3f795f55ab5157ffcbf3ecd Mon Sep 17 00:00:00 2001 From: therealyingtong Date: Wed, 13 May 2020 01:59:47 +0800 Subject: [PATCH 3/6] Add checks to z_ methods in rpcwallet Disallow Sprout shielding after Canopy in z_sendmany, z_shieldcoinbase, z_mergetoaddress, and zc_raw_joinsplit (deprecated) --- src/wallet/rpcwallet.cpp | 43 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 36838401d..ae38c4b0a 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3017,7 +3017,14 @@ UniValue zc_raw_joinsplit(const UniValue& params, bool fHelp) CAmount vpub_old(0); CAmount vpub_new(0); + int nextBlockHeight = chainActive.Height() + 1; + + const bool canopyActive = Params().GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_CANOPY); + if (params[3].get_real() != 0.0) + if (canopyActive) { + throw JSONRPCError(RPC_VERIFY_REJECTED, "Sprout shielding is not supported after Canopy"); + } vpub_old = AmountFromValue(params[3]); if (params[4].get_real() != 0.0) @@ -4063,6 +4070,15 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) RPC_INVALID_PARAMETER, "Cannot send between Sprout and Sapling addresses using z_sendmany"); } + + int nextBlockHeight = chainActive.Height() + 1; + + if (fromTaddr && toSprout) { + const bool canopyActive = Params().GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_CANOPY); + if (canopyActive) { + throw JSONRPCError(RPC_VERIFY_REJECTED, "Sprout shielding is not supported after Canopy"); + } + } } else { throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ")+address ); } @@ -4424,6 +4440,18 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ") + destaddress ); } + int nextBlockHeight = chainActive.Height() + 1; + const bool canopyActive = Params().GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_CANOPY); + + if (canopyActive) { + auto decodeAddr = DecodePaymentAddress(destaddress); + bool isToSproutZaddr = (boost::get(&decodeAddr) != nullptr); + + if (isToSproutZaddr) { + throw JSONRPCError(RPC_VERIFY_REJECTED, "Sprout shielding is not supported after Canopy activation"); + } + } + // Convert fee from currency format to zatoshis CAmount nFee = SHIELD_COINBASE_DEFAULT_MINERS_FEE; if (params.size() > 2) { @@ -4442,7 +4470,6 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) } } - int nextBlockHeight = chainActive.Height() + 1; const bool saplingActive = Params().GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_SAPLING); // We cannot create shielded transactions before Sapling activates. @@ -4644,6 +4671,8 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) // Keep track of addresses to spot duplicates std::set setAddress; + bool isFromNonSprout = false; + // Sources for (const UniValue& o : addresses.getValues()) { if (!o.isStr()) @@ -4653,18 +4682,24 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) if (address == "ANY_TADDR") { useAnyUTXO = true; + isFromNonSprout = true; } else if (address == "ANY_SPROUT") { useAnySprout = true; } else if (address == "ANY_SAPLING") { useAnySapling = true; + isFromNonSprout = true; } else { CTxDestination taddr = DecodeDestination(address); if (IsValidDestination(taddr)) { taddrs.insert(taddr); + isFromNonSprout = true; } else { auto zaddr = DecodePaymentAddress(address); if (IsValidPaymentAddress(zaddr)) { zaddrs.insert(zaddr); + if (boost::get(&zaddr) != nullptr) { + isFromNonSprout = true; + } } else { throw JSONRPCError(RPC_INVALID_PARAMETER, string("Unknown address format: ") + address); } @@ -4686,6 +4721,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) const int nextBlockHeight = chainActive.Height() + 1; const bool overwinterActive = Params().GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_OVERWINTER); const bool saplingActive = Params().GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_SAPLING); + const bool canopyActive = Params().GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_CANOPY); // Validate the destination address auto destaddress = params[1].get_str(); @@ -4709,6 +4745,11 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) } } + if (canopyActive && isFromNonSprout && isToSproutZaddr) { + // Value can be moved within Sprout, but not into Sprout. + throw JSONRPCError(RPC_VERIFY_REJECTED, "Sprout shielding is not supported after Canopy"); + } + // Convert fee from currency format to zatoshis CAmount nFee = SHIELD_COINBASE_DEFAULT_MINERS_FEE; if (params.size() > 2) { From 5e4d13b49db3c3b5e5669bf2d663ca79c2577e8f Mon Sep 17 00:00:00 2001 From: therealyingtong Date: Fri, 15 May 2020 05:16:15 +0800 Subject: [PATCH 4/6] Add RPC tests Test that Sprout shielding is accepted before Canopy and rejected after Canopy activation --- qa/pull-tester/rpc-tests.sh | 1 + qa/rpc-tests/remove_sprout_shielding.py | 129 ++++++++++++++++++++++++ 2 files changed, 130 insertions(+) create mode 100755 qa/rpc-tests/remove_sprout_shielding.py diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh index b6454a304..226d871e8 100755 --- a/qa/pull-tester/rpc-tests.sh +++ b/qa/pull-tester/rpc-tests.sh @@ -88,6 +88,7 @@ testScripts=( 'upgrade_golden.py' 'post_heartwood_rollback.py' 'feature_logging.py' + 'remove_sprout_shielding.py' ); testScriptsExt=( 'getblocktemplate_longpoll.py' diff --git a/qa/rpc-tests/remove_sprout_shielding.py b/qa/rpc-tests/remove_sprout_shielding.py new file mode 100755 index 000000000..1cf2ed0af --- /dev/null +++ b/qa/rpc-tests/remove_sprout_shielding.py @@ -0,0 +1,129 @@ +#!/usr/bin/env python3 +# Copyright (c) 2019 The Zcash developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or https://www.opensource.org/licenses/mit-license.php . + +from decimal import Decimal +from test_framework.authproxy import JSONRPCException +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + assert_raises, + bitcoind_processes, + connect_nodes_bi, + initialize_chain, + start_nodes, start_node, get_coinbase_address, + wait_and_assert_operationid_status, + check_node_log, + nuparams, BLOSSOM_BRANCH_ID, HEARTWOOD_BRANCH_ID, CANOPY_BRANCH_ID +) + +import logging +import time + +HAS_CANOPY = ['-nurejectoldversions=false', + nuparams(BLOSSOM_BRANCH_ID, 205), + nuparams(HEARTWOOD_BRANCH_ID, 210), + nuparams(CANOPY_BRANCH_ID, 220), +] +class RemoveSproutShieldingTest (BitcoinTestFramework): + + def setup_chain(self): + print("Initializing test directory "+self.options.tmpdir) + initialize_chain(self.options.tmpdir) + + def setup_nodes(self): + return start_nodes(4, self.options.tmpdir, extra_args=[HAS_CANOPY]*4) + + def run_test (self): + + # Generate blocks up to Heartwood activation + logging.info("Generating initial blocks. Current height is 200, advance to 210 (activate Heartwood but not Canopy)") + self.nodes[0].generate(10) + self.sync_all() + + # Shield coinbase to Sprout on node 0. Should pass + sprout_addr = self.nodes[0].z_getnewaddress('sprout') + myopid = self.nodes[0].z_shieldcoinbase(get_coinbase_address(self.nodes[0]), sprout_addr, 0)['opid'] + wait_and_assert_operationid_status(self.nodes[0], myopid) + print("taddr -> Sprout z_shieldcoinbase tx accepted before Canopy on node 0") + + self.nodes[0].generate(1) + self.sync_all() + assert_equal(self.nodes[0].z_getbalance(sprout_addr), Decimal('10')) + + # Fund taddr_0 from shielded coinbase on node 0 + taddr_0 = self.nodes[0].getnewaddress() + for _ in range(3): + recipients = [{"address": taddr_0, "amount": Decimal('1')}] + myopid = self.nodes[0].z_sendmany(sprout_addr, recipients, 1, 0) + wait_and_assert_operationid_status(self.nodes[0], myopid) + self.sync_all() + self.nodes[0].generate(1) + self.sync_all() + + # Create taddr -> Sprout transaction and mine on node 0 before it is Canopy-aware. Should pass + sendmany_tx_0 = self.nodes[0].z_sendmany(taddr_0, [{"address": self.nodes[1].z_getnewaddress('sprout'), "amount": 1}]) + wait_and_assert_operationid_status(self.nodes[0], sendmany_tx_0) + print("taddr -> Sprout z_sendmany tx accepted before Canopy on node 0") + + self.nodes[0].generate(1) + self.sync_all() + + # Create mergetoaddress taddr -> Sprout transaction and mine on node 0 before it is Canopy-aware. Should pass + merge_tx_0 = self.nodes[0].z_mergetoaddress(["ANY_TADDR"], self.nodes[1].z_getnewaddress('sprout')) + wait_and_assert_operationid_status(self.nodes[0], merge_tx_0['opid']) + print("taddr -> Sprout z_mergetoaddress tx accepted before Canopy on node 0") + + # Mine to one block before Canopy activation on node 0; adding value + # to the Sprout pool will fail now since the transaction must be + # included in the next (or later) block, after Canopy has activated. + self.nodes[0].generate(4) + self.sync_all() + + # Shield coinbase to Sprout on node 0. Should fail + errorString = '' + try: + sprout_addr = self.nodes[0].z_getnewaddress('sprout') + self.nodes[0].z_shieldcoinbase(get_coinbase_address(self.nodes[0]), sprout_addr, 0) + except JSONRPCException as e: + errorString = e.error['message'] + assert("Sprout shielding is not supported after Canopy" in errorString) + print("taddr -> Sprout z_shieldcoinbase tx rejected at Canopy activation on node 0") + + # Create taddr -> Sprout z_sendmany transaction on node 0. Should fail + errorString = '' + try: + sprout_addr = self.nodes[1].z_getnewaddress('sprout') + self.nodes[0].z_sendmany(taddr_0, [{"address": sprout_addr, "amount": 1}]) + except JSONRPCException as e: + errorString = e.error['message'] + assert("Sprout shielding is not supported after Canopy" in errorString) + print("taddr -> Sprout z_sendmany tx rejected at Canopy activation on node 0") + + # Create z_mergetoaddress [taddr, Sprout] -> Sprout transaction on node 0. Should fail + errorString = '' + try: + self.nodes[0].z_mergetoaddress(["ANY_TADDR", "ANY_SPROUT"], self.nodes[1].z_getnewaddress('sprout')) + except JSONRPCException as e: + errorString = e.error['message'] + assert("Sprout shielding is not supported after Canopy" in errorString) + print("[taddr, Sprout] -> Sprout z_mergetoaddress tx rejected at Canopy activation on node 0") + + # Create z_mergetoaddress Sprout -> Sprout transaction on node 0. Should pass + merge_tx_1 = self.nodes[0].z_mergetoaddress(["ANY_SPROUT"], self.nodes[1].z_getnewaddress('sprout')) + wait_and_assert_operationid_status(self.nodes[0], merge_tx_1['opid']) + print("Sprout -> Sprout z_mergetoaddress tx accepted at Canopy activation on node 0") + + self.nodes[0].generate(1) + self.sync_all() + + # Shield coinbase to Sapling on node 0. Should pass + sapling_addr = self.nodes[0].z_getnewaddress('sapling') + myopid = self.nodes[0].z_shieldcoinbase(get_coinbase_address(self.nodes[0]), sapling_addr, 0)['opid'] + wait_and_assert_operationid_status(self.nodes[0], myopid) + print("taddr -> Sapling z_shieldcoinbase tx accepted after Canopy on node 0") + +if __name__ == '__main__': + RemoveSproutShieldingTest().main() + From 7dd1889944e51dccdbc45c0d5a8fe90daa311608 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 29 Jun 2020 16:24:25 -0600 Subject: [PATCH 5/6] Trivial copyright fix. Co-authored-by: str4d --- qa/rpc-tests/remove_sprout_shielding.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/qa/rpc-tests/remove_sprout_shielding.py b/qa/rpc-tests/remove_sprout_shielding.py index 1cf2ed0af..ad397540a 100755 --- a/qa/rpc-tests/remove_sprout_shielding.py +++ b/qa/rpc-tests/remove_sprout_shielding.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -# Copyright (c) 2019 The Zcash developers +# Copyright (c) 2020 The Zcash developers # Distributed under the MIT software license, see the accompanying # file COPYING or https://www.opensource.org/licenses/mit-license.php . @@ -126,4 +126,3 @@ class RemoveSproutShieldingTest (BitcoinTestFramework): if __name__ == '__main__': RemoveSproutShieldingTest().main() - From 14a09a5fd68591a0149ae63127ebc14e7b9eb1e8 Mon Sep 17 00:00:00 2001 From: Sean Bowe Date: Thu, 2 Jul 2020 11:09:22 -0600 Subject: [PATCH 6/6] Remove unused imports from remove_sprout_shielding RPC test. --- qa/rpc-tests/remove_sprout_shielding.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/qa/rpc-tests/remove_sprout_shielding.py b/qa/rpc-tests/remove_sprout_shielding.py index ad397540a..2aa5bbcae 100755 --- a/qa/rpc-tests/remove_sprout_shielding.py +++ b/qa/rpc-tests/remove_sprout_shielding.py @@ -8,18 +8,13 @@ from test_framework.authproxy import JSONRPCException from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, - assert_raises, - bitcoind_processes, - connect_nodes_bi, initialize_chain, - start_nodes, start_node, get_coinbase_address, + start_nodes, get_coinbase_address, wait_and_assert_operationid_status, - check_node_log, nuparams, BLOSSOM_BRANCH_ID, HEARTWOOD_BRANCH_ID, CANOPY_BRANCH_ID ) import logging -import time HAS_CANOPY = ['-nurejectoldversions=false', nuparams(BLOSSOM_BRANCH_ID, 205),