From 53302a09817e5b799d345dfea432546a55a9d727 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Aug 2023 18:16:41 -0300 Subject: bugfix: addmultisigaddress, add unsupported operation for redeem scripts over 520 bytes Due to a bug in the legacy wallet, the p2sh maximum script size limit is also imposed on 'p2sh-segwit' and 'bech32' redeem scripts. Although redeem scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are technically valid for segwit output types, we don't want to enable this feature in legacy wallets for the following reasons: 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. --- src/wallet/rpc/addresses.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index bcc39b05b8..62188249da 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -296,7 +296,20 @@ RPCHelpMan addmultisigaddress() // Import scripts into the wallet for (const auto& [id, script] : provider.scripts) { - spk_man.AddCScript(script); + // 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 -- cgit v1.2.3