Report non-mandatory script failures correctly #7276

Merged
merged 1 commit into from Jan 4, 2016

Conversation

Projects
None yet
8 participants
@sipa
Member

sipa commented Jan 3, 2016

The same variable was reused for mandatory checking of script failures, so the reported error is always "No error". Use a new variable check2 so we don't overwrite the message produced by the first.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jan 3, 2016

Member

This case only makes sense when in AcceptToMemoryPool anyway, no non-mandatory flgs will be used when called from connectBlock. We could move this special case to AcceptToMemoryPool as well.
Something like jtimon@dc0dab8 (although that commit is based on previous commits so it wouldn't be exactly like that if done alone).
Maybe we can fix this in that way (also helping libconsensus encapsulation by removing the STANDARD_NOT_MANDATORY_VERIFY_FLAGS [relay policy] dependency from CheckInputs() ).

Or, as always, "that can be done later".
Otherwise utACK, good catch.

Member

jtimon commented Jan 3, 2016

This case only makes sense when in AcceptToMemoryPool anyway, no non-mandatory flgs will be used when called from connectBlock. We could move this special case to AcceptToMemoryPool as well.
Something like jtimon@dc0dab8 (although that commit is based on previous commits so it wouldn't be exactly like that if done alone).
Maybe we can fix this in that way (also helping libconsensus encapsulation by removing the STANDARD_NOT_MANDATORY_VERIFY_FLAGS [relay policy] dependency from CheckInputs() ).

Or, as always, "that can be done later".
Otherwise utACK, good catch.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jan 3, 2016

Contributor

utACK

Contributor

dcousens commented Jan 3, 2016

utACK

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 4, 2016

Member

Good catch! utACK 7ef8f3c

Member

MarcoFalke commented Jan 4, 2016

Good catch! utACK 7ef8f3c

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jan 4, 2016

Contributor

utACK sipa@7ef8f3c

Could probably eventually split CheckInputs() into a function for value consistency, fees, etc., and a function that checks the scripts themselves. That'd make running the latter twice with different flags cheap.

Contributor

petertodd commented Jan 4, 2016

utACK sipa@7ef8f3c

Could probably eventually split CheckInputs() into a function for value consistency, fees, etc., and a function that checks the scripts themselves. That'd make running the latter twice with different flags cheap.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jan 4, 2016

Contributor

Actually just ran into this my self, tested ACK @ 7ef8f3c

Contributor

dcousens commented Jan 4, 2016

Actually just ran into this my self, tested ACK @ 7ef8f3c

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
Member

jonasschnelli commented Jan 4, 2016

utACK 7ef8f3c

@jonasschnelli jonasschnelli added the Docs label Jan 4, 2016

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Jan 4, 2016

Member

utACK

On Monday, 4 January 2016, Jonas Schnelli notifications@github.com wrote:

utACK 7ef8f3c
7ef8f3c


Reply to this email directly or view it on GitHub
#7276 (comment).

Member

fanquake commented Jan 4, 2016

utACK

On Monday, 4 January 2016, Jonas Schnelli notifications@github.com wrote:

utACK 7ef8f3c
7ef8f3c


Reply to this email directly or view it on GitHub
#7276 (comment).

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 4, 2016

Member

utACK, this was sneaky

Member

laanwj commented Jan 4, 2016

utACK, this was sneaky

@laanwj laanwj merged commit 7ef8f3c into bitcoin:master Jan 4, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Jan 4, 2016

Merge pull request #7276
7ef8f3c Report non-mandatory script failures correctly (Pieter Wuille)

sipa added a commit that referenced this pull request Jan 4, 2016

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jan 4, 2016

Member

@petertodd perhaphs is now time to reopen #6445 (there's a more up to date version after a big consensus moveonly in jtimon@babe715 )? I closed it because "those refactors/optimizations are going to interfere with the mempool limiting work", but those changes are done now (and it was trivial to rebase, meaning the concern wasn't really justified).

Member

jtimon commented Jan 4, 2016

@petertodd perhaphs is now time to reopen #6445 (there's a more up to date version after a big consensus moveonly in jtimon@babe715 )? I closed it because "those refactors/optimizations are going to interfere with the mempool limiting work", but those changes are done now (and it was trivial to rebase, meaning the concern wasn't really justified).

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016

@str4d str4d referenced this pull request in zcash/zcash Jan 25, 2018

Merged

Overwinter SignatureHash #2903

zkbot added a commit to zcash/zcash that referenced this pull request Feb 8, 2018

Auto merge of #2903 - str4d:1408-sighash, r=<try>
Overwinter SignatureHash

Implements zcash/zips#129.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#6915
  - Only the rework of `mempool.check()` calls that the next PR depends on.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of  #2254. Closes #1408 and #2584.

zkbot added a commit to zcash/zcash that referenced this pull request Feb 8, 2018

Auto merge of #2903 - str4d:1408-sighash, r=<try>
Overwinter SignatureHash

Implements zcash/zips#129.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#6915
  - Only the rework of `mempool.check()` calls that the next PR depends on.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of  #2254. Closes #1408 and #2584.

zkbot added a commit to zcash/zcash that referenced this pull request Feb 19, 2018

Auto merge of #2903 - str4d:1408-sighash, r=<try>
Overwinter SignatureHash

Implements zcash/zips#129.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#6915
  - Only the rework of `mempool.check()` calls that the next PR depends on.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of #2074 and #2254. Closes #1408 and #2584.

zkbot added a commit to zcash/zcash that referenced this pull request Feb 20, 2018

Auto merge of #2903 - str4d:1408-sighash, r=<try>
Overwinter SignatureHash

Implements ZIP 143.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of #2074 and #2254. Closes #1408 and #2584.

zkbot added a commit to zcash/zcash that referenced this pull request Feb 20, 2018

Auto merge of #2903 - str4d:1408-sighash, r=str4d
Overwinter SignatureHash

Implements ZIP 143.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of #2074 and #2254. Closes #1408 and #2584.

@dagurval dagurval referenced this pull request in bitcoinxt/bitcoinxt Mar 19, 2018

Merged

Precompute sighashes #366

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