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

76c092ff80 wallet: warn against accidental unsafe older() import (Sjors Provoost)
592157b759 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] https://github.com/bitcoin/bitcoin/pull/33914

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

Tree-SHA512: 8e944e499bd4a43cc27eeb889f262b499b9b07aa07610f4a415ccb4e34a9110f9946646f446a54ac5bf17494d8d96a89e4a1fa278385db9b950468f27283e17a

test/functional/feature_bip68_sequence.py

@@ -24,6 +24,10 @@ from test_framework.messages import (
 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 @@ from test_framework.wallet import MiniWallet
 
 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 @@ MAX_PUBKEYS_PER_MULTI_A = 999
 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.authproxy import JSONRPCException
 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 @@ class ImportDescriptorsTest(BitcoinTestFramework):
         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 @@ class ImportDescriptorsTest(BitcoinTestFramework):
             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()