From 0c9fedfc45fa7cbd6801ca5fd756863ec9a6911c Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Aug 2023 17:45:34 -0300 Subject: fix incorrect multisig redeem script size limit for segwit The multisig script generation process currently fails when the user exceeds 15 keys, even when it shouldn't. The maximum number of keys allowed for segwit redeem scripts (p2sh-segwit and bech32) is 20 keys. This is because the redeem script placed in the witness is not restricted by the item size limit. The reason behind this issue is the utilization of the legacy p2sh redeem script restrictions on segwit ones. Redeem scripts longer than 520 bytes are blocked from being inserted into the keystore, which causes the signing process and the descriptor inference process to fail. This occurs because the multisig generation flow uses the same keystore as the legacy spkm (FillableSigningProvider), which contains the 520-byte limit. --- src/wallet/rpc/addresses.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'src/wallet') diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index bed9ec029a..bcc39b05b8 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -289,9 +289,17 @@ 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) { + spk_man.AddCScript(script); + } + + // Store destination in the addressbook pwallet->SetAddressBook(dest, label, AddressPurpose::SEND); // Make the descriptor -- cgit v1.2.3 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(-) (limited to 'src/wallet') 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