New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: script_error checking in script_invalid tests #7517

Merged
merged 3 commits into from Mar 14, 2016

Conversation

Projects
None yet
3 participants
@laanwj
Member

laanwj commented Feb 11, 2016

Check the returned script_error. Add expected script_error for generated as well as custom tests.

The specific error is not part of consensus, however it could avoid unclear reporting issues such as #6862 in the future. It also makes it easier to verify that all potential error conditions are checked.

It also reveals that overflow conditions return UNKNOWN_ERROR, there appears to be no specific error code for that.

Fixes #7513.

Changes are best reviewed using git --word-diff.

test: Move non-generated script_invalid test to the correct place
This test was introduced in 9fadf1c,
but accidentally added in the autogenerated area.

@laanwj laanwj added the Tests label Feb 11, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 11, 2016

Member

These are not returned in any of the script tests:

  • SCRIPT_ERR_CHECKMULTISIGVERIFY (OP_CHECKMULTISIGVERIFY)
  • SCRIPT_ERR_CHECKSIGVERIFY (OP_CHECKSIGVERIFY)
  • SCRIPT_ERR_NUMEQUALVERIFY (OP_NUMEQUALVERIFY)
  • SCRIPT_ERR_NEGATIVE_LOCKTIME (OP_CHECKLOCKTIMEVERIFY - BIP65)
  • SCRIPT_ERR_UNSATISFIED_LOCKTIME (OP_CHECKLOCKTIMEVERIFY - BIP65)
Member

laanwj commented Feb 11, 2016

These are not returned in any of the script tests:

  • SCRIPT_ERR_CHECKMULTISIGVERIFY (OP_CHECKMULTISIGVERIFY)
  • SCRIPT_ERR_CHECKSIGVERIFY (OP_CHECKSIGVERIFY)
  • SCRIPT_ERR_NUMEQUALVERIFY (OP_NUMEQUALVERIFY)
  • SCRIPT_ERR_NEGATIVE_LOCKTIME (OP_CHECKLOCKTIMEVERIFY - BIP65)
  • SCRIPT_ERR_UNSATISFIED_LOCKTIME (OP_CHECKLOCKTIMEVERIFY - BIP65)

laanwj added some commits Feb 11, 2016

test: Script_error checking in script_invalid tests
Check the returned script_error. Add expected script_error
for generated as well as custom tests.

The specific error is not part of consensus, however
it could avoid unclear reporting issues such as #6862 in the future.

Fixes #7513.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj
Member

laanwj commented Mar 1, 2016

@laanwj laanwj closed this Mar 2, 2016

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Mar 3, 2016

Member

@laanwj Closed on purpose? I didn't see this before, but sounds like a good idea for sure.

Member

theuni commented Mar 3, 2016

@laanwj Closed on purpose? I didn't see this before, but sounds like a good idea for sure.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 3, 2016

Member

@theuni Closed because I've been asking for review a few times for this, but never received any answer (neither positive nor negative) so I was assuming it was too much review work relative to some extra testing coverage.

Happy to reopen though!

Member

laanwj commented Mar 3, 2016

@theuni Closed because I've been asking for review a few times for this, but never received any answer (neither positive nor negative) so I was assuming it was too much review work relative to some extra testing coverage.

Happy to reopen though!

@laanwj laanwj reopened this Mar 3, 2016

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 5, 2016

Can't you use ScriptErrorString() instead of this array that needs separate maintainance?

sipa commented on src/test/script_tests.cpp in 0ecb340 Mar 5, 2016

Can't you use ScriptErrorString() instead of this array that needs separate maintainance?

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 6, 2016

Owner

ScriptErrorString returns a more extended, human readable error message. Personally I prefer these constant names:

  • Human-readable messages may change over time, would have to update every JSON testcase
  • Easy to make a typo
  • Looks really verbose to include the extended message on every JSON testcase
Owner

laanwj replied Mar 6, 2016

ScriptErrorString returns a more extended, human readable error message. Personally I prefer these constant names:

  • Human-readable messages may change over time, would have to update every JSON testcase
  • Easy to make a typo
  • Looks really verbose to include the extended message on every JSON testcase

@laanwj laanwj merged commit 0ecb340 into bitcoin:master Mar 14, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Mar 14, 2016

Merge #7517: test: script_error checking in script_invalid tests
0ecb340 test: Script_error checking in script_invalid tests (Wladimir J. van der Laan)
2317ad7 test: Re-introduce JSON pretty printing in test builder (Wladimir J. van der Laan)
b0ff857 test: Move non-generated script_invalid test to the correct place (Wladimir J. van der Laan)

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request May 4, 2016

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 12, 2016

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 12, 2016

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 13, 2016

@dgenr8 dgenr8 referenced this pull request Jul 16, 2017

Merged

Script testing improvements #217

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment