From a49503402b6bc21e3878e151c07529941d36aed0 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Wed, 15 May 2019 00:21:11 -0400
Subject: [PATCH] Make and get the multisig redeemscript and destination in one
 function instead of two

Instead of creating a redeemScript with CreateMultisigRedeemscript and
then getting the destination with AddAndGetDestinationForScript, do
both in the same function.

CreateMultisigRedeemscript is changed to AddAndGetMultisigDestination.
It creates the redeemScript and returns it via an output parameter. Then
it calls AddAndGetDestinationForScript to add the destination to the
keystore and get the proper destination.

This allows us to inspect the public keys in the redeemScript before creating
the destination so that the correct destination is used when uncompressed
pubkeys are in the multisig.
---
 src/rpc/misc.cpp                      |  4 ++--
 src/rpc/util.cpp                      | 24 +++++++++++++++------
 src/rpc/util.h                        |  3 ++-
 src/wallet/rpcwallet.cpp              |  4 ++--
 test/functional/rpc_createmultisig.py | 30 ++++++++++++++++++++++++++-
 5 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp
index 7008a83143..5ecf734210 100644
--- a/src/rpc/misc.cpp
+++ b/src/rpc/misc.cpp
@@ -130,9 +130,9 @@ static UniValue createmultisig(const JSONRPCRequest& request)
     }
 
     // Construct using pay-to-script-hash:
-    const CScript inner = CreateMultisigRedeemscript(required, pubkeys);
     CBasicKeyStore keystore;
-    const CTxDestination dest = AddAndGetDestinationForScript(keystore, inner, output_type);
+    CScript inner;
+    const CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, keystore, inner);
 
     UniValue result(UniValue::VOBJ);
     result.pushKV("address", EncodeDestination(dest));
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 9cdb22001f..4642cf16b1 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -4,6 +4,7 @@
 
 #include <key_io.h>
 #include <keystore.h>
+#include <outputtype.h>
 #include <rpc/util.h>
 #include <tinyformat.h>
 #include <util/strencodings.h>
@@ -150,8 +151,8 @@ CPubKey AddrToPubKey(CKeyStore* const keystore, const std::string& addr_in)
     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)
+// Creates a multisig address from a given list of public keys, number of signatures required, and the address type
+CTxDestination AddAndGetMultisigDestination(const int required, const std::vector<CPubKey>& pubkeys, OutputType type, CKeyStore& keystore, CScript& script_out)
 {
     // Gather public keys
     if (required < 1) {
@@ -164,13 +165,24 @@ CScript CreateMultisigRedeemscript(const int required, const std::vector<CPubKey
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Number of keys involved in the multisignature address creation > 16\nReduce the number");
     }
 
-    CScript result = GetScriptForMultisig(required, pubkeys);
+    script_out = 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)));
+    if (script_out.size() > MAX_SCRIPT_ELEMENT_SIZE) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, (strprintf("redeemScript exceeds size limit: %d > %d", script_out.size(), MAX_SCRIPT_ELEMENT_SIZE)));
     }
 
-    return result;
+    // Check if any keys are uncompressed. If so, the type is legacy
+    for (const CPubKey& pk : pubkeys) {
+        if (!pk.IsCompressed()) {
+            type = OutputType::LEGACY;
+            break;
+        }
+    }
+
+    // Make the address
+    CTxDestination dest = AddAndGetDestinationForScript(keystore, script_out, type);
+
+    return dest;
 }
 
 class DescribeAddressVisitor : public boost::static_visitor<UniValue>
diff --git a/src/rpc/util.h b/src/rpc/util.h
index e4fa8fc3d7..0eb2fef5c3 100644
--- a/src/rpc/util.h
+++ b/src/rpc/util.h
@@ -6,6 +6,7 @@
 #define BITCOIN_RPC_UTIL_H
 
 #include <node/transaction.h>
+#include <outputtype.h>
 #include <pubkey.h>
 #include <rpc/protocol.h>
 #include <script/standard.h>
@@ -70,7 +71,7 @@ extern std::string HelpExampleRpc(const std::string& methodname, const std::stri
 
 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);
+CTxDestination AddAndGetMultisigDestination(const int required, const std::vector<CPubKey>& pubkeys, OutputType type, CKeyStore& keystore, CScript& script_out);
 
 UniValue DescribeAddress(const CTxDestination& dest);
 
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 43e18a580d..69e8f0b16c 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -1023,8 +1023,8 @@ static UniValue addmultisigaddress(const JSONRPCRequest& request)
     }
 
     // Construct using pay-to-script-hash:
-    CScript inner = CreateMultisigRedeemscript(required, pubkeys);
-    CTxDestination dest = AddAndGetDestinationForScript(*pwallet, inner, output_type);
+    CScript inner;
+    CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, *pwallet, inner);
     pwallet->SetAddressBook(dest, label, "send");
 
     UniValue result(UniValue::VOBJ);
diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py
index 7abcd71bb8..58010f7c2e 100755
--- a/test/functional/rpc_createmultisig.py
+++ b/test/functional/rpc_createmultisig.py
@@ -7,9 +7,13 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_raises_rpc_error,
+    assert_equal,
 )
-import decimal
+from test_framework.key import ECPubKey
 
+import binascii
+import decimal
+import itertools
 
 class RpcCreateMultiSigTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -44,6 +48,30 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
 
         self.checkbalances()
 
+        # Test mixed compressed and uncompressed pubkeys
+        self.log.info('Mixed compressed and uncompressed multisigs are not allowed')
+        pk0 = node0.getaddressinfo(node0.getnewaddress())['pubkey']
+        pk1 = node1.getaddressinfo(node1.getnewaddress())['pubkey']
+        pk2 = node2.getaddressinfo(node2.getnewaddress())['pubkey']
+
+        # decompress pk2
+        pk_obj = ECPubKey()
+        pk_obj.set(binascii.unhexlify(pk2))
+        pk_obj.compressed = False
+        pk2 = binascii.hexlify(pk_obj.get_bytes()).decode()
+
+        # Check all permutations of keys because order matters apparently
+        for keys in itertools.permutations([pk0, pk1, pk2]):
+            # Results should be the same as this legacy one
+            legacy_addr = node0.createmultisig(2, keys, 'legacy')['address']
+            assert_equal(legacy_addr, node0.addmultisigaddress(2, keys, '', 'legacy')['address'])
+
+            # Generate addresses with the segwit types. These should all make legacy addresses
+            assert_equal(legacy_addr, node0.createmultisig(2, keys, 'bech32')['address'])
+            assert_equal(legacy_addr, node0.createmultisig(2, keys, 'p2sh-segwit')['address'])
+            assert_equal(legacy_addr, node0.addmultisigaddress(2, keys, '', 'bech32')['address'])
+            assert_equal(legacy_addr, node0.addmultisigaddress(2, keys, '', 'p2sh-segwit')['address'])
+
     def check_addmultisigaddress_errors(self):
         self.log.info('Check that addmultisigaddress fails when the private keys are missing')
         addresses = [self.nodes[1].getnewaddress(address_type='legacy') for _ in range(2)]