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.

In preparation the first commit moves FindNextChar from miniscript.{h,cpp} to parsing.{h,cpp} which already contains similar functions. It also documents it.

The second commit makes SEQUENCE_LOCKTIME flags reusable by other tests.

The main commit takes advantage of the fact that the descriptor has already been parsed for validity, so the CHECK_NONFATAL conditions should never happen.

A previous version of this PR prevented the import, unless the user opted in with an unsafe flag.

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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
#32784 (wallet: derivehdkey RPC to get xpub at arbitrary path by Sjors)
#32652 (wallet: add codex32 argument to addhdkey by roconnor-blockstream)
#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.

refactor: move FindNextChar to script/parsing

Also document it and take a reference.

c1e6a9f1e7

test: move SEQUENCE_LOCKTIME flags to script f966d7b8af

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.

8c6b70b6e0

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.

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

Code Coverage & Benchmarks

Reviews

Conflicts