Merge bitcoin#13381: RPC: creates possibility to preserve labels on i…

…mportprivkey

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

Pull request description:

  Closes bitcoin#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

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 ("").

src/wallet/rpcdump.cpp

@@ -157,7 +157,10 @@ UniValue importprivkey(const JSONRPCRequest& request)
         CKeyID vchAddress = pubkey.GetID();
         {
             pwallet->MarkDirty();
-            pwallet->SetAddressBook(vchAddress, strLabel, "receive");
+
+            if (!request.params[1].isNull()) {
+                pwallet->SetAddressBook(vchAddress, strLabel, "receive");
+            }
 
             // Don't throw error in case a key is already there
             if (pwallet->HaveKey(vchAddress)) {

test/functional/test_runner.py

@@ -184,6 +184,7 @@
     'mempool_accept.py',
     'mempool_expiry.py',
     'wallet_import_rescan.py',
+    'wallet_import_with_label.py',
     'rpc_bind.py --ipv4',
     'rpc_bind.py --ipv6',
     'rpc_bind.py --nonloopback',

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()