Closes #13087.
As discussed in the issue, this is a feature request instead of a bug report since the behaviour was as intended (i.e. label with default: ''). With this, the old behaviour is kept while the possibility to achieve the preservation of labels, as expected in the open issue, is added.
150@@ -151,9 +151,12 @@ UniValue importprivkey(const JSONRPCRequest& request)
151 CKeyID vchAddress = pubkey.GetID();
152 {
153 pwallet->MarkDirty();
154- // We don't know which corresponding address will be used; label them all
155+
156+ // We don't know which corresponding address will be used; label all new addresses and preserve the rest
157 for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
158- pwallet->SetAddressBook(dest, strLabel, "receive");
159+ bool has_label = pwallet->mapAddressBook.find(dest) != pwallet->mapAddressBook.end() && !pwallet->mapAddressBook[dest].name.empty();
I was thinking in something like
0if (request.params[1].isNull()) {
1 const auto& it = pwallet->mapAddressBook.find(dest);
2 if (it != pwallet->mapAddressBook.end()) strLabel = it->second;
3}
4pwallet->SetAddressBook(dest, strLabel, "receive");
0@@ -0,0 +1,93 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2017 The Bitcoin Core developers
150@@ -151,8 +151,13 @@ UniValue importprivkey(const JSONRPCRequest& request)
151 CKeyID vchAddress = pubkey.GetID();
152 {
153 pwallet->MarkDirty();
154- // We don't know which corresponding address will be used; label them all
155+
156+ // We don't know which corresponding address will be used; label all new addresses and preserve the rest
157+ // label all new addresses, and label existing addresses if a
158+ // label was passed.
159 for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
160+ if (request.params[1].isNull()) {
161+ const auto& it = pwallet->mapAddressBook.find(dest);
162+ if (it != pwallet->mapAddressBook.end()) strLabel = it->second.name;
Good catch! Suggestion:
0if (it == pwallet->mapAddressBook.end() || !request.params[1].isNull()) {
1 pwallet->SetAddressBook(dest, strLabel, "receive")
2}
This still changes behaviour: the default is now null (ie, current label) instead of "".
I don’t particularly care what the default is (null makes more sense), but the change should be documented and noted in release notes.
release-notes-pr13381.md file documenting the change.
Let me know if I missed something or should document this somewhere else.
Thanks!
@marcoagner Thanks for adding the release notes! I thought maybe the behavior change could be described more simply with the addition of a few examples; see below from some proposed text. I also changed the heading so that it’s clear that only a small part of the importprivkey API is being changed (I also removed the code style from the heading as those don’t display well in headings on parts of BitcoinCore.org, but that’s a local problem so feel free to add back the backtics if you like them).
0RPC importprivkey: new label behavior
1-------------------------------------
2
3Previously, `importprivkey`automatically added the default empty label
4("") to all addresses associated with imported private keys. Now it
5defaults to using any existing label for those addresses. For example:
6
7- Old behavior: you import a watch-only address with the label "cold
8 wallet". Later, you import the corresponding private key using the
9 default settings. The address's label is changed from "cold wallet"
10 to "".
11
12- New behavior: you import a watch-only address with the label "cold
13 wallet". Later, you import the corresponding private key using the
14 default settings. The address's label remains "cold wallet".
15
16In both the previous and current case, if you directly specify a label
17during the import, that label will override whatever previous label the
18addresses may have had. Also in both cases, if none of the addresses
19previously had a label, they will still receive the default empty label
20(""). Examples:
21
22- You import a watch-only address with the label "temporary". Later you
23 import the corresponding private key with the label "final". The
24 address's label will be changed to "final".
25
26- You import a private key for an address that was not previously in the
27 wallet. It's addresses will receive the default empty label ("").
@harding That’s much better, thanks. I’m changing to that on the next commit with some small (nit) differences. Here’s the diff so it’s easier to spot my edits:
04,5c4,5
1< Previously, `importprivkey`automatically added the default empty label
2< ("") to all addresses associated with imported private keys. Now it
3---
4> Previously, `importprivkey` automatically added the default empty label
5> ("") to all addresses associated with the imported private keys. Now it
621c21
7< (""). Examples:
8---
9> ("") as default. Examples:
1028c28,29
11< wallet. It's addresses will receive the default empty label ("").
12---
13> wallet using the default settings. Its addresses will receive the
14> default empty label ("").
Seems good?
@marcoagner LGTM, although the two uses of ‘default’ in this clause is a tiny bit awkward IMO (but, it’s clear, which is all that really matters):
they will still receive the default empty label ("") as default.
Maybe that second-to-last sentence could have its adverbial phrase “using the default settings” moved next to the verb it modifies, “import”, e.g.:
You use the default settings to import a private key for an address that was not previously in the wallet.
Thanks for updating, and thanks especially for catching my misuse of the apostrophe in the possessive its. :man_facepalming:
156+ // We don't know which corresponding address will be used;
157+ // label all new addresses, and label existing addresses if a
158+ // label was passed.
159 for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
160- pwallet->SetAddressBook(dest, strLabel, "receive");
161+ const auto& it = pwallet->mapAddressBook.find(dest);
find returns an iterator by value.
utACK 6115b5231954a38c400fa4808627dbb8e239ea68. Didn’t review the tests.
Non-blocking nit only.
0@@ -0,0 +1,29 @@
1+RPC importprivkey: new label behavior
2+-------------------------------------
3+
4+Previously, `importprivkey` automatically added the default empty label
5+("") to all addresses associated with the imported private keys. Now it
... imported private key. (singular)?
101@@ -102,7 +102,7 @@ UniValue importprivkey(const JSONRPCRequest& request)
102 "Hint: use importmulti to import more than one private key.\n"
103 "\nArguments:\n"
104 "1. \"privkey\" (string, required) The private key (see dumpprivkey)\n"
105- "2. \"label\" (string, optional, default=\"\") An optional label\n"
106+ "2. \"label\" (string, optional, default=current label if watch-only address exists, otherwise \"\") An optional label\n"
watch-only since the code below does’t validate that, it only checks if the label exists for each associated address. So default=current label if exists, otherwise \"\".
address between if and exists: default=current label if address exists, otherwise \"\". (I know it was probably just a typo; I’m commenting to make sure)
157+ // label all new addresses, and label existing addresses if a
158+ // label was passed.
159 for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
160- pwallet->SetAddressBook(dest, strLabel, "receive");
161+ const auto& it = pwallet->mapAddressBook.find(dest);
162+ if (it == pwallet->mapAddressBook.end() || !request.params[1].isNull()) {
Nit, maybe make it clear that params[1] has precedence — prevents a lookup if it does? Also, updating with the same label could be avoided, or am I missing something? Suggestion:
0for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
1 if (!request.params[1].isNull()) {
2 pwallet->SetAddressBook(dest, strLabel, "receive")
3 } else if (pwallet->mapAddressBook.count(dest) == 0) {
4 pwallet->SetAddressBook(dest, strLabel, "receive");
5 }
6}
0@@ -0,0 +1,126 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2018 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+"""Test the behavior of RPC importprivkey on set and unset labels of
6+watch-only addresses.
watch-only?
19+class ImportWithLabel(BitcoinTestFramework):
20+ def set_test_params(self):
21+ self.num_nodes = 2
22+ self.setup_clean_chain = True
23+
24+ def setup_network(self):
158+ // label was passed.
159 for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
160- pwallet->SetAddressBook(dest, strLabel, "receive");
161+ if (!request.params[1].isNull()) {
162+ pwallet->SetAddressBook(dest, strLabel, "receive");
163+ } else if (pwallet->mapAddressBook.count(dest) == 0) {
if statements are identical, why not write it as a single if (!request.params[1].isNull() || pwallet->mapAddressBook.count(dest)) { ?
request.params[1] has precedence. Bad idea? Both work.
|| statement (like your suggestion) will not call it if there is a params[1] in the first condition.
19+class ImportWithLabel(BitcoinTestFramework):
20+ def set_test_params(self):
21+ self.num_nodes = 2
22+ self.setup_clean_chain = True
23+
24+ def run_test(self):
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
112+
113+ assert(address_assert4.get('embedded'))
114+
115+ bcaddress_assert = self.nodes[1].getaddressinfo(
116+ address_assert4['embedded']['address']
117+ )
black to get it auto-formatted in accordance with PEP-8 :-)
black, thanks.
I was using Flake8. Besides the double-quotes for strings, it seems that black has a longer default line length, but I’m okay with (actually prefer) that. :)
I will just rebase, re-run tests, and provide a new commit.
- changes importprivkey behavior to overwrite existent label if one
is passed and keep existing ones if no label is passed
- tests behavior of importprivkey on existing address labels and
different same key destination
wallet_basic.py passed locally but seems to have failed on appveyor :thinking:
