Merge #16026: Ensure that uncompressed public keys in a multisig always returns a legacy address

a495034 Make and get the multisig redeemscript and destination in one function instead of two (Andrew Chow)

Pull request description:

  `CreateMultisigRedeemscript()` is changed to `AddAndGetMultisigDestination()` so that the process of constructing the redeemScript and then getting the `CTxDestination` are done in the same function. This allows that function to see what the keys in the multisig are so that the correct address type is returned from `AddAndGetDestinationForScript()`.

  This only effects the `createmultisig` and `addmultisigaddress` RPCs and does not change signing logic as #16022 does.

  Alternative to #16022 and #16012

  Fixes #16011

ACKs for commit a49503:

Tree-SHA512: 5b0154a714deea3b2cc3a54beb420c95eeeacf4ca30c40ca80940d9d640f8b03611b0fc14c2f0710bfd8a79e8d27ad7d9ae380b4b83d52b40ab201624f2a63f0

Author: meshcollider

src/rpc/misc.cpp

@@ -122,9 +122,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));

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>

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

src/wallet/rpcwallet.cpp

@@ -1036,8 +1036,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);

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 @@ def run_test(self):
 
         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)]