#32229 tests: Add unix timestamp tests for OP_CLTV and max mediantimepast tests
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #32229 tests: Add unix timestamp tests for OP_CLTV and max mediantimepast tests | Author silent 95d | 0 | Mild | 0 + 107 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
This pull request adds functional test coverage for OP_CHECKLOCKTIMEVERIFY (BIP65) with unix-timestamp locktimes in feature_cltv.py, verifying that blocks are rejected until median-time-past satisfies the locktime. It also adds tests in feature_csv_activation.py for the BIP113 edge case where median-time-past reaches UINT32_MAX, showing that transactions with nLockTime equal to UINT32_MAX can never be mined.
Problem: While unit tests in transaction_tests.cpp test OP_CLTV script verification directly, functional tests do not verify the interaction between BIP65 unix-timestamp locktimes, median-time-past, and block connection. In addition, BIP113 behavior near the uint32 timestamp boundary has no functional test coverage.
Reviewability
Stale · Author silent 95d
No author activity for 95 days; a review may wait a long time for a response.
Waiting on (from the thread read):
- author: follow up on sedited's review comments on test logic and diffs 2026-10-31
- author: response to sedited's review comments and suggestions on feature_cltv.py and feature_csv_activation.py 2026-10-31
Agreement
Mild
objection that needs an answer has none yet (sedited)
Thread notes: Added open suggestions and question from sedited's review comments on 2026-09-16. The PR is waiting on the author to follow up as prompted by sedited in c:6542630385. / Converted c:3991003329, rc:4027208332, and rc:4027859196 from objections to suggestions and questions per schema 2 rules, as they do not name concrete costs of merging as-is. Added suggestions and questions from mabu44 and sedited that were omitted under schema 1; sedited's stance is updated to support accordingly.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| mabu44 (none) | question, usefulness | whether the test covers scenarios that are not already covered in transactions_tests.cpp | accepted · reviewer accepted that this is an integration test exercising median-time-past and block validation | answered | yes | 2025-04-08: 'Does your test cover scenarios that are not covered in the existing tests?' Status from: 2025-04-10: mabu44: 'So this is an integration test of: - Transaction validity based on input script and transaction locktime' |
| mabu44 (none) | question, approach | whether a new time-locked transaction must be created or if spendtx can be reused | answered · author explained why a new tx is needed, leading to mabu44 pointing out duplicate transaction issue | answered | yes | 2025-04-08: 'Do we need to create a new time-locked transaction? Cannot we use the one that is already in spendtx?' Status from: 2025-04-08: Christewart: 'I believe so because the previous one was mined in a valid block here' |
| mabu44 (none) | objection, correctness | a test assertion could pass due to a duplicate transaction rather than the timelock failure being tested | fixed · fix pushed · author moved transaction creation in 024c3bf | changed | yes | 2025-04-10: 'Otherwise, the check at line 218 could succeed not for the timelock (that is what we expect) but because of the duplicated transaction.' Status from: 2025-04-10: Christewart: 'I've integrated your code review in 024c3bf' |
| sedited (member) | objection, approach | general BIP113 MTP/finality tests were misplaced in feature_cltv.py, and existing locktime test coverage was inaccurate in the description | fixed · fix pushed · author split MTP test into feature_csv_activation.py and updated description | answered | yes | 2026-03-03: 'Can you re-formulate the PR description to make it clear what this actually tests? Contrary to your current description, it looks like we are already testing unix timestamp values in the locktime exte' Status from: 2026-07-05: Christewart: 'Split the UINT32_MAX / median-time-past edge case out of feature_cltv.py and into feature_csv_activation.py, since that ' |
| sedited (member) | question, correctness | whether appending another scriptsig to the existing one is required since the test passes without it | fixed · fix pushed · author integrated all review comments in commit 1f05d8e | answered | yes | 2026-07-23: 'If I remove this line, the test still passes. Is it required? Doesn't this also just append another scriptsig to the existing one?' Status from: 2026-07-29: Christewart: 'I believe all your points are integrated into https://github.com/bitcoin/bitcoin/pull/32229/commits/1f05d8e0364fa4b009e558c97e821be70313b8bc' |
| sedited (member) | suggestion, approach | use assert_debug_log to capture the bad-txns-nonfinal rejection reason | fixed · fix pushed · author integrated all review comments in commit 1f05d8e | answered | yes | 2026-07-23: 'As done further above in the file to capture the rejection reason?' Status from: 2026-07-29: Christewart: 'I believe all your points are integrated into https://github.com/bitcoin/bitcoin/pull/32229/commits/1f05d8e0364fa4b009e558c97e821be70313b8bc' |
| sedited (member) | objection, maintenance | unnecessary modifications to lines in feature_cltv.py that do not alter test behavior add review overhead and noise | open | answered | no | 2026-09-16: 'Why is this line changed? Seems like it is not actually changing anything behaviour wise.' |
| sedited (member) | suggestion, maintenance | revert line modification since the test passes without changing it | open | answered | no | 2026-09-16: 'The test passes if this line is left the same, so maybe this should not be changed?' |
| sedited (member) | question, approach | why twelve blocks are needed instead of eleven | open | answered | no | 2026-09-16: 'Why twelve, doesn't this test only need eleven?' |
| sedited (member) | suggestion, approach | derive the locktime directly from mediantime | open | answered | no | 2026-09-16: 'Might it be better to derive this from the mediantime directly?' |
| sedited (member) | objection, correctness | the MTP edge case test in feature_csv_activation.py misses testing that a UINT32_MAX - 1 timelocked tx enters the mempool once MTP maxes out but cannot be mined | open | answered | no | 2026-09-16: 'The interesting case in my eyes is that on the final block, a UINT32_MAX - 1 timelocked transaction is considered standard and enters the mempool, but can't ever be mined.' |
Support:
- sedited (member) (Concept ACK, 2026-05-22) [read: test/functional/feature_cltv.py, test/functional/feature_csv_activation.py]: fine with having a test for documenting the UINT32_MAX MTP calculation edge case
Participants:
- mabu44 (none, objection, 8 statements 2025-04-08 to 2025-04-10): reviewed test logic, questioned scope vs existing tests, and identified a test issue with duplicate transactions that was fixed
- fanquake (member, neutral, 2 statements 2025-12-03 to 2026-04-30): cc'd reviewers and prompted the author for follow-up
- sedited (member, objection, 13 statements 2026-03-03 to 2026-10-31): Concept ACKed, but raised concerns on test scope, placement of BIP113 tests, unexplained diffs, and incomplete UINT32_MAX edge-case coverage
Files
114 lines under test/bench/ci.
- test/functional/feature_csv_activation.py +66/-2
- test/functional/feature_cltv.py +41/-5
Card
The short text the ranking pass compares across PRs.
Adds functional test coverage in feature_cltv.py and feature_csv_activation.py for OP_CHECKLOCKTIMEVERIFY with unix-timestamp locktimes and BIP113 median-time-past edge cases up to UINT32_MAX. It ensures blocks with CLTV-locked outputs are rejected until median-time-past passes the locktime, and confirms transactions locked to UINT32_MAX cannot be confirmed once MTP reaches the maximum uint32 value. Review is in a good state with a Concept ACK from sedited, who recently suggested refining the UINT32_MAX test cases.
Data
thread read through 2026-10-31 (29 statements), head 1f05d8e036
labels: Tests