CLTV: Add more tests to improve coverage #6368

Merged
merged 1 commit into from Jul 9, 2015

Conversation

Projects
None yet
3 participants
@eordano
Contributor

eordano commented Jul 3, 2015

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 laanwj added the Tests label Jul 3, 2015

@petertodd

View changes

src/test/data/tx_invalid.json
@@ -185,5 +185,21 @@
[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "HASH160 0x14 0xc5b93064159b3b2d6ab506a41b1f50463771b988 EQUAL"]],
"0100000001000100000000000000000000000000000000000000000000000000000000000000000000030251b1000000000100000000000000000000000000", "P2SH,CHECKLOCKTIMEVERIFY"],
+["Failure due to non-matching CHECKLOCKTIMEVERIFY kind"],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1"]],
+"01000000010001000000000000000000000000000000000000000000000000000000000000000000000251b100000000010000000000000000000065cd1d", "P2SH,CHECKLOCKTIMEVERIFY"],

This comment has been minimized.

@petertodd

petertodd Jul 3, 2015

Contributor

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?

@petertodd

petertodd Jul 3, 2015

Contributor

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?

@petertodd

View changes

src/test/data/tx_invalid.json
+
+["Failure due to an empty stack for CHECKLOCKTIMEVERIFY"],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1"]],
+"010000000100010000000000000000000000000000000000000000000000000000000000000000000001b1010000000100000000000000000000000000", "P2SH,CHECKLOCKTIMEVERIFY"],

This comment has been minimized.

@petertodd

petertodd Jul 3, 2015

Contributor

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?

@petertodd

petertodd Jul 3, 2015

Contributor

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?

@petertodd

View changes

src/test/data/tx_invalid.json
+
+["Failure due to a non-final input in CHECKLOCKTIMEVERIFY"],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1"]],
+"01000000010001000000000000000000000000000000000000000000000000000000000000000000000251b1ffffffff0100000000000000000002000000", "P2SH,CHECKLOCKTIMEVERIFY"],

This comment has been minimized.

@petertodd

petertodd Jul 3, 2015

Contributor

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.

@petertodd

petertodd Jul 3, 2015

Contributor

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.

@petertodd

View changes

src/test/data/tx_invalid.json
+
+["Failure due to a negative locktime in CHECKLOCKTIMEVERIFY"],
+[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "1"]],
+"010000000100010000000000000000000000000000000000000000000000000000000000000000000004005194b1010000000100000000000000000002000000", "P2SH,CHECKLOCKTIMEVERIFY"],

This comment has been minimized.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jul 3, 2015

Contributor

Thanks for the review!

Contributor

petertodd commented Jul 3, 2015

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)
@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jul 9, 2015

Contributor

Looks good now, ACK

Contributor

petertodd commented Jul 9, 2015

Looks good now, ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 9, 2015

Member

ACK

Member

laanwj commented Jul 9, 2015

ACK

@laanwj laanwj merged commit cb54d17 into bitcoin:master Jul 9, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 9, 2015

Merge pull request #6368
cb54d17 CLTV: Add more tests to improve coverage (Esteban Ordano)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment