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

76c092f wallet: warn against accidental unsafe older() import (Sjors Provoost)
592157b test: move SEQUENCE_LOCKTIME flags to script (Sjors Provoost)

Pull request description:

  [BIP 379](https://github.com/bitcoin/bips/blob/master/bip-0379.md) ([Miniscript](https://bitcoin.sipa.be/miniscript/)) allows relative height and time locks that have no consensus meaning in [BIP 68](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki) (relative timelocks) / [BIP 112](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) (`CHECKSEQUENCEVERIFY`). This is (ab)used by some protocols, e.g. [by Lightning to encode extra data](https://delvingbitcoin.org/t/exploring-extended-relative-timelocks/1818/23), 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:
  - [x] bitcoin/bitcoin#33914

ACKs for top commit:
  pythcoiner:
    reACK 76c092f
  achow101:
    ACK 76c092f
  rkrux:
    lgtm re-ACK 76c092f
  brunoerg:
    reACK 76c092f

Tree-SHA512: 8e944e499bd4a43cc27eeb889f262b499b9b07aa07610f4a415ccb4e34a9110f9946646f446a54ac5bf17494d8d96a89e4a1fa278385db9b950468f27283e17a

Author: achow101

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/feature_bip68_sequence.py

@@ -24,6 +24,10 @@
 from test_framework.script import (
     CScript,
     OP_TRUE,
+    SEQUENCE_LOCKTIME_DISABLE_FLAG,
+    SEQUENCE_LOCKTIME_TYPE_FLAG,
+    SEQUENCE_LOCKTIME_GRANULARITY,
+    SEQUENCE_LOCKTIME_MASK,
 )
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
@@ -36,11 +40,6 @@
 
 SCRIPT_W0_SH_OP_TRUE = script_to_p2wsh_script(CScript([OP_TRUE]))
 
-SEQUENCE_LOCKTIME_DISABLE_FLAG = (1<<31)
-SEQUENCE_LOCKTIME_TYPE_FLAG = (1<<22) # this means use time (0 means height)
-SEQUENCE_LOCKTIME_GRANULARITY = 9 # this is a bit-shift
-SEQUENCE_LOCKTIME_MASK = 0x0000ffff
-
 # RPC error for non-BIP68 final transactions
 NOT_FINAL_ERROR = "non-BIP68-final"
 

test/functional/test_framework/script.py

@@ -28,6 +28,11 @@
 LOCKTIME_THRESHOLD = 500000000
 ANNEX_TAG = 0x50
 
+SEQUENCE_LOCKTIME_DISABLE_FLAG = (1<<31)
+SEQUENCE_LOCKTIME_TYPE_FLAG = (1<<22) # this means use time (0 means height)
+SEQUENCE_LOCKTIME_GRANULARITY = 9 # this is a bit-shift
+SEQUENCE_LOCKTIME_MASK = 0x0000ffff
+
 LEAF_VERSION_TAPSCRIPT = 0xc0
 
 def hash160(s):

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 = (
+                f"time-based relative locktime: older({unsafe_value}) > (65535 * 512) seconds is unsafe"
+                if flag == SEQUENCE_LOCKTIME_TYPE_FLAG
+                else f"height-based relative locktime: older({unsafe_value}) > 65535 blocks 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()