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

pull jtimon wants to merge 4 commits into bitcoin:master from jtimon:script changing 11 files +57 −70

jtimon commented at 2:09 PM on July 18, 2014: contributor

-Remove unused function main:VerifySignature (Class CScriptCheck is being used directly instead.) -Remove CScriptCheck::nHashType (was always 0)
jtimon closed this on Jul 18, 2014
jtimon reopened this on Jul 18, 2014
sipa commented at 3:24 PM on July 18, 2014: member

Untested ACK.

I've always wondered what the nHashType flag was for, really...
jtimon renamed this:
~~Remove unused function main:VerifySignature~~
Don't pass nHashType to down to script.h/.cpp
on Jul 18, 2014
jtimon commented at 4:50 PM on July 18, 2014: contributor

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

in src/test/script_tests.cpp:None in 4afa7957ed outdated

 144 | @@ -145,7 +145,7 @@ BOOST_AUTO_TEST_CASE(script_valid)
 145 |          CScript scriptPubKey = ParseScript(scriptPubKeyString);
 146 |  
 147 |          CTransaction tx;
 148 | -        BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, tx, 0, flags, SIGHASH_NONE), strTest);

jtimon commented at 4:53 PM on July 18, 2014:

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.

in src/script.cpp:None in 4afa7957ed outdated

1161 | @@ -1162,10 +1162,7 @@ bool CheckSig(vector<unsigned char> vchSig, const vector<unsigned char> &vchPubK
1162 |      // Hash type is one byte tacked on to the end of the signature
1163 |      if (vchSig.empty())
1164 |          return false;
1165 | -    if (nHashType == 0)

jtimon commented at 4:54 PM on July 18, 2014:

This was always the case (except for the tests previously commented).

petertodd commented at 7:12 PM on July 18, 2014: 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 cross-referenced this on Jul 22, 2014 from issue Reuse sighash computations across evaluation by sipa
sipa commented at 8:53 AM on July 23, 2014: member

@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 commented at 9:58 AM on July 23, 2014: contributor

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

So untested ACK.
jtimon renamed this:
~~Don't pass nHashType to down to script.h/.cpp~~
Don't pass nHashType down to script.h/.cpp
on Jul 26, 2014
sipa commented at 12:34 PM on July 27, 2014: member

Tested ACK. Did a testnet sync from scratch with it (which likely has more cases of odd hashtypes than mainnet).
laanwj added the label Improvement on Jul 31, 2014
jtimon cross-referenced this on Aug 13, 2014 from issue libbitcoinscript by TheBlueMatt
jtimon commented at 1:10 PM on August 14, 2014: contributor

~~Blocked until #4692 is merged~~
jtimon force-pushed on Aug 28, 2014
jtimon commented at 9:49 AM on August 28, 2014: contributor

Rebased on top of #4754
jtimon force-pushed on Aug 28, 2014
jtimon cross-referenced this on Aug 28, 2014 from issue Remove CScriptCheck::nHashType (was always 0) by jtimon
jtimon force-pushed on Aug 31, 2014
jtimon commented at 1:48 PM on August 31, 2014: contributor

Rebased on top of #4755
jtimon force-pushed on Aug 31, 2014
jtimon cross-referenced this on Sep 1, 2014 from issue Preview: Compile a standalone VerifyScript() by jtimon
jtimon force-pushed on Sep 2, 2014
jtimon commented at 10:18 AM on September 2, 2014: contributor

Closing until #4754 and #4775 are merged.
jtimon closed this on Sep 2, 2014
jtimon reopened this on Sep 8, 2014
jtimon force-pushed on Sep 8, 2014
sipa commented at 9:19 PM on September 8, 2014: member

Tested ACK
sipa cross-referenced this on Sep 10, 2014 from issue Make signature caching optional, and move it out. by sipa
sipa commented at 6:02 PM on September 12, 2014: member

Needs rebase (but a rebased version is in #4890).
Remove unused function main:VerifySignature 358562b651
Remove CScriptCheck::nHashType (was always 0) ce3649fb61
Don't pass nHashType to VerifyScript 2b23a87599
Don't pass nHashType to EvalScript nor CheckSig 6dcfda2dc4
jtimon force-pushed on Sep 13, 2014
jtimon commented at 3:01 AM on September 13, 2014: contributor

Rebased
BitcoinPullTester commented at 3:10 AM on September 13, 2014: none

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 merged this on Sep 17, 2014
laanwj closed this on Sep 17, 2014
laanwj referenced this in commit 438c7e4cd2 on Sep 17, 2014
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#4562 Reuse sighash computations across evaluation #4692 libbitcoinscript #4754 Make script more modular #4755 Move code from script to wallet #4775 Remove CScriptCheck::nHashType (was always 0)#4809 Preview: Compile a standalone VerifyScript()#4890 Make signature caching optional, and move it out.