Replace OpenSSL hash functions with built-in implementations #4100

pull sipa wants to merge 13 commits into bitcoin:master from sipa:ownhash changing 26 files +1387 −610
  1. sipa commented at 10:00 pm on April 27, 2014: member
    • Introduces the classes CSHA256 and CSHA512 in crypto/sha2.{cpp,h}, CSHA1 in crypto/sha1.{cpp,h} and CRIPEMD160 in crypto/ripemd160.{cpp,h}.
    • The HMAC-SHA512 implementation is moved to sha2.{cpp,h} as well, with a similar interface.
    • In hash.h, CHash256 (=double SHA256) and CHash160 (SHA256 + RIPEMD160) are added.
    • The rest of hash.h is refactored to use those classes.
    • The internal miner is also modified to use CHash256 (hiding the inner SHA256 workings).
    • The getwork() RPC call is removed.
  2. sipa commented at 10:12 pm on April 27, 2014: member
    Benchmark of the built-in miner: old code 1111 khash/s, new code 1016 khash/s.
  3. sipa commented at 11:16 pm on April 27, 2014: member
    Microbenchmark for just double-SHA256 of a 80-byte block: old 1.26us, new 1.37us
  4. jgarzik commented at 11:55 pm on April 27, 2014: contributor

    IIRC, we suffer from the terribly annoying requirement that some getwork miners fail without midstate and hash1.

    Sure, those miners could be modified, but it seems disappointing to break an interface we want to remove anyway. The internal miner and GBT continue to see modern users, but I think it is time to retire getwork.

  5. luke-jr commented at 0:10 am on April 28, 2014: member
    AFAIK the only miner which needs midstate/hash1 is DiabloMiner.
  6. laanwj commented at 2:06 am on April 28, 2014: member

    Agree on removing getwork completely in next major release. It doesn’t scale to modern mining hardware, that alone is reason enough to get rid of it (we will have to adopt the contrib/pyminer to use getblocktemplate). I’m not convinced it should happen in this pull, though. Removing midstate seems like a fine intermediate solution.

    Nit: we’re ending up with a lot of hash-related implementation and header files. Maybe move them to a directory src/hash?

  7. laanwj added this to the milestone 0.10.0 on Apr 28, 2014
  8. gmaxwell commented at 5:15 am on April 28, 2014: contributor
    I also think we should remove getwork. We should retain an ability to mine in the stock distribution (either the integrated miner or some contrib/ solution) but that doesn’t imply keeping getwork. I don’t even use getwork while mining on testnet.
  9. sipa commented at 11:12 am on April 28, 2014: member
    Removed getwork(), and moved the SHA implementations to src/crypto/ (not src/hash/, as we’ll also want AES there eventually, I guess). I didn’t move hash.h itself there, as it doesn’t contain any actual crypto code (just wrappers), and it depends on more Bitcoin-specific stuff (this way, src/crypto/ is entirely dependencyless).
  10. laanwj commented at 2:16 pm on April 28, 2014: member
    Good point on crypto instead of hash. Woohoo, the pull-tester passed!
  11. sipa renamed this:
    Replace the use of OpenSSL's SHA2 by a native implementation.
    Replace the use of OpenSSL's SHA2 by a built-in implementation.
    on Apr 28, 2014
  12. sipa commented at 2:34 pm on April 28, 2014: member
    Ok, bug fixed. Seems I didn’t update the precomputed SHA256 state after updating the timestamp.
  13. sipa renamed this:
    Replace the use of OpenSSL's SHA2 by a built-in implementation.
    Replace OpenSSL hash functions with built-in implementations
    on Apr 30, 2014
  14. sipa commented at 11:03 pm on April 30, 2014: member
    Added RIPEMD160 as well.
  15. jgarzik commented at 1:02 pm on May 2, 2014: contributor

    untested ACK. One comment:

    {Read,Write}{BE,LE]{32,64} really wants to be in some sort of common header. One of my first reactions – though outside the scope of your initial conversion – was that the implementations of WriteBE* etc. is a “slow but safe” implementation. It should be a compiler intrinsic, as it can be on any gcc platform. And that requires a common header for such gizmos.

  16. sipa commented at 3:41 pm on May 2, 2014: member
    @Jeff: very preliminary test with __builtin_bswap32 results in a 4% speedup for the builtin miner (but may be noise).
  17. jgarzik commented at 3:49 pm on May 2, 2014: contributor

    Not surprised. Enormous amounts of effort have been expended on the glibc [header] side, gcc and CPU sides of the problem, to make byte swapping faster.

    As a result, you will see endian.h and friends play several tricks at compile-time, and then attempt to fall back to a compiler instrinsic if that does not work. Every modern CPU has a byte-swap instruction. Any networked LE or BE platform is constantly byte-swapping.

    The general principle, therefore, is to include your platform’s favorite endian.h, and use the macros therein. It should degenerate into a compiler intrinsic for us.

    In practice, this becomes painful as BSD and Linux diverge on the opinion of endian macro naming and header location.

  18. sipa commented at 10:01 pm on May 2, 2014: member

    More thorough test: 6% speedup. That’s probablt worth it given how many sha256’ing we do…

    EDIT: with __builtin_bswap32, the built-in miner is only 4% slower than the current OpenSSL-based implementation.

  19. sipa commented at 11:09 pm on May 2, 2014: member
    Moved the Read/Write functions to crypto/common.h, and made them use direct read/write and/or __builtin_bswap* when possible.
  20. sipa commented at 11:18 pm on May 2, 2014: member
    Ugh, endian.h doesn’t exist for mingw. This will need some autotools interaction :(
  21. sipa commented at 11:33 pm on May 2, 2014: member
    Small cheat: WIN32 is always little endian
  22. sipa commented at 7:21 pm on May 3, 2014: member
    \o/
  23. sipa commented at 1:10 pm on May 10, 2014: member
    Rebased.
  24. ghost commented at 12:42 pm on May 11, 2014: none
    Tested (linux) ACK.
  25. sipa commented at 8:12 pm on May 31, 2014: member
    Extended the hash unit tests.
  26. sipa commented at 11:12 pm on June 5, 2014: member
    No need to rush things, as 0.10 won’t be for any time soon, but what do people think about the degree of tests here? Anything more required?
  27. ghost commented at 7:08 am on June 6, 2014: none
    I think it would be better to get this merged now so it can run on testnet for a while.
  28. laanwj commented at 7:33 am on June 6, 2014: member
    @sipa Testing looks good to me. @drak There is no way to merge it only for testnet (as that would imply keeping both implementations and being able to switch between them, a waste of work). If you want to run this on testnet - which is a great idea - why not just pull sipa’s ‘ownhash’ branch?
  29. jgarzik commented at 1:58 am on June 7, 2014: contributor

    General re-ACK.

    Comments:

    • It would be nice to specify a precise size on the char buffer in various Finalize() arguments, and the language does permit that. However, there may be disadvantages.
    • On testing, the only thing I can suggest is a review of every callsite, and make sure that block import or some other populate code path (or test suite test) exercises that callsite.
    • It seems likely that the endian.h area will need further autotools configury work (though it’s fine for now)
  30. sipa commented at 9:15 pm on June 8, 2014: member
    Rebased, and added a test vector with a non-repeating long string (as suggested by @adam3us).
  31. theuni commented at 7:52 pm on June 9, 2014: member

    I’ve pushed up the autotools work as mentioned by @jgarzik here: https://github.com/theuni/bitcoin/commits/ownhash .

    Also, this really needs to be built as its own library because it is entirely self-contained. My branch contains those changes as well.

    There’s nothing there to derail this PR though, both of these things can be discussed/merged after.

  32. sipa commented at 8:38 pm on June 9, 2014: member
    Rebased.
  33. sipa commented at 7:15 am on June 10, 2014: member
    Added some commits from @theuni, to build the crypto code as a separate library.
  34. sipa commented at 7:24 am on June 10, 2014: member

    @jgarzik Suggestion for the output buffer size API?

    I wouldn’t mind a CSomething::OUTPUT_SIZE constant, but the code using it should be aware already anyway.

  35. sipa commented at 11:03 pm on June 12, 2014: member
    @jgarzik Added a [Class]::OUTPUT_SIZE static constant, and use it in the method definition of Finalize and in a few other places.
  36. jgarzik commented at 1:11 am on June 13, 2014: contributor
    +1
  37. sipa commented at 12:42 pm on June 21, 2014: member

    Rebased.

    Can I have some ACKs? @gmaxwell @laanwj?

  38. sipa commented at 2:12 pm on June 21, 2014: member
    Alternatively, I can move the non-crypto changes (just the wrapper interfaces, getwork/miner changes, unit tests, …) to a separate pullreq, as those should be no-ops.
  39. jgarzik commented at 2:46 pm on June 21, 2014: contributor

    FWIW, I announced the death of “getwork” on bitcoin-development. Nobody objected. @laanwj posted the only reply, which was a clarification for the audience.

    My ACK remains standing.

  40. laanwj commented at 3:20 pm on June 21, 2014: member
    ACK
  41. Add a built-in SHA256/SHA512 implementation. 
    This also moves the HMAC-SHA512 implementation to sha2.cpp.
    977cdadea8
  42. Switch script.cpp and hash.cpp to use sha2.cpp instead of OpenSSL. 7b4737c878
  43. Remove getwork() RPC call cf0c47b269
  44. Switch miner.cpp to use sha2 instead of OpenSSL. 85aab2a088
  45. Add built-in SHA-1 implementation. 1cc344ce42
  46. Move crypto implementations to src/crypto/ 13b5dfef64
  47. Add built-in RIPEMD-160 implementation a5bc9c0917
  48. Move {Read,Write}{LE,BE}{32,64} to common.h and use builtins if possible 7ecd9739d9
  49. Extend and move all crypto tests to crypto_tests.cpp 3820e01eb9
  50. build: move bitcoin-config.h to its own directory 
    This allows us to include its path without making other header includes valid.
    54372482a8
  51. crypto: explicitly check for byte read/write functions 
    Don't depend on hard-coded platform lists
    f2647cc0e9
  52. crypto: create a separate lib for crypto functions 
    This lib has no dependencies on other bitcoin functionality. Attempting to
use bitcoin headers will result in a failure to compile.
    4791b99e2d
  53. Add <Hasher>::OUTPUT_SIZE a0495bb68c
  54. BitcoinPullTester commented at 6:39 pm on June 21, 2014: none
    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a0495bb68c6eff9c732d458bacab10490d6452b4 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.
  55. gmaxwell commented at 8:42 pm on June 21, 2014: contributor
    Tested ACK.
  56. sipa merged this on Jun 21, 2014
  57. sipa closed this on Jun 21, 2014
  58. sipa referenced this in commit 8f59251b83 on Jun 21, 2014
  59. domob1812 referenced this in commit fa7e1e7e06 on Jun 13, 2018
  60. domob1812 referenced this in commit fd710eff5c on Jun 13, 2018
  61. DrahtBot locked this on Sep 8, 2021


sipa jgarzik luke-jr laanwj gmaxwell ghost theuni BitcoinPullTester

Milestone
0.10.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-12-13 09:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me