Skip to content

Commit 7ea3b85

Browse files
meshcollidermeshcollider
committed
Merge bitcoin#17578: rpc: simplify getaddressinfo labels, deprecate previous behavior
8925df8 doc: update release notes (Jon Atack) 8bb405b test: getaddressinfo labels purpose deprecation test (Jon Atack) 60aba1f rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack) 7851f14 rpc: incorporate review feedback from PR 17283 (Jon Atack) Pull request description: This PR builds on bitcoin#17283 (now merged) and is followed by bitcoin#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: - bitcoin#17283 (comment) - 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 bitcoin#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 8925df8. meshcollider: Code review ACK 8925df8 Tree-SHA512: c2b717209996da32b6484de7bb8800e7048410f9ce6afdb3e02a6866bd4a8f2c730f905fca27b10b877b91cf407f546e69e8c4feb9cd934325a6c71c166bd438
2 parents 45f1519 + 8925df8 commit 7ea3b85

File tree

10 files changed

+101
-78
lines changed

10 files changed

+101
-78
lines changed

‎doc/release-notes-17578.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
Deprecated or removed RPCs
2+
--------------------------
3+
4+
- The `getaddressinfo` RPC `labels` field now returns an array of label name
5+
strings. Previously, it returned an array of JSON objects containing `name` and
6+
`purpose` key/value pairs, which is now deprecated and will be removed in
7+
0.21. To re-enable the previous behavior, launch bitcoind with
8+
`-deprecatedrpc=labelspurpose`.

‎src/wallet/rpcwallet.cpp‎

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3726,6 +3726,8 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
37263726
return NullUniValue;
37273727
}
37283728

3729+
const std::string example_address = "\"bc1q09vm5lfy0j5reeulh4x5752q25uqqvz34hufdl\"";
3730+
37293731
RPCHelpMan{"getaddressinfo",
37303732
"\nReturn information about the given bitcoin address.\n"
37313733
"Some of the information will only be present if the address is in the active wallet.\n",
@@ -3760,32 +3762,33 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
37603762
" getaddressinfo output fields for the embedded address, excluding metadata (timestamp, hdkeypath,\n"
37613763
" hdseedid) and relation to the wallet (ismine, iswatchonly).\n"
37623764
" \"iscompressed\" : true|false, (boolean, optional) If the pubkey is compressed.\n"
3763-
" \"label\" : \"label\" (string) The label associated with the address. Defaults to \"\". Equivalent to the name field in the labels array.\n"
3765+
" \"label\" : \"label\" (string) The label associated with the address. Defaults to \"\". Equivalent to the label name in the labels array below.\n"
37643766
" \"timestamp\" : timestamp, (number, optional) The creation time of the key, if available, expressed in " + UNIX_EPOCH_TIME + ".\n"
37653767
" \"hdkeypath\" : \"keypath\" (string, optional) The HD keypath, if the key is HD and available.\n"
37663768
" \"hdseedid\" : \"<hash160>\" (string, optional) The Hash160 of the HD seed.\n"
37673769
" \"hdmasterfingerprint\" : \"<hash160>\" (string, optional) The fingerprint of the master key.\n"
3768-
" \"labels\" (object) An array of labels associated with the address. Currently limited to one label but returned\n"
3770+
" \"labels\" (json object) An array of labels associated with the address. Currently limited to one label but returned\n"
37693771
" as an array to keep the API stable if multiple labels are enabled in the future.\n"
37703772
" [\n"
3773+
" \"label name\" (string) The label name. Defaults to \"\". Equivalent to the label field above.\n\n"
3774+
" DEPRECATED, will be removed in 0.21. To re-enable, launch bitcoind with `-deprecatedrpc=labelspurpose`:\n"
37713775
" { (json object of label data)\n"
3772-
" \"name\": \"label name\" (string) The label name. Defaults to \"\". Equivalent to the label field above.\n"
3773-
" \"purpose\": \"purpose\" (string) The purpose of the associated address (send or receive).\n"
3774-
" },...\n"
3776+
" \"name\" : \"label name\" (string) The label name. Defaults to \"\". Equivalent to the label field above.\n"
3777+
" \"purpose\" : \"purpose\" (string) The purpose of the associated address (send or receive).\n"
3778+
" }\n"
37753779
" ]\n"
37763780
"}\n"
37773781
},
37783782
RPCExamples{
3779-
HelpExampleCli("getaddressinfo", "\"bc1q09vm5lfy0j5reeulh4x5752q25uqqvz34hufdl\"") +
3780-
HelpExampleRpc("getaddressinfo", "\"bc1q09vm5lfy0j5reeulh4x5752q25uqqvz34hufdl\"")
3783+
HelpExampleCli("getaddressinfo", example_address) +
3784+
HelpExampleRpc("getaddressinfo", example_address)
37813785
},
37823786
}.Check(request);
37833787

37843788
LOCK(pwallet->cs_wallet);
37853789

37863790
UniValue ret(UniValue::VOBJ);
37873791
CTxDestination dest = DecodeDestination(request.params[0].get_str());
3788-
37893792
// Make sure the destination is valid
37903793
if (!IsValidDestination(dest)) {
37913794
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
@@ -3811,24 +3814,18 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
38113814

38123815
ret.pushKV("iswatchonly", bool(mine & ISMINE_WATCH_ONLY));
38133816

3814-
// Return DescribeWalletAddress fields.
3815-
// Always returned: isscript, ischange, iswitness.
3816-
// Optional: witness_version, witness_program, script, hex, pubkeys (array),
3817-
// sigsrequired, pubkey, embedded, iscompressed.
38183817
UniValue detail = DescribeWalletAddress(pwallet, dest);
38193818
ret.pushKVs(detail);
38203819

38213820
// Return label field if existing. Currently only one label can be
38223821
// associated with an address, so the label should be equivalent to the
3823-
// value of the name key/value pair in the labels hash array below.
3822+
// value of the name key/value pair in the labels array below.
38243823
if (pwallet->mapAddressBook.count(dest)) {
38253824
ret.pushKV("label", pwallet->mapAddressBook[dest].name);
38263825
}
38273826

38283827
ret.pushKV("ischange", pwallet->IsChange(scriptPubKey));
38293828

3830-
// Fetch KeyMetadata, if present, for the timestamp, hdkeypath, hdseedid,
3831-
// and hdmasterfingerprint fields.
38323829
ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan(scriptPubKey);
38333830
if (spk_man) {
38343831
if (const CKeyMetadata* meta = spk_man->GetMetadata(dest)) {
@@ -3841,15 +3838,22 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
38413838
}
38423839
}
38433840

3844-
// Return a labels array containing a hash of key/value pairs for the label
3845-
// name and address purpose. The name value is equivalent to the label field
3846-
// above. Currently only one label can be associated with an address, but we
3847-
// return an array so the API remains stable if we allow multiple labels to
3848-
// be associated with an address in the future.
3841+
// Return a `labels` array containing the label associated with the address,
3842+
// equivalent to the `label` field above. Currently only one label can be
3843+
// associated with an address, but we return an array so the API remains
3844+
// stable if we allow multiple labels to be associated with an address in
3845+
// the future.
3846+
//
3847+
// DEPRECATED: The previous behavior of returning an array containing a JSON
3848+
// object of `name` and `purpose` key/value pairs has been deprecated.
38493849
UniValue labels(UniValue::VARR);
38503850
std::map<CTxDestination, CAddressBookData>::iterator mi = pwallet->mapAddressBook.find(dest);
38513851
if (mi != pwallet->mapAddressBook.end()) {
3852-
labels.push_back(AddressBookDataToJSON(mi->second, true));
3852+
if (pwallet->chain().rpcEnableDeprecated("labelspurpose")) {
3853+
labels.push_back(AddressBookDataToJSON(mi->second, true));
3854+
} else {
3855+
labels.push_back(mi->second.name);
3856+
}
38533857
}
38543858
ret.pushKV("labels", std::move(labels));
38553859

‎test/functional/rpc_getaddressinfo_labels_purpose_deprecation.py‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
#!/usr/bin/env python3
2+
# Copyright (c) 2020 The Bitcoin Core developers
3+
# Distributed under the MIT software license, see the accompanying
4+
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
"""
6+
Test deprecation of RPC getaddressinfo `labels` returning an array
7+
containing a JSON hash of `name` and purpose` key-value pairs. It now
8+
returns an array of label names.
9+
10+
"""
11+
from test_framework.test_framework import BitcoinTestFramework
12+
from test_framework.util import assert_equal
13+
14+
LABELS_TO_TEST = frozenset({"" , "New 𝅘𝅥𝅯 $<#>&!рыба Label"})
15+
16+
class GetAddressInfoLabelsPurposeDeprecationTest(BitcoinTestFramework):
17+
def set_test_params(self):
18+
self.num_nodes = 2
19+
self.setup_clean_chain = False
20+
# Start node[0] with -deprecatedrpc=labelspurpose and node[1] without.
21+
self.extra_args = [["-deprecatedrpc=labelspurpose"], []]
22+
23+
def skip_test_if_missing_module(self):
24+
self.skip_if_no_wallet()
25+
26+
def test_labels(self, node_num, label_name, expected_value):
27+
node = self.nodes[node_num]
28+
address = node.getnewaddress()
29+
if label_name != "":
30+
node.setlabel(address, label_name)
31+
self.log.info(" set label to {}".format(label_name))
32+
labels = node.getaddressinfo(address)["labels"]
33+
self.log.info(" labels = {}".format(labels))
34+
assert_equal(labels, expected_value)
35+
36+
def run_test(self):
37+
"""Test getaddressinfo labels with and without -deprecatedrpc flag."""
38+
self.log.info("Test getaddressinfo labels with -deprecatedrpc flag")
39+
for label in LABELS_TO_TEST:
40+
self.test_labels(node_num=0, label_name=label, expected_value=[{"name": label, "purpose": "receive"}])
41+
42+
self.log.info("Test getaddressinfo labels without -deprecatedrpc flag")
43+
for label in LABELS_TO_TEST:
44+
self.test_labels(node_num=1, label_name=label, expected_value=[label])
45+
46+
47+
if __name__ == '__main__':
48+
GetAddressInfoLabelsPurposeDeprecationTest().main()

‎test/functional/test_framework/wallet_util.py‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,11 +88,6 @@ def get_multisig(node):
8888
p2sh_p2wsh_script=CScript([OP_HASH160, witness_script, OP_EQUAL]).hex(),
8989
p2sh_p2wsh_addr=script_to_p2sh_p2wsh(script_code))
9090

91-
def labels_value(name="", purpose="receive"):
92-
"""Generate a getaddressinfo labels array from a name and purpose.
93-
Often used as the value of a labels kwarg for calling test_address below."""
94-
return [{"name": name, "purpose": purpose}]
95-
9691
def test_address(node, address, **kwargs):
9792
"""Get address info for `address` and test whether the returned values are as expected."""
9893
addr_info = node.getaddressinfo(address)

‎test/functional/test_runner.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,7 @@
212212
'p2p_permissions.py',
213213
'feature_blocksdir.py',
214214
'feature_config_args.py',
215+
'rpc_getaddressinfo_labels_purpose_deprecation.py',
215216
'rpc_help.py',
216217
'feature_help.py',
217218
'feature_shutdown.py',

‎test/functional/wallet_basic.py‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,7 @@
1515
connect_nodes,
1616
wait_until,
1717
)
18-
from test_framework.wallet_util import (
19-
labels_value,
20-
test_address,
21-
)
18+
from test_framework.wallet_util import test_address
2219

2320

2421
class WalletTest(BitcoinTestFramework):
@@ -395,7 +392,7 @@ def run_test(self):
395392
for label in [u'рыба', u'𝅘𝅥𝅯']:
396393
addr = self.nodes[0].getnewaddress()
397394
self.nodes[0].setlabel(addr, label)
398-
test_address(self.nodes[0], addr, label=label, labels=labels_value(name=label))
395+
test_address(self.nodes[0], addr, label=label, labels=[label])
399396
assert label in self.nodes[0].listlabels()
400397
self.nodes[0].rpc.ensure_ascii = True # restore to default
401398

‎test/functional/wallet_import_with_label.py‎

Lines changed: 10 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,7 @@
1111
"""
1212

1313
from test_framework.test_framework import BitcoinTestFramework
14-
from test_framework.wallet_util import (
15-
labels_value,
16-
test_address,
17-
)
14+
from test_framework.wallet_util import test_address
1815

1916

2017
class ImportWithLabel(BitcoinTestFramework):
@@ -40,19 +37,15 @@ def run_test(self):
4037
iswatchonly=True,
4138
ismine=False,
4239
label=label,
43-
labels=labels_value(name=label))
40+
labels=[label])
4441

4542
self.log.info(
4643
"Import the watch-only address's private key without a "
4744
"label and the address should keep its label."
4845
)
4946
priv_key = self.nodes[0].dumpprivkey(address)
5047
self.nodes[1].importprivkey(priv_key)
51-
52-
test_address(self.nodes[1],
53-
address,
54-
label=label,
55-
labels=labels_value(name=label))
48+
test_address(self.nodes[1], address, label=label, labels=[label])
5649

5750
self.log.info(
5851
"Test importaddress without label and importprivkey with label."
@@ -65,7 +58,7 @@ def run_test(self):
6558
iswatchonly=True,
6659
ismine=False,
6760
label="",
68-
labels=labels_value())
61+
labels=[""])
6962

7063
self.log.info(
7164
"Import the watch-only address's private key with a "
@@ -75,10 +68,7 @@ def run_test(self):
7568
label2 = "Test Label 2"
7669
self.nodes[1].importprivkey(priv_key2, label2)
7770

78-
test_address(self.nodes[1],
79-
address2,
80-
label=label2,
81-
labels=labels_value(name=label2))
71+
test_address(self.nodes[1], address2, label=label2, labels=[label2])
8272

8373
self.log.info("Test importaddress with label and importprivkey with label.")
8474
self.log.info("Import a watch-only address with a label.")
@@ -90,7 +80,7 @@ def run_test(self):
9080
iswatchonly=True,
9181
ismine=False,
9282
label=label3_addr,
93-
labels=labels_value(name=label3_addr))
83+
labels=[label3_addr])
9484

9585
self.log.info(
9686
"Import the watch-only address's private key with a "
@@ -100,10 +90,7 @@ def run_test(self):
10090
label3_priv = "Test Label 3 for importprivkey"
10191
self.nodes[1].importprivkey(priv_key3, label3_priv)
10292

103-
test_address(self.nodes[1],
104-
address3,
105-
label=label3_priv,
106-
labels=labels_value(name=label3_priv))
93+
test_address(self.nodes[1], address3, label=label3_priv, labels=[label3_priv])
10794

10895
self.log.info(
10996
"Test importprivkey won't label new dests with the same "
@@ -118,7 +105,7 @@ def run_test(self):
118105
iswatchonly=True,
119106
ismine=False,
120107
label=label4_addr,
121-
labels=labels_value(name=label4_addr),
108+
labels=[label4_addr],
122109
embedded=None)
123110

124111
self.log.info(
@@ -131,15 +118,9 @@ def run_test(self):
131118
self.nodes[1].importprivkey(priv_key4)
132119
embedded_addr = self.nodes[1].getaddressinfo(address4)['embedded']['address']
133120

134-
test_address(self.nodes[1],
135-
embedded_addr,
136-
label="",
137-
labels=labels_value())
121+
test_address(self.nodes[1], embedded_addr, label="", labels=[""])
138122

139-
test_address(self.nodes[1],
140-
address4,
141-
label=label4_addr,
142-
labels=labels_value(name=label4_addr))
123+
test_address(self.nodes[1], address4, label=label4_addr, labels=[label4_addr])
143124

144125
self.stop_nodes()
145126

‎test/functional/wallet_importmulti.py‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
from test_framework.wallet_util import (
3030
get_key,
3131
get_multisig,
32-
labels_value,
3332
test_address,
3433
)
3534

@@ -571,7 +570,7 @@ def run_test(self):
571570
solvable=True,
572571
ismine=True,
573572
label=p2sh_p2wpkh_label,
574-
labels=labels_value(name=p2sh_p2wpkh_label))
573+
labels=[p2sh_p2wpkh_label])
575574

576575
# Test ranged descriptor fails if range is not specified
577576
xpriv = "tprv8ZgxMBicQKsPeuVhWwi6wuMQGfPKi9Li5GtX35jVNknACgqe3CY4g5xgkfDDJcmtF7o1QnxWDRYw4H5P26PXq7sbcUkEqeR4fg3Kxp2tigg"
@@ -643,7 +642,7 @@ def run_test(self):
643642
solvable=True,
644643
ismine=False,
645644
label=p2pkh_label,
646-
labels=labels_value(name=p2pkh_label))
645+
labels=[p2pkh_label])
647646

648647
# Test import fails if both desc and scriptPubKey are provided
649648
key = get_key(self.nodes[0])

‎test/functional/wallet_labels.py‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,8 @@
1313

1414
from test_framework.test_framework import BitcoinTestFramework
1515
from test_framework.util import assert_equal, assert_raises_rpc_error
16-
from test_framework.wallet_util import (
17-
labels_value,
18-
test_address,
19-
)
16+
from test_framework.wallet_util import test_address
17+
2018

2119
class WalletLabelsTest(BitcoinTestFramework):
2220
def set_test_params(self):
@@ -157,12 +155,7 @@ def verify(self, node):
157155
if self.receive_address is not None:
158156
assert self.receive_address in self.addresses
159157
for address in self.addresses:
160-
test_address(
161-
node,
162-
address,
163-
label=self.name,
164-
labels=labels_value(name=self.name, purpose=self.purpose[address])
165-
)
158+
test_address(node, address, label=self.name, labels=[self.name])
166159
assert self.name in node.listlabels()
167160
assert_equal(
168161
node.getaddressesbylabel(self.name),

‎test/functional/wallet_listreceivedby.py‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,7 @@
1111
assert_equal,
1212
assert_raises_rpc_error,
1313
)
14-
from test_framework.wallet_util import (
15-
labels_value,
16-
test_address,
17-
)
14+
from test_framework.wallet_util import test_address
1815

1916

2017
class ReceivedByTest(BitcoinTestFramework):
@@ -131,7 +128,7 @@ def run_test(self):
131128
# set pre-state
132129
label = ''
133130
address = self.nodes[1].getnewaddress()
134-
test_address(self.nodes[1], address, label=label, labels=labels_value(name=label))
131+
test_address(self.nodes[1], address, label=label, labels=[label])
135132
received_by_label_json = [r for r in self.nodes[1].listreceivedbylabel() if r["label"] == label][0]
136133
balance_by_label = self.nodes[1].getreceivedbylabel(label)
137134

0 commit comments

Comments
 (0)