Merge #13381: RPC: creates possibility to preserve labels on importprivkey

a6b5ec18f rpc: creates possibility to preserve labels on importprivkey (marcoagner)

Pull request description:

  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.

Tree-SHA512: b33be50e1e7f62f7ddfae953177ba0926e2d848961f9fac7501c2b513322c0cb95787745d07d137488267bad1104ecfdbe800c6747f94162eb07c976835c1386

4 files changed, 172 insertions, 3 deletions

diff --git a/doc/release-notes-pr13381.md b/doc/release-notes-pr13381.md
new file mode 100644
index 0000000000..75faad9906
--- /dev/null
+++ b/doc/release-notes-pr13381.md
@@ -0,0 +1,29 @@
+RPC importprivkey: new label behavior
+-------------------------------------
+
+Previously, `importprivkey` automatically added the default empty label
+("") to all addresses associated with the imported private key.  Now it
+defaults to using any existing label for those addresses.  For example:
+
+- Old behavior: you import a watch-only address with the label "cold
+  wallet".  Later, you import the corresponding private key using the
+  default settings.  The address's label is changed from "cold wallet"
+  to "".
+
+- New behavior: you import a watch-only address with the label "cold
+  wallet".  Later, you import the corresponding private key using the
+  default settings.  The address's label remains "cold wallet".
+
+In both the previous and current case, if you directly specify a label
+during the import, that label will override whatever previous label the
+addresses may have had.  Also in both cases, if none of the addresses
+previously had a label, they will still receive the default empty label
+("").  Examples:
+
+- You import a watch-only address with the label "temporary".  Later you
+  import the corresponding private key with the label "final".  The
+  address's label will be changed to "final".
+
+- You use the default settings to import a private key for an address that
+  was not previously in the wallet.  Its addresses will receive the default
+  empty label ("").
diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index e1e4fc51fe..7dbe0c8462 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -113,7 +113,7 @@ UniValue importprivkey(const JSONRPCRequest& request)
             "Hint: use importmulti to import more than one private key.\n"
             "\nArguments:\n"
             "1. \"privkey\"          (string, required) The private key (see dumpprivkey)\n"
-            "2. \"label\"            (string, optional, default=\"\") An optional label\n"
+            "2. \"label\"            (string, optional, default=current label if address exists, otherwise \"\") An optional label\n"
             "3. rescan               (boolean, optional, default=true) Rescan the wallet for transactions\n"
             "\nNote: This call can take over an hour to complete if rescan is true, during that time, other rpc calls\n"
             "may report that the imported key exists but related transactions are still missing, leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes.\n"
@@ -163,9 +163,14 @@ UniValue importprivkey(const JSONRPCRequest& request)
         CKeyID vchAddress = pubkey.GetID();
         {
             pwallet->MarkDirty();
-            // We don't know which corresponding address will be used; label them all
+
+            // We don't know which corresponding address will be used;
+            // label all new addresses, and label existing addresses if a
+            // label was passed.
             for (const auto& dest : GetAllDestinationsForKey(pubkey)) {
-                pwallet->SetAddressBook(dest, strLabel, "receive");
+                if (!request.params[1].isNull() || pwallet->mapAddressBook.count(dest) == 0) {
+                    pwallet->SetAddressBook(dest, strLabel, "receive");
+                }
             }
 
             // Don't throw error in case a key is already there
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 90b333f45a..ad53788b06 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -155,6 +155,7 @@ BASE_SCRIPTS = [
     'feature_nulldummy.py',
     'mempool_accept.py',
     'wallet_import_rescan.py',
+    'wallet_import_with_label.py',
     'rpc_bind.py --ipv4',
     'rpc_bind.py --ipv6',
     'rpc_bind.py --nonloopback',
diff --git a/test/functional/wallet_import_with_label.py b/test/functional/wallet_import_with_label.py
new file mode 100755
index 0000000000..95acaa752e
--- /dev/null
+++ b/test/functional/wallet_import_with_label.py
@@ -0,0 +1,134 @@
+#!/usr/bin/env python3
+# Copyright (c) 2018 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 the behavior of RPC importprivkey on set and unset labels of
+addresses.
+
+It tests different cases in which an address is imported with importaddress
+with or without a label and then its private key is imported with importprivkey
+with and without a label.
+"""
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
+
+
+class ImportWithLabel(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 2
+        self.setup_clean_chain = True
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
+    def run_test(self):
+        """Main test logic"""
+
+        self.log.info(
+            "Test importaddress with label and importprivkey without label."
+        )
+        self.log.info("Import a watch-only address with a label.")
+        address = self.nodes[0].getnewaddress()
+        label = "Test Label"
+        self.nodes[1].importaddress(address, label)
+        address_assert = self.nodes[1].getaddressinfo(address)
+
+        assert_equal(address_assert["iswatchonly"], True)
+        assert_equal(address_assert["ismine"], False)
+        assert_equal(address_assert["label"], label)
+
+        self.log.info(
+            "Import the watch-only address's private key without a "
+            "label and the address should keep its label."
+        )
+        priv_key = self.nodes[0].dumpprivkey(address)
+        self.nodes[1].importprivkey(priv_key)
+
+        assert_equal(label, self.nodes[1].getaddressinfo(address)["label"])
+
+        self.log.info(
+            "Test importaddress without label and importprivkey with label."
+        )
+        self.log.info("Import a watch-only address without a label.")
+        address2 = self.nodes[0].getnewaddress()
+        self.nodes[1].importaddress(address2)
+        address_assert2 = self.nodes[1].getaddressinfo(address2)
+
+        assert_equal(address_assert2["iswatchonly"], True)
+        assert_equal(address_assert2["ismine"], False)
+        assert_equal(address_assert2["label"], "")
+
+        self.log.info(
+            "Import the watch-only address's private key with a "
+            "label and the address should have its label updated."
+        )
+        priv_key2 = self.nodes[0].dumpprivkey(address2)
+        label2 = "Test Label 2"
+        self.nodes[1].importprivkey(priv_key2, label2)
+
+        assert_equal(label2, self.nodes[1].getaddressinfo(address2)["label"])
+
+        self.log.info("Test importaddress with label and importprivkey with label.")
+        self.log.info("Import a watch-only address with a label.")
+        address3 = self.nodes[0].getnewaddress()
+        label3_addr = "Test Label 3 for importaddress"
+        self.nodes[1].importaddress(address3, label3_addr)
+        address_assert3 = self.nodes[1].getaddressinfo(address3)
+
+        assert_equal(address_assert3["iswatchonly"], True)
+        assert_equal(address_assert3["ismine"], False)
+        assert_equal(address_assert3["label"], label3_addr)
+
+        self.log.info(
+            "Import the watch-only address's private key with a "
+            "label and the address should have its label updated."
+        )
+        priv_key3 = self.nodes[0].dumpprivkey(address3)
+        label3_priv = "Test Label 3 for importprivkey"
+        self.nodes[1].importprivkey(priv_key3, label3_priv)
+
+        assert_equal(label3_priv, self.nodes[1].getaddressinfo(address3)["label"])
+
+        self.log.info(
+            "Test importprivkey won't label new dests with the same "
+            "label as others labeled dests for the same key."
+        )
+        self.log.info("Import a watch-only legacy address with a label.")
+        address4 = self.nodes[0].getnewaddress()
+        label4_addr = "Test Label 4 for importaddress"
+        self.nodes[1].importaddress(address4, label4_addr)
+        address_assert4 = self.nodes[1].getaddressinfo(address4)
+
+        assert_equal(address_assert4["iswatchonly"], True)
+        assert_equal(address_assert4["ismine"], False)
+        assert_equal(address_assert4["label"], label4_addr)
+
+        self.log.info("Asserts address has no embedded field with dests.")
+
+        assert_equal(address_assert4.get("embedded"), None)
+
+        self.log.info(
+            "Import the watch-only address's private key without a "
+            "label and new destinations for the key should have an "
+            "empty label while the 'old' destination should keep "
+            "its label."
+        )
+        priv_key4 = self.nodes[0].dumpprivkey(address4)
+        self.nodes[1].importprivkey(priv_key4)
+        address_assert4 = self.nodes[1].getaddressinfo(address4)
+
+        assert address_assert4.get("embedded")
+
+        bcaddress_assert = self.nodes[1].getaddressinfo(
+            address_assert4["embedded"]["address"]
+        )
+
+        assert_equal(address_assert4["label"], label4_addr)
+        assert_equal(bcaddress_assert["label"], "")
+
+        self.stop_nodes()
+
+
+if __name__ == "__main__":
+    ImportWithLabel().main()