wallet: warn against accidental unsafe older() import

Sjors commented at 12:00 pm on August 5, 2025: member

BIP 379 (Miniscript) allows relative height and time locks that have no consensus meaning in BIP 68 (relative timelocks) / BIP 112 (CHECKSEQUENCEVERIFY). This is (ab)used by some protocols, e.g. by Lightning to encode extra data, but is unsafe when used unintentionally: older(65536) is equivalent to older(1).

This PR emits a warning when importdescriptors contains such a descriptor.

The first commit makes SEQUENCE_LOCKTIME flags reusable by other tests.

The main commit adds the ForEachNode helper to miniscript.h which is then used in the MiniscriptDescriptor constructor to check for Fragment::OLDER with unsafe values. These are stored in m_warnings, which the RPC code then collects via Warnings().

It adds both a unit and functional test.

A previous version of this PR prevented the import, unless the user opted in with an unsafe flag. It also used string parsing in the RPC code.

DrahtBot added the label Wallet on Aug 5, 2025

DrahtBot commented at 12:00 pm on August 5, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33135.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	brunoerg, pythcoiner

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/872 (Menu action to export a watchonly wallet by achow101)
#33392 (wallet, rpc: add UTXO set check and incremental rescan to importdescriptors by musaHaruna)
#32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
#32652 (wallet: add codex32 argument to addhdkey by roconnor-blockstream)
#32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
#31713 (miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up) by hodlinator)
#29136 (wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/wallet/rpc/backup.cpp:166 in fac503e7d5 outdated

159@@ -158,6 +160,30 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
160         if (parsed_descs.empty()) {
161             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
162         }
163+
164+        // Additional safety checks for valid descriptors
165+        size_t pos{0};
166+        while (!unsafe && (pos = descriptor.find("older(", pos)) != std::string::npos) {

w0xlt commented at 7:14 pm on August 5, 2025:

Wouldn’t it be better to have this logic inside the miniscript files?

Sjors commented at 7:20 pm on August 5, 2025:

Maybe, but I’m worried it would add a ton of code churn in order to either:

bubble these values up; or
have the descriptor and miniscript parser take an unsafe argument to change its behavior (or some more general parse_options argument)

Sjors commented at 6:55 am on August 6, 2025:

That said, I’m not a huge fan of this much string parsing logic either.

in test/functional/wallet_importdescriptors.py:779 in fac503e7d5 outdated

771@@ -770,5 +772,48 @@ def run_test(self):
772             assert_equal(w_multipath.getrawchangeaddress(address_type="bech32"), w_multisplit.getrawchangeaddress(address_type="bech32"))
773         assert_equal(sorted(w_multipath.listdescriptors()["descriptors"], key=lambda x: x["desc"]), sorted(w_multisplit.listdescriptors()["descriptors"], key=lambda x: x["desc"]))
774 
775+        self.log.info("Test older() safety")
776+
777+        for flag in [0, SEQUENCE_LOCKTIME_TYPE_FLAG]:
778+            self.log.debug("Importing a safe value always works")
779+            height_delta = 65535 + (1 << 22 if flag is SEQUENCE_LOCKTIME_TYPE_FLAG else 0)

w0xlt commented at 8:48 pm on August 5, 2025:

nit

0            height_delta = 65535 + (flag if flag is SEQUENCE_LOCKTIME_TYPE_FLAG else 0)

in src/wallet/rpc/backup.cpp:348 in fac503e7d5 outdated

344@@ -319,6 +345,7 @@ RPCHelpMan importdescriptors()
345                                     },
346                                     {"internal", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether matching outputs should be treated as not incoming payments (e.g. change)"},
347                                     {"label", RPCArg::Type::STR, RPCArg::Default{""}, "Label to assign to the address, only allowed with internal=false. Disabled for ranged descriptors"},
348+                                    {"unsafe", RPCArg::Type::BOOL, RPCArg::Default{false}, "Allow the import of descriptors that contain height or time locks with no consensus meaning, e.g. older(65536)"},

brunoerg commented at 6:29 pm on August 21, 2025:

fac503e7d551d1cbeb11cdb43b222f24c6de002e: Worth adding this field in the examples (HelpExampleCli)?

brunoerg commented at 6:40 pm on August 21, 2025:

Also, it would need a release note.

Sjors commented at 7:55 am on September 1, 2025:

I dropped this option, so probably no longer needed to document it? I expect it’s quite rare for people to even encounter the warning.

brunoerg commented at 6:39 pm on August 21, 2025: contributor

I’ve reviewed the code and it looks great - Just a question: Instead of having this new field and prevent the import (any user have reported an accidental import?), couldn’t we just throw a warning?

Sjors commented at 8:32 am on August 22, 2025: member

@brunoerg perhaps a warning is good enough yes. In that case we wouldn’t need the unsafe option. Will give it some thought.

Sjors force-pushed on Sep 1, 2025

Sjors renamed this:
~~wallet: prevent accidental unsafe older() import~~
wallet: warn against accidental unsafe older() import
on Sep 1, 2025

Sjors commented at 7:53 am on September 1, 2025: member

I switched to emitting a warning.

I’ll look into modifying miniscript / descriptor classes to avoid the string parsing. Though unless people really don’t like it, I think it could be done as a followup.

test: move SEQUENCE_LOCKTIME flags to script 27d5f6507f

Sjors force-pushed on Sep 9, 2025

Sjors commented at 4:13 pm on September 9, 2025: member

Rebased. Fixed bug in the functional test found by an AI overlord, apparently using is isn’t safe: https://stackoverflow.com/a/28864111

I’ll look into modifying miniscript / descriptor classes to avoid the string parsing.

Done, and also added a unit test. It’s a bit more code and complexity, but more robust and extendable than parsing strings.

Since c1e6a9f1e7331fa2c1c73a0dd17056570283e443 is no longer relevant for this PR I dropped it.

Sjors force-pushed on Sep 9, 2025

brunoerg approved

brunoerg commented at 7:32 pm on September 9, 2025: contributor

ACK fe06b92429206aee66d33c843195235f4bb27181 - This new approach of just warning is better than the previous one, nice! I haven’t reviewed the code in depth, just did a light code review but tested the behavior in practice and worked fine.

Also, I ran a mutation analysis on these changes and the only uncaught mutant is:

0         // Traverse miniscript tree for unsafe use of older()
1         miniscript::ForEachNode(*m_node, [&](const miniscript::Node<uint32_t>& node) {
2-            if (node.fragment == miniscript::Fragment::OLDER) {
3+            if (1==1) {
4                 const uint32_t raw = node.k;
5                 const uint32_t value_part = raw & ~CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG;
6                 if (value_part > CTxIn::SEQUENCE_LOCKTIME_MASK) {

it means we’re not really testing that the warning is produced exclusively for the older() fragment, nothing critical and can be ignored but I think a simple test case could solve it. e.g.:

 0diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
 1index 9e06e78fe9..fb35dfd05a 100644
 2--- a/src/test/descriptor_tests.cpp
 3+++ b/src/test/descriptor_tests.cpp
 4@@ -1260,6 +1260,15 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
 5
 6 BOOST_AUTO_TEST_CASE(descriptor_older_warnings)
 7 {
 8+    {
 9+        FlatSigningProvider keys;
10+        std::string err;
11+        // Using after() with a large timestamp (> 65535)
12+        auto descs = Parse("wsh(and_v(v:pk(0379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3),after(1000000)))", keys, err, /*require_checksum=*/false);
13+        BOOST_REQUIRE(!descs.empty());
14+        BOOST_CHECK(descs[0]->Warnings().empty()); // Ensure the older() warning is really being applied only for this fragment
15+    }
16+

Sjors force-pushed on Sep 10, 2025

Sjors commented at 6:34 am on September 10, 2025: member

@brunoerg thanks, I added your test against false positive warnings for after(), which passes.

fe06b92429206aee66d33c843195235f4bb27181 -> 5e9737ff493a96566f4fc11b1733b4b5a5393f6b: compare

brunoerg approved

brunoerg commented at 2:32 pm on September 10, 2025: contributor

reACK 5e9737ff493a96566f4fc11b1733b4b5a5393f6b

in src/script/descriptor.h:169 in 5e9737ff49 outdated

164@@ -165,6 +165,9 @@ struct Descriptor {
165      * @param[out] ext_pubs Any extended public keys
166      */
167     virtual void GetPubKeys(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const = 0;
168+
169+    // Semantic/safety warnings (includes subdescriptors).

pythcoiner commented at 12:09 pm on September 13, 2025:

nit (for consistency)

0    /** Semantic/safety warnings (includes subdescriptors). */

in src/script/miniscript.h:10 in 5e9737ff49 outdated

 6@@ -7,8 +7,10 @@
 7 
 8 #include <algorithm>
 9 #include <compare>
10+#include <concepts>

pythcoiner commented at 1:02 pm on September 13, 2025:

nit: not sure if those includes are stricly required?

Sjors commented at 8:00 am on September 15, 2025:

There’s some sort of linter command that can check this, but I forgot which.

pythcoiner commented at 6:23 pm on September 15, 2025:

build success even w/o those 2 (added) includes on my side

Sjors commented at 8:00 am on September 16, 2025:

That doesn’t really say much, because indirect includes will succeed but we try to avoid those. Though maybe not for std stuff?

Sjors commented at 8:14 am on September 18, 2025:

The Tidy CI job runs IWYU and doesn’t complain: https://github.com/bitcoin/bitcoin/actions/runs/17605378583/job/50015040929?pr=33135#step:9:6218

in test/functional/wallet_importdescriptors.py:62 in 5e9737ff49 outdated

58@@ -58,6 +59,7 @@ def test_importdesc(self, req, success, error_code=None, error_message=None, war
59         if 'warnings' in result[0]:
60             observed_warnings = result[0]['warnings']
61         assert_equal("\n".join(sorted(warnings)), "\n".join(sorted(observed_warnings)))
62+        self.log.debug(result)

pythcoiner commented at 1:13 pm on September 13, 2025:

nit: should we keep this debug log?

Sjors commented at 7:59 am on September 15, 2025:

Yes, it’s only printed when you set the log level to debug and it’s useful for debugging if the test breaks.

pythcoiner commented at 8:02 am on September 15, 2025: none

tACK 5e9737ff49 code review + test importing few “Liana” descriptor with timelocks > 65535

in src/script/descriptor.cpp:1555 in 5e9737ff49 outdated

1551+                const uint32_t raw = node.k;
1552+                const uint32_t value_part = raw & ~CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG;
1553+                if (value_part > CTxIn::SEQUENCE_LOCKTIME_MASK) {
1554+                    const bool is_time = (raw & CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG) != 0;
1555+                    if (is_time) {
1556+                        m_warnings.push_back(strprintf("older(%u - 1<<22) > 65535 is unsafe", raw));

achow101 commented at 8:31 pm on September 17, 2025:

In 5e9737ff493a96566f4fc11b1733b4b5a5393f6b “wallet: warn against accidental unsafe older() import”

This warning is basically incomprehensible. It would be better to actually write out what it means, i.e. “time based relative locktime > 65535 is unsafe”

Sjors commented at 8:15 am on September 18, 2025:

I went for a slight variant: relative locktime > 65535 * 512 seconds is unsafe

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.

fb72cc33be

Sjors force-pushed on Sep 18, 2025

wallet: warn against accidental unsafe older() import #33135

Code Coverage & Benchmarks

Reviews

Conflicts