From 4fcd7850b0a8e840d1709a3416957fa2359a0d2c Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Wed, 8 Jan 2020 11:23:58 +1300 Subject: [PATCH] Merge #17578: rpc: simplify getaddressinfo labels, deprecate previous behavior 8925df86c4df16b1070343fef8e4d238f3cc3bd1 doc: update release notes (Jon Atack) 8bb405bbadf11391ccba7b334b4cfe66dc85b390 test: getaddressinfo labels purpose deprecation test (Jon Atack) 60aba1f2f11529add115d963d05599130288ae28 rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack) 7851f14ccf2bcd1e9b2ad48e5e08881be06d9d21 rpc: incorporate review feedback from PR 17283 (Jon Atack) Pull request description: This PR builds on #17283 (now merged) and is followed by #17585. It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs. before ``` "labels": [ { "name": "DOUBLE SPEND", "purpose": "receive" } ``` after ``` "labels": [ "DOUBLE SPEND" ] ``` The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`. For context, see: - https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001 - http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output. Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in #17585) and add support for multiple labels per address. This PR will unblock those. ACKs for top commit: jnewbery: reACK 8925df8 promag: Code review ACK 8925df86c4df16b1070343fef8e4d238f3cc3bd1. meshcollider: Code review ACK 8925df86c4df16b1070343fef8e4d238f3cc3bd1 Tree-SHA512: c2b717209996da32b6484de7bb8800e7048410f9ce6afdb3e02a6866bd4a8f2c730f905fca27b10b877b91cf407f546e69e8c4feb9cd934325a6c71c166bd438 --- doc/release-notes-17578.md | 8 ++++ src/wallet/rpcwallet.cpp | 36 +++++++------- ...taddressinfo_labels_purpose_deprecation.py | 48 +++++++++++++++++++ test/functional/test_framework/wallet_util.py | 5 -- test/functional/test_runner.py | 1 + test/functional/wallet_basic.py | 7 +-- test/functional/wallet_import_with_label.py | 35 ++++---------- test/functional/wallet_importmulti.py | 3 +- test/functional/wallet_labels.py | 13 ++--- test/functional/wallet_listreceivedby.py | 7 +-- 10 files changed, 94 insertions(+), 69 deletions(-) create mode 100644 doc/release-notes-17578.md create mode 100755 test/functional/rpc_getaddressinfo_labels_purpose_deprecation.py diff --git a/doc/release-notes-17578.md b/doc/release-notes-17578.md new file mode 100644 index 0000000000..1b07436bb1 --- /dev/null +++ b/doc/release-notes-17578.md @@ -0,0 +1,8 @@ +Deprecated or removed RPCs +-------------------------- + +- The `getaddressinfo` RPC `labels` field now returns an array of label name + 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 + 0.21. To re-enable the previous behavior, launch bitcoind with + `-deprecatedrpc=labelspurpose`. diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index e11e3ad6ae..b932dc4443 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3622,6 +3622,8 @@ static UniValue AddressBookDataToJSON(const CAddressBookData& data, const bool v UniValue getaddressinfo(const JSONRPCRequest& request) { + const std::string example_address = "\"XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg\""; + RPCHelpMan{"getaddressinfo", "\nReturn information about the given dash address.\n" "Some of the information will only be present if the address is in the active wallet.\n", @@ -3652,7 +3654,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 field in the labels array."}, + {RPCResult::Type::STR, "label", "The label associated with the address. Defaults to \"\". Equivalent to the name label in 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."}, @@ -3670,8 +3672,8 @@ UniValue getaddressinfo(const JSONRPCRequest& request) } }, RPCExamples{ - HelpExampleCli("getaddressinfo", "\"XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg\"") - + HelpExampleRpc("getaddressinfo", "\"XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg\"") + HelpExampleCli("getaddressinfo", example_address) + + HelpExampleRpc("getaddressinfo", example_address) }, }.Check(request); @@ -3683,7 +3685,6 @@ UniValue getaddressinfo(const JSONRPCRequest& request) UniValue ret(UniValue::VOBJ); CTxDestination dest = DecodeDestination(request.params[0].get_str()); - // Make sure the destination is valid if (!IsValidDestination(dest)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address"); @@ -3709,24 +3710,18 @@ UniValue getaddressinfo(const JSONRPCRequest& request) ret.pushKV("iswatchonly", bool(mine & ISMINE_WATCH_ONLY)); - // Return DescribeWalletAddress fields. - // Always returned: isscript, ischange, iswitness. - // Optional: witness_version, witness_program, script, hex, pubkeys (array), - // sigsrequired, pubkey, embedded, iscompressed. UniValue detail = DescribeWalletAddress(pwallet, dest); ret.pushKVs(detail); // Return label field if existing. Currently only one label can be // associated with an address, so the label should be equivalent to the - // value of the name key/value pair in the labels hash array below. + // value of the name key/value pair in the labels array below. if (pwallet->mapAddressBook.count(dest)) { ret.pushKV("label", pwallet->mapAddressBook.at(dest).name); } ret.pushKV("ischange", pwallet->IsChange(scriptPubKey)); - // Fetch KeyMetadata, if present, for the timestamp, hdkeypath, hdseedid, - // and hdmasterfingerprint fields. ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan(scriptPubKey); if (spk_man) { const PKHash *pkhash = std::get_if(&dest); @@ -3746,15 +3741,22 @@ UniValue getaddressinfo(const JSONRPCRequest& request) } } - // Return a labels array containing a hash of key/value pairs for the label - // name and address purpose. The name value is equivalent to the label field - // above. Currently only one label can be 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. + // Return a `labels` array containing the label associated with the address, + // equivalent to the `label` field above. Currently only one label can be + // 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::const_iterator mi = pwallet->mapAddressBook.find(dest); if (mi != pwallet->mapAddressBook.end()) { - labels.push_back(AddressBookDataToJSON(mi->second, true)); + if (pwallet->chain().rpcEnableDeprecated("labelspurpose")) { + labels.push_back(AddressBookDataToJSON(mi->second, true)); + } else { + labels.push_back(mi->second.name); + } } ret.pushKV("labels", std::move(labels)); diff --git a/test/functional/rpc_getaddressinfo_labels_purpose_deprecation.py b/test/functional/rpc_getaddressinfo_labels_purpose_deprecation.py new file mode 100755 index 0000000000..1049440d49 --- /dev/null +++ b/test/functional/rpc_getaddressinfo_labels_purpose_deprecation.py @@ -0,0 +1,48 @@ +#!/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 RPC getaddressinfo `labels` returning an array +containing a JSON hash of `name` and purpose` key-value pairs. It now +returns an array of label names. + +""" +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal + +LABELS_TO_TEST = frozenset({"" , "New 𝅘𝅥𝅯 $<#>&!рыба Label"}) + +class GetAddressInfoLabelsPurposeDeprecationTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 2 + self.setup_clean_chain = False + # Start node[0] with -deprecatedrpc=labelspurpose and node[1] without. + self.extra_args = [["-deprecatedrpc=labelspurpose"], []] + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + + def test_labels(self, node_num, label_name, expected_value): + node = self.nodes[node_num] + address = node.getnewaddress() + if label_name != "": + node.setlabel(address, label_name) + self.log.info(" set label to {}".format(label_name)) + labels = node.getaddressinfo(address)["labels"] + self.log.info(" labels = {}".format(labels)) + assert_equal(labels, expected_value) + + def run_test(self): + """Test getaddressinfo labels with and without -deprecatedrpc flag.""" + self.log.info("Test getaddressinfo labels with -deprecatedrpc flag") + for label in LABELS_TO_TEST: + self.test_labels(node_num=0, label_name=label, expected_value=[{"name": label, "purpose": "receive"}]) + + self.log.info("Test getaddressinfo labels without -deprecatedrpc flag") + for label in LABELS_TO_TEST: + self.test_labels(node_num=1, label_name=label, expected_value=[label]) + + +if __name__ == '__main__': + GetAddressInfoLabelsPurposeDeprecationTest().main() diff --git a/test/functional/test_framework/wallet_util.py b/test/functional/test_framework/wallet_util.py index ea621d4fd7..49e6af0dba 100755 --- a/test/functional/test_framework/wallet_util.py +++ b/test/functional/test_framework/wallet_util.py @@ -63,11 +63,6 @@ def get_multisig(node): p2sh_addr=script_to_p2sh(script_code), redeem_script=script_code.hex()) -def labels_value(name="", purpose="receive"): - """Generate a getaddressinfo labels array from a name and purpose. - Often used as the value of a labels kwarg for calling test_address below.""" - return [{"name": name, "purpose": purpose}] - def test_address(node, address, **kwargs): """Get address info for `address` and test whether the returned values are as expected.""" addr_info = node.getaddressinfo(address) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 941378d8ea..318332312a 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -265,6 +265,7 @@ BASE_SCRIPTS = [ 'feature_config_args.py', 'feature_settings.py', 'rpc_getdescriptorinfo.py', + 'rpc_getaddressinfo_labels_purpose_deprecation.py', 'rpc_help.py', 'feature_help.py', # Don't append tests at the end to avoid merge conflicts diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index b1be280e58..7b42c2ef43 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -14,10 +14,7 @@ from test_framework.util import ( count_bytes, wait_until, ) -from test_framework.wallet_util import ( - labels_value, - test_address, -) +from test_framework.wallet_util import test_address class WalletTest(BitcoinTestFramework): @@ -397,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=labels_value(name=label)) + test_address(self.nodes[0], addr, label=label, labels=[label]) assert label in self.nodes[0].listlabels() self.nodes[0].rpc.ensure_ascii = True # restore to default diff --git a/test/functional/wallet_import_with_label.py b/test/functional/wallet_import_with_label.py index cac85df6d8..8de4e3290a 100755 --- a/test/functional/wallet_import_with_label.py +++ b/test/functional/wallet_import_with_label.py @@ -11,10 +11,7 @@ with and without a label. """ from test_framework.test_framework import BitcoinTestFramework -from test_framework.wallet_util import ( - labels_value, - test_address, -) +from test_framework.wallet_util import test_address class ImportWithLabel(BitcoinTestFramework): @@ -40,7 +37,7 @@ class ImportWithLabel(BitcoinTestFramework): iswatchonly=True, ismine=False, label=label, - labels=labels_value(name=label)) + labels=[label]) self.log.info( "Import the watch-only address's private key without a " @@ -48,11 +45,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=labels_value(name=label)) + test_address(self.nodes[1], address, label=label, labels=[label]) self.log.info( "Test importaddress without label and importprivkey with label." @@ -65,7 +58,7 @@ class ImportWithLabel(BitcoinTestFramework): iswatchonly=True, ismine=False, label="", - labels=labels_value()) + labels=[""]) self.log.info( "Import the watch-only address's private key with a " @@ -75,10 +68,7 @@ class ImportWithLabel(BitcoinTestFramework): label2 = "Test Label 2" self.nodes[1].importprivkey(priv_key2, label2) - test_address(self.nodes[1], - address2, - label=label2, - labels=labels_value(name=label2)) + test_address(self.nodes[1], address2, label=label2, labels=[label2]) self.log.info("Test importaddress with label and importprivkey with label.") self.log.info("Import a watch-only address with a label.") @@ -90,7 +80,7 @@ class ImportWithLabel(BitcoinTestFramework): iswatchonly=True, ismine=False, label=label3_addr, - labels=labels_value(name=label3_addr)) + labels=[label3_addr]) self.log.info( "Import the watch-only address's private key with a " @@ -100,10 +90,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=labels_value(name=label3_priv)) + test_address(self.nodes[1], address3, label=label3_priv, labels=[label3_priv]) self.log.info( "Test importprivkey won't label new dests with the same " @@ -118,7 +105,7 @@ class ImportWithLabel(BitcoinTestFramework): iswatchonly=True, ismine=False, label=label4_addr, - labels=labels_value(name=label4_addr)) + labels=[label4_addr]) self.log.info( "Import the watch-only address's private key without a " @@ -128,10 +115,8 @@ 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=labels_value(name=label4_addr)) + + test_address(self.nodes[1], address4, label=label4_addr, labels=[label4_addr]) self.stop_nodes() diff --git a/test/functional/wallet_importmulti.py b/test/functional/wallet_importmulti.py index 4162c1b214..76bd009c32 100755 --- a/test/functional/wallet_importmulti.py +++ b/test/functional/wallet_importmulti.py @@ -29,7 +29,6 @@ from test_framework.util import ( from test_framework.wallet_util import ( get_key, get_multisig, - labels_value, test_address, ) @@ -504,7 +503,7 @@ class ImportMultiTest(BitcoinTestFramework): solvable=True, ismine=False, label=p2pkh_label, - labels=labels_value(name=p2pkh_label)) + labels=[p2pkh_label]) # Test import fails if both desc and scriptPubKey are provided key = get_key(self.nodes[0]) diff --git a/test/functional/wallet_labels.py b/test/functional/wallet_labels.py index 24d5134185..da86a08dbe 100755 --- a/test/functional/wallet_labels.py +++ b/test/functional/wallet_labels.py @@ -13,10 +13,8 @@ from collections import defaultdict from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal, assert_raises_rpc_error -from test_framework.wallet_util import ( - labels_value, - test_address, -) +from test_framework.wallet_util import test_address + class WalletLabelsTest(BitcoinTestFramework): def set_test_params(self): @@ -159,12 +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=labels_value(name=self.name, purpose=self.purpose[address]) - ) + test_address(node, address, label=self.name, labels=[self.name]) assert self.name in node.listlabels() assert_equal( node.getaddressesbylabel(self.name), diff --git a/test/functional/wallet_listreceivedby.py b/test/functional/wallet_listreceivedby.py index 4f3bc35ba5..dca4e94e8b 100755 --- a/test/functional/wallet_listreceivedby.py +++ b/test/functional/wallet_listreceivedby.py @@ -11,10 +11,7 @@ from test_framework.util import ( assert_equal, assert_raises_rpc_error, ) -from test_framework.wallet_util import ( - labels_value, - test_address, -) +from test_framework.wallet_util import test_address class ReceivedByTest(BitcoinTestFramework): @@ -131,7 +128,7 @@ class ReceivedByTest(BitcoinTestFramework): # set pre-state label = '' address = self.nodes[1].getnewaddress() - test_address(self.nodes[1], address, label=label, labels=labels_value(name=label)) + test_address(self.nodes[1], address, label=label, 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)