diff --git a/doc/release-notes.md b/doc/release-notes.md --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -22,3 +22,33 @@ - The `importmulti` RPC will now contain a new per-request `warnings` field with strings that explain when fields are being ignored or inconsistent, if any. + +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 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -116,7 +116,8 @@ /* default_val */ "", "The private key (see dumpprivkey)"}, {"label", RPCArg::Type::STR, /* opt */ true, /* default_val */ - "\"\"", "An optional label"}, + "current label if address exists, otherwise \"\"", + "An optional label"}, {"rescan", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "true", "Rescan the wallet for transactions"}, @@ -185,10 +186,15 @@ 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/wallet_import_with_label.py b/test/functional/wallet_import_with_label.py new file mode 100755 --- /dev/null +++ b/test/functional/wallet_import_with_label.py @@ -0,0 +1,164 @@ +#!/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.address import script_to_p2sh +from test_framework.script import ( + CScript, + OP_CHECKSIG, + OP_DUP, + OP_EQUALVERIFY, + OP_HASH160, + hash160, +) + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + hex_str_to_bytes, +) + + +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." + ) + + # Build a P2SH manually for this test. + priv_key4 = self.nodes[0].dumpprivkey(address4) + pubkey4 = self.nodes[0].getaddressinfo(address4)['pubkey'] + pkh4 = hash160(hex_str_to_bytes(pubkey4)) + script4 = CScript( + [OP_DUP, OP_HASH160, pkh4, OP_EQUALVERIFY, OP_CHECKSIG]) + p2shaddr4 = script_to_p2sh(script4) + + self.nodes[1].importmulti([{ + "scriptPubKey": {"address": p2shaddr4}, + "timestamp": "now", + "redeemscript": script4.hex(), + "keys": [priv_key4], + }]) + + p2shaddrinfo = self.nodes[1].getaddressinfo(p2shaddr4) + + assert p2shaddrinfo.get("embedded") + + embedded_addrinfo = self.nodes[1].getaddressinfo( + p2shaddrinfo["embedded"]["address"] + ) + + assert_equal(p2shaddrinfo["label"], "") + assert_equal(embedded_addrinfo["label"], label4_addr) + + self.stop_nodes() + + +if __name__ == "__main__": + ImportWithLabel().main()