Skip to content

Report non-mandatory script failures correctly #7276

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 4, 2016
Merged

Report non-mandatory script failures correctly #7276

merged 1 commit into from
Jan 4, 2016

Conversation

sipa
Copy link
Member

@sipa 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.

@sipa sipa mentioned this pull request Jan 3, 2016
Closed
@jtimon
Copy link
Contributor

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
Copy link
Contributor

dcousens commented Jan 3, 2016

utACK

@maflcko
Copy link
Member

maflcko commented Jan 4, 2016

Good catch! utACK 7ef8f3c

@petertodd
Copy link
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.

@dcousens
Copy link
Contributor

dcousens commented Jan 4, 2016

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

@jonasschnelli
Copy link
Contributor

utACK 7ef8f3c

@fanquake
Copy link
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
Copy link
Member

laanwj commented Jan 4, 2016

utACK, this was sneaky

@laanwj laanwj merged commit 7ef8f3c into bitcoin:master Jan 4, 2016
laanwj added a commit that referenced this pull request Jan 4, 2016
@laanwj
Merge pull request #7276
d032b5b 
7ef8f3c Report non-mandatory script failures correctly (Pieter Wuille)
sipa added a commit that referenced this pull request Jan 4, 2016
@sipa @laanwj
Report non-mandatory script failures correctly
76de36f 
Github-Pull: #7276
Rebased-From: 7ef8f3c
@jtimon
Copy link
Contributor

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 pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016
@sipa @luke-jr
Report non-mandatory script failures correctly
fed8e13 
Github-Pull: bitcoin#7276
Rebased-From: 7ef8f3c
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016
@sipa @luke-jr
Report non-mandatory script failures correctly
eea84de 
Github-Pull: bitcoin#7276
Rebased-From: 7ef8f3c
@sipa sipa mentioned this pull request Feb 10, 2016
Closed
@jtimon jtimon mentioned this pull request Feb 14, 2016
Closed
@str4d str4d mentioned this pull request Jan 25, 2018
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Feb 8, 2018
@zkbot
Auto merge of #2903 - str4d:1408-sighash, r=<try>
e1f3a15 
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
@zkbot
Auto merge of #2903 - str4d:1408-sighash, r=<try>
eb3128a 
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
@zkbot
Auto merge of #2903 - str4d:1408-sighash, r=<try>
6db1012 
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
@zkbot
Auto merge of #2903 - str4d:1408-sighash, r=str4d
8487be8 
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 mentioned this pull request Mar 19, 2018
Merged
@furszy furszy mentioned this pull request Jun 6, 2020
Merged
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Aug 5, 2020
@random-zebra
Merge #1666: Base work for the Sapling signatureHash
5a09215 
d1d15c8 Fix missing sigverion in main_test.cpp CreateDummyScriptSigWithKey. (furszy)
a034daf Rename to PrecomputedTransactionData (furszy)
b4b181b Unit test for sighash caching (furszy)
2ef3872 Report non-mandatory script failures correctly. (furszy)
446d340 Precompute sighashes (furszy)
dfd24eb Update wallet_txn_close.py test: (furszy)
a5170f0 BIP143: Signing logic. (furszy)
d2dd547 BIP143: Verification logic. (furszy)
dccc3c6 Refactor script validation to observe amounts (furszy)
daf044a Reduce unnecessary hashing in signrawtransaction (furszy)

Pull request description:

  Base work for the new transaction digest algorithm for signature verification on PIVX Sapling transactions.

  Essentially, an implementation of BIP143 + few more good commits that found down the rabbit hole.

  Back ports:

  * bitcoin#7276
  * bitcoin#7976
  * bitcoin#8118
  * bitcoin#8149 (only amount validation and SignatureHash commits).
  * bitcoin#6088 (only the dummy signature one - will be removed once #1663 get merged -).
  * bitcoin#6379
  * bitcoin#8524

  Next step over this area (need 1553 merged to be able to push it) is the further specialization of BIP143 into our custom implementation of ZIP143 (with a different digest algorithm definition using our tx data and hash personalization).

ACKs for top commit:
  Fuzzbawls:
    utACK d1d15c8
  random-zebra:
    ACK d1d15c8 and merging...

Tree-SHA512: 7665cccf095c5bce0b18ef7ab8fcf7bede9304993b48f1af9c352c568861dec728d1d68671aab857b73d46567678492c4b97c24644a15f3f29fc4d723b183522
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sipa @jtimon @dcousens @maflcko @petertodd @jonasschnelli @fanquake @laanwj