wallet: prevent accidental unsafe older() import #33135

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 prevents accidental import of such descriptors. They will be rejected unless marked unsafe in importdescriptors.

   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.

2. refactor: move FindNextChar to script/parsing

   Also document it and take a reference.

   904044ea03
3. test: move SEQUENCE_LOCKTIME flags to script 552bc916fb
4. wallet: prevent 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 prevents accidental import of such descriptors. They will be rejected unless
   marked 'unsafe' in importdescriptors.
   
   Wallets that already contain such a descriptor are not impacted.

   fac503e7d5
5. DrahtBot added the label Wallet on Aug 5, 2025
6. 
   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.

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

   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.
8. in test/functional/wallet_importdescriptors.py:779 in fac503e7d5

   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)
   

Contributors
Sjors DrahtBot w0xlt

Labels
Wallet