Merge bitcoin/bitcoin#19801: test: check for all possible OP_CLTV fail reasons in feature_cltv.py (BIP 65)

b01cd9471f435bb36b8ed5211a56baad51111ad2 test: check that _all_ invalid-CLTV txs are rejected after BIP65 activation (Sebastian Falbesoner)
dbc19814743cb12960a99793197c811e2750a06b test: check that _all_ invalid-CLTV txs are allowed in a block pre-BIP65 (Sebastian Falbesoner)
8d0ce50c4826529a2d30ffc850bce4d44da6019b test: prepare cltv_invalidate to test all failure reasons in feature_cltv.py (Sebastian Falbesoner)
ce994e1202c4820b1ad5c375d3d671fd0a18e092 test: add tx modfication helper function in feature_cltv.py (Sebastian Falbesoner)

Pull request description:

  The functional test for [BIP65](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki) / `OP_CHECKLOCKTIMEVERIFY` (`feature_cltv.py`) currently only tests one out of five conditions that lead to failure of the op-code -- by prepending the script `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to a tx's first input's scriptSig, the case of "_the top item on the stack is less than 0_" is checked:

  f8462a6d27/test/functional/feature_cltv.py (L26-L35)

  This PR adds the other cases (5 in total) by taking an integer argument to the function `cltv_invalidate` that is called in a loop instead of only once per testing scenario. Here is the full list of failure conditions and how they are tested (note that the scriptSig should still be valid before activation of BIP65, when `OP_CLTV` is simply a no-op):
  * _the stack is empty_
  ➡️  prepending `OP_CHECKLOCKTIMEVERIFY` to scriptSig
  * _the top item on the stack is less than 0_
  ➡️  prepending `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
  * _the lock-time type (height vs. timestamp) of the top stack item and the nLockTime field are not the same_
  ➡️  prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
  ➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=1296688602 (genesis block timestamp)
  * _the top stack item is greater than the transaction's nLockTime field_
  ➡️  prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
  ➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=500
  * _the nSequence field of the txin is 0xffffffff_
  ➡️  prepending `OPNum(500) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
  ➡️ setting tx.vin[0].nSequence=0xffffffff and tx.nCheckTimeLock=500

  The first commit creates a helper function for the tx modification and also includes some tidying up like turning single-line to multi-line Python imports where necessary and cleaning up some PEP8 warnings. The second commit prepares the invalidation function `cltv_invalidate` and the third and the fourth use it and check for the expected reject reason strings ("Operation not valid with the current stack size", "Negative locktime" and "Locktime requirement not satisfied").

ACKs for top commit:
  MarcoFalke:
    review ACK b01cd9471f435bb36b8ed5211a56baad51111ad2 🐣

Tree-SHA512: dd82ae86e2bc4f3ab9bb1cfc9f04e4431b2b59c8aaf2a9f4b28654a1577e003fb43c500f99d76ff57e96262168e1cad7c1a0d71158e4b01063737e8f4be1e07d

test/functional/feature_cltv.py

@@ -8,10 +8,23 @@ Test that the CHECKLOCKTIMEVERIFY soft-fork activates at (regtest) block height
 1351.
 """
 
-from test_framework.blocktools import create_coinbase, create_block, create_transaction
+from test_framework.blocktools import (
-from test_framework.messages import CTransaction, msg_block
+    create_block,
+    create_coinbase,
+    create_transaction,
+)
+from test_framework.messages import (
+    CTransaction,
+    msg_block,
+)
 from test_framework.p2p import P2PInterface
-from test_framework.script import CScript, OP_1NEGATE, OP_CHECKLOCKTIMEVERIFY, OP_DROP, CScriptNum
+from test_framework.script import (
+    CScript,
+    CScriptNum,
+    OP_1NEGATE,
+    OP_CHECKLOCKTIMEVERIFY,
+    OP_DROP,
+)
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
@@ -24,34 +37,56 @@ from io import BytesIO
 CLTV_HEIGHT = 1351
 
 
-def cltv_invalidate(tx):
+# Helper function to modify a transaction by
-    '''Modify the signature in vin 0 of the tx to fail CLTV
+# 1) prepending a given script to the scriptSig of vin 0 and
-
+# 2) (optionally) modify the nSequence of vin 0 and the tx's nLockTime
-    Prepends -1 CLTV DROP in the scriptSig itself.
+def cltv_modify_tx(node, tx, prepend_scriptsig, nsequence=None, nlocktime=None):
-
+    if nsequence is not None:
-    TODO: test more ways that transactions using CLTV could be invalid (eg
+        tx.vin[0].nSequence = nsequence
-    locktime requirements fail, sequence time requirements fail, etc).
+        tx.nLockTime = nlocktime
-    '''
-    tx.vin[0].scriptSig = CScript([OP_1NEGATE, OP_CHECKLOCKTIMEVERIFY, OP_DROP] +
-                                  list(CScript(tx.vin[0].scriptSig)))
-
-def cltv_validate(node, tx, height):
-    '''Modify the signature in vin 0 of the tx to pass CLTV
-    Prepends <height> CLTV DROP in the scriptSig, and sets
-    the locktime to height'''
-    tx.vin[0].nSequence = 0
-    tx.nLockTime = height
 
         # Need to re-sign, since nSequence and nLockTime changed
         signed_result = node.signrawtransactionwithwallet(tx.serialize().hex())
         new_tx = CTransaction()
         new_tx.deserialize(BytesIO(hex_str_to_bytes(signed_result['hex'])))
+    else:
+        new_tx = tx
 
-    new_tx.vin[0].scriptSig = CScript([CScriptNum(height), OP_CHECKLOCKTIMEVERIFY, OP_DROP] +
+    new_tx.vin[0].scriptSig = CScript(prepend_scriptsig + list(CScript(new_tx.vin[0].scriptSig)))
-                                  list(CScript(new_tx.vin[0].scriptSig)))
     return new_tx
 
 
+def cltv_invalidate(node, tx, failure_reason):
+    # Modify the signature in vin 0 and nSequence/nLockTime of the tx to fail CLTV
+    #
+    # According to BIP65, OP_CHECKLOCKTIMEVERIFY can fail due the following reasons:
+    # 1) the stack is empty
+    # 2) the top item on the stack is less than 0
+    # 3) the lock-time type (height vs. timestamp) of the top stack item and the
+    #    nLockTime field are not the same
+    # 4) the top stack item is greater than the transaction's nLockTime field
+    # 5) the nSequence field of the txin is 0xffffffff
+    assert failure_reason in range(5)
+    scheme = [
+        # | Script to prepend to scriptSig                  | nSequence  | nLockTime    |
+        # +-------------------------------------------------+------------+--------------+
+        [[OP_CHECKLOCKTIMEVERIFY],                            None,       None],
+        [[OP_1NEGATE, OP_CHECKLOCKTIMEVERIFY, OP_DROP],       None,       None],
+        [[CScriptNum(1000), OP_CHECKLOCKTIMEVERIFY, OP_DROP], 0,          1296688602],  # timestamp of genesis block
+        [[CScriptNum(1000), OP_CHECKLOCKTIMEVERIFY, OP_DROP], 0,          500],
+        [[CScriptNum(500),  OP_CHECKLOCKTIMEVERIFY, OP_DROP], 0xffffffff, 500],
+    ][failure_reason]
+
+    return cltv_modify_tx(node, tx, prepend_scriptsig=scheme[0], nsequence=scheme[1], nlocktime=scheme[2])
+
+
+def cltv_validate(node, tx, height):
+    # Modify the signature in vin 0 and nSequence/nLockTime of the tx to pass CLTV
+    scheme = [[CScriptNum(height), OP_CHECKLOCKTIMEVERIFY, OP_DROP], 0, height]
+
+    return cltv_modify_tx(node, tx, prepend_scriptsig=scheme[0], nsequence=scheme[1], nlocktime=scheme[2])
+
+
 class BIP65Test(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
@@ -66,8 +101,7 @@ class BIP65Test(BitcoinTestFramework):
         self.rpc_timeout = 480
 
     def test_cltv_info(self, *, is_active):
-        assert_equal(self.nodes[0].getblockchaininfo()['softforks']['bip65'],
+        assert_equal(self.nodes[0].getblockchaininfo()['softforks']['bip65'], {
-            {
                 "active": is_active,
                 "height": CLTV_HEIGHT,
                 "type": "buried",
@@ -86,18 +120,22 @@ class BIP65Test(BitcoinTestFramework):
         self.coinbase_txids = [self.nodes[0].getblock(b)['tx'][0] for b in self.nodes[0].generate(CLTV_HEIGHT - 2)]
         self.nodeaddress = self.nodes[0].getnewaddress()
 
-        self.log.info("Test that an invalid-according-to-CLTV transaction can still appear in a block")
+        self.log.info("Test that invalid-according-to-CLTV transactions can still appear in a block")
 
-        spendtx = create_transaction(self.nodes[0], self.coinbase_txids[0],
+        # create one invalid tx per CLTV failure reason (5 in total) and collect them
+        invalid_ctlv_txs = []
+        for i in range(5):
+            spendtx = create_transaction(self.nodes[0], self.coinbase_txids[i],
                                          self.nodeaddress, amount=1.0)
-        cltv_invalidate(spendtx)
+            spendtx = cltv_invalidate(self.nodes[0], spendtx, i)
             spendtx.rehash()
+            invalid_ctlv_txs.append(spendtx)
 
         tip = self.nodes[0].getbestblockhash()
         block_time = self.nodes[0].getblockheader(tip)['mediantime'] + 1
         block = create_block(int(tip, 16), create_coinbase(CLTV_HEIGHT - 1), block_time)
         block.nVersion = 3
-        block.vtx.append(spendtx)
+        block.vtx.extend(invalid_ctlv_txs)
         block.hashMerkleRoot = block.calc_merkle_root()
         block.solve()
 
@@ -118,24 +156,35 @@ class BIP65Test(BitcoinTestFramework):
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip)
             peer.sync_with_ping()
 
-        self.log.info("Test that invalid-according-to-cltv transactions cannot appear in a block")
+        self.log.info("Test that invalid-according-to-CLTV transactions cannot appear in a block")
         block.nVersion = 4
+        block.vtx.append(CTransaction()) # dummy tx after coinbase that will be replaced later
 
-        spendtx = create_transaction(self.nodes[0], self.coinbase_txids[1],
+        # create and test one invalid tx per CLTV failure reason (5 in total)
+        for i in range(5):
+            spendtx = create_transaction(self.nodes[0], self.coinbase_txids[10+i],
                                          self.nodeaddress, amount=1.0)
-        cltv_invalidate(spendtx)
+            spendtx = cltv_invalidate(self.nodes[0], spendtx, i)
             spendtx.rehash()
 
+            expected_cltv_reject_reason = [
+                "non-mandatory-script-verify-flag (Operation not valid with the current stack size)",
+                "non-mandatory-script-verify-flag (Negative locktime)",
+                "non-mandatory-script-verify-flag (Locktime requirement not satisfied)",
+                "non-mandatory-script-verify-flag (Locktime requirement not satisfied)",
+                "non-mandatory-script-verify-flag (Locktime requirement not satisfied)",
+            ][i]
             # First we show that this tx is valid except for CLTV by getting it
             # rejected from the mempool for exactly that reason.
-        assert_raises_rpc_error(-26, 'non-mandatory-script-verify-flag (Negative locktime)', self.nodes[0].sendrawtransaction, spendtx.serialize().hex(), 0)
+            assert_raises_rpc_error(-26, expected_cltv_reject_reason, self.nodes[0].sendrawtransaction, spendtx.serialize().hex(), 0)
 
             # Now we verify that a block with this transaction is also invalid.
-        block.vtx.append(spendtx)
+            block.vtx[1] = spendtx
             block.hashMerkleRoot = block.calc_merkle_root()
             block.solve()
 
-        with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputScripts on {} failed with non-mandatory-script-verify-flag (Negative locktime)'.format(block.vtx[-1].hash)]):
+            with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputScripts on {} failed with {}'.format(
+                                                block.vtx[-1].hash, expected_cltv_reject_reason)]):
                 peer.send_and_ping(msg_block(block))
                 assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip)
                 peer.sync_with_ping()