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.

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

Replace OpenSSL hash functions with built-in implementations #4100

Merged
merged 13 commits into from Jun 21, 2014

Conversation

@sipa
Copy link
Member

sipa commented Apr 27, 2014

  • 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.
@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Apr 27, 2014

Benchmark of the built-in miner: old code 1111 khash/s, new code 1016 khash/s.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Apr 27, 2014

Microbenchmark for just double-SHA256 of a 80-byte block: old 1.26us, new 1.37us

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented Apr 27, 2014

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.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Apr 28, 2014

AFAIK the only miner which needs midstate/hash1 is DiabloMiner.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 28, 2014

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?

@laanwj laanwj added this to the 0.10.0 milestone Apr 28, 2014
@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Apr 28, 2014

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.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Apr 28, 2014

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).

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 28, 2014

Good point on crypto instead of hash.
Woohoo, the pull-tester passed!

@sipa sipa changed the title Replace the use of OpenSSL's SHA2 by a native implementation. Replace the use of OpenSSL's SHA2 by a built-in implementation. Apr 28, 2014
@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Apr 28, 2014

Ok, bug fixed. Seems I didn't update the precomputed SHA256 state after updating the timestamp.

@sipa sipa changed the title Replace the use of OpenSSL's SHA2 by a built-in implementation. Replace OpenSSL hash functions with built-in implementations Apr 30, 2014
@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Apr 30, 2014

Added RIPEMD160 as well.

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented May 2, 2014

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.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented May 2, 2014

@jeff: very preliminary test with __builtin_bswap32 results in a 4% speedup
for the builtin miner (but may be noise).

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented May 2, 2014

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.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented May 2, 2014

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.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented May 2, 2014

Moved the Read/Write functions to crypto/common.h, and made them use direct read/write and/or __builtin_bswap* when possible.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented May 2, 2014

Ugh, endian.h doesn't exist for mingw. This will need some autotools interaction :(

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented May 2, 2014

Small cheat: WIN32 is always little endian

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented May 3, 2014

\o/

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented May 10, 2014

Rebased.

@ghost

This comment has been minimized.

Copy link

ghost commented May 11, 2014

Tested (linux) ACK.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented May 31, 2014

Extended the hash unit tests.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Jun 5, 2014

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?

@ghost

This comment has been minimized.

Copy link

ghost commented Jun 6, 2014

I think it would be better to get this merged now so it can run on testnet
for a while.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jun 6, 2014

@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?

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented Jun 7, 2014

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)
@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Jun 9, 2014

Rebased.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Jun 10, 2014

Added some commits from @theuni, to build the crypto code as a separate library.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Jun 10, 2014

@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.

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Jun 12, 2014

@jgarzik Added a [Class]::OUTPUT_SIZE static constant, and use it in the method definition of Finalize and in a few other places.

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented Jun 13, 2014

+1

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Jun 21, 2014

Rebased.

Can I have some ACKs? @gmaxwell @laanwj?

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Jun 21, 2014

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.

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented Jun 21, 2014

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.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jun 21, 2014

ACK

@BitcoinPullTester

This comment has been minimized.

Copy link

BitcoinPullTester commented Jun 21, 2014

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.

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Jun 21, 2014

Tested ACK.

@sipa sipa merged commit a0495bb into bitcoin:master Jun 21, 2014
sipa added a commit that referenced this pull request Jun 21, 2014
a0495bb Add <Hasher>::OUTPUT_SIZE (Pieter Wuille)
4791b99 crypto: create a separate lib for crypto functions (Cory Fields)
f2647cc crypto: explicitly check for byte read/write functions (Cory Fields)
5437248 build: move bitcoin-config.h to its own directory (Cory Fields)
3820e01 Extend and move all crypto tests to crypto_tests.cpp (Pieter Wuille)
7ecd973 Move {Read,Write}{LE,BE}{32,64} to common.h and use builtins if possible (Pieter Wuille)
a5bc9c0 Add built-in RIPEMD-160 implementation (Pieter Wuille)
13b5dfe Move crypto implementations to src/crypto/ (Pieter Wuille)
1cc344c Add built-in SHA-1 implementation. (Pieter Wuille)
85aab2a Switch miner.cpp to use sha2 instead of OpenSSL. (Pieter Wuille)
cf0c47b Remove getwork() RPC call (Pieter Wuille)
7b4737c Switch script.cpp and hash.cpp to use sha2.cpp instead of OpenSSL. (Pieter Wuille)
977cdad Add a built-in SHA256/SHA512 implementation. (Pieter Wuille)
@domob1812 domob1812 mentioned this pull request Jun 8, 2018
domob1812 added a commit to xaya/xaya that referenced this pull request Jun 13, 2018
This adds back an implementation of getwork (together with creatework
and submitwork, performing each of the two "forms" of getwork and without
the need for a local wallet).

This was removed in upstream Bitcoin in
bitcoin/bitcoin#4100.  The new version, however,
is based on Namecoin's merge-mining code and thus easy to maintain.
It is also necessary to support stand-alone mining with PoW data in the
future out-of-the-box with existing mining software.

I've tested that it works on testnet with our modified cpuminer.  Proper
regtests for it will be added as a follow-up.
domob1812 added a commit to xaya/xaya that referenced this pull request Jun 13, 2018
This adds back an implementation of getwork (together with creatework
and submitwork, performing each of the two "forms" of getwork and without
the need for a local wallet).

This was removed in upstream Bitcoin in
bitcoin/bitcoin#4100.  The new version, however,
is based on Namecoin's merge-mining code and thus easy to maintain.
It is also necessary to support stand-alone mining with PoW data in the
future out-of-the-box with existing mining software.

I've tested that it works on testnet with our modified cpuminer.  Proper
regtests for it will be added as a follow-up.

Implements #33, except
for the still missing regtests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.