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

2451a21 test: addmultisigaddress, coverage for script size limits (furszy)
53302a0 bugfix: addmultisigaddress, add unsupported operation for redeem scripts over 520 bytes (furszy)
9be6065 test: coverage for 16-20 segwit multisig scripts (furszy)
9d9a91c rpc: bugfix, incorrect segwit redeem script size used in signrawtransactionwithkey (furszy)
0c9fedf fix incorrect multisig redeem script size limit for segwit (furszy)
f7a173b test: rpc_createmultisig, decouple 'test_sortedmulti_descriptors_bip67' (furszy)
4f33dbd test: rpc_createmultisig, decouple 'test_mixing_uncompressed_and_compressed_keys' (furszy)
25a8170 test: rpc_createmultisig, remove unnecessary checkbalances() (furszy)
b5a3289 test: refactor, multiple cleanups in rpc_createmultisig.py (furszy)
3635d43 test: rpc_createmultisig, remove manual wallet initialization (furszy)

Pull request description:

  Fixing bitcoin#28250 (comment) 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 2451a21
  theStack:
    Code-review ACK 2451a21
  achow101:
    ACK 2451a21

Tree-SHA512: 71794533cbd46b3a1079fb4e9d190d3ea3b615de0cbfa443466e14f05e4616ca90e12ce2bf07113515ea8113e64a560ad572bb9ea9d4835b6fb67b6ae596167f

Author: achow101

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 {

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

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

src/rpc/rawtransaction.cpp

@@ -785,15 +785,19 @@ 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];
         CKey key = DecodeSecret(k.get_str());
         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):

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,

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

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

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

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