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

pull Sjors wants to merge 2 commits into bitcoin:master from Sjors:2025/08/older-safety changing 9 files +144 −6
  1. 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.

  2. DrahtBot added the label Wallet on Aug 5, 2025
  3. 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
    ACK pythcoiner, brunoerg

    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)
    • #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.

  4. 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:

    1. bubble these values up; or
    2. 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.
  5. 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)
  6. 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.
  7. 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?
  8. 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.
  9. Sjors force-pushed on Sep 1, 2025
  10. Sjors renamed this:
    wallet: prevent accidental unsafe older() import
    wallet: warn against accidental unsafe older() import
    on Sep 1, 2025
  11. 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.

  12. test: move SEQUENCE_LOCKTIME flags to script 27d5f6507f
  13. Sjors force-pushed on Sep 9, 2025
  14. 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.

  15. Sjors force-pushed on Sep 9, 2025
  16. Sjors force-pushed on Sep 9, 2025
  17. brunoerg approved
  18. 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+
  19. Sjors force-pushed on Sep 10, 2025
  20. Sjors force-pushed on Sep 10, 2025
  21. 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

  22. brunoerg approved
  23. brunoerg commented at 2:32 pm on September 10, 2025: contributor
    reACK 5e9737ff493a96566f4fc11b1733b4b5a5393f6b
  24. 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). */
  25. 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
  26. 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.
  27. pythcoiner commented at 8:02 am on September 15, 2025: none
    tACK 5e9737ff49 code review + test importing few “Liana” descriptor with timelocks > 65535
  28. 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
  29. 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
  30. Sjors force-pushed on Sep 18, 2025
  31. scgbckbone commented at 1:52 am on October 21, 2025: none
    What is the reason for allowing to import older(65536) ? I do not see rationale here, only mention of “previous version” that disallowed importing. I’d prefer older version if that is the case. Complete prohibition, even with unsafe. Importing older(65536) is not what user wants if resulting script is older(1) anyways.
  32. Rob1Ham commented at 9:03 pm on October 22, 2025: none

    I have a question as it relates to relative timelocks using the older() fragment if one were programming an epoch time based timelock.

    The smallest value that can be used to have the miniscript compiler interpret a relative timelock is older(4194305) (calculated by following the BIP68 spec of: 1 |= (1 « 22)). This is a 1 unit timelock of duration 512 seconds.

    The maximum value for a relative epoch timelock is older(4259839) which is 65,535 unit timelock, resulting in 33,554,431 seconds, or 388 days.

    For example, if you wanted a 4 day epoch time based timelock, you could use older(4194979) (512 seconds maps neatly onto 4 days, which is 512 * 675 = 345600 seconds = 4 days.

    This was something I stumbled upon 2 years ago going deeper into the miniscript compiler, I personally am not using relative timelocks with epoch time right now (and don’t know anyone who does), but wanted to call this out.

  33. scgbckbone commented at 11:48 pm on October 22, 2025: none

    Thanks Rob. I completely forget about that 22nd bit to change to time-based lock.

    To update my previous comment: anything without consensus meaning should not be allowed to import.

    Only older(1 - 65535) & older(4194305 - 4259839) should be importable.

  34. Sjors commented at 10:55 am on October 24, 2025: member

    As I mentioned in the PR description, there are applications - including the lightning protocol - that use these extra bits. I don’t know if they use descriptors internally to represent them, but if they do, then banning these values would mean they can’t import these descriptors.

    There’s also nothing in the BIP that bans (or even discourages or warns about) these values, so we wouldn’t be compatible with other wallets.

  35. pythcoiner commented at 11:28 am on October 24, 2025: none
    reACK fb72cc3
  36. DrahtBot requested review from brunoerg on Oct 24, 2025
  37. brunoerg approved
  38. brunoerg commented at 2:09 pm on October 24, 2025: contributor
    reACK fb72cc33be570d12fc6b76146fc89047e58f5aaf
  39. Sjors requested review from w0xlt on Oct 24, 2025


Sjors DrahtBot w0xlt brunoerg pythcoiner achow101 scgbckbone Rob1Ham


w0xlt

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-11-02 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me