Merge 74fa29e12e into 433412fd84

Ensure legacy wallet migration skips the never standard bare multisig with +3 keys
and consensus-invalid multisig scripts. Treating them as valid causes migration to
crash because we are enforcing this rules within the descriptors parsing logic.

src/policy/policy.cpp

@@ -87,7 +87,7 @@ bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_
         unsigned char m = vSolutions.front()[0];
         unsigned char n = vSolutions.back()[0];
         // Support up to x-of-3 multisig txns as standard
-        if (n < 1 || n > 3)
+        if (n < 1 || n > MAX_BARE_MULTISIG_PUBKEYS_NUM)
             return false;
         if (m < 1 || m > n)
             return false;

src/policy/policy.h

@@ -37,6 +37,8 @@ static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000};
 static constexpr unsigned int DEFAULT_BYTES_PER_SIGOP{20};
 /** Default for -permitbaremultisig */
 static constexpr bool DEFAULT_PERMIT_BAREMULTISIG{true};
+/** The maximum number of pubkeys in a bare multisig output script */
+static constexpr unsigned int MAX_BARE_MULTISIG_PUBKEYS_NUM{3};
 /** The maximum number of witness stack items in a standard P2WSH script */
 static constexpr unsigned int MAX_STANDARD_P2WSH_STACK_ITEMS{100};
 /** The maximum size in bytes of each witness stack item in a standard P2WSH script */

src/script/descriptor.cpp

@@ -1848,8 +1848,8 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
             return {};
         }
         if (ctx == ParseScriptContext::TOP) {
-            if (providers.size() > 3) {
-                error = strprintf("Cannot have %u pubkeys in bare multisig; only at most 3 pubkeys", providers.size());
+            if (providers.size() > MAX_BARE_MULTISIG_PUBKEYS_NUM) {
+                error = strprintf("Cannot have %u pubkeys in bare multisig; only at most %d pubkeys", providers.size(), MAX_BARE_MULTISIG_PUBKEYS_NUM);
                 return {};
             }
         }

src/wallet/scriptpubkeyman.cpp

@@ -7,6 +7,7 @@
 #include <logging.h>
 #include <node/types.h>
 #include <outputtype.h>
+#include <policy/policy.h>
 #include <script/descriptor.h>
 #include <script/script.h>
 #include <script/sign.h>
@@ -185,11 +186,6 @@ IsMineResult IsMineInner(const LegacyDataSPKM& keystore, const CScript& scriptPu
 
     case TxoutType::MULTISIG:
     {
-        // Never treat bare multisig outputs as ours (they can still be made watchonly-though)
-        if (sigversion == IsMineSigVersion::TOP) {
-            break;
-        }
-
         // Only consider transactions "mine" if we own ALL the
         // keys involved. Multi-signature transactions that are
         // partially owned (somebody else has a key that can spend
@@ -203,6 +199,16 @@ IsMineResult IsMineInner(const LegacyDataSPKM& keystore, const CScript& scriptPu
                 }
             }
         }
+        // Follow consensus rules, never treat too large legacy multisig scripts as valid
+        if (sigversion == IsMineSigVersion::P2SH && scriptPubKey.size() > MAX_SCRIPT_ELEMENT_SIZE) {
+            return IsMineResult::INVALID;
+        }
+
+        // Never treat bare multisig outputs as ours (they can still be made watchonly-though)
+        if (sigversion == IsMineSigVersion::TOP) {
+            if (keys.size() > MAX_BARE_MULTISIG_PUBKEYS_NUM) return IsMineResult::INVALID; // These are standard wise non-spendable
+            break;
+        }
         if (HaveKeys(keys, keystore)) {
             ret = std::max(ret, IsMineResult::SPENDABLE);
         }

test/functional/wallet_migration.py

@@ -314,6 +314,40 @@ class WalletMigrationTest(BitcoinTestFramework):
         assert_equal(ms1_solvable.getbalance(), 0)
         assert_equal(ms1_solvable.listtransactions(), [])
 
+    def test_multisig_invalid(self):
+        self.log.info("Test migration of a legacy-wise non-standard bare multisig")
+        wallet = self.create_legacy_wallet("multi_nonstandard")
+
+        # Create enough keys for all coming tests
+        addys = [wallet.getnewaddress()] * 20
+        pubkeys = []
+        privkeys = []
+        for addr in addys:
+            pubkeys.append(wallet.getaddressinfo(addr)['pubkey'])
+            privkeys.append(wallet.dumpprivkey(addr))
+
+        # Create a non-standard multi(4, keys)
+        res = wallet.createmultisig(4, pubkeys[:4])
+        # Import script as a bare multisig. This is standard-wise non-spendable, and it is not allowed descriptors' wise
+        wallet.importaddress(address=res['redeemScript'])
+
+        # Now migrate it and verify we don't crash due to a non-allowed descriptor migration
+        wallet.migratewallet()
+        wallet.unloadwallet()
+
+        ##############################################################
+        # Import a consensus-wise invalid p2sh multisig with 20 keys #
+        ##############################################################
+        self.log.info("Test importing an invalid p2sh multisig")
+        wallet = self.create_legacy_wallet("large_multi")
+        res = wallet.createmultisig(20, pubkeys, "bech32")
+        script_sh_pkh = script_to_p2sh_script(res['redeemScript'])
+        wallet.importaddress(address=res['redeemScript'])
+        wallet.importaddress(address=script_sh_pkh.hex())
+
+        # Now migrate it and verify we don't crash due to a non-allowed descriptor migration
+        wallet.migratewallet()
+        wallet.unloadwallet()
 
     def test_other_watchonly(self):
         default = self.master_node.get_wallet_rpc(self.default_wallet_name)
@@ -1069,6 +1103,7 @@ class WalletMigrationTest(BitcoinTestFramework):
         # TODO: Test the actual records in the wallet for these tests too. The behavior may be correct, but the data written may not be what we actually want
         self.test_basic()
         self.test_multisig()
+        self.test_multisig_invalid()
         self.test_other_watchonly()
         self.test_no_privkeys()
         self.test_pk_coinbases()