tests: Add unix timestamp tests for OP_CLTV #32229

pull Christewart wants to merge 3 commits into bitcoin:master from Christewart:2025-04-06-locktime-tests changing 1 files +66 −5
  1. Christewart commented at 4:44 pm on April 7, 2025: contributor

    This PR adds tests for the UNIX timestamp portion of BIP65. I don’t believe this is currently tested in the python test suite (not sure about c++).

    Apologies if this is actually tested somewhere else and this is duplicate.

    There is some interesting interplay between median-time-past consensus rules (BIP113) and lock times. This PR attempts to test that logic as well - for instance I don’t believe it is possible to spend a UTXO that has a output script with locktime 0xffff_ffff or 0xffff_fffe

    Unfortunately it does seem like these tests aren’t necessarily always testing OP_CLTV logic as median-time-past ends up being the reason blocks cannot be mined for edge cases.

  2. DrahtBot commented at 4:44 pm on April 7, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32229.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

  3. DrahtBot added the label Tests on Apr 7, 2025
  4. Christewart force-pushed on Apr 7, 2025
  5. DrahtBot added the label CI failed on Apr 7, 2025
  6. Christewart force-pushed on Apr 8, 2025
  7. DrahtBot removed the label CI failed on Apr 8, 2025
  8. in test/functional/feature_cltv.py:210 in 69b6bb9438 outdated 
    205+        tip = block.sha256
206+
207+        current_time = int(time.time())
208+        locktime = current_time + (60 * 60) # 1 hour
209+
210+        self.log.info("Test that one block mined with correct wall clock time does not satisfy meadian-time-past")
    mabu44 commented at 8:21 am on April 8, 2025:
    nit: typo “meadian”
  9. in test/functional/feature_cltv.py:230 in 69b6bb9438 outdated 
    225+            peer.send_and_ping(msg_block(b))
226+            tip = b.sha256
227+
228+        # now our median-past-time should allow for a transaction with
229+        # a locktime of +1 hour to be confirmed
230+        spendtx = wallet.create_self_transfer()['tx']
    mabu44 commented at 8:27 am on April 8, 2025:
    Do we need to create a new time-locked transaction? Cannot we use the one that is already in spendtx?
    Christewart commented at 4:26 pm on April 8, 2025:
    I believe so because the previous one was mined in a valid block here: https://github.com/Christewart/bitcoin/blob/69b6bb9438095f84ce90aecfc4e76b0ff21cc2b8/test/functional/feature_cltv.py#L213
    mabu44 commented at 11:44 am on April 10, 2025:
    This tx was mined in line 195, not 213. I would move the creation of the transaction from line 230 to line 209. Otherwise, the check at line 218 could succeed not for the timelock (that is what we expect) but because of the duplicated transaction.
  10. in test/functional/feature_cltv.py:247 in 69b6bb9438 outdated 
    242+        UINT32_MAX = 2**32 - 1
243+        self.nodes[0].setmocktime(UINT32_MAX)
244+        # need to re-add connection as setting mocktime times out the p2p connection
245+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
246+        tip = block2.sha256
247+        # 6 blocks are allowed to have UINT32_MAX block time
    mabu44 commented at 8:43 am on April 8, 2025:
    Here you are testing what will happen in the year 2106 if no changes are deployed in the meantime. Right?
  11. mabu44 commented at 12:01 pm on April 8, 2025: none
    The Unix-timestamp portion of BIP65 is currently tested in the c++ unit tests in “transactions_tests.cpp”. Test data includes transactions that have OP_CLTV with Unix timestamps (the files are /src/test/data/tx_valid.json and tx_invalid.json). Does your test cover scenarios that are not covered in the existing tests?
  12. Christewart commented at 4:23 pm on April 8, 2025: contributor

    Hi @mabu44 . Thanks for taking a look at this.

    Do your test cover scenarios that are not covered in the existing tests?

    Yes I believe so. I took a look at the values in tx_valid.json and tx_invalid.json. Here is a table

    OP_CLTV parameter Transaction locktime field Valid
    0 0 true
    499999999 499999999 true
    0 499999999 true
    500000000 500000000 true
    4294967295 4294967295 true
    500000000 4294967295 true
    0 0 true
    -11 500000000 true
    -19 4294967294 true
    0 0 true
    1 0 false
    499999999 499999998 false
    500000001 500000000 false
    4294967295 4294967294 false
    -1 0 false
    -1 500000000 false
    0 0 false
    0 0 false
    0 500000000 false
    499999999 500000000 false
    500000000 0 false
    500000000 499999999 false
    4294967296 4294967295 false
    2147483648 2147483647 false
    0 0 false

    This test also directly tests the median-time-past logic. This is not tested in tx_{valid,invalid}.json as we just directly call VerifyScript() for individual transactions in the test files.

    Interestingly, i think there may be one test case overlap with tx_valid.json. While this transaction could be valid with relay policy I don’t believe a transaction with these locktimes would be allowed to be included in a block? This is because median-time-past logic for a block with this transaction could be not be satisfied as per my last test case here.

    | 4294967295 | 4294967295 | true |

    Some more eyes on this could be good as I may be making a mistake.

  13. tests: Add unix timestamp tests for OP_CLTV 062c31b72e
  14. Christewart force-pushed on Apr 8, 2025
  15. Add assertion to check that the max locktime satisfiable under BIP113 is UINT32_MAX - 2 01d2068fb6
  16. in test/functional/feature_cltv.py:232 in 01d2068fb6 outdated 
    227+
228+        # now our median-past-time should allow for a transaction with
229+        # a locktime of +1 hour to be confirmed
230+        spendtx = wallet.create_self_transfer()['tx']
231+        cltv_validate(spendtx,locktime)
232+        block2.vtx.append(spendtx)
    mabu44 commented at 11:46 am on April 10, 2025:
    This line can be removed
  17. in test/functional/feature_cltv.py:247 in 01d2068fb6 outdated 
    242+        UINT32_MAX = 2**32 - 1
243+        self.nodes[0].setmocktime(UINT32_MAX)
244+        # need to re-add connection as setting mocktime times out the p2p connection
245+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
246+        tip = block2.sha256
247+        # 6 blocks are allowed to have UINT32_MAX block time
    mabu44 commented at 11:47 am on April 10, 2025:
    This comment was not updated to reflect what the code is doing (same for the comments at line 256 and 258).
  18. mabu44 commented at 12:03 pm on April 10, 2025: none

    So this is an integration test of:

    with some more edge cases relative to the year 2106. Right?

    Interestingly, i think there may be one test case overlap with tx_valid.json. While this transaction could be valid with relay policy I don’t believe a transaction with these locktimes would be allowed to be included in a block? This is because median-time-past logic for a block with this transaction could be not be satisfied as per my last test case here.

    | 4294967295 | 4294967295 | true |

    I think that such a transaction will become relay-valid in 2106, but, unless consensus changes, it will never be mined because no blocks will be produced to mine it.

  19. Address codreview from mabu44 024c3bf3c7
  20. Christewart commented at 5:11 pm on April 10, 2025: contributor

    Thank you again for the careful review. I’ve integrated your code review in 024c3bf

    So this is an integration test of:

    * _Transaction validity based on input script and transaction locktime_: currently tested in [transactions_tests.cpp](https://github.com/bitcoin/bitcoin/blob/master/src/test/transaction_tests.cpp);

* _Transaction validity based on median block time_: currently tested in [miner_tests.cpp (lines 525-534)](https://github.com/bitcoin/bitcoin/blob/b8cefeb221490868d62b7a0695f8b5ea392d3654/src/test/miner_tests.cpp#L525-L534) and [feature_csv_activation.py (lines 340-355)](https://github.com/bitcoin/bitcoin/blob/b8cefeb221490868d62b7a0695f8b5ea392d3654/test/functional/feature_csv_activation.py#L340-L355)

    with some more edge cases relative to the year 2106. Right?

    Yes.

    Interestingly, i think there may be one test case overlap with tx_valid.json. While this transaction could be valid with relay policy I don’t believe a transaction with these locktimes would be allowed to be included in a block? This is because median-time-past logic for a block with this transaction could be not be satisfied as per my last test case here.

    | 4294967295 | 4294967295 | true |

    I think that such a transaction will become relay-valid in 2106, but, unless consensus changes, it will never be mined because no blocks will be produced to mine it.

    That is my understanding as well :+1:


Christewart DrahtBot mabu44

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-04-16 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me