Merge #11415: [RPC] Disallow using addresses in createmultisig

1df206f Disallow using addresses in createmultisig (Andrew Chow)

Pull request description:

  This PR should be the last part of #7965.

  This PR makes createmultisig only accept public keys and marks the old functionality of accepting addresses as deprecated.

  It also splits `_createmultisig_redeemscript` into two functions, `_createmultisig_getpubkeys` and `_createmultisig_getaddr_pubkeys`. `_createmultisig_getpubkeys` retrieves public keys from the RPC parameters and `_createmultisig_getaddr_pubkeys` retrieves addresses' public keys from the wallet. `_createmultisig_getaddr_pubkeys` requires the wallet and is only used by `addwitnessaddress` (except when `createmultisig` is used in deprecated mode).

  `addwitnessaddress`'s API is also changed. Instead of returning just an address, it now returns the same thing as `createmultisig`: a JSON object with two fields, address and redeemscript.

Tree-SHA512: a5796e41935ad5e47d8165ff996a8b20d5112b5fc1a06a6d3c7f5513c13e7628a4fd37ec30fde05d8b15abfed51bc250710140f6834b13f64d0a0e47a3817969

Author: laanwj

src/Makefile.am

@@ -133,6 +133,7 @@ BITCOIN_CORE_H = \
   rpc/safemode.h \
   rpc/server.h \
   rpc/register.h \
+  rpc/util.h \
   scheduler.h \
   script/sigcache.h \
   script/sign.h \
@@ -352,6 +353,7 @@ libbitcoin_util_a_SOURCES = \
   fs.cpp \
   random.cpp \
   rpc/protocol.cpp \
+  rpc/util.cpp \
   support/cleanse.cpp \
   sync.cpp \
   threadinterrupt.cpp \

src/rpc/misc.cpp

@@ -15,6 +15,7 @@
 #include <netbase.h>
 #include <rpc/blockchain.h>
 #include <rpc/server.h>
+#include <rpc/util.h>
 #include <timedata.h>
 #include <util.h>
 #include <utilstrencodings.h>
@@ -254,88 +255,21 @@ UniValue validateaddress(const JSONRPCRequest& request)
 // Needed even with !ENABLE_WALLET, to pass (ignored) pointers around
 class CWallet;
 
-/**
- * Used by addmultisigaddress / createmultisig:
- */
-CScript _createmultisig_redeemScript(CWallet * const pwallet, const UniValue& params)
-{
-    int nRequired = params[0].get_int();
-    const UniValue& keys = params[1].get_array();
-
-    // Gather public keys
-    if (nRequired < 1)
-        throw std::runtime_error("a multisignature address must require at least one key to redeem");
-    if ((int)keys.size() < nRequired)
-        throw std::runtime_error(
-            strprintf("not enough keys supplied "
-                      "(got %u keys, but need at least %d to redeem)", keys.size(), nRequired));
-    if (keys.size() > 16)
-        throw std::runtime_error("Number of addresses involved in the multisignature address creation > 16\nReduce the number");
-    std::vector<CPubKey> pubkeys;
-    pubkeys.resize(keys.size());
-    for (unsigned int i = 0; i < keys.size(); i++)
-    {
-        const std::string& ks = keys[i].get_str();
-#ifdef ENABLE_WALLET
-        // Case 1: Bitcoin address and we have full public key:
-        CTxDestination dest = DecodeDestination(ks);
-        if (pwallet && IsValidDestination(dest)) {
-            CKeyID key = GetKeyForDestination(*pwallet, dest);
-            if (key.IsNull()) {
-                throw std::runtime_error(strprintf("%s does not refer to a key", ks));
-            }
-            CPubKey vchPubKey;
-            if (!pwallet->GetPubKey(key, vchPubKey)) {
-                throw std::runtime_error(strprintf("no full public key for address %s", ks));
-            }
-            if (!vchPubKey.IsFullyValid())
-                throw std::runtime_error(" Invalid public key: "+ks);
-            pubkeys[i] = vchPubKey;
-        }
-
-        // Case 2: hex public key
-        else
-#endif
-        if (IsHex(ks))
-        {
-            CPubKey vchPubKey(ParseHex(ks));
-            if (!vchPubKey.IsFullyValid())
-                throw std::runtime_error(" Invalid public key: "+ks);
-            pubkeys[i] = vchPubKey;
-        }
-        else
-        {
-            throw std::runtime_error(" Invalid public key: "+ks);
-        }
-    }
-    CScript result = GetScriptForMultisig(nRequired, pubkeys);
-
-    if (result.size() > MAX_SCRIPT_ELEMENT_SIZE)
-        throw std::runtime_error(
-                strprintf("redeemScript exceeds size limit: %d > %d", result.size(), MAX_SCRIPT_ELEMENT_SIZE));
-
-    return result;
-}
-
 UniValue createmultisig(const JSONRPCRequest& request)
 {
-#ifdef ENABLE_WALLET
-    CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
-#else
-    CWallet * const pwallet = nullptr;
-#endif
-
     if (request.fHelp || request.params.size() < 2 || request.params.size() > 2)
     {
         std::string msg = "createmultisig nrequired [\"key\",...]\n"
             "\nCreates a multi-signature address with n signature of m keys required.\n"
             "It returns a json object with the address and redeemScript.\n"
-
+            "DEPRECATION WARNING: Using addresses with createmultisig is deprecated. Clients must\n"
+            "transition to using addmultisigaddress to create multisig addresses with addresses known\n"
+            "to the wallet before upgrading to v0.17. To use the deprecated functionality, start bitcoind with -deprecatedrpc=createmultisig\n"
             "\nArguments:\n"
-            "1. nrequired      (numeric, required) The number of required signatures out of the n keys or addresses.\n"
-            "2. \"keys\"       (string, required) A json array of keys which are bitcoin addresses or hex-encoded public keys\n"
+            "1. nrequired                    (numeric, required) The number of required signatures out of the n keys or addresses.\n"
+            "2. \"keys\"                       (string, required) A json array of hex-encoded public keys\n"
             "     [\n"
-            "       \"key\"    (string) bitcoin address or hex-encoded public key\n"
+            "       \"key\"                    (string) The hex-encoded public key\n"
             "       ,...\n"
             "     ]\n"
 
@@ -346,16 +280,37 @@ UniValue createmultisig(const JSONRPCRequest& request)
             "}\n"
 
             "\nExamples:\n"
-            "\nCreate a multisig address from 2 addresses\n"
-            + HelpExampleCli("createmultisig", "2 \"[\\\"16sSauSf5pF2UkUwvKGq4qjNRzBZYqgEL5\\\",\\\"171sgjn4YtPu27adkKGrdDwzRTxnRkBfKV\\\"]\"") +
+            "\nCreate a multisig address from 2 public keys\n"
+            + HelpExampleCli("createmultisig", "2 \"[\\\"03789ed0bb717d88f7d321a368d905e7430207ebbd82bd342cf11ae157a7ace5fd\\\",\\\"03dbc6764b8884a92e871274b87583e6d5c2a58819473e17e107ef3f6aa5a61626\\\"]\"") +
             "\nAs a json rpc call\n"
-            + HelpExampleRpc("createmultisig", "2, \"[\\\"16sSauSf5pF2UkUwvKGq4qjNRzBZYqgEL5\\\",\\\"171sgjn4YtPu27adkKGrdDwzRTxnRkBfKV\\\"]\"")
+            + HelpExampleRpc("createmultisig", "2, \"[\\\"03789ed0bb717d88f7d321a368d905e7430207ebbd82bd342cf11ae157a7ace5fd\\\",\\\"03dbc6764b8884a92e871274b87583e6d5c2a58819473e17e107ef3f6aa5a61626\\\"]\"")
         ;
         throw std::runtime_error(msg);
     }
 
+    int required = request.params[0].get_int();
+
+    // Get the public keys
+    const UniValue& keys = request.params[1].get_array();
+    std::vector<CPubKey> pubkeys;
+    for (unsigned int i = 0; i < keys.size(); ++i) {
+        if (IsHex(keys[i].get_str()) && (keys[i].get_str().length() == 66 || keys[i].get_str().length() == 130)) {
+            pubkeys.push_back(HexToPubKey(keys[i].get_str()));
+        } else {
+#ifdef ENABLE_WALLET
+            CWallet* const pwallet = GetWalletForJSONRPCRequest(request);
+            if (IsDeprecatedRPCEnabled("createmultisig") && EnsureWalletIsAvailable(pwallet, false)) {
+                pubkeys.push_back(AddrToPubKey(pwallet, keys[i].get_str()));
+            } else
+#endif
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid public key: %s\nNote that from v0.16, createmultisig no longer accepts addresses."
+            " Clients must transition to using addmultisigaddress to create multisig addresses with addresses known to the wallet before upgrading to v0.17."
+            " To use the deprecated functionality, start bitcoind with -deprecatedrpc=createmultisig", keys[i].get_str()));
+        }
+    }
+
     // Construct using pay-to-script-hash:
-    CScript inner = _createmultisig_redeemScript(pwallet, request.params);
+    CScript inner = CreateMultisigRedeemscript(required, pubkeys);
     CScriptID innerID(inner);
 
     UniValue result(UniValue::VOBJ);

src/rpc/util.cpp

@@ -0,0 +1,68 @@
+// Copyright (c) 2017 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <base58.h>
+#include <keystore.h>
+#include <pubkey.h>
+#include <rpc/protocol.h>
+#include <rpc/util.h>
+#include <tinyformat.h>
+#include <utilstrencodings.h>
+
+// Converts a hex string to a public key if possible
+CPubKey HexToPubKey(const std::string& hex_in)
+{
+    if (!IsHex(hex_in)) {
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + hex_in);
+    }
+    CPubKey vchPubKey(ParseHex(hex_in));
+    if (!vchPubKey.IsFullyValid()) {
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid public key: " + hex_in);
+    }
+    return vchPubKey;
+}
+
+// Retrieves a public key for an address from the given CKeyStore
+CPubKey AddrToPubKey(CKeyStore* const keystore, const std::string& addr_in)
+{
+    CTxDestination dest = DecodeDestination(addr_in);
+    if (!IsValidDestination(dest)) {
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address: " + addr_in);
+    }
+    CKeyID key = GetKeyForDestination(*keystore, dest);
+    if (key.IsNull()) {
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("%s does not refer to a key", addr_in));
+    }
+    CPubKey vchPubKey;
+    if (!keystore->GetPubKey(key, vchPubKey)) {
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("no full public key for address %s", addr_in));
+    }
+    if (!vchPubKey.IsFullyValid()) {
+       throw JSONRPCError(RPC_INTERNAL_ERROR, "Wallet contains an invalid public key");
+    }
+    return vchPubKey;
+}
+
+// Creates a multisig redeemscript from a given list of public keys and number required.
+CScript CreateMultisigRedeemscript(const int required, const std::vector<CPubKey>& pubkeys)
+{
+    // Gather public keys
+    if (required < 1) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "a multisignature address must require at least one key to redeem");
+    }
+    if ((int)pubkeys.size() < required) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("not enough keys supplied (got %u keys, but need at least %d to redeem)", pubkeys.size(), required));
+    }
+    if (pubkeys.size() > 16) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Number of keys involved in the multisignature address creation > 16\nReduce the number");
+    }
+
+    CScript result = GetScriptForMultisig(required, pubkeys);
+
+    if (result.size() > MAX_SCRIPT_ELEMENT_SIZE) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, (strprintf("redeemScript exceeds size limit: %d > %d", result.size(), MAX_SCRIPT_ELEMENT_SIZE)));
+    }
+
+    return result;
+}

src/rpc/util.h

@@ -0,0 +1,19 @@
+// Copyright (c) 2017 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_RPC_UTIL_H
+#define BITCOIN_RPC_UTIL_H
+
+#include <string>
+#include <vector>
+
+class CKeyStore;
+class CPubKey;
+class CScript;
+
+CPubKey HexToPubKey(const std::string& hex_in);
+CPubKey AddrToPubKey(CKeyStore* const keystore, const std::string& addr_in);
+CScript CreateMultisigRedeemscript(const int required, const std::vector<CPubKey>& pubkeys);
+
+#endif // BITCOIN_RPC_UTIL_H

src/wallet/rpcwallet.cpp

@@ -18,6 +18,7 @@
 #include <rpc/mining.h>
 #include <rpc/safemode.h>
 #include <rpc/server.h>
+#include <rpc/util.h>
 #include <script/sign.h>
 #include <timedata.h>
 #include <util.h>
@@ -1161,9 +1162,6 @@ UniValue sendmany(const JSONRPCRequest& request)
     return wtx.GetHash().GetHex();
 }
 
-// Defined in rpc/misc.cpp
-extern CScript _createmultisig_redeemScript(CWallet * const pwallet, const UniValue& params);
-
 UniValue addmultisigaddress(const JSONRPCRequest& request)
 {
     CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
@@ -1181,16 +1179,22 @@ UniValue addmultisigaddress(const JSONRPCRequest& request)
             "If 'account' is specified (DEPRECATED), assign address to that account.\n"
 
             "\nArguments:\n"
-            "1. nrequired        (numeric, required) The number of required signatures out of the n keys or addresses.\n"
-            "2. \"keys\"         (string, required) A json array of bitcoin addresses or hex-encoded public keys\n"
+            "1. nrequired                      (numeric, required) The number of required signatures out of the n keys or addresses.\n"
+            "2. \"keys\"                         (string, required) A json array of bitcoin addresses or hex-encoded public keys\n"
             "     [\n"
-            "       \"address\"  (string) bitcoin address or hex-encoded public key\n"
+            "       \"address\"                  (string) bitcoin address or hex-encoded public key\n"
             "       ...,\n"
             "     ]\n"
-            "3. \"account\"      (string, optional) DEPRECATED. An account to assign the addresses to.\n"
+            "3. \"account\"                      (string, optional) DEPRECATED. An account to assign the addresses to.\n"
 
             "\nResult:\n"
-            "\"address\"         (string) A bitcoin address associated with the keys.\n"
+            "{\n"
+            "  \"address\":\"multisigaddress\",    (string) The value of the new multisig address.\n"
+            "  \"redeemScript\":\"script\"         (string) The string value of the hex-encoded redemption script.\n"
+            "}\n"
+            "\nResult (DEPRECATED. To see this result in v0.16 instead, please start bitcoind with -deprecatedrpc=addmultisigaddress).\n"
+            "        clients should transition to the new output api before upgrading to v0.17.\n"
+            "\"address\"                         (string) A bitcoin address associated with the keys.\n"
 
             "\nExamples:\n"
             "\nAdd a multisig address from 2 addresses\n"
@@ -1207,14 +1211,34 @@ UniValue addmultisigaddress(const JSONRPCRequest& request)
     if (!request.params[2].isNull())
         strAccount = AccountFromValue(request.params[2]);
 
+    int required = request.params[0].get_int();
+
+    // Get the public keys
+    const UniValue& keys_or_addrs = request.params[1].get_array();
+    std::vector<CPubKey> pubkeys;
+    for (unsigned int i = 0; i < keys_or_addrs.size(); ++i) {
+        if (IsHex(keys_or_addrs[i].get_str()) && (keys_or_addrs[i].get_str().length() == 66 || keys_or_addrs[i].get_str().length() == 130)) {
+            pubkeys.push_back(HexToPubKey(keys_or_addrs[i].get_str()));
+        } else {
+            pubkeys.push_back(AddrToPubKey(pwallet, keys_or_addrs[i].get_str()));
+        }
+    }
+
     // Construct using pay-to-script-hash:
-    CScript inner = _createmultisig_redeemScript(pwallet, request.params);
+    CScript inner = CreateMultisigRedeemscript(required, pubkeys);
     pwallet->AddCScript(inner);
-
     CTxDestination dest = pwallet->AddAndGetDestinationForScript(inner, g_address_type);
-
     pwallet->SetAddressBook(dest, strAccount, "send");
-    return EncodeDestination(dest);
+
+    // Return old style interface
+    if (IsDeprecatedRPCEnabled("addmultisigaddress")) {
+        return EncodeDestination(dest);
+    }
+
+    UniValue result(UniValue::VOBJ);
+    result.pushKV("address", EncodeDestination(dest));
+    result.pushKV("redeemScript", HexStr(inner.begin(), inner.end()));
+    return result;
 }
 
 class Witnessifier : public boost::static_visitor<bool>

test/functional/address_types.py

@@ -117,14 +117,14 @@ def run_test(self):
 
         # addmultisigaddress with at least 1 uncompressed key should return a legacy address.
         for node in range(4):
-            self.test_address(node, self.nodes[node].addmultisigaddress(2, [uncompressed_1, uncompressed_2]), True, 'legacy')
-            self.test_address(node, self.nodes[node].addmultisigaddress(2, [compressed_1, uncompressed_2]), True, 'legacy')
-            self.test_address(node, self.nodes[node].addmultisigaddress(2, [uncompressed_1, compressed_2]), True, 'legacy')
+            self.test_address(node, self.nodes[node].addmultisigaddress(2, [uncompressed_1, uncompressed_2])['address'], True, 'legacy')
+            self.test_address(node, self.nodes[node].addmultisigaddress(2, [compressed_1, uncompressed_2])['address'], True, 'legacy')
+            self.test_address(node, self.nodes[node].addmultisigaddress(2, [uncompressed_1, compressed_2])['address'], True, 'legacy')
         # addmultisigaddress with all compressed keys should return the appropriate address type (even when the keys are not ours).
-        self.test_address(0, self.nodes[0].addmultisigaddress(2, [compressed_1, compressed_2]), True, 'legacy')
-        self.test_address(1, self.nodes[1].addmultisigaddress(2, [compressed_1, compressed_2]), True, 'p2sh-segwit')
-        self.test_address(2, self.nodes[2].addmultisigaddress(2, [compressed_1, compressed_2]), True, 'p2sh-segwit')
-        self.test_address(3, self.nodes[3].addmultisigaddress(2, [compressed_1, compressed_2]), True, 'bech32')
+        self.test_address(0, self.nodes[0].addmultisigaddress(2, [compressed_1, compressed_2])['address'], True, 'legacy')
+        self.test_address(1, self.nodes[1].addmultisigaddress(2, [compressed_1, compressed_2])['address'], True, 'p2sh-segwit')
+        self.test_address(2, self.nodes[2].addmultisigaddress(2, [compressed_1, compressed_2])['address'], True, 'p2sh-segwit')
+        self.test_address(3, self.nodes[3].addmultisigaddress(2, [compressed_1, compressed_2])['address'], True, 'bech32')
 
         for explicit_type, multisig, from_node in itertools.product([False, True], [False, True], range(4)):
             address_type = None
@@ -155,7 +155,7 @@ def run_test(self):
                 else:
                     addr1 = self.nodes[to_node].getnewaddress()
                     addr2 = self.nodes[to_node].getnewaddress()
-                    address = self.nodes[to_node].addmultisigaddress(2, [addr1, addr2])
+                    address = self.nodes[to_node].addmultisigaddress(2, [addr1, addr2])['address']
 
                 # Do some sanity checking on the created address
                 if address_type is not None:

test/functional/deprecated_rpc.py

@@ -10,7 +10,7 @@ class DeprecatedRpcTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
         self.setup_clean_chain = True
-        self.extra_args = [[], ["-deprecatedrpc=estimatefee"]]
+        self.extra_args = [[], ["-deprecatedrpc=estimatefee", "-deprecatedrpc=createmultisig"]]
 
     def run_test(self):
         self.log.info("estimatefee: Shows deprecated message")
@@ -19,5 +19,9 @@ def run_test(self):
         self.log.info("Using -deprecatedrpc=estimatefee bypasses the error")
         self.nodes[1].estimatefee(1)
 
+        self.log.info("Make sure that -deprecatedrpc=createmultisig allows it to take addresses")
+        assert_raises_rpc_error(-5, "Invalid public key", self.nodes[0].createmultisig, 1, [self.nodes[0].getnewaddress()])
+        self.nodes[1].createmultisig(1, [self.nodes[1].getnewaddress()])
+
 if __name__ == '__main__':
     DeprecatedRpcTest().main()