test: add CLTV and CHECK(MULTI)SIGVERIFY failure-path vectors to script

azuchi commented at 4:53 AM on July 6, 2026: none

While reviewing spec coverage of src/test/data/script_tests.json against the script interpreter, I found two gaps that are testable within this file's harness but were never covered:

1. OP_CHECKSIGVERIFY / OP_CHECKMULTISIGVERIFY failure paths

OP_CHECKSIGVERIFY never appears anywhere in the file, and no vector expects the CHECKSIGVERIFY or CHECKMULTISIGVERIFY script errors, so the VERIFY tail of both opcodes (interpreter.cpp, case OP_CHECKSIGVERIFY) is untested here. This commit adds static vectors that fail the signature check with an empty signature and a valid pubkey, so each opcode returns its opcode-specific error code. The success paths require real signatures and remain covered by the auto-generated tests and functional tests.

2. CHECKLOCKTIMEVERIFY (BIP65) failure paths

SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY is never set by any vector: CHECKLOCKTIMEVERIFY only appears as an unflagged NOP, so none of the BIP65 semantics are exercised, while the equivalent CHECKSEQUENCEVERIFY section has existed since #7994. This commit adds a section mirroring the CSV tests, covering every failure path reachable in this harness:

empty stack → INVALID_STACK_OPERATION
negative operand → NEGATIVE_LOCKTIME
negative zero (0x80), evaluated as 0 by CScriptNum → UNSATISFIED_LOCKTIME rather than NEGATIVE_LOCKTIME
non-minimal encoding under MINIMALDATA → SCRIPTNUM
final input nSequence (lock time requirement itself satisfied) → UNSATISFIED_LOCKTIME
operand greater than the tx nLockTime → UNSATISFIED_LOCKTIME
height/time type mismatch → UNSATISFIED_LOCKTIME
5-byte operand (2^32) accepted by the parser, then failing the type check → UNSATISFIED_LOCKTIME

Unlike CSV (where an operand with bit 31 set makes the opcode pass without calling CheckSequence), the CLTV success path cannot be expressed in this file, because the test harness spends with nLockTime=0 and a final nSequence; it is covered by tx_valid.json and functional tests instead. A comment in the JSON notes this.

3. Negative zero vector for the existing CHECKSEQUENCEVERIFY section Following review feedback, the third commit adds the same negative-zero vector to the existing CSV section: the footgun is identical there (a re-implementation treating any operand with the sign bit set as negative would return NEGATIVE_LOCKTIME instead of reaching CheckSequence), and it keeps the two sections mirrored.

test: add CHECKSIGVERIFY/CHECKMULTISIGVERIFY failure script test vectors a86a96d17b

DrahtBot added the label Tests on Jul 6, 2026

DrahtBot commented at 4:53 AM on July 6, 2026: 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/35664.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Stale ACK	sedited, darosior

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

in src/test/data/script_tests.json:2586 in 88b926f1c5 outdated

2581 | +["0x0100", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY,MINIMALDATA", "SCRIPTNUM", "CLTV fails if stack top is not minimally encoded"],
2582 | +["0", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "UNSATISFIED_LOCKTIME", "CLTV fails if the input's nSequence is final, even when the lock time requirement is met"],
2583 | +["499999999", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "UNSATISFIED_LOCKTIME", "CLTV fails if the stack operand (height) is greater than the tx nLockTime"],
2584 | +["500000000", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "UNSATISFIED_LOCKTIME", "CLTV fails if the operand is time-based while the tx nLockTime is height-based"],
2585 | +["0x050000000001", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "UNSATISFIED_LOCKTIME",
2586 | + "CLTV accepts a 5-byte operand (2^32), but it is time-based while the tx nLockTime is height-based"],

sedited commented at 1:02 PM on July 6, 2026:

I'm not sure about adding these three here. Are they actually exercising the logic as described the comments and not just failing on the first condition in CheckLockTime?

azuchi commented at 1:19 AM on July 7, 2026:

Not quite — between them (together with the "0" vector just above) all three return false branches of CheckLockTime are exercised. With this harness the spending tx has nLockTime=0 (height-type) and a final nSequence, and CheckLockTime checks, in order: (1) type mismatch, (2) numeric comparison, (3) final nSequence.

499999999 is height-type like the tx nLockTime, so it passes the first condition and fails on the second one (the numeric comparison, 499999999 > 0).
500000000 fails on the first condition, which is exactly what its comment describes (time-type operand vs height-type tx nLockTime).
0x050000000001 does fail on the same first condition as the previous vector, but its purpose is different: it checks that CLTV's special 5-byte CScriptNum (interpreter.cpp: CScriptNum(stacktop(-1), fRequireMinimal, 5)) accepts an operand beyond the 4-byte range instead of failing to parse, mirroring the existing 0x050000000001 vector in the CHECKSEQUENCEVERIFY section.
The "0" vector above passes both the type check and the numeric comparison and fails only on the final-nSequence condition.

Happy to reword the comment on the 5-byte vector to make its intent clearer, e.g. "CLTV accepts a 5-byte operand (2^32 = 4294967296), which normal arithmetic opcodes would reject; being >= 500000000 it is then treated as time-based and mismatches the height-based tx nLockTime".

sedited commented at 9:40 AM on July 7, 2026:

Thanks for enumerating this for me. Read through this again, and I think the comments are fine actually.

sedited commented at 1:02 PM on July 6, 2026: contributor

Concept ACK

Edit: Related to #32229

sedited approved

sedited commented at 9:41 AM on July 7, 2026: contributor

ACK 88b926f1c584db854b519a4b1293aed7ae38cfd8

in src/test/data/script_tests.json:2580 in 88b926f1c5 outdated

2572 | @@ -2573,6 +2573,18 @@
2573 |  ["0", "0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 CHECKSIGVERIFY 1", "P2SH,STRICTENC", "CHECKSIGVERIFY", "CHECKSIGVERIFY fails with its own error code when the signature check fails"],
2574 |  ["0 0", "1 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 1 CHECKMULTISIGVERIFY 1", "P2SH,STRICTENC", "CHECKMULTISIGVERIFY", "CHECKMULTISIGVERIFY fails with its own error code when the signature check fails"],
2575 |  
2576 | +["CHECKLOCKTIMEVERIFY tests"],
2577 | +["All tests below can only exercise failure paths: the spending transaction in these tests"],
2578 | +["has nLockTime 0 and a final nSequence, so CheckLockTime never succeeds."],
2579 | +["", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "INVALID_STACK_OPERATION", "CLTV automatically fails on an empty stack"],
2580 | +["-1", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "NEGATIVE_LOCKTIME", "CLTV automatically fails if stack top is negative"],

darosior commented at 1:43 PM on July 7, 2026:

A test with the negative zero 0x80 could be interesting to have here. (Especially since this test is mostly useful to re-implementations of Bitcoin, and this is a common footgun.)

azuchi commented at 12:59 AM on July 8, 2026:

Good idea, added in 37edf0e. CScriptNum evaluates the negative zero 0x80as 0, so it passes the nLockTime < 0 check and fails inside CheckLockTime with UNSATISFIED_LOCKTIME rather than NEGATIVE_LOCKTIME — which is exactly the behavior a naive "sign bit set means negative" re-implementation would get wrong.
The same footgun applies to CHECKSEQUENCEVERIFY, so I also added the equivalent vector to that section in a separate commit c4068cf.

in src/test/data/script_tests.json:2581 in 88b926f1c5

2572 | @@ -2573,6 +2573,18 @@
2573 |  ["0", "0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 CHECKSIGVERIFY 1", "P2SH,STRICTENC", "CHECKSIGVERIFY", "CHECKSIGVERIFY fails with its own error code when the signature check fails"],
2574 |  ["0 0", "1 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 1 CHECKMULTISIGVERIFY 1", "P2SH,STRICTENC", "CHECKMULTISIGVERIFY", "CHECKMULTISIGVERIFY fails with its own error code when the signature check fails"],
2575 |  
2576 | +["CHECKLOCKTIMEVERIFY tests"],
2577 | +["All tests below can only exercise failure paths: the spending transaction in these tests"],
2578 | +["has nLockTime 0 and a final nSequence, so CheckLockTime never succeeds."],
2579 | +["", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "INVALID_STACK_OPERATION", "CLTV automatically fails on an empty stack"],
2580 | +["-1", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "NEGATIVE_LOCKTIME", "CLTV automatically fails if stack top is negative"],
2581 | +["0x0100", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY,MINIMALDATA", "SCRIPTNUM", "CLTV fails if stack top is not minimally encoded"],

darosior commented at 1:47 PM on July 7, 2026:

nit: maybe specify in the description that it's only a standardness check. "CLTV use is non-standard if stack top is not minimally encoded".

azuchi commented at 12:56 AM on July 8, 2026:

Done in 37edf0e, reworded as suggested.

in src/test/data/script_tests.json:2582 in 88b926f1c5 outdated

2577 | +["All tests below can only exercise failure paths: the spending transaction in these tests"],
2578 | +["has nLockTime 0 and a final nSequence, so CheckLockTime never succeeds."],
2579 | +["", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "INVALID_STACK_OPERATION", "CLTV automatically fails on an empty stack"],
2580 | +["-1", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "NEGATIVE_LOCKTIME", "CLTV automatically fails if stack top is negative"],
2581 | +["0x0100", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY,MINIMALDATA", "SCRIPTNUM", "CLTV fails if stack top is not minimally encoded"],
2582 | +["0", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "UNSATISFIED_LOCKTIME", "CLTV fails if the input's nSequence is final, even when the lock time requirement is met"],

darosior commented at 1:48 PM on July 7, 2026:

Neat

in src/test/data/script_tests.json:2586 in 88b926f1c5 outdated

2581 | +["0x0100", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY,MINIMALDATA", "SCRIPTNUM", "CLTV fails if stack top is not minimally encoded"],
2582 | +["0", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "UNSATISFIED_LOCKTIME", "CLTV fails if the input's nSequence is final, even when the lock time requirement is met"],
2583 | +["499999999", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "UNSATISFIED_LOCKTIME", "CLTV fails if the stack operand (height) is greater than the tx nLockTime"],
2584 | +["500000000", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "UNSATISFIED_LOCKTIME", "CLTV fails if the operand is time-based while the tx nLockTime is height-based"],
2585 | +["0x050000000001", "CHECKLOCKTIMEVERIFY", "CHECKLOCKTIMEVERIFY", "UNSATISFIED_LOCKTIME",
2586 | + "CLTV accepts a 5-byte operand (2^32), but it is time-based while the tx nLockTime is height-based"],

darosior commented at 1:51 PM on July 7, 2026:

Hmm. Untested, but i think a height that meets the timelock requirement but is non-minimally encoded on 5 bytes would be valid by consensus, and could be an interesting test case to have here.

darosior commented at 1:54 PM on July 7, 2026:

Oh but it would require a different spending transaction in the test. I don't mean to scope creep this, no need to add it here.

If someone wants to do that as a followup, feel free to ping me for review.

darosior approved

darosior commented at 1:54 PM on July 7, 2026: member

utACK 88b926f1c584db854b519a4b1293aed7ae38cfd8

Some suggestions for improvements in the comments, but this change is already a net-benefit as is.

test: add CHECKLOCKTIMEVERIFY failure-path script test vectors 37edf0e233

azuchi force-pushed on Jul 8, 2026

test: add negative zero CSV failure script test vector c4068cf37b

DrahtBot added the label CI failed on Jul 8, 2026

DrahtBot removed the label CI failed on Jul 8, 2026

test: add CLTV and CHECK(MULTI)SIGVERIFY failure-path vectors to script_tests.json #35664

Code Coverage & Benchmarks

Reviews