Merge bitcoin/bitcoin#28307: rpc, wallet: fix incorrect segwit redeem script size limit

2451a217dd2c21b6d2f2b2699ceddd0bf9073019 test: addmultisigaddress, coverage for script size limits (furszy)
53302a09817e5b799d345dfea432546a55a9d727 bugfix: addmultisigaddress, add unsupported operation for redeem scripts over 520 bytes (furszy)
9be6065cc03f2408f290a332b203eef9c9cebf24 test: coverage for 16-20 segwit multisig scripts (furszy)
9d9a91c4ea6b3bb32ef4131bca86f1d6683fc901 rpc: bugfix, incorrect segwit redeem script size used in signrawtransactionwithkey (furszy)
0c9fedfc45fa7cbd6801ca5fd756863ec9a6911c fix incorrect multisig redeem script size limit for segwit (furszy)
f7a173b5785cda460470df9a74a0e0f94d7f9a18 test: rpc_createmultisig, decouple 'test_sortedmulti_descriptors_bip67' (furszy)
4f33dbd8f8c0e29f37b04e6af6d2c7905ecceaf6 test: rpc_createmultisig, decouple 'test_mixing_uncompressed_and_compressed_keys' (furszy)
25a81705d376e8c96dad45436ae3fca975b3daf5 test: rpc_createmultisig, remove unnecessary checkbalances() (furszy)
b5a328943362cfac6e90fd4e1b167c357d53b7d4 test: refactor, multiple cleanups in rpc_createmultisig.py (furszy)
3635d432681847313c098f9827483372a840e70f test: rpc_createmultisig, remove manual wallet initialization (furszy)

Pull request description:

  Fixing https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1674830104 and more.

  Currently, redeem scripts longer than 520 bytes, which are technically valid under segwit rules, have flaws in the following processes:
  1) The multisig creation process fails to deduce the output descriptor, resulting in the generation of an incorrect descriptor. Additionally, the accompanying user warning is also inaccurate.
  2) The `signrawtransactionwithkey` RPC command fail to sign them.
  3) The legacy wallet `addmultisigaddress` wrongly discards them.

  The issue arises because most of these flows are utilizing the legacy spkm keystore, which imposes
  the [p2sh max redeem script size rule](https://github.com/bitcoin/bitcoin/blob/ded687334031f4790ef6a36b999fb30a79dcf7b3/src/script/signingprovider.cpp#L160) on all scripts. Which blocks segwit redeem scripts longer than
  the max element size in all the previously mentioned processes (`createmultisig`, `addmultisigaddress`, and
  `signrawtransactionwithkey`).

  This PR fixes the problem, enabling the creation of multisig output descriptors involving more than 15 keys and
  allowing the signing of these scripts, along with other post-segwit redeem scripts that surpass the 520-byte
  p2sh limit.

  Important note:
  Instead of adding support for these longer redeem scripts in the legacy wallet, an "unsupported operation"
  error has been added. The reasons behind this decision are:

  1) The introduction of this feature brings about a compatibility-breaking change that requires downgrade
      protection; older wallets would be unable to interact with these "new" legacy wallets.

  2) Considering the ongoing deprecation of the legacy spkm, this issue provides another compelling
      reason to transition towards descriptors.

  Testing notes:
  To easily verify each of the fixes, I decoupled the tests into standalone commits. So they can be
  cherry-picked on top of master. Where `rpc_createmultisig.py` (with and without the `--legacy-wallet`
  arg) will fail without the bugs fixes commits.

  Extra note:
  The initial commits improves the `rpc_createmultisig.py` test in many ways. I found this test very
  antiquated, screaming for an update and cleanup.

ACKs for top commit:
  pinheadmz:
    ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
  theStack:
    Code-review ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
  achow101:
    ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019

Tree-SHA512: 71794533cbd46b3a1079fb4e9d190d3ea3b615de0cbfa443466e14f05e4616ca90e12ce2bf07113515ea8113e64a560ad572bb9ea9d4835b6fb67b6ae596167f

11 files changed, 167 insertions, 140 deletions

diff --git a/src/outputtype.cpp b/src/outputtype.cpp
index c72d9deacb..8c2b76494b 100644
--- a/src/outputtype.cpp
+++ b/src/outputtype.cpp
@@ -81,11 +81,11 @@ std::vector<CTxDestination> GetAllDestinationsForKey(const CPubKey& key)
     }
 }
 
-CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, const CScript& script, OutputType type)
+CTxDestination AddAndGetDestinationForScript(FlatSigningProvider& keystore, const CScript& script, OutputType type)
 {
     // Add script to keystore
-    keystore.AddCScript(script);
-    // Note that scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are not yet supported.
+    keystore.scripts.emplace(CScriptID(script), script);
+
     switch (type) {
     case OutputType::LEGACY:
         return ScriptHash(script);
@@ -94,7 +94,7 @@ CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore,
         CTxDestination witdest = WitnessV0ScriptHash(script);
         CScript witprog = GetScriptForDestination(witdest);
         // Add the redeemscript, so that P2WSH and P2SH-P2WSH outputs are recognized as ours.
-        keystore.AddCScript(witprog);
+        keystore.scripts.emplace(CScriptID(witprog), witprog);
         if (type == OutputType::BECH32) {
             return witdest;
         } else {
diff --git a/src/outputtype.h b/src/outputtype.h
index a2d5942320..feef7991a6 100644
--- a/src/outputtype.h
+++ b/src/outputtype.h
@@ -46,7 +46,7 @@ std::vector<CTxDestination> GetAllDestinationsForKey(const CPubKey& key);
  * This function will automatically add the script (and any other
  * necessary scripts) to the keystore.
  */
-CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, const CScript& script, OutputType);
+CTxDestination AddAndGetDestinationForScript(FlatSigningProvider& keystore, const CScript& script, OutputType);
 
 /** Get the OutputType for a CTxDestination */
 std::optional<OutputType> OutputTypeFromDestination(const CTxDestination& dest);
diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp
index f771b31db0..65a9be2762 100644
--- a/src/rpc/output_script.cpp
+++ b/src/rpc/output_script.cpp
@@ -139,8 +139,7 @@ static RPCHelpMan createmultisig()
                 output_type = parsed.value();
             }
 
-            // Construct using pay-to-script-hash:
-            FillableSigningProvider keystore;
+            FlatSigningProvider keystore;
             CScript inner;
             const CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, keystore, inner);
 
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 7fa6652f9e..2ecaeeaf40 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -785,7 +785,7 @@ static RPCHelpMan signrawtransactionwithkey()
         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input.");
     }
 
-    FillableSigningProvider keystore;
+    FlatSigningProvider keystore;
     const UniValue& keys = request.params[1].get_array();
     for (unsigned int idx = 0; idx < keys.size(); ++idx) {
         UniValue k = keys[idx];
@@ -793,7 +793,11 @@ static RPCHelpMan signrawtransactionwithkey()
         if (!key.IsValid()) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key");
         }
-        keystore.AddKey(key);
+
+        CPubKey pubkey = key.GetPubKey();
+        CKeyID key_id = pubkey.GetID();
+        keystore.pubkeys.emplace(key_id, pubkey);
+        keystore.keys.emplace(key_id, key);
     }
 
     // Fetch previous transactions (inputs):
diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp
index a62f90215a..53f943bb9e 100644
--- a/src/rpc/rawtransaction_util.cpp
+++ b/src/rpc/rawtransaction_util.cpp
@@ -181,7 +181,7 @@ static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std::
     vErrorsRet.push_back(std::move(entry));
 }
 
-void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keystore, std::map<COutPoint, Coin>& coins)
+void ParsePrevouts(const UniValue& prevTxsUnival, FlatSigningProvider* keystore, std::map<COutPoint, Coin>& coins)
 {
     if (!prevTxsUnival.isNull()) {
         const UniValue& prevTxs = prevTxsUnival.get_array();
@@ -247,11 +247,11 @@ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keyst
                 // work from witnessScript when possible
                 std::vector<unsigned char> scriptData(!ws.isNull() ? ParseHexV(ws, "witnessScript") : ParseHexV(rs, "redeemScript"));
                 CScript script(scriptData.begin(), scriptData.end());
-                keystore->AddCScript(script);
+                keystore->scripts.emplace(CScriptID(script), script);
                 // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH).
                 // This is done for redeemScript only for compatibility, it is encouraged to use the explicit witnessScript field instead.
                 CScript witness_output_script{GetScriptForDestination(WitnessV0ScriptHash(script))};
-                keystore->AddCScript(witness_output_script);
+                keystore->scripts.emplace(CScriptID(witness_output_script), witness_output_script);
 
                 if (!ws.isNull() && !rs.isNull()) {
                     // if both witnessScript and redeemScript are provided,
diff --git a/src/rpc/rawtransaction_util.h b/src/rpc/rawtransaction_util.h
index 964d0b095b..40d6bbba87 100644
--- a/src/rpc/rawtransaction_util.h
+++ b/src/rpc/rawtransaction_util.h
@@ -12,7 +12,7 @@
 #include <optional>
 
 struct bilingual_str;
-class FillableSigningProvider;
+struct FlatSigningProvider;
 class UniValue;
 struct CMutableTransaction;
 class Coin;
@@ -38,7 +38,7 @@ void SignTransactionResultToJSON(CMutableTransaction& mtx, bool complete, const
   * @param  keystore      A pointer to the temporary keystore if there is one
   * @param  coins         Map of unspent outputs - coins in mempool and current chain UTXO set, may be extended by previous txns outputs after call
   */
-void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keystore, std::map<COutPoint, Coin>& coins);
+void ParsePrevouts(const UniValue& prevTxsUnival, FlatSigningProvider* keystore, std::map<COutPoint, Coin>& coins);
 
 /** Normalize univalue-represented inputs and add them to the transaction */
 void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, bool rbf);
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index fe829ae9a4..9600258de9 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -228,7 +228,7 @@ CPubKey AddrToPubKey(const FillableSigningProvider& keystore, const std::string&
 }
 
 // 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, FillableSigningProvider& keystore, CScript& script_out)
+CTxDestination AddAndGetMultisigDestination(const int required, const std::vector<CPubKey>& pubkeys, OutputType type, FlatSigningProvider& keystore, CScript& script_out)
 {
     // Gather public keys
     if (required < 1) {
diff --git a/src/rpc/util.h b/src/rpc/util.h
index 394a429a29..51ecaff13c 100644
--- a/src/rpc/util.h
+++ b/src/rpc/util.h
@@ -117,7 +117,7 @@ std::string HelpExampleRpcNamed(const std::string& methodname, const RPCArgList&
 
 CPubKey HexToPubKey(const std::string& hex_in);
 CPubKey AddrToPubKey(const FillableSigningProvider& keystore, const std::string& addr_in);
-CTxDestination AddAndGetMultisigDestination(const int required, const std::vector<CPubKey>& pubkeys, OutputType type, FillableSigningProvider& keystore, CScript& script_out);
+CTxDestination AddAndGetMultisigDestination(const int required, const std::vector<CPubKey>& pubkeys, OutputType type, FlatSigningProvider& keystore, CScript& script_out);
 
 UniValue DescribeAddress(const CTxDestination& dest);
 
diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index 17bb6320a1..0c2ad06eea 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -287,9 +287,30 @@ RPCHelpMan addmultisigaddress()
         output_type = parsed.value();
     }
 
-    // Construct using pay-to-script-hash:
+    // Construct multisig scripts
+    FlatSigningProvider provider;
     CScript inner;
-    CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, spk_man, inner);
+    CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, provider, inner);
+
+    // Import scripts into the wallet
+    for (const auto& [id, script] : provider.scripts) {
+        // Due to a bug in the legacy wallet, the p2sh maximum script size limit is also imposed on 'p2sh-segwit' and 'bech32' redeem scripts.
+        // Even when redeem scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are valid for segwit output types, we don't want to
+        // enable it because:
+        // 1) It introduces a compatibility-breaking change requiring downgrade protection; older wallets would be unable to interact with these "new" legacy wallets.
+        // 2) Considering the ongoing deprecation of the legacy spkm, this issue adds another good reason to transition towards descriptors.
+        if (script.size() > MAX_SCRIPT_ELEMENT_SIZE) throw JSONRPCError(RPC_WALLET_ERROR, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts");
+
+        if (!spk_man.AddCScript(script)) {
+            if (CScript inner_script; spk_man.GetCScript(CScriptID(script), inner_script)) {
+                CHECK_NONFATAL(inner_script == script); // Nothing to add, script already contained by the wallet
+                continue;
+            }
+            throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Error importing script into the wallet"));
+        }
+    }
+
+    // Store destination in the addressbook
     pwallet->SetAddressBook(dest, label, AddressPurpose::SEND);
 
     // Make the descriptor
diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py
index 65d7b4c422..fdac3623d3 100755
--- a/test/functional/rpc_createmultisig.py
+++ b/test/functional/rpc_createmultisig.py
@@ -9,10 +9,9 @@ import json
 import os
 
 from test_framework.address import address_to_scriptpubkey
-from test_framework.blocktools import COINBASE_MATURITY
-from test_framework.authproxy import JSONRPCException
 from test_framework.descriptors import descsum_create, drop_origins
 from test_framework.key import ECPubKey
+from test_framework.messages import COIN
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_raises_rpc_error,
@@ -32,88 +31,40 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
         self.setup_clean_chain = True
         self.num_nodes = 3
         self.supports_cli = False
+        self.enable_wallet_if_possible()
 
-    def get_keys(self):
+    def create_keys(self, num_keys):
         self.pub = []
         self.priv = []
-        node0, node1, node2 = self.nodes
-        for _ in range(self.nkeys):
+        for _ in range(num_keys):
             privkey, pubkey = generate_keypair(wif=True)
             self.pub.append(pubkey.hex())
             self.priv.append(privkey)
-        if self.is_bdb_compiled():
-            self.final = node2.getnewaddress()
-        else:
-            self.final = getnewdestination('bech32')[2]
+
+    def create_wallet(self, node, wallet_name):
+        node.createwallet(wallet_name=wallet_name, disable_private_keys=True)
+        return node.get_wallet_rpc(wallet_name)
 
     def run_test(self):
         node0, node1, node2 = self.nodes
         self.wallet = MiniWallet(test_node=node0)
 
-        if self.is_bdb_compiled():
-            self.import_deterministic_coinbase_privkeys()
+        if self.is_wallet_compiled():
             self.check_addmultisigaddress_errors()
 
         self.log.info('Generating blocks ...')
         self.generate(self.wallet, 149)
 
-        self.moved = 0
-        for self.nkeys in [3, 5]:
-            for self.nsigs in [2, 3]:
-                for self.output_type in ["bech32", "p2sh-segwit", "legacy"]:
-                    self.get_keys()
-                    self.do_multisig()
-        if self.is_bdb_compiled():
-            self.checkbalances()
-
-        # Test mixed compressed and uncompressed pubkeys
-        self.log.info('Mixed compressed and uncompressed multisigs are not allowed')
-        pk0, pk1, pk2 = [getnewdestination('bech32')[0].hex() for _ in range(3)]
-
-        # decompress pk2
-        pk_obj = ECPubKey()
-        pk_obj.set(bytes.fromhex(pk2))
-        pk_obj.compressed = False
-        pk2 = pk_obj.get_bytes().hex()
-
-        if self.is_bdb_compiled():
-            node0.createwallet(wallet_name='wmulti0', disable_private_keys=True)
-            wmulti0 = node0.get_wallet_rpc('wmulti0')
+        wallet_multi = self.create_wallet(node1, 'wmulti') if self._requires_wallet else None
+        self.create_keys(21)  # max number of allowed keys + 1
+        m_of_n = [(2, 3), (3, 3), (2, 5), (3, 5), (10, 15), (15, 15)]
+        for (sigs, keys) in m_of_n:
+            for output_type in ["bech32", "p2sh-segwit", "legacy"]:
+                self.do_multisig(keys, sigs, output_type, wallet_multi)
 
-        # 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']
-
-            if self.is_bdb_compiled():
-                result = wmulti0.addmultisigaddress(2, keys, '', 'legacy')
-                assert_equal(legacy_addr, result['address'])
-                assert 'warnings' not in result
-
-            # Generate addresses with the segwit types. These should all make legacy addresses
-            err_msg = ["Unable to make chosen address type, please ensure no uncompressed public keys are present."]
-
-            for addr_type in ['bech32', 'p2sh-segwit']:
-                result = self.nodes[0].createmultisig(nrequired=2, keys=keys, address_type=addr_type)
-                assert_equal(legacy_addr, result['address'])
-                assert_equal(result['warnings'], err_msg)
-
-                if self.is_bdb_compiled():
-                    result = wmulti0.addmultisigaddress(nrequired=2, keys=keys, address_type=addr_type)
-                    assert_equal(legacy_addr, result['address'])
-                    assert_equal(result['warnings'], err_msg)
-
-        self.log.info('Testing sortedmulti descriptors with BIP 67 test vectors')
-        with open(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data/rpc_bip67.json'), encoding='utf-8') as f:
-            vectors = json.load(f)
-
-        for t in vectors:
-            key_str = ','.join(t['keys'])
-            desc = descsum_create('sh(sortedmulti(2,{}))'.format(key_str))
-            assert_equal(self.nodes[0].deriveaddresses(desc)[0], t['address'])
-            sorted_key_str = ','.join(t['sorted_keys'])
-            sorted_key_desc = descsum_create('sh(multi(2,{}))'.format(sorted_key_str))
-            assert_equal(self.nodes[0].deriveaddresses(sorted_key_desc)[0], t['address'])
+        self.test_multisig_script_limit(wallet_multi)
+        self.test_mixing_uncompressed_and_compressed_keys(node0, wallet_multi)
+        self.test_sortedmulti_descriptors_bip67()
 
         # Check that bech32m is currently not allowed
         assert_raises_rpc_error(-5, "createmultisig cannot create bech32m multisig addresses", self.nodes[0].createmultisig, 2, self.pub, "bech32m")
@@ -133,117 +84,165 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
         pubs = [self.nodes[1].getaddressinfo(addr)["pubkey"] for addr in addresses]
         assert_raises_rpc_error(-5, "Bech32m multisig addresses cannot be created with legacy wallets", self.nodes[0].addmultisigaddress, 2, pubs, "", "bech32m")
 
-    def checkbalances(self):
-        node0, node1, node2 = self.nodes
-        self.generate(node0, COINBASE_MATURITY)
+    def test_multisig_script_limit(self, wallet_multi):
+        node1 = self.nodes[1]
+        pubkeys = self.pub[0:20]
 
-        bal0 = node0.getbalance()
-        bal1 = node1.getbalance()
-        bal2 = node2.getbalance()
-        balw = self.wallet.get_balance()
+        self.log.info('Test legacy redeem script max size limit')
+        assert_raises_rpc_error(-8, "redeemScript exceeds size limit: 684 > 520", node1.createmultisig, 16, pubkeys, 'legacy')
 
-        height = node0.getblockchaininfo()["blocks"]
-        assert 150 < height < 350
-        total = 149 * 50 + (height - 149 - 100) * 25
-        assert bal1 == 0
-        assert bal2 == self.moved
-        assert_equal(bal0 + bal1 + bal2 + balw, total)
+        self.log.info('Test valid 16-20 multisig p2sh-legacy and bech32 (no wallet)')
+        self.do_multisig(nkeys=20, nsigs=16, output_type="p2sh-segwit", wallet_multi=None)
+        self.do_multisig(nkeys=20, nsigs=16, output_type="bech32", wallet_multi=None)
 
-    def do_multisig(self):
-        node0, node1, node2 = self.nodes
+        self.log.info('Test invalid 16-21 multisig p2sh-legacy and bech32 (no wallet)')
+        assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, 'p2sh-segwit')
+        assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, 'bech32')
 
-        if self.is_bdb_compiled():
-            if 'wmulti' not in node1.listwallets():
-                try:
-                    node1.loadwallet('wmulti')
-                except JSONRPCException as e:
-                    path = self.nodes[1].wallets_path / "wmulti"
-                    if e.error['code'] == -18 and "Wallet file verification failed. Failed to load database path '{}'. Path does not exist.".format(path) in e.error['message']:
-                        node1.createwallet(wallet_name='wmulti', disable_private_keys=True)
-                    else:
-                        raise
-            wmulti = node1.get_wallet_rpc('wmulti')
+        # Check legacy wallet related command
+        self.log.info('Test legacy redeem script max size limit (with wallet)')
+        if wallet_multi is not None and not self.options.descriptors:
+            assert_raises_rpc_error(-8, "redeemScript exceeds size limit: 684 > 520", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'legacy')
+
+            self.log.info('Test legacy wallet unsupported operation. 16-20 multisig p2sh-legacy and bech32 generation')
+            # Due an internal limitation on legacy wallets, the redeem script limit also applies to p2sh-segwit and bech32 (even when the scripts are valid)
+            # We take this as a "good thing" to tell users to upgrade to descriptors.
+            assert_raises_rpc_error(-4, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'p2sh-segwit')
+            assert_raises_rpc_error(-4, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'bech32')
+
+    def do_multisig(self, nkeys, nsigs, output_type, wallet_multi):
+        node0, node1, node2 = self.nodes
+        pub_keys = self.pub[0: nkeys]
+        priv_keys = self.priv[0: nkeys]
 
         # Construct the expected descriptor
-        desc = 'multi({},{})'.format(self.nsigs, ','.join(self.pub))
-        if self.output_type == 'legacy':
+        desc = 'multi({},{})'.format(nsigs, ','.join(pub_keys))
+        if output_type == 'legacy':
             desc = 'sh({})'.format(desc)
-        elif self.output_type == 'p2sh-segwit':
+        elif output_type == 'p2sh-segwit':
             desc = 'sh(wsh({}))'.format(desc)
-        elif self.output_type == 'bech32':
+        elif output_type == 'bech32':
             desc = 'wsh({})'.format(desc)
         desc = descsum_create(desc)
 
-        msig = node2.createmultisig(self.nsigs, self.pub, self.output_type)
+        msig = node2.createmultisig(nsigs, pub_keys, output_type)
         assert 'warnings' not in msig
         madd = msig["address"]
         mredeem = msig["redeemScript"]
         assert_equal(desc, msig['descriptor'])
-        if self.output_type == 'bech32':
+        if output_type == 'bech32':
             assert madd[0:4] == "bcrt"  # actually a bech32 address
 
-        if self.is_bdb_compiled():
+        if wallet_multi is not None:
             # compare against addmultisigaddress
-            msigw = wmulti.addmultisigaddress(self.nsigs, self.pub, None, self.output_type)
+            msigw = wallet_multi.addmultisigaddress(nsigs, pub_keys, None, output_type)
             maddw = msigw["address"]
             mredeemw = msigw["redeemScript"]
             assert_equal(desc, drop_origins(msigw['descriptor']))
             # addmultisigiaddress and createmultisig work the same
             assert maddw == madd
             assert mredeemw == mredeem
-            wmulti.unloadwallet()
 
         spk = address_to_scriptpubkey(madd)
-        txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=spk, amount=1300)["txid"]
-        tx = node0.getrawtransaction(txid, True)
-        vout = [v["n"] for v in tx["vout"] if madd == v["scriptPubKey"]["address"]]
-        assert len(vout) == 1
-        vout = vout[0]
-        scriptPubKey = tx["vout"][vout]["scriptPubKey"]["hex"]
-        value = tx["vout"][vout]["value"]
-        prevtxs = [{"txid": txid, "vout": vout, "scriptPubKey": scriptPubKey, "redeemScript": mredeem, "amount": value}]
+        value = decimal.Decimal("0.00004000")
+        tx = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=spk, amount=int(value * COIN))
+        prevtxs = [{"txid": tx["txid"], "vout": tx["sent_vout"], "scriptPubKey": spk.hex(), "redeemScript": mredeem, "amount": value}]
 
         self.generate(node0, 1)
 
-        outval = value - decimal.Decimal("0.00001000")
-        rawtx = node2.createrawtransaction([{"txid": txid, "vout": vout}], [{self.final: outval}])
+        outval = value - decimal.Decimal("0.00002000")  # deduce fee (must be higher than the min relay fee)
+        # send coins to node2 when wallet is enabled
+        node2_balance = node2.getbalances()['mine']['trusted'] if self.is_wallet_compiled() else 0
+        out_addr = node2.getnewaddress() if self.is_wallet_compiled() else getnewdestination('bech32')[2]
+        rawtx = node2.createrawtransaction([{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{out_addr: outval}])
 
         prevtx_err = dict(prevtxs[0])
         del prevtx_err["redeemScript"]
 
-        assert_raises_rpc_error(-8, "Missing redeemScript/witnessScript", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
+        assert_raises_rpc_error(-8, "Missing redeemScript/witnessScript", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err])
 
         # if witnessScript specified, all ok
         prevtx_err["witnessScript"] = prevtxs[0]["redeemScript"]
-        node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
+        node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs-1], [prevtx_err])
 
         # both specified, also ok
         prevtx_err["redeemScript"] = prevtxs[0]["redeemScript"]
-        node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
+        node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs-1], [prevtx_err])
 
         # redeemScript mismatch to witnessScript
         prevtx_err["redeemScript"] = "6a" # OP_RETURN
-        assert_raises_rpc_error(-8, "redeemScript does not correspond to witnessScript", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
+        assert_raises_rpc_error(-8, "redeemScript does not correspond to witnessScript", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err])
 
         # redeemScript does not match scriptPubKey
         del prevtx_err["witnessScript"]
-        assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
+        assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err])
 
         # witnessScript does not match scriptPubKey
         prevtx_err["witnessScript"] = prevtx_err["redeemScript"]
         del prevtx_err["redeemScript"]
-        assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
+        assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err])
 
-        rawtx2 = node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs - 1], prevtxs)
-        rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [self.priv[-1]], prevtxs)
+        rawtx2 = node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs - 1], prevtxs)
+        rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [priv_keys[-1]], prevtxs)
+        assert rawtx3['complete']
 
-        self.moved += outval
         tx = node0.sendrawtransaction(rawtx3["hex"], 0)
         blk = self.generate(node0, 1)[0]
         assert tx in node0.getblock(blk)["tx"]
 
+        # When the wallet is enabled, assert node2 sees the incoming amount
+        if self.is_wallet_compiled():
+            assert_equal(node2.getbalances()['mine']['trusted'], node2_balance + outval)
+
         txinfo = node0.getrawtransaction(tx, True, blk)
-        self.log.info("n/m=%d/%d %s size=%d vsize=%d weight=%d" % (self.nsigs, self.nkeys, self.output_type, txinfo["size"], txinfo["vsize"], txinfo["weight"]))
+        self.log.info("n/m=%d/%d %s size=%d vsize=%d weight=%d" % (nsigs, nkeys, output_type, txinfo["size"], txinfo["vsize"], txinfo["weight"]))
+
+    def test_mixing_uncompressed_and_compressed_keys(self, node, wallet_multi):
+        self.log.info('Mixed compressed and uncompressed multisigs are not allowed')
+        pk0, pk1, pk2 = [getnewdestination('bech32')[0].hex() for _ in range(3)]
+
+        # decompress pk2
+        pk_obj = ECPubKey()
+        pk_obj.set(bytes.fromhex(pk2))
+        pk_obj.compressed = False
+        pk2 = pk_obj.get_bytes().hex()
+
+        # 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 = node.createmultisig(2, keys, 'legacy')['address']
+
+            if wallet_multi is not None:
+                # 'addmultisigaddress' should return the same address
+                result = wallet_multi.addmultisigaddress(2, keys, '', 'legacy')
+                assert_equal(legacy_addr, result['address'])
+                assert 'warnings' not in result
+
+            # Generate addresses with the segwit types. These should all make legacy addresses
+            err_msg = ["Unable to make chosen address type, please ensure no uncompressed public keys are present."]
+
+            for addr_type in ['bech32', 'p2sh-segwit']:
+                result = self.nodes[0].createmultisig(nrequired=2, keys=keys, address_type=addr_type)
+                assert_equal(legacy_addr, result['address'])
+                assert_equal(result['warnings'], err_msg)
+
+                if wallet_multi is not None:
+                    result = wallet_multi.addmultisigaddress(nrequired=2, keys=keys, address_type=addr_type)
+                    assert_equal(legacy_addr, result['address'])
+                    assert_equal(result['warnings'], err_msg)
+
+    def test_sortedmulti_descriptors_bip67(self):
+        self.log.info('Testing sortedmulti descriptors with BIP 67 test vectors')
+        with open(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data/rpc_bip67.json'), encoding='utf-8') as f:
+            vectors = json.load(f)
+
+        for t in vectors:
+            key_str = ','.join(t['keys'])
+            desc = descsum_create('sh(sortedmulti(2,{}))'.format(key_str))
+            assert_equal(self.nodes[0].deriveaddresses(desc)[0], t['address'])
+            sorted_key_str = ','.join(t['sorted_keys'])
+            sorted_key_desc = descsum_create('sh(multi(2,{}))'.format(sorted_key_str))
+            assert_equal(self.nodes[0].deriveaddresses(sorted_key_desc)[0], t['address'])
 
 
 if __name__ == '__main__':
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 620eeef270..4231cbf495 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -444,6 +444,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
                 n.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors, load_on_startup=True)
             n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase', rescan=True)
 
+    # Only enables wallet support when the module is available
+    def enable_wallet_if_possible(self):
+        self._requires_wallet = self.is_wallet_compiled()
+
     def run_test(self):
         """Tests must override this method to define test logic"""
         raise NotImplementedError