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

1. 
   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)
2. jtimon closed this on Jul 18, 2014

   ---

3. jtimon reopened this on Jul 18, 2014

   ---

4. 
   sipa commented at 3:24 pm on July 18, 2014: member

   Untested ACK.

   I’ve always wondered what the nHashType flag was for, really…

5. jtimon renamed this:
   Remove unused function main:VerifySignature
   Don't pass nHashType to down to script.h/.cpp
   on Jul 18, 2014
6. 
   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?)
7. in src/test/script_tests.cpp: 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.
8. in src/script.cpp: 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).
9. 
   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.
10. 
    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.
11. 
    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.

12. 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
13. 
    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).
14. laanwj added the label Improvement on Jul 31, 2014
15. 
    jtimon commented at 1:10 pm on August 14, 2014: contributor

    Blocked until #4692 is merged
16. jtimon force-pushed on Aug 28, 2014
17. 
    jtimon commented at 9:49 am on August 28, 2014: contributor

    Rebased on top of #4754
18. jtimon force-pushed on Aug 28, 2014
19. jtimon force-pushed on Aug 31, 2014
20. 
    jtimon commented at 1:48 pm on August 31, 2014: contributor

    Rebased on top of #4755
21. jtimon force-pushed on Aug 31, 2014
22. jtimon force-pushed on Sep 2, 2014
23. 
    jtimon commented at 10:18 am on September 2, 2014: contributor

    Closing until #4754 and #4775 are merged.
24. jtimon closed this on Sep 2, 2014

    ---

25. jtimon reopened this on Sep 8, 2014

    ---

26. jtimon force-pushed on Sep 8, 2014
27. 
    sipa commented at 9:19 pm on September 8, 2014: member

    Tested ACK
28. 
    sipa commented at 6:02 pm on September 12, 2014: member

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

    Rebased
35. 
    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.
36. laanwj merged this on Sep 17, 2014
37. laanwj closed this on Sep 17, 2014

    ---

38. laanwj referenced this in commit 438c7e4cd2 on Sep 17, 2014
39. MarcoFalke locked this on Sep 8, 2021

Contributors
jtimon sipa petertodd BitcoinPullTester

Labels
Refactoring