From e22aa8b22d640b425287493e8c88ed494f044ca8 Mon Sep 17 00:00:00 2001
From: furszy <matiasfurszyfer@protonmail.com>
Date: Mon, 2 Dec 2024 11:12:50 -0500
Subject: [PATCH 1/3] refactor: replace hardcoded number, introduce
 'MAX_BARE_MULTISIG_PUBKEYS_NUM'

---
 src/policy/policy.cpp     | 2 +-
 src/policy/policy.h       | 2 ++
 src/script/descriptor.cpp | 4 ++--
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
index ed336928235..ebd999ef1b8 100644
--- a/src/policy/policy.cpp
+++ b/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;
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 4412f2db87a..d73e39b9d74 100644
--- a/src/policy/policy.h
+++ b/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 */
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 5026470edcf..9a1c442cf07 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1852,8 +1852,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 {};
             }
         }

From cdaa3a58dc16d6a27248dc4cdec2dd1909eee7fe Mon Sep 17 00:00:00 2001
From: furszy <matiasfurszyfer@protonmail.com>
Date: Tue, 26 Nov 2024 18:03:17 -0500
Subject: [PATCH 2/3] wallet: bugfix, stop treating multisig
 consensus-invalid/unspendable scripts as ours

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/wallet/scriptpubkeyman.cpp | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 62384056dc6..9a6d2b809bc 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/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);
         }

From 74fa29e12e379d7be8ad2dd261ee68efaf7a9e07 Mon Sep 17 00:00:00 2001
From: furszy <matiasfurszyfer@protonmail.com>
Date: Tue, 26 Nov 2024 18:05:26 -0500
Subject: [PATCH 3/3] test: migration, add coverage for
 consensus-invalid/unspendable multisig scripts

---
 test/functional/wallet_migration.py | 35 +++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 48ad8e4f2a1..e7e57a3e84b 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -299,6 +299,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.nodes[0].get_wallet_rpc(self.default_wallet_name)
@@ -1019,6 +1053,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()