From 76c092ff805833a9adf84f669f0455bc2e0bba8b Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 2 Dec 2025 12:24:22 +0100 Subject: [PATCH] wallet: warn against accidental unsafe older() import BIP 379 allows height and time locks that have no consensus meaning in BIP 68 / BIP 112. This is used by some protocols like Lightning to encode extra data, but is unsafe when used unintentionally. E.g. older(65536) is equivalent to older(1). This commit emits a warning when importing such a descriptor. It introduces a helper ForEachNode to traverse all miniscript nodes. --- src/script/descriptor.cpp | 31 +++++++++++++- src/script/descriptor.h | 3 ++ src/script/miniscript.h | 17 ++++++++ src/test/descriptor_tests.cpp | 46 +++++++++++++++++++++ src/wallet/rpc/backup.cpp | 4 ++ src/wallet/test/walletload_tests.cpp | 1 + test/functional/wallet_importdescriptors.py | 37 +++++++++++++++++ 7 files changed, 138 insertions(+), 1 deletion(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 6363d176433..c530c888c43 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -791,6 +791,8 @@ protected: const std::vector> m_pubkey_args; //! The string name of the descriptor function. const std::string m_name; + //! Warnings (not including subdescriptors). + std::vector m_warnings; //! The sub-descriptor arguments (empty for everything but SH and WSH). //! In doc/descriptors.m this is referred to as SCRIPT expressions sh(SCRIPT) @@ -990,6 +992,16 @@ public: } virtual std::unique_ptr Clone() const = 0; + + // NOLINTNEXTLINE(misc-no-recursion) + std::vector Warnings() const override { + std::vector all = m_warnings; + for (const auto& sub : m_subdescriptor_args) { + auto sub_w = sub->Warnings(); + all.insert(all.end(), sub_w.begin(), sub_w.end()); + } + return all; + } }; /** A parsed addr(A) descriptor. */ @@ -1537,7 +1549,24 @@ protected: public: MiniscriptDescriptor(std::vector> providers, miniscript::NodeRef node) - : DescriptorImpl(std::move(providers), "?"), m_node(std::move(node)) {} + : DescriptorImpl(std::move(providers), "?"), m_node(std::move(node)) + { + // Traverse miniscript tree for unsafe use of older() + miniscript::ForEachNode(*m_node, [&](const miniscript::Node& node) { + if (node.fragment == miniscript::Fragment::OLDER) { + const uint32_t raw = node.k; + const uint32_t value_part = raw & ~CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG; + if (value_part > CTxIn::SEQUENCE_LOCKTIME_MASK) { + const bool is_time_based = (raw & CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG) != 0; + if (is_time_based) { + m_warnings.push_back(strprintf("time-based relative locktime: older(%u) > (65535 * 512) seconds is unsafe", raw)); + } else { + m_warnings.push_back(strprintf("height-based relative locktime: older(%u) > 65535 blocks is unsafe", raw)); + } + } + } + }); + } bool ToStringHelper(const SigningProvider* arg, std::string& out, const StringType type, const DescriptorCache* cache = nullptr) const override diff --git a/src/script/descriptor.h b/src/script/descriptor.h index a6744b92b71..472e908ee23 100644 --- a/src/script/descriptor.h +++ b/src/script/descriptor.h @@ -165,6 +165,9 @@ struct Descriptor { * @param[out] ext_pubs Any extended public keys */ virtual void GetPubKeys(std::set& pubkeys, std::set& ext_pubs) const = 0; + + /** Semantic/safety warnings (includes subdescriptors). */ + virtual std::vector Warnings() const = 0; }; /** Parse a `descriptor` string. Included private keys are put in `out`. diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 54ae777cf92..c71203ed260 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -7,8 +7,10 @@ #include #include +#include #include #include +#include #include #include #include @@ -195,6 +197,21 @@ template using NodeRef = std::unique_ptr>; template NodeRef MakeNodeRef(Args&&... args) { return std::make_unique>(std::forward(args)...); } +//! Unordered traversal of a miniscript node tree. +template &> Fn> +void ForEachNode(const Node& root, Fn&& fn) +{ + std::vector>> stack{root}; + while (!stack.empty()) { + const Node& node = stack.back(); + std::invoke(fn, node); + stack.pop_back(); + for (const auto& sub : node.subs) { + stack.emplace_back(*sub); + } + } +} + //! The different node types in miniscript. enum class Fragment { JUST_0, //!< OP_0 diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp index 97f416dd7aa..0bc34146cf7 100644 --- a/src/test/descriptor_tests.cpp +++ b/src/test/descriptor_tests.cpp @@ -1271,4 +1271,50 @@ BOOST_AUTO_TEST_CASE(descriptor_literal_null_byte) BOOST_REQUIRE_MESSAGE(!descs.empty(), err); } +BOOST_AUTO_TEST_CASE(descriptor_older_warnings) +{ + // A safe boundary value should yield no warnings. + { + FlatSigningProvider keys; + std::string err; + auto descs = Parse("wsh(and_v(v:pk(0379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3),older(65535)))", keys, err, /*require_checksum=*/false); + BOOST_REQUIRE_MESSAGE(!descs.empty(), err); + BOOST_CHECK(descs[0]->Warnings().empty()); + } + + // Height-based unsafe value (65536) should produce one warning. + { + FlatSigningProvider keys; + std::string err; + const uint32_t height_unsafe = 65536; + auto descs = Parse(strprintf("wsh(and_v(v:pk(0379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3),older(%u)))", height_unsafe), keys, err, /*require_checksum=*/false); + BOOST_REQUIRE_MESSAGE(!descs.empty(), err); + const auto& ws = descs[0]->Warnings(); + BOOST_REQUIRE_EQUAL(ws.size(), 1U); + BOOST_CHECK_EQUAL(ws[0], strprintf("height-based relative locktime: older(%u) > 65535 blocks is unsafe", height_unsafe)); + } + + // Time-based unsafe value: add SEQUENCE_LOCKTIME_TYPE_FLAG (1<<22) + { + FlatSigningProvider keys; + std::string err; + const uint32_t time_unsafe = 65536 | CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG; + auto descs = Parse(strprintf("wsh(and_v(v:pk(0379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3),older(%u)))", time_unsafe), keys, err, /*require_checksum=*/false); + BOOST_REQUIRE_MESSAGE(!descs.empty(), err); + const auto& warnings = descs[0]->Warnings(); + BOOST_REQUIRE_EQUAL(warnings.size(), 1U); + BOOST_CHECK_EQUAL(warnings[0], strprintf("time-based relative locktime: older(%u) > (65535 * 512) seconds is unsafe", time_unsafe)); + } + + // Ensure no false positive warnings for absolute timelocks + { + FlatSigningProvider keys; + std::string err; + // Using after() with a large timestamp (> 65535) + auto descs = Parse("wsh(and_v(v:pk(0379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3),after(1000000)))", keys, err, /*require_checksum=*/false); + BOOST_REQUIRE_MESSAGE(!descs.empty(), err); + BOOST_CHECK(descs[0]->Warnings().empty()); + } +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 5cb40304a71..2697eef6e21 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -240,6 +240,10 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c } parsed_desc->ExpandPrivate(0, keys, expand_keys); + for (const auto& w : parsed_desc->Warnings()) { + warnings.push_back(w); + } + // Check if all private keys are provided bool have_all_privkeys = !expand_keys.keys.empty(); for (const auto& entry : expand_keys.origins) { diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp index 0c69849d0b6..b3ab35a2ecb 100644 --- a/src/wallet/test/walletload_tests.cpp +++ b/src/wallet/test/walletload_tests.cpp @@ -35,6 +35,7 @@ public: std::optional MaxSatisfactionWeight(bool) const override { return {}; } std::optional MaxSatisfactionElems() const override { return {}; } void GetPubKeys(std::set& pubkeys, std::set& ext_pubs) const override {} + std::vector Warnings() const override { return {}; } }; BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup) diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py index f14c99c62dc..a18668b6bec 100755 --- a/test/functional/wallet_importdescriptors.py +++ b/test/functional/wallet_importdescriptors.py @@ -22,6 +22,7 @@ from test_framework.authproxy import JSONRPCException from test_framework.blocktools import COINBASE_MATURITY from test_framework.test_framework import BitcoinTestFramework from test_framework.descriptors import descsum_create +from test_framework.script import SEQUENCE_LOCKTIME_TYPE_FLAG from test_framework.util import ( assert_equal, assert_raises_rpc_error, @@ -58,6 +59,7 @@ class ImportDescriptorsTest(BitcoinTestFramework): if 'warnings' in result[0]: observed_warnings = result[0]['warnings'] assert_equal("\n".join(sorted(warnings)), "\n".join(sorted(observed_warnings))) + self.log.debug(result) assert_equal(result[0]['success'], success) if error_code is not None: assert_equal(result[0]['error']['code'], error_code) @@ -783,5 +785,40 @@ class ImportDescriptorsTest(BitcoinTestFramework): assert_equal(w_multipath.getrawchangeaddress(address_type="bech32"), w_multisplit.getrawchangeaddress(address_type="bech32")) assert_equal(sorted(w_multipath.listdescriptors()["descriptors"], key=lambda x: x["desc"]), sorted(w_multisplit.listdescriptors()["descriptors"], key=lambda x: x["desc"])) + self.log.info("Test older() safety") + + for flag in [0, SEQUENCE_LOCKTIME_TYPE_FLAG]: + self.log.debug("Importing a safe value always works") + safe_value = (65535 | flag) + self.test_importdesc( + { + 'desc': descsum_create(f"wsh(and_v(v:pk([12345678/0h/0h]{xpub}/*),older({safe_value})))"), + 'active': True, + 'range': [0, 2], + 'timestamp': 'now' + }, + success=True + ) + + self.log.debug("Importing an unsafe value results in a warning") + unsafe_value = safe_value + 1 + desc = descsum_create(f"wsh(and_v(v:pk([12345678/0h/0h]{xpub}/*),older({unsafe_value})))") + expected_warning = ( + f"time-based relative locktime: older({unsafe_value}) > (65535 * 512) seconds is unsafe" + if flag == SEQUENCE_LOCKTIME_TYPE_FLAG + else f"height-based relative locktime: older({unsafe_value}) > 65535 blocks is unsafe" + ) + self.test_importdesc( + { + 'desc': desc, + 'active': True, + 'range': [0, 2], + 'timestamp': 'now' + }, + success=True, + warnings=[expected_warning], + ) + + if __name__ == '__main__': ImportDescriptorsTest(__file__).main()