Merge #9332: Let wallet importmulti RPC accept labels for standard scriptPubKeys

98ea64cf23 Let wallet importmulti RPC accept labels for standard scriptPubKeys (Russell Yanofsky)

Pull request description:

  Allow importmulti RPC to apply address labels when importing standard scriptPubKeys. This makes the importmulti RPC less finnicky about import formats and also simpler internally.

Tree-SHA512: 102426b21239f1fa5f38162dc3f4145572caef76e63906afd786b7aff1670d6cd93456f8d85f737588eedc49c11bef2e1e8019b8b2cbf6097c77b3501b0cab1f

src/wallet/rpcdump.cpp

@@ -849,6 +849,9 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
 
             std::vector<unsigned char> vData(ParseHex(output));
             script = CScript(vData.begin(), vData.end());
+            if (!ExtractDestination(script, dest) && !internal) {
+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Internal must be set to true for nonstandard scriptPubKey imports.");
+            }
         }
 
         // Watchonly and private keys
@@ -861,11 +864,6 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Incompatibility found between internal and label");
         }
 
-        // Not having Internal + Script
-        if (!internal && isScript) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "Internal must be set for hex scriptPubKey");
-        }
-
         // Keys / PubKeys size check.
         if (!isP2SH && (keys.size() > 1 || pubKeys.size() > 1)) { // Address / scriptPubKey
             throw JSONRPCError(RPC_INVALID_PARAMETER, "More than private key given for one address");
@@ -969,21 +967,10 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
                 CTxDestination pubkey_dest = pubKey.GetID();
 
                 // Consistency check.
-                if (!isScript && !(pubkey_dest == dest)) {
+                if (!(pubkey_dest == dest)) {
                     throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Consistency check failed");
                 }
 
-                // Consistency check.
-                if (isScript) {
-                    CTxDestination destination;
-
-                    if (ExtractDestination(script, destination)) {
-                        if (!(destination == pubkey_dest)) {
-                            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Consistency check failed");
-                        }
-                    }
-                }
-
                 CScript pubKeyScript = GetScriptForDestination(pubkey_dest);
 
                 if (::IsMine(*pwallet, pubKeyScript) == ISMINE_SPENDABLE) {
@@ -1034,21 +1021,10 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
                 CTxDestination pubkey_dest = pubKey.GetID();
 
                 // Consistency check.
-                if (!isScript && !(pubkey_dest == dest)) {
+                if (!(pubkey_dest == dest)) {
                     throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Consistency check failed");
                 }
 
-                // Consistency check.
-                if (isScript) {
-                    CTxDestination destination;
-
-                    if (ExtractDestination(script, destination)) {
-                        if (!(destination == pubkey_dest)) {
-                            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Consistency check failed");
-                        }
-                    }
-                }
-
                 CKeyID vchAddress = pubKey.GetID();
                 pwallet->MarkDirty();
                 pwallet->SetAddressBook(vchAddress, label, "receive");
@@ -1080,11 +1056,9 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
                     throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
                 }
 
-                if (scriptPubKey.getType() == UniValue::VOBJ) {
+                // add to address book or update label
-                    // add to address book or update label
+                if (IsValidDestination(dest)) {
-                    if (IsValidDestination(dest)) {
+                    pwallet->SetAddressBook(dest, label, "receive");
-                        pwallet->SetAddressBook(dest, label, "receive");
-                    }
                 }
 
                 success = true;

test/functional/wallet_import_rescan.py

@@ -26,7 +26,7 @@ import collections
 import enum
 import itertools
 
-Call = enum.Enum("Call", "single multi")
+Call = enum.Enum("Call", "single multiaddress multiscript")
 Data = enum.Enum("Data", "address pub priv")
 Rescan = enum.Enum("Rescan", "no yes late_timestamp")
 
@@ -53,11 +53,11 @@ class Variant(collections.namedtuple("Variant", "call data rescan prune")):
                 response = self.try_rpc(self.node.importprivkey, privkey=self.key, rescan=rescan)
             assert_equal(response, None)
 
-        elif self.call == Call.multi:
+        elif self.call in (Call.multiaddress, Call.multiscript):
             response = self.node.importmulti([{
                 "scriptPubKey": {
                     "address": self.address["address"]
-                },
+                } if self.call == Call.multiaddress else self.address["scriptPubKey"],
                 "timestamp": timestamp + TIMESTAMP_WINDOW + (1 if self.rescan == Rescan.late_timestamp else 0),
                 "pubkeys": [self.address["pubkey"]] if self.data == Data.pub else [],
                 "keys": [self.key] if self.data == Data.priv else [],
@@ -126,7 +126,7 @@ class ImportRescanTest(BitcoinTestFramework):
         for i, variant in enumerate(IMPORT_VARIANTS):
             variant.address = self.nodes[1].getaddressinfo(self.nodes[1].getnewaddress())
             variant.key = self.nodes[1].dumpprivkey(variant.address["address"])
-            variant.initial_amount = 10 - (i + 1) / 4.0
+            variant.initial_amount = 1 - (i + 1) / 64
             variant.initial_txid = self.nodes[0].sendtoaddress(variant.address["address"], variant.initial_amount)
 
         # Generate a block containing the initial transactions, then another
@@ -156,7 +156,7 @@ class ImportRescanTest(BitcoinTestFramework):
 
         # Create new transactions sending to each address.
         for i, variant in enumerate(IMPORT_VARIANTS):
-            variant.sent_amount = 10 - (2 * i + 1) / 8.0
+            variant.sent_amount = 1 - (2 * i + 1) / 128
             variant.sent_txid = self.nodes[0].sendtoaddress(variant.address["address"], variant.sent_amount)
 
         # Generate a block containing the new transactions.

test/functional/wallet_importmulti.py

@@ -3,6 +3,8 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test the importmulti RPC."""
+
+from test_framework import script
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal, assert_greater_than, assert_raises_rpc_error
 
@@ -79,16 +81,17 @@ class ImportMultiTest (BitcoinTestFramework):
         assert_equal(address_assert['ismine'], False)
         assert_equal(address_assert['timestamp'], timestamp)
 
-        # ScriptPubKey + !internal
+        # Nonstandard scriptPubKey + !internal
-        self.log.info("Should not import a scriptPubKey without internal flag")
+        self.log.info("Should not import a nonstandard scriptPubKey without internal flag")
+        nonstandardScriptPubKey = address['scriptPubKey'] + bytes_to_hex_str(script.CScript([script.OP_NOP]))
         address = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
         result = self.nodes[1].importmulti([{
-            "scriptPubKey": address['scriptPubKey'],
+            "scriptPubKey": nonstandardScriptPubKey,
             "timestamp": "now",
         }])
         assert_equal(result[0]['success'], False)
         assert_equal(result[0]['error']['code'], -8)
-        assert_equal(result[0]['error']['message'], 'Internal must be set for hex scriptPubKey')
+        assert_equal(result[0]['error']['message'], 'Internal must be set to true for nonstandard scriptPubKey imports.')
         address_assert = self.nodes[1].getaddressinfo(address['address'])
         assert_equal(address_assert['iswatchonly'], False)
         assert_equal(address_assert['ismine'], False)
@@ -128,18 +131,18 @@ class ImportMultiTest (BitcoinTestFramework):
         assert_equal(address_assert['ismine'], False)
         assert_equal(address_assert['timestamp'], timestamp)
 
-        # ScriptPubKey + Public key + !internal
+        # Nonstandard scriptPubKey + Public key + !internal
-        self.log.info("Should not import a scriptPubKey without internal and with public key")
+        self.log.info("Should not import a nonstandard scriptPubKey without internal and with public key")
         address = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
         request = [{
-            "scriptPubKey": address['scriptPubKey'],
+            "scriptPubKey": nonstandardScriptPubKey,
             "timestamp": "now",
             "pubkeys": [ address['pubkey'] ]
         }]
         result = self.nodes[1].importmulti(request)
         assert_equal(result[0]['success'], False)
         assert_equal(result[0]['error']['code'], -8)
-        assert_equal(result[0]['error']['message'], 'Internal must be set for hex scriptPubKey')
+        assert_equal(result[0]['error']['message'], 'Internal must be set to true for nonstandard scriptPubKey imports.')
         address_assert = self.nodes[1].getaddressinfo(address['address'])
         assert_equal(address_assert['iswatchonly'], False)
         assert_equal(address_assert['ismine'], False)
@@ -207,17 +210,17 @@ class ImportMultiTest (BitcoinTestFramework):
         assert_equal(address_assert['ismine'], True)
         assert_equal(address_assert['timestamp'], timestamp)
 
-        # ScriptPubKey + Private key + !internal
+        # Nonstandard scriptPubKey + Private key + !internal
-        self.log.info("Should not import a scriptPubKey without internal and with private key")
+        self.log.info("Should not import a nonstandard scriptPubKey without internal and with private key")
         address = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
         result = self.nodes[1].importmulti([{
-            "scriptPubKey": address['scriptPubKey'],
+            "scriptPubKey": nonstandardScriptPubKey,
             "timestamp": "now",
             "keys": [ self.nodes[0].dumpprivkey(address['address']) ]
         }])
         assert_equal(result[0]['success'], False)
         assert_equal(result[0]['error']['code'], -8)
-        assert_equal(result[0]['error']['message'], 'Internal must be set for hex scriptPubKey')
+        assert_equal(result[0]['error']['message'], 'Internal must be set to true for nonstandard scriptPubKey imports.')
         address_assert = self.nodes[1].getaddressinfo(address['address'])
         assert_equal(address_assert['iswatchonly'], False)
         assert_equal(address_assert['ismine'], False)