Replace SignatureHash() with class TxSignatureHasher #4989

1. 
   jtimon commented at 7:02 am on September 27, 2014: contributor

   Is built on top of #4890. May interfere with #4981.
2. 
   jtimon commented at 7:47 am on September 27, 2014: contributor

   This could continue with https://github.com/jtimon/bitcoin/tree/sighash2 and https://github.com/jtimon/bitcoin/tree/sighash3 or simply https://github.com/jtimon/bitcoin/tree/sighash_abstract. With an abstract class the libscript could use a minimal duplicated version of CTransaction if we decide to do that, probably only temporarily, until core is more divided (again, if we decide that including the whole core.o in libscript is too much right now).
3. in src/script/interpreter.h: in d89fa6c718 outdated

   10+
   11 #include <vector>
   12 #include <stdint.h>
   13 #include <string>
   14 
   15+class uint256;
   

   ---

   ---

   

   Diapolo commented at 11:55 am on September 27, 2014:

   Nit: This should stay at the end.
4. in src/script/sigcache.h: in d89fa6c718 outdated

   21+    CachingSignatureChecker(const SignatureHasher& hasherIn, bool storeIn=true) : SignatureChecker(hasherIn), store(storeIn) { }
   22+
   23+    bool VerifySignature(const std::vector<unsigned char>& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const;
   24+};
   25+
   26+#endif
   

   ---

   ---

   

   Diapolo commented at 11:56 am on September 27, 2014:

   Should be: #endif // H_BITCOIN_SCRIPT_SIGCACHE
5. 
   Diapolo commented at 11:57 am on September 27, 2014: none

   May I suggest changing the copyright (in the files header) of edited files directly to be just MIT :)?
6. 
   jtimon commented at 6:35 pm on September 27, 2014: contributor

   I think all nits belong to #4890 but I will happily rebase on top of a corrected version.
7. jtimon force-pushed on Sep 29, 2014
8. jtimon force-pushed on Sep 29, 2014
9. 
   jtimon commented at 11:44 pm on September 29, 2014: contributor

   Mhmm, @theuni one of the tests builds didn’t failed when it had to https://travis-ci.org/bitcoin/bitcoin/builds/36618762
10. 
    sipa commented at 11:45 pm on September 29, 2014: member

    It failed to compile…
11. 
    jtimon commented at 11:47 pm on September 29, 2014: contributor

    @sipa, I know and I fixed that, but the travis builds should had all failed due to the compiling error, but build 5/7 didn’t failed. So maybe there’s something wrong with it, I don’t know.
12. 
    sipa commented at 11:54 pm on September 29, 2014: member

    @jtimon Sorry, I misread your comment. Seems we don’t compile the unit tests for OSX. @theuni?
13. 
    BitcoinPullTester commented at 11:59 pm on September 29, 2014: none

    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4989_7ffba2382622ec9529cdd318cc12bc50ebf5b0b3/ 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.
14. 
    theuni commented at 0:26 am on September 30, 2014: member

    Yes, OSX doesn’t build the tests because they can’t run there (it’s a cross build from Linux). I figured we had enough builders handling the tests, so instead the OSX spends its time testing the dmg packing process.

    I’d be fine with switching the tests on so that they build if you guys would prefer that, but I’m not sure it would do much for us.

15. 
    jtimon commented at 1:21 am on September 30, 2014: contributor

    It’s fine, I thought you may not be aware of this or something. Thanks for the explanation.
16. 
    theuni commented at 1:51 am on September 30, 2014: member

    @jtimon Thanks, you were right to point it out. The tests were created somewhat arbitrarily, I assumed they would be ever-evolving. If you think something needs more coverage, by all means mention it :)
17. jtimon force-pushed on Oct 6, 2014
18. 
    jtimon commented at 0:50 am on October 7, 2014: contributor

    Here’s another version of this PR built on top of #5054 : https://github.com/jtimon/bitcoin/tree/sighash_explicit
19. jtimon force-pushed on Oct 7, 2014
20. jtimon renamed this:
    Replace SignatureHash() with class SignatureHasher
    Replace SignatureHash() with class TxSignatureHasher
    on Oct 7, 2014
21. 
    jtimon commented at 10:50 pm on October 7, 2014: contributor

    Changed the class name from SignatureHasher to TxSignatureHasher Also rebased on top of #4981 since that will probably be merged first.
22. jtimon force-pushed on Oct 8, 2014
23. 
    jtimon commented at 0:27 am on October 8, 2014: contributor

    Went back to being independent from #4981 since the rebase is straight forward anyway. Now the original SignatureHash function is maintained but renamed and moved to the anonymous namespace.
24. jtimon force-pushed on Oct 9, 2014
25. Replace SignatureHash() with class TxSignatureHasher 399e8ce6d7
26. Change SignatureChecker constructor and make its TxSignatureHasher a reference ce4929ab6d
27. jtimon force-pushed on Oct 9, 2014
28. 
    jtimon commented at 0:48 am on October 9, 2014: contributor

    Always instantiate a new CTransaction in TxSignatureHasher’s constructor and not only when a CMutableTransaction is provided as suggested by @laanwj.
29. 
    jtimon commented at 9:14 pm on October 29, 2014: contributor

    Closing until at least 0.10
30. jtimon closed this on Oct 29, 2014

    ---

31. MarcoFalke locked this on Sep 8, 2021

Contributors
jtimon Diapolo sipa BitcoinPullTester theuni