wallet: warn against accidental unsafe older() import
What changed, and why it matters
This change adds a warning when users import a Bitcoin wallet descriptor containing an 'older()' time or block lock that looks safe but actually has no consensus effect. Because of how Bitcoin encodes relative locktimes, values above 65535 silently wrap around: older(65536) behaves like older(1). Someone could accidentally create a wallet that unlocks far sooner than they think. The patch does not block the import; it only warns.
Treat this as a useful hardening change, not a critical vulnerability fix. Users who previously imported miniscript descriptors with older() should review them for values >65535 (height) or >(65535*512) seconds (time). Wallet software relying on Bitcoin Core descriptors should surface these warnings to users and consider rejecting unsafe imports explicitly if policy requires it.
Security signals we found
New warning path for unsafe miniscript older() values
Consensus encoding ambiguity in BIP 68 nSequence relative locktimes
Advisory-only mitigation; import is not rejected
Tests added covering safe boundary, unsafe height, unsafe time, and after() non-warning
Evidence from the diff
BIP 379 miniscript’s older() fragment encodes BIP 68 / BIP 112 relative locktimes using a 16-bit value field. The nSequence encoding has a type flag (1<<22) for time-based locks and a 16-bit mask (0xFFFF). Values whose masked part exceed 65535 are not representable in consensus, so they are reduced modulo 65536. The commit introduces a ForEachNode helper to traverse miniscript nodes and, during MiniscriptDescriptor construction, checks each older() node. If value_part > SEQUENCE_LOCKTIME_MASK, it records a warning string. Warnings are aggregated across subdescriptors and surfaced in importdescriptors RPC results. It is purely advisory; import still succeeds.
Changed components
src/script/descriptor.cppsrc/script/descriptor.hsrc/script/miniscript.hsrc/wallet/rpc/backup.cppwallet descriptor import RPCInspect captured patch +138 / −1
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 6363d176..c530c888 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -791,6 +791,8 @@ protected:
const std::vector<std::unique_ptr<PubkeyProvider>> m_pubkey_args;
//! The string name of the descriptor function.
const std::string m_name;
+ //! Warnings (not including subdescriptors).
+ std::vector<std::string> 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<DescriptorImpl> Clone() const = 0;
+
+ // NOLINTNEXTLINE(misc-no-recursion)
+ std::vector<std::string> Warnings() const override {
+ std::vector<std::string> 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<std::unique_ptr<PubkeyProvider>> providers, miniscript::NodeRef<uint32_t> 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<uint32_t>& 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 a6744b92..472e908e 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<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const = 0;
+
+ /** Semantic/safety warnings (includes subdescriptors). */
+ virtual std::vector<std::string> 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 54ae777c..c71203ed 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -7,8 +7,10 @@
#include <algorithm>
#include <compare>
+#include <concepts>
#include <cstdint>
#include <cstdlib>
+#include <functional>
#include <iterator>
#include <memory>
#include <optional>
@@ -195,6 +197,21 @@ template<typename Key> using NodeRef = std::unique_ptr<const Node<Key>>;
template<typename Key, typename... Args>
NodeRef<Key> MakeNodeRef(Args&&... args) { return std::make_unique<const Node<Key>>(std::forward<Args>(args)...); }
+//! Unordered traversal of a miniscript node tree.
+template <typename Key, std::invocable<const Node<Key>&> Fn>
+void ForEachNode(const Node<Key>& root, Fn&& fn)
+{
+ std::vector<std::reference_wrapper<const Node<Key>>> stack{root};
+ while (!stack.empty()) {
+ const Node<Key>& 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 97f416dd..0bc34146 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 5cb40304..2697eef6 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 0c69849d..b3ab35a2 100644
--- a/src/wallet/test/walletload_tests.cpp
+++ b/src/wallet/test/walletload_tests.cpp
@@ -35,6 +35,7 @@ public:
std::optional<int64_t> MaxSatisfactionWeight(bool) const override { return {}; }
std::optional<int64_t> MaxSatisfactionElems() const override { return {}; }
void GetPubKeys(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const override {}
+ std::vector<std::string> 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 f14c99c6..a18668b6 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()
Why this scored 38/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.