Merge #12953: Deprecate accounts

cead28b [docs] Add release notes for deprecated 'account' API (John Newbery)
72c9575 [wallet] [tests] Add tests for accounts/labels APIs (John Newbery)
109e05d [wallet] [rpc] Deprecate wallet 'account' API (John Newbery)
3576ab1 [wallet] [rpc] Deprecate account RPC methods (John Newbery)
3db1ba0 [tests] Set -deprecatedrpc=accounts in tests (John Newbery)
4e671f0 [tests] Rename rpc_listtransactions.py to wallet_listtransactions.py (John Newbery)
a28b907 [wallet] [rpc] Remove duplicate entries in rpcwallet.cpp's CRPCCommand table (John Newbery)

Pull request description:

  Deprecate all accounts functionality and make it only accessible by using `-deprecatedrpc=accounts`.

  Accounts specific RPCs, account arguments, and account related results all require the `-deprecatedrpc=accunts` startup option now in order to see account things.

  Several wallet functional tests use the accounts system. Those tests are unchanged, except to start the nodes with `-deprecatedrpc=accounts`. We can slowly migrate those tests to use the 'label' API instead of the 'account' API before accounts are fully removed.

Tree-SHA512: 89f4ae2fe6de4a1422f1817b0997ae22d63ab5a1a558362ce923a3871f3e42963405d6573c69c27f1764679cdee5b51bf52202cc407f1361bfd8066d652f3f37

Author: laanwj

doc/release-notes-pr12892.md

@@ -1,8 +1,10 @@
-'label' API for wallet
-----------------------
+'label' and 'account' APIs for wallet
+-------------------------------------
 
 A new 'label' API has been introduced for the wallet. This is intended as a
-replacement for the deprecated 'account' API.
+replacement for the deprecated 'account' API. The 'account' can continue to
+be used in V0.17 by starting bitcoind with the '-deprecatedrpc=accounts'
+argument, and will be fully removed in V0.18.
 
 The label RPC methods mirror the account functionality, with the following functional differences:
 
@@ -27,6 +29,9 @@ Here are the changes to RPC methods:
 
 | Changed Method         | Notes   |
 | :--------------------- | :------ |
-| `addmultisigaddress`   | Renamed `account` named parameter to `label`. Still accepts `account` for backward compatibility. |
-| `getnewaddress`        | Renamed `account` named parameter to `label`. Still accepts `account` for backward compatibility. |
-| `listunspent`          | Returns new `label` fields, along with `account` fields for backward compatibility. |
+| `addmultisigaddress`   | Renamed `account` named parameter to `label`. Still accepts `account` for backward compatibility if running with '-deprecatedrpc=accounts'. |
+| `getnewaddress`        | Renamed `account` named parameter to `label`. Still accepts `account` for backward compatibility. if running with '-deprecatedrpc=accounts' |
+| `listunspent`          | Returns new `label` fields. `account` field will be returned for backward compatibility if running with '-deprecatedrpc=accounts' |
+| `sendmany`             | The `account` named parameter has been renamed to `dummy`. If provided, the `dummy` parameter must be set to the empty string, unless running with the `-deprecatedrpc=accounts` argument (in which case functionality is unchanged). |
+| `listtransactions`     | The `account` named parameter has been renamed to `dummy`. If provided, the `dummy` parameter must be set to the string `*`, unless running with the `-deprecatedrpc=accounts` argument (in which case functionality is unchanged). |
+| `getbalance`           | `account`, `minconf` and `include_watchonly` parameters are deprecated, and can only be used if running with '-deprecatedrpc=accounts' |

src/wallet/rpcwallet.cpp

@@ -4,12 +4,13 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test deprecation of RPC calls."""
 from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_raises_rpc_error
 
 class DeprecatedRpcTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
         self.setup_clean_chain = True
-        self.extra_args = [[], ["-deprecatedrpc=validateaddress"]]
+        self.extra_args = [[], ["-deprecatedrpc=validateaddress", "-deprecatedrpc=accounts"]]
 
     def run_test(self):
         # This test should be used to verify correct behaviour of deprecated
@@ -20,11 +21,92 @@ def run_test(self):
         # self.nodes[1].createmultisig(1, [self.nodes[1].getnewaddress()])
 
         self.log.info("Test validateaddress deprecation")
-        SOME_ADDRESS = "mnvGjUy3NMj67yJ6gkK5o9e5RS33Z2Vqcu" # This is just some random address to pass as a parameter to validateaddress
+        SOME_ADDRESS = "mnvGjUy3NMj67yJ6gkK5o9e5RS33Z2Vqcu"  # This is just some random address to pass as a parameter to validateaddress
         dep_validate_address = self.nodes[0].validateaddress(SOME_ADDRESS)
         assert "ismine" not in dep_validate_address
         not_dep_val = self.nodes[1].validateaddress(SOME_ADDRESS)
         assert "ismine" in not_dep_val
 
+        self.log.info("Test accounts deprecation")
+        # The following account RPC methods are deprecated:
+        # - getaccount
+        # - getaccountaddress
+        # - getaddressesbyaccount
+        # - getreceivedbyaccount
+        # - listaccouts
+        # - listreceivedbyaccount
+        # - move
+        # - setaccount
+        #
+        # The following 'label' RPC methods are usable both with and without the
+        # -deprecatedrpc=accounts switch enabled.
+        # - getlabeladdress
+        # - getaddressesbylabel
+        # - getreceivedbylabel
+        # - listlabels
+        # - listreceivedbylabel
+        # - setlabel
+        #
+        address0 = self.nodes[0].getnewaddress()
+        self.nodes[0].generatetoaddress(101, address0)
+        address1 = self.nodes[1].getnewaddress()
+        self.nodes[1].generatetoaddress(101, address1)
+
+        self.log.info("- getaccount")
+        assert_raises_rpc_error(-32, "getaccount is deprecated", self.nodes[0].getaccount, address0)
+        self.nodes[1].getaccount(address1)
+
+        self.log.info("- setaccount")
+        assert_raises_rpc_error(-32, "setaccount is deprecated", self.nodes[0].setaccount, address0, "label0")
+        self.nodes[1].setaccount(address1, "label1")
+
+        self.log.info("- setlabel")
+        self.nodes[0].setlabel(address0, "label0")
+        self.nodes[1].setlabel(address1, "label1")
+
+        self.log.info("- getaccountaddress")
+        assert_raises_rpc_error(-32, "getaccountaddress is deprecated", self.nodes[0].getaccountaddress, "label0")
+        self.nodes[1].getaccountaddress("label1")
+
+        self.log.info("- getlabeladdress")
+        self.nodes[0].getlabeladdress("label0")
+        self.nodes[1].getlabeladdress("label1")
+
+        self.log.info("- getaddressesbyaccount")
+        assert_raises_rpc_error(-32, "getaddressesbyaccount is deprecated", self.nodes[0].getaddressesbyaccount, "label0")
+        self.nodes[1].getaddressesbyaccount("label1")
+
+        self.log.info("- getaddressesbylabel")
+        self.nodes[0].getaddressesbylabel("label0")
+        self.nodes[1].getaddressesbylabel("label1")
+
+        self.log.info("- getreceivedbyaccount")
+        assert_raises_rpc_error(-32, "getreceivedbyaccount is deprecated", self.nodes[0].getreceivedbyaccount, "label0")
+        self.nodes[1].getreceivedbyaccount("label1")
+
+        self.log.info("- getreceivedbylabel")
+        self.nodes[0].getreceivedbylabel("label0")
+        self.nodes[1].getreceivedbylabel("label1")
+
+        self.log.info("- listaccounts")
+        assert_raises_rpc_error(-32, "listaccounts is deprecated", self.nodes[0].listaccounts)
+        self.nodes[1].listaccounts()
+
+        self.log.info("- listlabels")
+        self.nodes[0].listlabels()
+        self.nodes[1].listlabels()
+
+        self.log.info("- listreceivedbyaccount")
+        assert_raises_rpc_error(-32, "listreceivedbyaccount is deprecated", self.nodes[0].listreceivedbyaccount)
+        self.nodes[1].listreceivedbyaccount()
+
+        self.log.info("- listreceivedbylabel")
+        self.nodes[0].listreceivedbylabel()
+        self.nodes[1].listreceivedbylabel()
+
+        self.log.info("- move")
+        assert_raises_rpc_error(-32, "move is deprecated", self.nodes[0].move, "label0", "label0b", 10)
+        self.nodes[1].move("label1", "label1b", 10)
+
 if __name__ == '__main__':
     DeprecatedRpcTest().main()

test/functional/rpc_deprecated.py

@@ -70,7 +70,7 @@
     'wallet_labels.py',
     'p2p_segwit.py',
     'wallet_dump.py',
-    'rpc_listtransactions.py',
+    'wallet_listtransactions.py',
     # vv Tests less than 60s vv
     'p2p_sendheaders.py',
     'wallet_zapwallettxes.py',

test/functional/test_runner.py

@@ -10,9 +10,10 @@ class WalletTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 4
         self.setup_clean_chain = True
+        self.extra_args = [['-deprecatedrpc=accounts']] * 4
 
     def setup_network(self):
-        self.add_nodes(4)
+        self.add_nodes(4, self.extra_args)
         self.start_node(0)
         self.start_node(1)
         self.start_node(2)
@@ -376,9 +377,9 @@ def run_test(self):
             self.log.info("check " + m)
             self.stop_nodes()
             # set lower ancestor limit for later
-            self.start_node(0, [m, "-limitancestorcount="+str(chainlimit)])
-            self.start_node(1, [m, "-limitancestorcount="+str(chainlimit)])
-            self.start_node(2, [m, "-limitancestorcount="+str(chainlimit)])
+            self.start_node(0, [m, "-deprecatedrpc=accounts", "-limitancestorcount="+str(chainlimit)])
+            self.start_node(1, [m, "-deprecatedrpc=accounts", "-limitancestorcount="+str(chainlimit)])
+            self.start_node(2, [m, "-deprecatedrpc=accounts", "-limitancestorcount="+str(chainlimit)])
             if m == '-reindex':
                 # reindex will leave rpc warm up "early"; Wait for it to finish
                 wait_until(lambda: [block_count] * 3 == [self.nodes[i].getblockcount() for i in range(3)])
@@ -426,7 +427,7 @@ def run_test(self):
         # Try with walletrejectlongchains
         # Double chain limit but require combining inputs, so we pass SelectCoinsMinConf
         self.stop_node(0)
-        self.start_node(0, extra_args=["-walletrejectlongchains", "-limitancestorcount="+str(2*chainlimit)])
+        self.start_node(0, extra_args=["-deprecatedrpc=accounts", "-walletrejectlongchains", "-limitancestorcount="+str(2*chainlimit)])
 
         # wait for loadmempool
         timeout = 10

test/functional/wallet_basic.py

@@ -119,7 +119,7 @@ def set_test_params(self):
         self.num_nodes = 2 + len(IMPORT_NODES)
 
     def setup_network(self):
-        extra_args = [["-addresstype=legacy"] for _ in range(self.num_nodes)]
+        extra_args = [["-addresstype=legacy", '-deprecatedrpc=accounts'] for _ in range(self.num_nodes)]
         for i, import_node in enumerate(IMPORT_NODES, 2):
             if import_node.prune:
                 extra_args[i] += ["-prune=1"]

test/functional/wallet_import_rescan.py

@@ -10,6 +10,7 @@ class ImportPrunedFundsTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 2
+        self.extra_args = [['-deprecatedrpc=accounts']] * 2
 
     def run_test(self):
         self.log.info("Mining blocks...")

test/functional/wallet_importprunedfunds.py

@@ -25,7 +25,7 @@ class KeypoolRestoreTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 2
-        self.extra_args = [[], ['-keypool=100', '-keypoolmin=20']]
+        self.extra_args = [['-deprecatedrpc=accounts'], ['-deprecatedrpc=accounts', '-keypool=100', '-keypoolmin=20']]
 
     def run_test(self):
         wallet_path = os.path.join(self.nodes[1].datadir, "regtest", "wallets", "wallet.dat")

test/functional/wallet_keypool_topup.py

@@ -6,25 +6,38 @@
 
 RPCs tested are:
     - getlabeladdress
-    - getaddressesbyaccount
+    - getaddressesbyaccount/getaddressesbylabel
     - listaddressgroupings
     - setlabel
     - sendfrom (with account arguments)
     - move (with account arguments)
+
+Run the test twice - once using the accounts API and once using the labels API.
+The accounts API test can be removed in V0.18.
 """
 from collections import defaultdict
 
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal
+from test_framework.util import assert_equal, assert_raises_rpc_error
 
 class WalletLabelsTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
-        self.num_nodes = 1
-        self.extra_args = [[]]
+        self.num_nodes = 2
+        self.extra_args = [['-deprecatedrpc=accounts'], []]
+
+    def setup_network(self):
+        """Don't connect nodes."""
+        self.setup_nodes()
 
     def run_test(self):
-        node = self.nodes[0]
+        """Run the test twice - once using the accounts API and once using the labels API."""
+        self.log.info("Test accounts API")
+        self._run_subtest(True, self.nodes[0])
+        self.log.info("Test labels API")
+        self._run_subtest(False, self.nodes[1])
+
+    def _run_subtest(self, accounts_api, node):
         # Check that there's no UTXO on any of the nodes
         assert_equal(len(node.listunspent()), 0)
 
@@ -77,7 +90,7 @@ def run_test(self):
 
         # Create labels and make sure subsequent label API calls
         # recognize the label/address associations.
-        labels = [Label(name) for name in ("a", "b", "c", "d", "e")]
+        labels = [Label(name, accounts_api) for name in ("a", "b", "c", "d", "e")]
         for label in labels:
             label.add_receive_address(node.getlabeladdress(label=label.name, force=True))
             label.verify(node)
@@ -101,29 +114,34 @@ def run_test(self):
 
         # Check that sendfrom label reduces listaccounts balances.
         for i, label in enumerate(labels):
-            to_label = labels[(i+1) % len(labels)]
+            to_label = labels[(i + 1) % len(labels)]
             node.sendfrom(label.name, to_label.receive_address, amount_to_send)
         node.generate(1)
         for label in labels:
             label.add_receive_address(node.getlabeladdress(label.name))
             label.verify(node)
             assert_equal(node.getreceivedbylabel(label.name), 2)
-            node.move(label.name, "", node.getbalance(label.name))
+            if accounts_api:
+                node.move(label.name, "", node.getbalance(label.name))
             label.verify(node)
         node.generate(101)
         expected_account_balances = {"": 5200}
         for label in labels:
             expected_account_balances[label.name] = 0
-        assert_equal(node.listaccounts(), expected_account_balances)
-        assert_equal(node.getbalance(""), 5200)
+        if accounts_api:
+            assert_equal(node.listaccounts(), expected_account_balances)
+            assert_equal(node.getbalance(""), 5200)
 
         # Check that setlabel can assign a label to a new unused address.
         for label in labels:
             address = node.getlabeladdress(label="", force=True)
             node.setlabel(address, label.name)
             label.add_address(address)
             label.verify(node)
-            assert(address not in node.getaddressesbyaccount(""))
+            if accounts_api:
+                assert(address not in node.getaddressesbyaccount(""))
+            else:
+                assert_raises_rpc_error(-11, "No addresses with label", node.getaddressesbylabel, "")
 
         # Check that addmultisigaddress can assign labels.
         for label in labels:
@@ -136,8 +154,9 @@ def run_test(self):
             label.verify(node)
             node.sendfrom("", multisig_address, 50)
         node.generate(101)
-        for label in labels:
-            assert_equal(node.getbalance(label.name), 50)
+        if accounts_api:
+            for label in labels:
+                assert_equal(node.getbalance(label.name), 50)
 
         # Check that setlabel can change the label of an address from a
         # different label.
@@ -156,9 +175,10 @@ def run_test(self):
         change_label(node, labels[2].receive_address, labels[2], labels[2])
 
 class Label:
-    def __init__(self, name):
+    def __init__(self, name, accounts_api):
         # Label name
         self.name = name
+        self.accounts_api = accounts_api
         # Current receiving address associated with this label.
         self.receive_address = None
         # List of all addresses assigned with this label
@@ -184,13 +204,16 @@ def verify(self, node):
                 node.getaddressinfo(address)['labels'][0],
                 {"name": self.name,
                  "purpose": self.purpose[address]})
-            assert_equal(node.getaccount(address), self.name)
+            if self.accounts_api:
+                assert_equal(node.getaccount(address), self.name)
+            else:
+                assert_equal(node.getaddressinfo(address)['label'], self.name)
 
         assert_equal(
             node.getaddressesbylabel(self.name),
             {address: {"purpose": self.purpose[address]} for address in self.addresses})
-        assert_equal(
-            set(node.getaddressesbyaccount(self.name)), set(self.addresses))
+        if self.accounts_api:
+            assert_equal(set(node.getaddressesbyaccount(self.name)), set(self.addresses))
 
 
 def change_label(node, address, old_label, new_label):

test/functional/wallet_labels.py

@@ -14,6 +14,7 @@
 class ReceivedByTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
+        self.extra_args = [['-deprecatedrpc=accounts']] * 2
 
     def run_test(self):
         # Generate block to get out of IBD