Skip to content

Conversation

@theStack
Copy link
Contributor

@theStack theStack commented Aug 25, 2020

The functional test for BIP65 / 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:

def cltv_invalidate(tx):
'''Modify the signature in vin 0 of the tx to fail CLTV
Prepends -1 CLTV DROP in the scriptSig itself.
TODO: test more ways that transactions using CLTV could be invalid (eg
locktime requirements fail, sequence time requirements fail, etc).
'''
tx.vin[0].scriptSig = CScript([OP_1NEGATE, OP_CHECKLOCKTIMEVERIFY, OP_DROP] +
list(CScript(tx.vin[0].scriptSig)))

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").

@fanquake fanquake added the Tests label Aug 25, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 25, 2020

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@instagibbs
Copy link
Member

concept ACK

@theStack
Copy link
Contributor Author

Rebased on master.

@theStack theStack force-pushed the 20200825-test-check-all-failure-reasons-for-CLTV branch from 0bf3040 to ebfc246 Compare September 27, 2020 18:19
@theStack
Copy link
Contributor Author

Rebased on master again.

@theStack theStack force-pushed the 20200825-test-check-all-failure-reasons-for-CLTV branch from ebfc246 to b01cd94 Compare March 31, 2021 19:41
@theStack
Copy link
Contributor Author

Rebased on master.

@sipa
Copy link
Member

sipa commented Mar 31, 2021

Concept ACK

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review ACK b01cd94 🐣

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK b01cd9471f435bb36b8ed5211a56baad51111ad2 🐣
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiGwwv/ehsKGNFfYIyNn3Pb7H8mC7j6SfFijNHeg+rDldGw1lNA3MLjMvPGhs4v
+uuAbS74JCv4T6fEjw8K4ODx6ZU/+fyckT8HL2Qxx9TbqPN9+3Gc5Ww68137+RRW
mOzNy1S4Z6MPu3OtBZxkHSKHXPKF6sSTDyqjrxgslpOFNDceirBIahFTHPXy2geh
gZYLsQYtbY0kDJBLcfCh4vvPW5IpLO2Cr5iRQkVmj31upzvYniKmWtuCwmwrXZG9
mAKP0o9AGnfYWCufH1NpDBp4/JBrRITk+nz0bOgxYO99FiJMalX+YTP6xlkOIGgI
ChrXmz7/qe3FXypz9lA+hL44R7/ApLWhCfc4oK9bbHwSVqbjnTh5V20+EjYSK3ER
5IPjBDjbTN1768iQMRIjq2Aj9zQFrVwCkzlW56zwlECZoFHGEzDOWimHkVWd1vZR
qSvVIya8GmuqwWpEASi9O5yXArtqF+Vzr7s9Ki8TiTbGFOQFFlNFLgTXkOXi4Mrr
DC1R6djf
=TLhB
-----END PGP SIGNATURE-----

Timestamp of file with hash 0aea310db82e5dcfdbcc25d7e801d24635a1a608c8b997bbb98494c524f90747 -

@maflcko maflcko merged commit 4b5659c into bitcoin:master Apr 22, 2021
@maflcko
Copy link
Member

maflcko commented Apr 22, 2021

Reworked the test some more in #21754

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 22, 2021
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants