From e1d6179b61a2d4766c5eb6183a839fcaa6e566fa Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 1 Dec 2024 12:59:24 -0500 Subject: [PATCH] descriptors: inference process, do not return unparsable descriptors The internal script-to-descriptor inference procedure shouldn't return invalid descriptors or ones that fail the string-to-descriptor parsing process. These two procedures should always support seamless roundtrips. This inlines `InferScript`/`InferDescriptor` to the actual descriptors `ParseScript` logic for the multisig path, ensuring only valid descriptors are produced. Examples where this presents an issue: 1) Because the legacy wallet allowed the import of invalid scripts, the process could end up inferring and storing an unparsable descriptor, which crashes the software during any subsequent wallet load 2) The `decodescript()` RPC command currently return unusable multisig descriptors. For example, providing a P2SH multisig redeem script with more than 520 bytes results in a descriptor when it shouldn't due to its large size. --- src/core_write.cpp | 4 +++- src/rpc/blockchain.cpp | 7 ++++--- src/rpc/output_script.cpp | 1 + src/rpc/rawtransaction.cpp | 15 ++++++++++----- src/script/descriptor.cpp | 13 ++++++++++++- src/wallet/rpc/addresses.cpp | 10 +++++++--- test/functional/rpc_decodescript.py | 10 ++++++++++ 7 files changed, 47 insertions(+), 13 deletions(-) diff --git a/src/core_write.cpp b/src/core_write.cpp index 253dfde1006..c9fded11136 100644 --- a/src/core_write.cpp +++ b/src/core_write.cpp @@ -153,7 +153,9 @@ void ScriptToUniv(const CScript& script, UniValue& out, bool include_hex, bool i out.pushKV("asm", ScriptToAsmStr(script)); if (include_address) { - out.pushKV("desc", InferDescriptor(script, provider ? *provider : DUMMY_SIGNING_PROVIDER)->ToString()); + auto desc = InferDescriptor(script, provider ? *provider : DUMMY_SIGNING_PROVIDER); + // future: make 'InferDescriptor' return the error cause + if (desc) out.pushKV("desc", desc->ToString()); } if (include_hex) { out.pushKV("hex", HexStr(script)); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 6a57a3c4878..5fcb9ff0d2f 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1105,7 +1105,7 @@ static RPCHelpMan gettxout() {RPCResult::Type::STR_AMOUNT, "value", "The transaction value in " + CURRENCY_UNIT}, {RPCResult::Type::OBJ, "scriptPubKey", "", { {RPCResult::Type::STR, "asm", "Disassembly of the output script"}, - {RPCResult::Type::STR, "desc", "Inferred descriptor for the output"}, + {RPCResult::Type::STR, "desc", /*optional=*/true, "Inferred top level descriptor for the script (if any)"}, {RPCResult::Type::STR_HEX, "hex", "The raw output script bytes, hex-encoded"}, {RPCResult::Type::STR, "type", "The type, eg pubkeyhash"}, {RPCResult::Type::STR, "address", /*optional=*/true, "The Bitcoin address (only if a well-defined address exists)"}, @@ -2287,9 +2287,10 @@ static RPCHelpMan scantxoutset() FlatSigningProvider provider; auto scripts = EvalDescriptorStringOrObject(scanobject, provider); for (CScript& script : scripts) { - std::string inferred = InferDescriptor(script, provider)->ToString(); + auto desc = InferDescriptor(script, provider); + if (!desc) throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid descriptor found: '%s'", scanobject.get_str())); needles.emplace(script); - descriptors.emplace(std::move(script), std::move(inferred)); + descriptors.emplace(std::move(script), desc->ToString()); } } diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp index 5de2847be99..b32bccaf6ce 100644 --- a/src/rpc/output_script.cpp +++ b/src/rpc/output_script.cpp @@ -146,6 +146,7 @@ static RPCHelpMan createmultisig() // Make the descriptor std::unique_ptr descriptor = InferDescriptor(GetScriptForDestination(dest), keystore); + if (!descriptor) throw JSONRPCError(RPC_INTERNAL_ERROR, "Invalid descriptor generated"); UniValue result(UniValue::VOBJ); result.pushKV("address", EncodeDestination(dest)); diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 184fff9386d..271b52855af 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -493,7 +493,7 @@ static RPCHelpMan decodescript() RPCResult::Type::OBJ, "", "", { {RPCResult::Type::STR, "asm", "Disassembly of the script"}, - {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"}, + {RPCResult::Type::STR, "desc", /*optional=*/true, "Inferred top level descriptor for the script (if any)"}, {RPCResult::Type::STR, "type", "The output type (e.g. " + GetAllOutputTypes() + ")"}, {RPCResult::Type::STR, "address", /*optional=*/true, "The Bitcoin address (only if a well-defined address exists)"}, {RPCResult::Type::STR, "p2sh", /*optional=*/true, @@ -505,7 +505,7 @@ static RPCHelpMan decodescript() {RPCResult::Type::STR_HEX, "hex", "The raw output script bytes, hex-encoded"}, {RPCResult::Type::STR, "type", "The type of the output script (e.g. witness_v0_keyhash or witness_v0_scripthash)"}, {RPCResult::Type::STR, "address", /*optional=*/true, "The Bitcoin address (only if a well-defined address exists)"}, - {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"}, + {RPCResult::Type::STR, "desc", /*optional=*/true, "Inferred descriptor for the script (if any)"}, {RPCResult::Type::STR, "p2sh-segwit", "address of the P2SH script wrapping this witness redeem script"}, }}, }, @@ -529,9 +529,14 @@ static RPCHelpMan decodescript() std::vector> solutions_data; const TxoutType which_type{Solver(script, solutions_data)}; + bool can_wrap_p2sh = true; const bool can_wrap{[&] { switch (which_type) { - case TxoutType::MULTISIG: + case TxoutType::MULTISIG: { + // Verify it doesn't exceed p2sh consensus limits + can_wrap_p2sh = script.size() <= MAX_SCRIPT_ELEMENT_SIZE; + break; + } case TxoutType::NONSTANDARD: case TxoutType::PUBKEY: case TxoutType::PUBKEYHASH: @@ -561,7 +566,7 @@ static RPCHelpMan decodescript() }()}; if (can_wrap) { - r.pushKV("p2sh", EncodeDestination(ScriptHash(script))); + if (can_wrap_p2sh) r.pushKV("p2sh", EncodeDestination(ScriptHash(script))); // P2SH and witness programs cannot be wrapped in P2WSH, if this script // is a witness program, don't return addresses for a segwit programs. const bool can_wrap_P2WSH{[&] { @@ -822,7 +827,7 @@ const RPCResult decodepsbt_inputs{ {RPCResult::Type::OBJ, "scriptPubKey", "", { {RPCResult::Type::STR, "asm", "Disassembly of the output script"}, - {RPCResult::Type::STR, "desc", "Inferred descriptor for the output"}, + {RPCResult::Type::STR, "desc", /*optional=*/true, "Inferred top level descriptor for the script (if any)"}, {RPCResult::Type::STR_HEX, "hex", "The raw output script bytes, hex-encoded"}, {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"}, {RPCResult::Type::STR, "address", /*optional=*/true, "The Bitcoin address (only if a well-defined address exists)"}, diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 9a1c442cf07..a2eb764706d 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -1853,7 +1853,7 @@ std::vector> ParseScript(uint32_t& key_exp_index } if (ctx == ParseScriptContext::TOP) { 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); + error = strprintf("Cannot have %u pubkeys in bare multisig; only at most %u pubkeys", providers.size(), MAX_BARE_MULTISIG_PUBKEYS_NUM); return {}; } } @@ -2239,6 +2239,17 @@ std::unique_ptr InferScript(const CScript& script, ParseScriptCo break; } } + + // Check bare multisig pubkeys number limit + if (ctx == ParseScriptContext::TOP && providers.size() > MAX_BARE_MULTISIG_PUBKEYS_NUM) { + return nullptr; + } + + // Verify we don't exceed consensus limits + if (ctx == ParseScriptContext::P2SH && script.size() > MAX_SCRIPT_ELEMENT_SIZE) { + return nullptr; + } + if (ok) return std::make_unique((int)data[0][0], std::move(providers)); } if (txntype == TxoutType::SCRIPTHASH && ctx == ParseScriptContext::TOP) { diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 1c2951deeec..8b9d6da7b24 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -292,6 +292,13 @@ RPCHelpMan addmultisigaddress() CScript inner; CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, provider, inner); + // Before importing the scripts into the wallet, check we can make a descriptor out of it. + std::unique_ptr descriptor = InferDescriptor(GetScriptForDestination(dest), spk_man); + if (!descriptor) { + // Shouldn't happen; we crafted the destination internally. + throw JSONRPCError(RPC_INTERNAL_ERROR, "Invalid descriptor generated"); + } + // 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. @@ -313,9 +320,6 @@ RPCHelpMan addmultisigaddress() // Store destination in the addressbook pwallet->SetAddressBook(dest, label, AddressPurpose::SEND); - // Make the descriptor - std::unique_ptr descriptor = InferDescriptor(GetScriptForDestination(dest), spk_man); - UniValue result(UniValue::VOBJ); result.pushKV("address", EncodeDestination(dest)); result.pushKV("redeemScript", HexStr(inner)); diff --git a/test/functional/rpc_decodescript.py b/test/functional/rpc_decodescript.py index 4f127fa8b7f..507b4628a62 100755 --- a/test/functional/rpc_decodescript.py +++ b/test/functional/rpc_decodescript.py @@ -96,6 +96,16 @@ class DecodeScriptTest(BitcoinTestFramework): assert_equal('witness_v0_scripthash', rpc_result['segwit']['type']) assert_equal('0 ' + multisig_script_hash, rpc_result['segwit']['asm']) + # Check decoding a multisig redeem script doesn't return an invalid descriptor + self.log.info("- multisig invalid") + # Decode a +520 bytes consensus-invalid p2sh multisig - provided script translates to multi(20, keys) + large_multisig_script = '0114' + push_public_key * 20 + '0114' + 'ae' + rpc_result = self.nodes[0].decodescript(large_multisig_script) + # Verify it is valid only under segwit rules, not for bare multisig nor p2sh. + assert 'desc' not in rpc_result + assert 'p2sh' not in rpc_result + assert 'segwit' in rpc_result + self.log.info ("- P2SH") # OP_HASH160 OP_EQUAL. # push_public_key_hash here should actually be the hash of a redeem script.