Merge #17585: rpc: deprecate getaddressinfo label

d3bc18408146e91b3836f72360ff6fa2420b6887 doc: update release notes with getaddressinfo label deprecation (Jon Atack)
72af93f36479dc12d795f1d05fa3d8fbd9b293bd test: getaddressinfo label deprecation test (Jon Atack)
d48875fa20d0b71b978cb3d1f85dd9ec14e664cc rpc: deprecate getaddressinfo label field (Jon Atack)
dc0cabeda49a7edbfa71df22846721b6f6224aea test: remove getaddressinfo label tests (Jon Atack)
c7654af6f830577a54df12b5d65df93532db0dc2 doc: address pr17578 review feedback (Jon Atack)

Pull request description:

  This PR builds on #17578 (now merged) and deprecates the rpc getaddressinfo `label` field. The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=label`.

  See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001 for more context.

  Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=label` flag while verifying the rpc getaddressinfo output and help text.

  Next step: add support for multiple labels.

ACKs for top commit:
  jnewbery:
    ACK d3bc18408146e91b3836f72360ff6fa2420b6887
  laanwj:
    ACK d3bc18408146e91b3836f72360ff6fa2420b6887
  meshcollider:
    utACK d3bc18408146e91b3836f72360ff6fa2420b6887

Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d

doc/release-notes-17578.md

@@ -1,8 +1,13 @@
 Deprecated or removed RPCs
 --------------------------
 
-- The `getaddressinfo` RPC `labels` field now returns an array of label name
+- RPC `getaddressinfo` changes:
-  strings. Previously, it returned an array of JSON objects containing `name` and
+
-  `purpose` key/value pairs, which is now deprecated and will be removed in
+  - the `label` field has been deprecated in favor of the `labels` field and
-  0.21. To re-enable the previous behavior, launch bitcoind with
+    will be removed in 0.21. It can be re-enabled in the interim by launching
-  `-deprecatedrpc=labelspurpose`.
+    with `-deprecatedrpc=label`.
+
+  - the `labels` behavior of returning an array of JSON objects containing name
+    and purpose key/value pairs has been deprecated in favor of an array of
+    label names and will be removed in 0.21. The previous behavior can be
+    re-enabled in the interim by launching with `-deprecatedrpc=labelspurpose`.

src/wallet/rpcwallet.cpp

@@ -3651,7 +3651,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
                 {RPCResult::Type::NUM, "sigsrequired", /* optional */ true, "The number of signatures required to spend multisig output (only if script is multisig)."},
                 {RPCResult::Type::STR_HEX, "pubkey", /* optional */ true, "The hex value of the raw public key, for single-key addresses."},
                 {RPCResult::Type::BOOL, "iscompressed", /* optional */ true, "If the pubkey is compressed."},
-                {RPCResult::Type::STR, "label", "The label associated with the address. Defaults to \"\". Equivalent to the name label in the labels array below."},
+                {RPCResult::Type::STR, "label", "DEPRECATED. The label associated with the address. Defaults to \"\". Replaced by the labels array below."},
                 {RPCResult::Type::NUM_TIME, "timestamp", /* optional */ true, "The creation time of the key, if available, expressed in " + UNIX_EPOCH_TIME + "."},
                 {RPCResult::Type::STR_HEX, "hdchainid", /* optional */ true, "The ID of the HD chain."},
                 {RPCResult::Type::STR, "hdkeypath", /* optional */ true, "The HD keypath, if the key is HD and available."},
@@ -3659,10 +3659,10 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
                 {RPCResult::Type::STR_HEX, "hdmasterfingerprint", /* optional */ true, "The fingerprint of the master key."},
                 {RPCResult::Type::ARR, "labels", "An array of labels associated with the address. Currently limited to one label but returned as an array to keep the API stable if multiple labels are enabled in the future.",
                 {
-                    {RPCResult::Type::STR, "label name", "The label name. Defaults to \"\". Equivalent to the label field above."},
+                    {RPCResult::Type::STR, "label name", "The label name. Defaults to \"\"."},
                     {RPCResult::Type::OBJ, "", "json object of label data",
                     {
-                        {RPCResult::Type::STR, "name", "The label name. Defaults to \"\". Equivalent to the label field above."},
+                        {RPCResult::Type::STR, "name", "The label name. Defaults to \"\"."},
                         {RPCResult::Type::STR, "purpose", "The Purpose of the associated address (send or receive)."},
                     }},
                 }},
@@ -3710,10 +3710,10 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
     UniValue detail = DescribeWalletAddress(pwallet, dest);
     ret.pushKVs(detail);
 
-    // Return label field if existing. Currently only one label can be
+    // DEPRECATED: Return label field if existing. Currently only one label can
-    // associated with an address, so the label should be equivalent to the
+    // be associated with an address, so the label should be equivalent to the
     // value of the name key/value pair in the labels array below.
-    if (pwallet->mapAddressBook.count(dest)) {
+    if ((pwallet->chain().rpcEnableDeprecated("label")) && (pwallet->mapAddressBook.count(dest))) {
         ret.pushKV("label", pwallet->mapAddressBook.at(dest).name);
     }
 
@@ -3743,12 +3743,11 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
     // associated with an address, but we return an array so the API remains
     // stable if we allow multiple labels to be associated with an address in
     // the future.
-    //
-    // DEPRECATED: The previous behavior of returning an array containing a JSON
-    // object of `name` and `purpose` key/value pairs has been deprecated.
     UniValue labels(UniValue::VARR);
     std::map<CTxDestination, CAddressBookData>::const_iterator mi = pwallet->mapAddressBook.find(dest);
     if (mi != pwallet->mapAddressBook.end()) {
+        // DEPRECATED: The previous behavior of returning an array containing a
+        // JSON object of `name` and `purpose` key/value pairs is deprecated.
         if (pwallet->chain().rpcEnableDeprecated("labelspurpose")) {
             labels.push_back(AddressBookDataToJSON(mi->second, true));
         } else {

src/wallet/test/wallet_tests.cpp

@@ -310,10 +310,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
 // Verify getaddressinfo RPC produces more or less expected results
 BOOST_FIXTURE_TEST_CASE(rpc_getaddressinfo, TestChain100Setup)
 {
-    NodeContext node;
+    std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(m_node.chain.get(), "", CreateMockWalletDatabase());
-    auto chain = interfaces::MakeChain(node);
-
-    std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(chain.get(), "", CreateMockWalletDatabase());
     wallet->SetupLegacyScriptPubKeyMan();
     AddWallet(wallet);
     CoreContext context{m_node};
@@ -369,12 +366,15 @@ BOOST_FIXTURE_TEST_CASE(rpc_getaddressinfo, TestChain100Setup)
     BOOST_CHECK_EQUAL(find_value(response, "iswatchonly").get_bool(), false);
     BOOST_CHECK_EQUAL(find_value(response, "isscript").get_bool(), true);
     BOOST_CHECK_EQUAL(find_value(response, "ischange").get_bool(), false);
-    BOOST_CHECK_EQUAL(find_value(response, "label").get_str(), "");
     BOOST_CHECK_EQUAL(find_value(response, "sigsrequired").get_int(), 2);
+    BOOST_CHECK(find_value(response, "label").isNull());
 
+    UniValue labels = find_value(response, "labels").get_array();
     UniValue pubkeys = find_value(response, "pubkeys").get_array();
     UniValue addresses = find_value(response, "addresses").get_array();
 
+    BOOST_CHECK_EQUAL(labels.size(), 1);
+    BOOST_CHECK_EQUAL(labels[0].get_str(), "");
     BOOST_CHECK_EQUAL(addresses.size(), 2);
     BOOST_CHECK_EQUAL(addresses[0].get_str(), addr1);
     BOOST_CHECK_EQUAL(addresses[1].get_str(), addr2);

test/functional/rpc_getaddressinfo_label_deprecation.py

@@ -0,0 +1,43 @@
+#!/usr/bin/env python3
+# Copyright (c) 2020 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""
+Test deprecation of the RPC getaddressinfo `label` field. It has been
+superceded by the `labels` field.
+
+"""
+from test_framework.test_framework import BitcoinTestFramework
+
+class GetAddressInfoLabelDeprecationTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 2
+        self.setup_clean_chain = False
+        # Start node[0] with -deprecatedrpc=label, and node[1] without.
+        self.extra_args = [["-deprecatedrpc=label"], []]
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
+    def test_label_with_deprecatedrpc_flag(self):
+        self.log.info("Test getaddressinfo label with -deprecatedrpc flag")
+        node = self.nodes[0]
+        address = node.getnewaddress()
+        info = node.getaddressinfo(address)
+        assert "label" in info
+
+    def test_label_without_deprecatedrpc_flag(self):
+        self.log.info("Test getaddressinfo label without -deprecatedrpc flag")
+        node = self.nodes[1]
+        address = node.getnewaddress()
+        info = node.getaddressinfo(address)
+        assert "label" not in info
+
+    def run_test(self):
+        """Test getaddressinfo label with and without -deprecatedrpc flag."""
+        self.test_label_with_deprecatedrpc_flag()
+        self.test_label_without_deprecatedrpc_flag()
+
+
+if __name__ == '__main__':
+    GetAddressInfoLabelDeprecationTest().main()

test/functional/rpc_getaddressinfo_labels_purpose_deprecation.py

@@ -4,8 +4,8 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """
 Test deprecation of RPC getaddressinfo `labels` returning an array
-containing a JSON hash of `name` and purpose` key-value pairs. It now
+containing a JSON object of `name` and purpose` key-value pairs. It now
-returns an array of label names.
+returns an array containing only the label name.
 
 """
 from test_framework.test_framework import BitcoinTestFramework

test/functional/test_runner.py

@@ -266,6 +266,7 @@ BASE_SCRIPTS = [
     'feature_settings.py',
     'rpc_getdescriptorinfo.py',
     'rpc_getaddressinfo_labels_purpose_deprecation.py',
+    'rpc_getaddressinfo_label_deprecation.py',
     'rpc_help.py',
     'feature_help.py',
     # Don't append tests at the end to avoid merge conflicts

test/functional/wallet_basic.py

@@ -394,7 +394,7 @@ class WalletTest(BitcoinTestFramework):
             for label in [u'рыба', u'𝅘𝅥𝅯']:
                 addr = self.nodes[0].getnewaddress()
                 self.nodes[0].setlabel(addr, label)
-                test_address(self.nodes[0], addr, label=label, labels=[label])
+                test_address(self.nodes[0], addr, labels=[label])
                 assert label in self.nodes[0].listlabels()
         self.nodes[0].rpc.ensure_ascii = True  # restore to default
 

test/functional/wallet_import_with_label.py

@@ -36,7 +36,6 @@ class ImportWithLabel(BitcoinTestFramework):
                      address,
                      iswatchonly=True,
                      ismine=False,
-                     label=label,
                      labels=[label])
 
         self.log.info(
@@ -45,7 +44,7 @@ class ImportWithLabel(BitcoinTestFramework):
         )
         priv_key = self.nodes[0].dumpprivkey(address)
         self.nodes[1].importprivkey(priv_key)
-        test_address(self.nodes[1], address, label=label, labels=[label])
+        test_address(self.nodes[1], address, labels=[label])
 
         self.log.info(
             "Test importaddress without label and importprivkey with label."
@@ -57,7 +56,6 @@ class ImportWithLabel(BitcoinTestFramework):
                      address2,
                      iswatchonly=True,
                      ismine=False,
-                     label="",
                      labels=[""])
 
         self.log.info(
@@ -68,7 +66,7 @@ class ImportWithLabel(BitcoinTestFramework):
         label2 = "Test Label 2"
         self.nodes[1].importprivkey(priv_key2, label2)
 
-        test_address(self.nodes[1], address2, label=label2, labels=[label2])
+        test_address(self.nodes[1], address2, labels=[label2])
 
         self.log.info("Test importaddress with label and importprivkey with label.")
         self.log.info("Import a watch-only address with a label.")
@@ -79,7 +77,6 @@ class ImportWithLabel(BitcoinTestFramework):
                      address3,
                      iswatchonly=True,
                      ismine=False,
-                     label=label3_addr,
                      labels=[label3_addr])
 
         self.log.info(
@@ -90,7 +87,7 @@ class ImportWithLabel(BitcoinTestFramework):
         label3_priv = "Test Label 3 for importprivkey"
         self.nodes[1].importprivkey(priv_key3, label3_priv)
 
-        test_address(self.nodes[1], address3, label=label3_priv, labels=[label3_priv])
+        test_address(self.nodes[1], address3, labels=[label3_priv])
 
         self.log.info(
             "Test importprivkey won't label new dests with the same "
@@ -104,7 +101,6 @@ class ImportWithLabel(BitcoinTestFramework):
                      address4,
                      iswatchonly=True,
                      ismine=False,
-                     label=label4_addr,
                      labels=[label4_addr])
 
         self.log.info(
@@ -116,7 +112,7 @@ class ImportWithLabel(BitcoinTestFramework):
         priv_key4 = self.nodes[0].dumpprivkey(address4)
         self.nodes[1].importprivkey(priv_key4)
 
-        test_address(self.nodes[1], address4, label=label4_addr, labels=[label4_addr])
+        test_address(self.nodes[1], address4, labels=[label4_addr])
 
         self.stop_nodes()
 

test/functional/wallet_importmulti.py

@@ -502,7 +502,6 @@ class ImportMultiTest(BitcoinTestFramework):
                      key.p2pkh_addr,
                      solvable=True,
                      ismine=False,
-                     label=p2pkh_label,
                      labels=[p2pkh_label])
 
         # Test import fails if both desc and scriptPubKey are provided

test/functional/wallet_labels.py

@@ -157,7 +157,7 @@ class Label:
         if self.receive_address is not None:
             assert self.receive_address in self.addresses
         for address in self.addresses:
-            test_address(node, address, label=self.name, labels=[self.name])
+            test_address(node, address, labels=[self.name])
         assert self.name in node.listlabels()
         assert_equal(
             node.getaddressesbylabel(self.name),

test/functional/wallet_listreceivedby.py

@@ -128,7 +128,7 @@ class ReceivedByTest(BitcoinTestFramework):
         # set pre-state
         label = ''
         address = self.nodes[1].getnewaddress()
-        test_address(self.nodes[1], address, label=label, labels=[label])
+        test_address(self.nodes[1], address, labels=[label])
         received_by_label_json = [r for r in self.nodes[1].listreceivedbylabel() if r["label"] == label][0]
         balance_by_label = self.nodes[1].getreceivedbylabel(label)