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

Adding P2SH(p2pkh) script test case #8090

Merged
merged 1 commit into from May 31, 2016

Conversation

Projects
None yet
6 participants
@Christewart
Contributor

Christewart commented May 23, 2016

The happy path for a p2sh script with redeem script type p2pkh was previously untested, adding a test case for this inside of script_tests.json

@jonasschnelli jonasschnelli added the Tests label May 23, 2016

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 24, 2016

Contributor

Please squash into one commit (see 90963e5#diff-6a3371457528722a734f3c51d9238c13R46).

Contributor

paveljanik commented May 24, 2016

Please squash into one commit (see 90963e5#diff-6a3371457528722a734f3c51d9238c13R46).

@Christewart

This comment has been minimized.

Show comment
Hide comment
@Christewart

Christewart May 24, 2016

Contributor

Not sure why Travis CI failed on this build - I don't know Travis CI that well but it looks like it is unrelated to my change?

Contributor

Christewart commented May 24, 2016

Not sure why Travis CI failed on this build - I don't know Travis CI that well but it looks like it is unrelated to my change?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 24, 2016

Member

Agree that the failure looks unrelated. But yikes, that looks like a somewhat scary error that was stumbled upon, will investigate...

Member

sdaftuar commented May 24, 2016

Agree that the failure looks unrelated. But yikes, that looks like a somewhat scary error that was stumbled upon, will investigate...

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 24, 2016

Contributor

ACK 32833a4

Travis failure is indeed unrelated.

Contributor

paveljanik commented May 24, 2016

ACK 32833a4

Travis failure is indeed unrelated.

@Christewart

This comment has been minimized.

Show comment
Hide comment
@Christewart

Christewart May 25, 2016

Contributor

@paveljanik @sdaftuar Is there a way to restart this travis build?

Contributor

Christewart commented May 25, 2016

@paveljanik @sdaftuar Is there a way to restart this travis build?

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 25, 2016

Contributor

Lokks like it is already done - all checks have passed.

Contributor

paveljanik commented May 25, 2016

Lokks like it is already done - all checks have passed.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 25, 2016

Member

@Christewart Please don't add manually created test cases inside the "automically generated test cases" block, as that block gets overwritten.

Ideally, add this as an automatically generated test here: https://github.com/bitcoin/bitcoin/blob/v0.12.1/src/test/script_tests.cpp#L353

Member

sipa commented May 25, 2016

@Christewart Please don't add manually created test cases inside the "automically generated test cases" block, as that block gets overwritten.

Ideally, add this as an automatically generated test here: https://github.com/bitcoin/bitcoin/blob/v0.12.1/src/test/script_tests.cpp#L353

@laanwj

View changes

Show outdated Hide outdated src/test/script_tests.cpp
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 30, 2016

Member

Sorry for this taking so long, just moving the test case would have been fine, but now that you change script_tests.cpp you need to make sure it doesn't generate the new json every time.

Member

laanwj commented May 30, 2016

Sorry for this taking so long, just moving the test case would have been fine, but now that you change script_tests.cpp you need to make sure it doesn't generate the new json every time.

Adding P2SH(p2pkh) script test case
Fixing formatting

Adding test case into automatically generated test case set

Clean up commits

removing extra whitespace from eol

Removing extra whitespace on macro line
@Christewart

This comment has been minimized.

Show comment
Hide comment
@Christewart

Christewart May 30, 2016

Contributor

@laanwj Should be fixed - I always seem to add unnecessary spaces/new lines without realizing it in my commits :-)

Contributor

Christewart commented May 30, 2016

@laanwj Should be fixed - I always seem to add unnecessary spaces/new lines without realizing it in my commits :-)

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 30, 2016

Member

utACK b682960

Member

sipa commented May 30, 2016

utACK b682960

@@ -427,7 +427,10 @@ BOOST_AUTO_TEST_CASE(script_build)
tests.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0C) << OP_CHECKSIG,
"P2SH(P2PK), bad redeemscript", SCRIPT_VERIFY_P2SH, true
).PushSig(keys.key0).PushRedeem().DamagePush(10).ScriptError(SCRIPT_ERR_EVAL_FALSE));

This comment has been minimized.

@paveljanik

paveljanik May 30, 2016

Contributor

Pure cosmetic nit: you have added tab here.

@paveljanik

paveljanik May 30, 2016

Contributor

Pure cosmetic nit: you have added tab here.

This comment has been minimized.

@laanwj

laanwj May 31, 2016

Member

Nit^2: I think it's not a tab but four spaces 🐌

@laanwj

laanwj May 31, 2016

Member

Nit^2: I think it's not a tab but four spaces 🐌

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 30, 2016

Contributor

utACK b682960

Contributor

paveljanik commented May 30, 2016

utACK b682960

@laanwj laanwj merged commit b682960 into bitcoin:master May 31, 2016

1 check passed

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

laanwj added a commit that referenced this pull request May 31, 2016

Merge #8090: Adding P2SH(p2pkh) script test case
b682960 Adding P2SH(p2pkh) script test case (Chris Stewart)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 31, 2016

Member

Not going to hold this up on a cosmetic nit.

Member

laanwj commented May 31, 2016

Not going to hold this up on a cosmetic nit.

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #8090: Adding P2SH(p2pkh) script test case
b682960 Adding P2SH(p2pkh) script test case (Chris Stewart)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #8090: Adding P2SH(p2pkh) script test case
b682960 Adding P2SH(p2pkh) script test case (Chris Stewart)

codablock added a commit to codablock/dash that referenced this pull request Dec 22, 2017

Merge #8090: Adding P2SH(p2pkh) script test case
b682960 Adding P2SH(p2pkh) script test case (Chris Stewart)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment