Auto merge of #3910 - Eirik0:3909-fix-mergetoaddress, r=Eirik0

Fix z_mergetoaddress sending from ANY_SPROUT/ANY_SAPLING when the wallet contains both note types

Closes #3909

qa/pull-tester/rpc-tests.sh

@@ -31,6 +31,7 @@ testScripts=(
     'wallet_listnotes.py'
     'mergetoaddress_sprout.py'
     'mergetoaddress_sapling.py'
+    'mergetoaddress_mixednotes.py'
     'listtransactions.py'
     'mempool_resurrect_test.py'
     'txn_doublespend.py'

qa/rpc-tests/mergetoaddress_helper.py

@@ -17,11 +17,12 @@ from decimal import Decimal
 def assert_mergetoaddress_exception(expected_error_msg, merge_to_address_lambda):
     try:
         merge_to_address_lambda()
-        fail("Expected exception: %s" % expected_error_msg)
     except JSONRPCException as e:
         assert_equal(expected_error_msg, e.error['message'])
     except Exception as e:
         fail("Expected JSONRPCException. Found %s" % repr(e))
+    else:
+        fail("Expected exception: %s" % expected_error_msg)
 
 
 class MergeToAddressHelper:
@@ -152,6 +153,11 @@ class MergeToAddressHelper:
             "Destination address is also the only source address, and all its funds are already merged.",
             lambda: test.nodes[0].z_mergetoaddress([mytaddr], mytaddr))
 
+        # Merging will fail for this specific case where it would spend a fee and do nothing
+        assert_mergetoaddress_exception(
+            "Cannot send from both Sprout and Sapling addresses using z_mergetoaddress",
+            lambda: test.nodes[0].z_mergetoaddress(["ANY_SPROUT", "ANY_SAPLING"], mytaddr))
+
         # Merge UTXOs from node 0 of value 30, standard fee of 0.00010000
         result = test.nodes[0].z_mergetoaddress([mytaddr, mytaddr2, mytaddr3], myzaddr)
         wait_and_assert_operationid_status(test.nodes[0], result['opid'])

qa/rpc-tests/mergetoaddress_mixednotes.py

@@ -0,0 +1,82 @@
+#!/usr/bin/env python
+# Copyright (c) 2019 The Zcash developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+import sys; assert sys.version_info < (3,), ur"This script does not run under Python 3. Please use Python 2.7.x."
+
+from decimal import Decimal
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal, get_coinbase_address, \
+    initialize_chain_clean, start_nodes, wait_and_assert_operationid_status
+from mergetoaddress_helper import assert_mergetoaddress_exception
+
+
+class MergeToAddressMixedNotes(BitcoinTestFramework):
+    def setup_nodes(self):
+        return start_nodes(4, self.options.tmpdir, [[
+            '-nuparams=5ba81b19:100',  # Overwinter
+            '-nuparams=76b809bb:100',  # Sapling
+            '-experimentalfeatures', '-zmergetoaddress'
+        ]] * 4)
+
+    def setup_chain(self):
+        print("Initializing test directory " + self.options.tmpdir)
+        initialize_chain_clean(self.options.tmpdir, 4)
+
+    def run_test(self):
+        print "Mining blocks..."
+        self.nodes[0].generate(102)
+        self.sync_all()
+
+        # Send some ZEC to Sprout/Sapling addresses
+        coinbase_addr = get_coinbase_address(self.nodes[0])
+        sproutAddr = self.nodes[0].z_getnewaddress('sprout')
+        saplingAddr = self.nodes[0].z_getnewaddress('sapling')
+        t_addr = self.nodes[1].getnewaddress()
+
+        opid = self.nodes[0].z_sendmany(coinbase_addr, [{"address": sproutAddr, "amount": Decimal('10')}], 1, 0)
+        wait_and_assert_operationid_status(self.nodes[0], opid)
+        self.nodes[0].generate(1)
+        self.sync_all()
+        assert_equal(self.nodes[0].z_getbalance(sproutAddr), Decimal('10'))
+        assert_equal(self.nodes[0].z_getbalance(saplingAddr), Decimal('0'))
+        assert_equal(Decimal(self.nodes[1].z_gettotalbalance()["transparent"]), Decimal('0'))
+        # Make sure we cannot use "ANY_SPROUT" and "ANY_SAPLING" even if we only have Sprout Notes
+        assert_mergetoaddress_exception(
+            "Cannot send from both Sprout and Sapling addresses using z_mergetoaddress",
+            lambda: self.nodes[0].z_mergetoaddress(["ANY_SPROUT", "ANY_SAPLING"], t_addr))
+        opid = self.nodes[0].z_sendmany(coinbase_addr, [{"address": saplingAddr, "amount": Decimal('10')}], 1, 0)
+        wait_and_assert_operationid_status(self.nodes[0], opid)
+        self.nodes[0].generate(1)
+        self.sync_all()
+
+        assert_equal(Decimal(self.nodes[1].z_gettotalbalance()["transparent"]), Decimal('0'))
+
+        # Merge Sprout -> taddr
+        result = self.nodes[0].z_mergetoaddress(["ANY_SPROUT"], t_addr, 0)
+        wait_and_assert_operationid_status(self.nodes[0], result['opid'])
+        self.nodes[0].generate(1)
+        self.sync_all()
+
+        assert_equal(self.nodes[0].z_getbalance(sproutAddr), Decimal('0'))
+        assert_equal(self.nodes[0].z_getbalance(saplingAddr), Decimal('10'))
+        assert_equal(Decimal(self.nodes[1].z_gettotalbalance()["transparent"]), Decimal('10'))
+
+        # Make sure we cannot use "ANY_SPROUT" and "ANY_SAPLING" even if we only have Sapling Notes
+        assert_mergetoaddress_exception(
+            "Cannot send from both Sprout and Sapling addresses using z_mergetoaddress",
+            lambda: self.nodes[0].z_mergetoaddress(["ANY_SPROUT", "ANY_SAPLING"], t_addr))
+        # Merge Sapling -> taddr
+        result = self.nodes[0].z_mergetoaddress(["ANY_SAPLING"], t_addr, 0)
+        wait_and_assert_operationid_status(self.nodes[0], result['opid'])
+        self.nodes[0].generate(1)
+        self.sync_all()
+
+        assert_equal(self.nodes[0].z_getbalance(sproutAddr), Decimal('0'))
+        assert_equal(self.nodes[0].z_getbalance(saplingAddr), Decimal('0'))
+        assert_equal(Decimal(self.nodes[1].z_gettotalbalance()["transparent"]), Decimal('20'))
+
+
+if __name__ == '__main__':
+    MergeToAddressMixedNotes().main()

src/wallet/rpcwallet.cpp

@@ -4297,7 +4297,9 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
             "                             - \"ANY_TADDR\":   Merge UTXOs from any taddrs belonging to the wallet.\n"
             "                             - \"ANY_SPROUT\":  Merge notes from any Sprout zaddrs belonging to the wallet.\n"
             "                             - \"ANY_SAPLING\": Merge notes from any Sapling zaddrs belonging to the wallet.\n"
-            "                         If a special string is given, any given addresses of that type will be counted as duplicates and cause an error.\n"
+            "                         While it is possible to use a variety of different combinations of addresses and the above values,\n"
+            "                         it is not possible to send funds from both sprout and sapling addresses simultaneously. If a special\n"
+            "                         string is given, any given addresses of that type will be counted as duplicates and cause an error.\n"
             "    [\n"
             "      \"address\"          (string) Can be a taddr or a zaddr\n"
             "      ,...\n"
@@ -4532,8 +4534,15 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
         if (!saplingActive && saplingEntries.size() > 0) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, Sapling has not activated");
         }
+        // Do not include Sprout/Sapling notes if using "ANY_SAPLING"/"ANY_SPROUT" respectively
+        if (useAnySprout) {
+            saplingEntries.clear();
+        }
+        if (useAnySapling) {
+            sproutEntries.clear();
+        }
         // Sending from both Sprout and Sapling is currently unsupported using z_mergetoaddress
-        if (sproutEntries.size() > 0 && saplingEntries.size() > 0) {
+        if ((sproutEntries.size() > 0 && saplingEntries.size() > 0) || (useAnySprout && useAnySapling)) {
             throw JSONRPCError(
                 RPC_INVALID_PARAMETER,
                 "Cannot send from both Sprout and Sapling addresses using z_mergetoaddress");