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

Sjors · 2025-08-05T12:00:42Z

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.

Based on:

Change Parse descriptor argument to string_view #33914

DrahtBot · 2025-08-05T12:00:47Z

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
ACK	rkrux, brunoerg, pythcoiner, achow101

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33392 (wallet, rpc: add UTXO set check and incremental rescan to importdescriptors by musaHaruna)
#32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
#32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
#31713 (miniscript refactor: Remove unique_ptr-indirection by hodlinator)
#31668 (Added rescan option for import descriptors by saikiran57)
#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.

src/wallet/rpc/backup.cpp

test/functional/wallet_importdescriptors.py

src/wallet/rpc/backup.cpp

brunoerg

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 · 2025-08-22T08:32:44Z

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

Sjors · 2025-09-01T07:53:03Z

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.

Sjors · 2025-09-09T16:13:10Z

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.

brunoerg

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:

         // Traverse miniscript tree for unsafe use of older()
         miniscript::ForEachNode(*m_node, [&](const miniscript::Node<uint32_t>& node) {
-            if (node.fragment == miniscript::Fragment::OLDER) {
+            if (1==1) {
                 const uint32_t raw = node.k;
                 const uint32_t value_part = raw & ~CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG;
                 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.:

diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
index 9e06e78fe9..fb35dfd05a 100644
--- a/src/test/descriptor_tests.cpp
+++ b/src/test/descriptor_tests.cpp
@@ -1260,6 +1260,15 @@ BOOST_AUTO_TEST_CASE(descriptor_test)

 BOOST_AUTO_TEST_CASE(descriptor_older_warnings)
 {
+    {
+        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(!descs.empty());
+        BOOST_CHECK(descs[0]->Warnings().empty()); // Ensure the older() warning is really being applied only for this fragment
+    }
+

Sjors · 2025-09-10T06:34:37Z

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

fe06b92429206aee66d33c843195235f4bb27181 -> 5e9737ff493a96566f4fc11b1733b4b5a5393f6b: compare

brunoerg

reACK 5e9737ff493a96566f4fc11b1733b4b5a5393f6b

src/script/descriptor.h

pythcoiner · 2025-09-13T13:02:40Z

src/script/miniscript.h

@@ -7,8 +7,10 @@

 #include <algorithm>
 #include <compare>
+#include <concepts>


nit: not sure if those includes are stricly required?

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

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

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

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

pythcoiner · 2025-09-13T13:13:26Z

test/functional/wallet_importdescriptors.py

@@ -58,6 +59,7 @@ def test_importdesc(self, req, success, error_code=None, error_message=None, war
        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)


nit: should we keep this debug log?

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

pythcoiner · 2025-09-15T08:02:39Z

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

achow101 · 2025-09-17T20:31:30Z

src/script/descriptor.cpp

+                if (value_part > CTxIn::SEQUENCE_LOCKTIME_MASK) {
+                    const bool is_time = (raw & CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG) != 0;
+                    if (is_time) {
+                        m_warnings.push_back(strprintf("older(%u - 1<<22) > 65535 is unsafe", raw));


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"

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

DrahtBot · 2025-11-20T12:27:31Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/actions/runs/19535494720/job/55931077158}
_{LLM reason (✨ experimental): Lint failures caused by missing SEQUENCE_LOCKTIME_TYPE_FLAG in test_framework.script during Python lint checks.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Sjors · 2025-11-20T13:00:38Z

Lost the first commit in that rebase, let's try again...

c0bfe72 Change Parse descriptor argument to string_view (Sjors Provoost) Pull request description: While investigating a silent merge conflict in #33135 I noticed that #32983 changed the descriptor `Parse` function signature from `const std::string& descriptor` to `std::span<const char> descriptor`. Calling that new version of `Parse` with a string literal will trigger a confusing "Invalid characters in payload" due to the trailing "\0". It can be worked around by having (the test) wrap string literals in `std::string()`, but that's easy to forget. Using `string_view` is easier and more compact than (as a previous version of this PR did) checking for trailing `\0`. Also add a test. ACKs for top commit: maflcko: review ACK c0bfe72 🍨 enirox001: tACK c0bfe72 stickies-v: ACK c0bfe72 rkrux: crACK c0bfe72 Tree-SHA512: 6b20307f834dae66826c8763f6c2ba0071f4e369375184cb5ff8543b85220fcaf33a47ddb065e418d1af3ed9a3fac401a7854f8924f52aab2b000b1f65328f2c

Sjors · 2025-11-27T10:13:09Z

Rebased after #33914 so the test should pass again.

rkrux

lgtm ACK 189437a3dedefce3945ab2a8c05785eb283c2c6f

Shared a non-blocking nit.

src/script/descriptor.cpp

brunoerg

reACK 189437a3dedefce3945ab2a8c05785eb283c2c6f

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.

rkrux

lgtm re-ACK 76c092f

Sjors · 2025-12-29T05:11:30Z

@brunoerg @pythcoiner can you re-review the latest push?

brunoerg

reACK 76c092f

pythcoiner · 2026-01-02T07:55:17Z

reACK 76c092f

achow101 · 2026-01-03T00:00:02Z

ACK 76c092f

…followup item from PR bitcoin#33135 by refactoring descriptor classes to avoid string parsing instead of emitting warnings.

DrahtBot added the Wallet label Aug 5, 2025

Sjors mentioned this pull request Aug 5, 2025

MuSig2 power Sjors/bitcoin#91

Open

This was referenced Aug 5, 2025

Have createwalletdescriptor auto-detect an unused(KEY) #32861

Draft

wallet: derivehdkey RPC to get xpub at arbitrary path #32784

Draft

wallet: add codex32 argument to addhdkey #32652

Closed

w0xlt reviewed Aug 5, 2025

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

w0xlt reviewed Aug 5, 2025

View reviewed changes

test/functional/wallet_importdescriptors.py Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 5, 2025

wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor #29136

Open

brunoerg reviewed Aug 21, 2025

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

brunoerg reviewed Aug 21, 2025

View reviewed changes

Sjors force-pushed the 2025/08/older-safety branch from fac503e to 8c6b70b Compare September 1, 2025 07:48

Sjors changed the title ~~wallet: prevent accidental unsafe older() import~~ wallet: warn against accidental unsafe older() import Sep 1, 2025

Sjors force-pushed the 2025/08/older-safety branch from 8c6b70b to 3aa611d Compare September 9, 2025 16:12

Sjors force-pushed the 2025/08/older-safety branch 2 times, most recently from 2f684f2 to fe06b92 Compare September 9, 2025 17:14

brunoerg approved these changes Sep 9, 2025

View reviewed changes

Sjors force-pushed the 2025/08/older-safety branch 2 times, most recently from 9431dc3 to 5e9737f Compare September 10, 2025 06:34

brunoerg approved these changes Sep 10, 2025

View reviewed changes

This was referenced Sep 11, 2025

wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet #32489

Open

miniscript refactor: Remove unique_ptr-indirection #31713

Open

Menu action to export a watchonly wallet bitcoin-core/gui#872

Draft

pythcoiner reviewed Sep 13, 2025

View reviewed changes

achow101 reviewed Sep 17, 2025

View reviewed changes

Sjors force-pushed the 2025/08/older-safety branch from 6edd8d4 to bbfa35d Compare November 20, 2025 13:01

DrahtBot removed the CI failed label Nov 20, 2025

test: move SEQUENCE_LOCKTIME flags to script

592157b

Sjors force-pushed the 2025/08/older-safety branch from bbfa35d to 189437a Compare November 27, 2025 10:12

Sjors marked this pull request as ready for review November 27, 2025 10:13

rkrux approved these changes Dec 1, 2025

View reviewed changes

src/script/descriptor.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from achow101, brunoerg and pythcoiner December 1, 2025 13:23

brunoerg approved these changes Dec 1, 2025

View reviewed changes

Sjors force-pushed the 2025/08/older-safety branch from 189437a to 45eb536 Compare December 2, 2025 08:51

DrahtBot added the CI failed label Dec 2, 2025

Sjors force-pushed the 2025/08/older-safety branch from 45eb536 to 76c092f Compare December 2, 2025 11:24

DrahtBot removed the CI failed label Dec 2, 2025

rkrux approved these changes Dec 2, 2025

View reviewed changes

DrahtBot requested a review from brunoerg December 2, 2025 13:05

DrahtBot mentioned this pull request Dec 6, 2025

Added rescan option for import descriptors #31668

Open

brunoerg approved these changes Dec 29, 2025

View reviewed changes

achow101 merged commit 2628de7 into bitcoin:master Jan 3, 2026
25 checks passed

kevkevinpal mentioned this pull request Jan 3, 2026

wallet: warn against accidental unsafe older() import kevkevinpal/bitcoin#239

Closed

kevkevinpal mentioned this pull request Jan 4, 2026

Modify miniscript/descriptor classes to avoid string parsing Address… kevkevinpal/bitcoin#240

Closed

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

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

Uh oh!

Conversation

Sjors commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Sep 9, 2025

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pythcoiner commented Sep 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 20, 2025

Uh oh!

Sjors commented Nov 20, 2025

Uh oh!

Sjors commented Nov 27, 2025

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Dec 29, 2025

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Aug 5, 2025 •

edited

Loading

DrahtBot commented Aug 5, 2025 •

edited

Loading

Sjors commented Aug 22, 2025 •

edited

Loading

Sjors commented Sep 1, 2025 •

edited

Loading

Sjors commented Sep 10, 2025 •

edited

Loading

Sjors Sep 18, 2025 •

edited

Loading