Skip to content
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

Don't pass nHashType down to script.h/.cpp #4555

Merged
merged 4 commits into from
Sep 17, 2014
Merged

Don't pass nHashType down to script.h/.cpp #4555

merged 4 commits into from
Sep 17, 2014

Conversation

jtimon
Copy link
Contributor

@jtimon jtimon commented Jul 18, 2014

-Remove unused function main:VerifySignature (Class CScriptCheck is being used directly instead.)
-Remove CScriptCheck::nHashType (was always 0)

@jtimon jtimon closed this Jul 18, 2014
@jtimon jtimon reopened this Jul 18, 2014
@sipa
Copy link
Member

sipa commented Jul 18, 2014

Untested ACK.

I've always wondered what the nHashType flag was for, really...

@jtimon jtimon changed the title Remove unused function main:VerifySignature Don't pass nHashType to down to script.h/.cpp Jul 18, 2014
@jtimon
Copy link
Contributor Author

jtimon commented Jul 18, 2014

Removed nHashType from EvalScript and CheckSig as well. Maybe I can unify some of the commits (all of them?)

jtimon
jtimon reviewed Jul 18, 2014
View reviewed changes
src/test/script_tests.cpp
@@ -145,7 +145,7 @@ BOOST_AUTO_TEST_CASE(script_valid)
CScript scriptPubKey = ParseScript(scriptPubKeyString);

CTransaction tx;
BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, tx, 0, flags, SIGHASH_NONE), strTest);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the only place where something different from 0 (SIGHASH_NONE = 2) was passed down, but the tests are running just fine without passing it.

@petertodd
Copy link
Contributor

@sipa nHashType is useful to determine what hash types are being used in scriptSigs; I'm actually working on a patch to eliminate a SIGHASH_ANYONECANPAY-related DoS attack that needs it. That said, it might be more useful to have a mechanism the hash types used are added to a list, or for that matter, a generic callback is called to let the callee apply whatever logic they need.

@jtimon jtimon mentioned this pull request Jul 22, 2014
Closed
@sipa
Copy link
Member

sipa commented Jul 23, 2014

@petertodd I'm not convinced that's currently a use case worth keeping the nHashType parameter for, especially as the implementation is not certain.

@petertodd
Copy link
Contributor

@sipa Yeah, a sighash callback probably makes more sense and keeps more code out of the consensus critical section.

So untested ACK.

@jtimon jtimon changed the title Don't pass nHashType to down to script.h/.cpp Don't pass nHashType down to script.h/.cpp Jul 26, 2014
@sipa
Copy link
Member

sipa commented Jul 27, 2014

Tested ACK. Did a testnet sync from scratch with it (which likely has more cases of odd hashtypes than mainnet).

@jtimon jtimon mentioned this pull request Aug 13, 2014
Closed
@jtimon
Copy link
Contributor Author

jtimon commented Aug 14, 2014

Blocked until #4692 is merged

@jtimon
Copy link
Contributor Author

jtimon commented Aug 28, 2014

Rebased on top of #4754

@jtimon jtimon mentioned this pull request Aug 28, 2014
Closed
@jtimon
Copy link
Contributor Author

jtimon commented Aug 31, 2014

Rebased on top of #4755

@jtimon jtimon mentioned this pull request Sep 1, 2014
Closed
@jtimon
Copy link
Contributor Author

jtimon commented Sep 2, 2014

Closing until #4754 and #4775 are merged.

@jtimon jtimon closed this Sep 2, 2014
@jtimon jtimon reopened this Sep 8, 2014
@sipa
Copy link
Member

sipa commented Sep 8, 2014

Tested ACK

@sipa sipa mentioned this pull request Sep 10, 2014
Merged
@sipa
Copy link
Member

sipa commented Sep 12, 2014

Needs rebase (but a rebased version is in #4890).

@jtimon
Copy link
Contributor Author

jtimon commented Sep 13, 2014

Rebased

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4555_6dcfda2dc48bee2148acd571dce7d3f09608d7a2/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@laanwj laanwj merged commit 6dcfda2 into bitcoin:master Sep 17, 2014
laanwj added a commit that referenced this pull request Sep 17, 2014
@laanwj
Merge pull request #4555
438c7e4 
6dcfda2 Don't pass nHashType to EvalScript nor CheckSig (jtimon)
2b23a87 Don't pass nHashType to VerifyScript (jtimon)
ce3649f Remove CScriptCheck::nHashType (was always 0) (jtimon)
358562b Remove unused function main:VerifySignature (jtimon)
@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
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jtimon @sipa @petertodd @BitcoinPullTester @laanwj