CLTV: Add more tests to improve coverage #6368

eordano commented at 2:03 AM on July 3, 2015: contributor

This should be a pretty straightforward code review, adding a few tests to the script interpreter when CHECKLOCKTIMEVERIFY is enabled.

The test cases improve coverage for these cases:

The CLTV operand type mismatches the tx locktime. In the script it is 1 (interpreted as block height), but in the tx is 500000000 (interpreted as date)
The stack is empty when executing OP_CLTV
The tx is final by having only one input with MAX_INT sequence number
The operand for CLTV is negative (after OP_0 OP_1 OP_SUB)

laanwj added the label Tests on Jul 3, 2015

in src/test/data/tx_invalid.json:None in 9a759eb36b outdated

 184 | @@ -185,5 +185,21 @@
 185 |  [[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0xc5b93064159b3b2d6ab506a41b1f50463771b988 EQUAL"]],
 186 |  "0100000001000100000000000000000000000000000000000000000000000000000000000000000000030251b1000000000100000000000000000000000000", "P2SH,CHECKLOCKTIMEVERIFY"],
 187 |  
 188 | +["Failure due to non-matching CHECKLOCKTIMEVERIFY kind"],
 189 | +[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1"]],
 190 | +"01000000010001000000000000000000000000000000000000000000000000000000000000000000000251b100000000010000000000000000000065cd1d", "P2SH,CHECKLOCKTIMEVERIFY"],

petertodd commented at 10:54 PM on July 3, 2015:

Seems that this test is nearly identical to the one at https://github.com/eordano/bitcoin/blob/test/cltv/src/test/data/tx_invalid.json#L160, other than being in the scriptSig vs. scriptPubKey, and using 1 as the threshold rather than 0. Is that correct, or am I misunderstanding something?

in src/test/data/tx_invalid.json:None in 9a759eb36b outdated

 189 | +[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1"]],
 190 | +"01000000010001000000000000000000000000000000000000000000000000000000000000000000000251b100000000010000000000000000000065cd1d", "P2SH,CHECKLOCKTIMEVERIFY"],
 191 | +
 192 | +["Failure due to an empty stack for CHECKLOCKTIMEVERIFY"],
 193 | +[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1"]],
 194 | +"010000000100010000000000000000000000000000000000000000000000000000000000000000000001b1010000000100000000000000000000000000", "P2SH,CHECKLOCKTIMEVERIFY"],

petertodd commented at 10:56 PM on July 3, 2015:

Duplicate of https://github.com/eordano/bitcoin/blob/test/cltv/src/test/data/tx_invalid.json#L139, but triggered in the scriptSig instead of scriptPubKey. How about you move this test to be next to that one to keep the similar testcases together?

in src/test/data/tx_invalid.json:None in 9a759eb36b outdated

 193 | +[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1"]],
 194 | +"010000000100010000000000000000000000000000000000000000000000000000000000000000000001b1010000000100000000000000000000000000", "P2SH,CHECKLOCKTIMEVERIFY"],
 195 | +
 196 | +["Failure due to a non-final input in CHECKLOCKTIMEVERIFY"],
 197 | +[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1"]],
 198 | +"01000000010001000000000000000000000000000000000000000000000000000000000000000000000251b1ffffffff0100000000000000000002000000", "P2SH,CHECKLOCKTIMEVERIFY"],

petertodd commented at 10:58 PM on July 3, 2015:

Again, lets move this to be next to https://github.com/eordano/bitcoin/blob/test/cltv/src/test/data/tx_invalid.json#L151 to keep the similar tests together. Very good idea to test this one though with both scriptPubKey and scriptSig versions! Come to think of it, adding a version that's in a redeemScript as well it wouldn't be a bad idea.

in src/test/data/tx_invalid.json:None in 9a759eb36b outdated

 197 | +[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1"]],
 198 | +"01000000010001000000000000000000000000000000000000000000000000000000000000000000000251b1ffffffff0100000000000000000002000000", "P2SH,CHECKLOCKTIMEVERIFY"],
 199 | +
 200 | +["Failure due to a negative locktime in CHECKLOCKTIMEVERIFY"],
 201 | +[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1"]],
 202 | +"010000000100010000000000000000000000000000000000000000000000000000000000000000000004005194b1010000000100000000000000000002000000", "P2SH,CHECKLOCKTIMEVERIFY"],

petertodd commented at 11:02 PM on July 3, 2015:

Lets move this one next to https://github.com/eordano/bitcoin/blob/test/cltv/src/test/data/tx_invalid.json#L147

petertodd commented at 11:02 PM on July 3, 2015: contributor

Thanks for the review!

CLTV: Add more tests to improve coverage

Four cases included:

* The CLTV operand type mismatches the tx locktime. In the script it is
  1 (interpreted as block height), but in the tx is 500000000
  (interpreted as date)
* The stack is empty when executing OP_CLTV
* The tx is final by having only one input with MAX_INT sequence number
* The operand for CLTV is negative (after OP_0 OP_1 OP_SUB)

cb54d17355

petertodd commented at 4:11 AM on July 9, 2015: contributor

Looks good now, ACK

laanwj commented at 8:55 AM on July 9, 2015: member

ACK

laanwj merged this on Jul 9, 2015

laanwj closed this on Jul 9, 2015

laanwj referenced this in commit 7fc25c2e5d on Jul 9, 2015

MarcoFalke locked this on Sep 8, 2021