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.

Author: Sjors

src/script/descriptor.cpp

@@ -791,6 +791,8 @@ class DescriptorImpl : public Descriptor
     const std::vector<std::unique_ptr<PubkeyProvider>> m_pubkey_args;
     //! The string name of the descriptor function.
     const std::string m_name;
+    //! Warnings (not including subdescriptors).
+    std::vector<std::string> m_warnings;
 
     //! The sub-descriptor arguments (empty for everything but SH and WSH).
     //! In doc/descriptors.m this is referred to as SCRIPT expressions sh(SCRIPT)
@@ -990,6 +992,16 @@ class DescriptorImpl : public Descriptor
     }
 
     virtual std::unique_ptr<DescriptorImpl> Clone() const = 0;
+
+    // NOLINTNEXTLINE(misc-no-recursion)
+    std::vector<std::string> Warnings() const override {
+        std::vector<std::string> all = m_warnings;
+        for (const auto& sub : m_subdescriptor_args) {
+            auto sub_w = sub->Warnings();
+            all.insert(all.end(), sub_w.begin(), sub_w.end());
+        }
+        return all;
+    }
 };
 
 /** A parsed addr(A) descriptor. */
@@ -1537,7 +1549,24 @@ class MiniscriptDescriptor final : public DescriptorImpl
 
 public:
     MiniscriptDescriptor(std::vector<std::unique_ptr<PubkeyProvider>> providers, miniscript::NodeRef<uint32_t> node)
-        : DescriptorImpl(std::move(providers), "?"), m_node(std::move(node)) {}
+        : DescriptorImpl(std::move(providers), "?"), m_node(std::move(node))
+    {
+        // Traverse miniscript tree for unsafe use of older()
+        miniscript::ForEachNode(*m_node, [&](const miniscript::Node<uint32_t>& node) {
+            if (node.fragment == miniscript::Fragment::OLDER) {
+                const uint32_t raw = node.k;
+                const uint32_t value_part = raw & ~CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG;
+                if (value_part > CTxIn::SEQUENCE_LOCKTIME_MASK) {
+                    const bool is_time_based = (raw & CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG) != 0;
+                    if (is_time_based) {
+                        m_warnings.push_back(strprintf("time-based relative locktime: older(%u) > (65535 * 512) seconds is unsafe", raw));
+                    } else {
+                        m_warnings.push_back(strprintf("height-based relative locktime: older(%u) > 65535 blocks is unsafe", raw));
+                    }
+                }
+            }
+        });
+    }
 
     bool ToStringHelper(const SigningProvider* arg, std::string& out, const StringType type,
                         const DescriptorCache* cache = nullptr) const override

src/script/descriptor.h

@@ -165,6 +165,9 @@ struct Descriptor {
      * @param[out] ext_pubs Any extended public keys
      */
     virtual void GetPubKeys(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const = 0;
+
+    /** Semantic/safety warnings (includes subdescriptors). */
+    virtual std::vector<std::string> Warnings() const = 0;
 };
 
 /** Parse a `descriptor` string. Included private keys are put in `out`.

src/script/miniscript.h

@@ -7,8 +7,10 @@
 
 #include <algorithm>
 #include <compare>
+#include <concepts>
 #include <cstdint>
 #include <cstdlib>
+#include <functional>
 #include <iterator>
 #include <memory>
 #include <optional>
@@ -195,6 +197,21 @@ template<typename Key> using NodeRef = std::unique_ptr<const Node<Key>>;
 template<typename Key, typename... Args>
 NodeRef<Key> MakeNodeRef(Args&&... args) { return std::make_unique<const Node<Key>>(std::forward<Args>(args)...); }
 
+//! Unordered traversal of a miniscript node tree.
+template <typename Key, std::invocable<const Node<Key>&> Fn>
+void ForEachNode(const Node<Key>& root, Fn&& fn)
+{
+    std::vector<std::reference_wrapper<const Node<Key>>> stack{root};
+    while (!stack.empty()) {
+        const Node<Key>& node = stack.back();
+        std::invoke(fn, node);
+        stack.pop_back();
+        for (const auto& sub : node.subs) {
+            stack.emplace_back(*sub);
+        }
+    }
+}
+
 //! The different node types in miniscript.
 enum class Fragment {
     JUST_0,    //!< OP_0

src/test/descriptor_tests.cpp

@@ -1271,4 +1271,50 @@ BOOST_AUTO_TEST_CASE(descriptor_literal_null_byte)
     BOOST_REQUIRE_MESSAGE(!descs.empty(), err);
 }
 
+BOOST_AUTO_TEST_CASE(descriptor_older_warnings)
+{
+    // A safe boundary value should yield no warnings.
+    {
+        FlatSigningProvider keys;
+        std::string err;
+        auto descs = Parse("wsh(and_v(v:pk(0379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3),older(65535)))", keys, err, /*require_checksum=*/false);
+        BOOST_REQUIRE_MESSAGE(!descs.empty(), err);
+        BOOST_CHECK(descs[0]->Warnings().empty());
+    }
+
+    // Height-based unsafe value (65536) should produce one warning.
+    {
+        FlatSigningProvider keys;
+        std::string err;
+        const uint32_t height_unsafe = 65536;
+        auto descs = Parse(strprintf("wsh(and_v(v:pk(0379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3),older(%u)))", height_unsafe), keys, err, /*require_checksum=*/false);
+        BOOST_REQUIRE_MESSAGE(!descs.empty(), err);
+        const auto& ws = descs[0]->Warnings();
+        BOOST_REQUIRE_EQUAL(ws.size(), 1U);
+        BOOST_CHECK_EQUAL(ws[0], strprintf("height-based relative locktime: older(%u) > 65535 blocks is unsafe", height_unsafe));
+    }
+
+    // Time-based unsafe value: add SEQUENCE_LOCKTIME_TYPE_FLAG (1<<22)
+    {
+        FlatSigningProvider keys;
+        std::string err;
+        const uint32_t time_unsafe = 65536 | CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG;
+        auto descs = Parse(strprintf("wsh(and_v(v:pk(0379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3),older(%u)))", time_unsafe), keys, err, /*require_checksum=*/false);
+        BOOST_REQUIRE_MESSAGE(!descs.empty(), err);
+        const auto& warnings = descs[0]->Warnings();
+        BOOST_REQUIRE_EQUAL(warnings.size(), 1U);
+        BOOST_CHECK_EQUAL(warnings[0], strprintf("time-based relative locktime: older(%u) > (65535 * 512) seconds is unsafe", time_unsafe));
+    }
+
+    // Ensure no false positive warnings for absolute timelocks
+    {
+        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_MESSAGE(!descs.empty(), err);
+        BOOST_CHECK(descs[0]->Warnings().empty());
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/wallet/rpc/backup.cpp

@@ -240,6 +240,10 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
             }
             parsed_desc->ExpandPrivate(0, keys, expand_keys);
 
+            for (const auto& w : parsed_desc->Warnings()) {
+               warnings.push_back(w);
+            }
+
             // Check if all private keys are provided
             bool have_all_privkeys = !expand_keys.keys.empty();
             for (const auto& entry : expand_keys.origins) {

src/wallet/test/walletload_tests.cpp

@@ -35,6 +35,7 @@ class DummyDescriptor final : public Descriptor {
     std::optional<int64_t> MaxSatisfactionWeight(bool) const override { return {}; }
     std::optional<int64_t> MaxSatisfactionElems() const override { return {}; }
     void GetPubKeys(std::set<CPubKey>& pubkeys, std::set<CExtPubKey>& ext_pubs) const override {}
+    std::vector<std::string> Warnings() const override { return {}; }
 };
 
 BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup)

test/functional/wallet_importdescriptors.py

@@ -22,6 +22,7 @@
 from test_framework.blocktools import COINBASE_MATURITY
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.descriptors import descsum_create
+from test_framework.script import SEQUENCE_LOCKTIME_TYPE_FLAG
 from test_framework.util import (
     assert_equal,
     assert_raises_rpc_error,
@@ -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)
         assert_equal(result[0]['success'], success)
         if error_code is not None:
             assert_equal(result[0]['error']['code'], error_code)
@@ -783,5 +785,40 @@ def run_test(self):
             assert_equal(w_multipath.getrawchangeaddress(address_type="bech32"), w_multisplit.getrawchangeaddress(address_type="bech32"))
         assert_equal(sorted(w_multipath.listdescriptors()["descriptors"], key=lambda x: x["desc"]), sorted(w_multisplit.listdescriptors()["descriptors"], key=lambda x: x["desc"]))
 
+        self.log.info("Test older() safety")
+
+        for flag in [0, SEQUENCE_LOCKTIME_TYPE_FLAG]:
+            self.log.debug("Importing a safe value always works")
+            safe_value = (65535 | flag)
+            self.test_importdesc(
+                {
+                    'desc': descsum_create(f"wsh(and_v(v:pk([12345678/0h/0h]{xpub}/*),older({safe_value})))"),
+                    'active': True,
+                    'range': [0, 2],
+                    'timestamp': 'now'
+                },
+                success=True
+            )
+
+            self.log.debug("Importing an unsafe value results in a warning")
+            unsafe_value = safe_value + 1
+            desc = descsum_create(f"wsh(and_v(v:pk([12345678/0h/0h]{xpub}/*),older({unsafe_value})))")
+            expected_warning = (
+                "relative locktime > 65535 * 512 seconds is unsafe"
+                if flag == SEQUENCE_LOCKTIME_TYPE_FLAG
+                else f"older({unsafe_value}) > 65535 is unsafe"
+            )
+            self.test_importdesc(
+                {
+                    'desc': desc,
+                    'active': True,
+                    'range': [0, 2],
+                    'timestamp': 'now'
+                },
+                success=True,
+                warnings=[expected_warning],
+            )
+
+
 if __name__ == '__main__':
     ImportDescriptorsTest(__file__).main()