From de1b86a429204f87dc184f3f75a6beebe9bb6f57 Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 12 Oct 2018 22:21:00 -0700 Subject: [PATCH 1/2] For #3359. RPCs transferring funds return error if Sapling addresses are used before Sapling activation. --- qa/rpc-tests/wallet_sapling.py | 31 +++++++++++++++++++++++++++++-- src/wallet/rpcwallet.cpp | 20 ++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/qa/rpc-tests/wallet_sapling.py b/qa/rpc-tests/wallet_sapling.py index 1aecba5d3..3c8809a1d 100755 --- a/qa/rpc-tests/wallet_sapling.py +++ b/qa/rpc-tests/wallet_sapling.py @@ -19,17 +19,44 @@ class WalletSaplingTest(BitcoinTestFramework): def setup_nodes(self): return start_nodes(4, self.options.tmpdir, [[ '-nuparams=5ba81b19:201', # Overwinter - '-nuparams=76b809bb:201', # Sapling + '-nuparams=76b809bb:203', # Sapling ]] * 4) def run_test(self): # Sanity-check the test harness assert_equal(self.nodes[0].getblockcount(), 200) - # Activate Sapling + # Activate Overwinter self.nodes[2].generate(1) self.sync_all() + # Verify RPCs disallow Sapling value transfer if Sapling is not active + tmp_taddr = self.nodes[3].getnewaddress() + tmp_zaddr = self.nodes[3].z_getnewaddress('sapling') + try: + recipients = [] + recipients.append({"address": tmp_zaddr, "amount": Decimal('20')}) + self.nodes[3].z_sendmany(tmp_taddr, recipients, 1, 0) + raise AssertionError("Should have thrown an exception") + except JSONRPCException as e: + assert_equal("Invalid parameter, Sapling has not activated", e.error['message']) + try: + recipients = [] + recipients.append({"address": tmp_taddr, "amount": Decimal('20')}) + self.nodes[3].z_sendmany(tmp_zaddr, recipients, 1, 0) + raise AssertionError("Should have thrown an exception") + except JSONRPCException as e: + assert_equal("Invalid parameter, Sapling has not activated", e.error['message']) + try: + self.nodes[3].z_shieldcoinbase(tmp_taddr, tmp_zaddr) + raise AssertionError("Should have thrown an exception") + except JSONRPCException as e: + assert_equal("Invalid parameter, Sapling has not activated", e.error['message']) + + # Activate Sapling + self.nodes[2].generate(2) + self.sync_all() + taddr0 = self.nodes[0].getnewaddress() # Skip over the address containing node 1's coinbase self.nodes[1].getnewaddress() diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4bbf6271c..508c6dacc 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3795,6 +3795,13 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) } } + // If Sapling is not active, do not allow sending from or sending to Sapling addresses. + if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { + if (fromSapling || containsSaplingOutput) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling has not activated"); + } + } + // As a sanity check, estimate and verify that the size of the transaction will be valid. // Depending on the input notes, the actual tx size may turn out to be larger and perhaps invalid. size_t txsize = 0; @@ -3984,6 +3991,19 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; } + // If Sapling is not active, do not allow sending to a Sapling address. + if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { + auto res = DecodePaymentAddress(destaddress); + if (IsValidPaymentAddress(res)) { + bool toSapling = boost::get(&res) != nullptr; + if (toSapling) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling has not activated"); + } + } else { + throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ") + destaddress ); + } + } + // Prepare to get coinbase utxos std::vector inputs; CAmount shieldedValue = 0; From 61caa4661913c2971e6348ce9354bad9d7f88b55 Mon Sep 17 00:00:00 2001 From: Simon Date: Sat, 13 Oct 2018 08:10:10 -0700 Subject: [PATCH 2/2] For #3359. Return error if Sapling addresses passed to RPC z_mergetoaddress. RPC z_mergetoaddress does not support Sapling yet but the existing error reporting was not clear to users. --- qa/rpc-tests/wallet_sapling.py | 13 +++++++++++++ src/wallet/rpcwallet.cpp | 26 ++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/qa/rpc-tests/wallet_sapling.py b/qa/rpc-tests/wallet_sapling.py index 3c8809a1d..1abb1e8dc 100755 --- a/qa/rpc-tests/wallet_sapling.py +++ b/qa/rpc-tests/wallet_sapling.py @@ -20,6 +20,7 @@ class WalletSaplingTest(BitcoinTestFramework): return start_nodes(4, self.options.tmpdir, [[ '-nuparams=5ba81b19:201', # Overwinter '-nuparams=76b809bb:203', # Sapling + '-experimentalfeatures', '-zmergetoaddress', ]] * 4) def run_test(self): @@ -53,6 +54,18 @@ class WalletSaplingTest(BitcoinTestFramework): except JSONRPCException as e: assert_equal("Invalid parameter, Sapling has not activated", e.error['message']) + # Verify z_mergetoaddress RPC does not support Sapling yet + try: + self.nodes[3].z_mergetoaddress([tmp_taddr], tmp_zaddr) + raise AssertionError("Should have thrown an exception") + except JSONRPCException as e: + assert_equal("Invalid parameter, Sapling is not supported yet by z_mergetoadress", e.error['message']) + try: + self.nodes[3].z_mergetoaddress([tmp_zaddr], tmp_taddr) + raise AssertionError("Should have thrown an exception") + except JSONRPCException as e: + assert_equal("Invalid parameter, Sapling is not supported yet by z_mergetoadress", e.error['message']) + # Activate Sapling self.nodes[2].generate(2) self.sync_all() diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 508c6dacc..d6940aeb7 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4204,6 +4204,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) std::set setAddress; // Sources + bool containsSaplingZaddrSource = false; for (const UniValue& o : addresses.getValues()) { if (!o.isStr()) throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, expected string"); @@ -4229,6 +4230,9 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) if (!(useAny || useAnyNote)) { zaddrs.insert(zaddr); } + // Check if z-addr is Sapling + bool isSapling = boost::get(&zaddr) != nullptr; + containsSaplingZaddrSource |= isSapling; } else { throw JSONRPCError( RPC_INVALID_PARAMETER, @@ -4245,10 +4249,19 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) // Validate the destination address auto destaddress = params[1].get_str(); bool isToZaddr = false; + bool isToSaplingZaddr = false; CTxDestination taddr = DecodeDestination(destaddress); if (!IsValidDestination(taddr)) { if (IsValidPaymentAddressString(destaddress)) { isToZaddr = true; + + // Is this a Sapling address? + auto res = DecodePaymentAddress(destaddress); + if (IsValidPaymentAddress(res)) { + isToSaplingZaddr = boost::get(&res) != nullptr; + } else { + throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ") + destaddress ); + } } else { throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, unknown address format: ") + destaddress ); } @@ -4302,6 +4315,19 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) max_tx_size = MAX_TX_SIZE_BEFORE_SAPLING; } + // This RPC does not support Sapling yet. + if (isToSaplingZaddr || containsSaplingZaddrSource) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling is not supported yet by z_mergetoadress"); + } + + // If this RPC does support Sapling... + // If Sapling is not active, do not allow sending from or sending to Sapling addresses. + if (!NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_SAPLING)) { + if (isToSaplingZaddr || containsSaplingZaddrSource) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling has not activated"); + } + } + // Prepare to get UTXOs and notes std::vector utxoInputs; std::vector noteInputs;