Remove miner optimization #4423
Conversation
|
2014-06-26 23:07:19 ERROR: CheckProofOfWork() : hash doesn't match nBits |
src/miner.cpp
Outdated
There was a problem hiding this comment.
Remove the negation. Did you test this?
|
If you additionally remove the "CheckProofOfWork() : hash doesn't match nBits" (which causes a few 100000 lines of logging to be written while mining otherwise), the speed of the miner only drops by a factor 2-2.5x. I'm not sure I consider that acceptable, but I expected a lot more (it's decoding nBits into a uint256 in every iterations...). |
|
Could as well remove the internal miner completely, instead of crippling it first... |
|
Still NACK on removing. |
|
Anyhow, I don't feel like this discussion again, ACK if the pulltester OK. |
There was a problem hiding this comment.
Isn't this the only usage of AllowMinDifficultyBlocks() anyway? I also think this is testnet related and has nothing to do with just making the code here more clear, right?
There was a problem hiding this comment.
Params().AllowMinDifficultyBlocks() is for testnet, yes. It's also called inside UpdateTime and inside GetNextWorkRequired.
But this condition is no longer necessary because UpdateTime already updates pblock->nBits in testnet case and there's no hashTarget variable to update with pblock->nBits
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4423_fa5da469388ad5ef4520eed59c279ce220dca4b5/ for binaries and test log. |
|
NAK. Just leave it as-is, or remove it completely. Making it slower is just dumb. If you want an example of mining code, look at in-tree pyminer or out-of-tree cpuminer. |
|
(there's also BFGMiner and libblkmaker's GBT example which uses libgcrypt for hashing) |
|
Perhaps you really should create a pull, which removes everything related to the internal miner. If this is done properly and we add some doc how to quickly mine regtest or testnet, I'm going to accept the voice of the majority! |
|
The python tests do create blocks, I believe. We'll need a replacement for those committed to the repo if we want to drop the internal miner completely. |
|
Yes, the regression tests use the internal miner to create chains. So NACK from me on removing (unless replaced by something better). |
|
Ok, closing. |
|
Did anyone tested how much slower this is for the regtest case (the one we're using and want to keep)? |
|
I benchamrked your earlier naive miner (which just looped and performed CheckProofOfWork on each solution); IIRC it was 2-3x slower. |
|
@sipa I understand that your benchmarks have been maybe in hash/s which is what someone CPU mining with bitcoind in either mainnet or testnet would care about. My point is, as discussed we want to maintain the regtest miner which we use for testing. So my question was, did anybody tested how the performance of the tests suffers with this? Anyway there's other solutions to encapsulate the proof of work related parts of miner.o in pow.o without removing the optimization so I think it's worth asking here. But in my opinion one thing is clear: all of them will look much nicer if I can just get rid of the hashmeter first. Anyway, jokes apart, I'm just looking for some more feedback on how to encapsulate the mining part of the pow in the Proof class. |
|
For regtest mode the speed of the internal miner is not relevant, which is why I don't care strongly about this. It seems other don't like making it slower (presumably for other purposes?), though. |
Speed up generating blocks in regression test mode, by moving block-creating and nonce-finding directly into the setgenerate RPC call (instead of starting up a mining thread and waiting for it to find a block). This makes the forknotify RPC test three times quicker, for example (10 seconds runtime instead of 30 seconds, assuming the initial blockchain cache is already built).
8 participants
Nobody is using the miner besides testing, there's more efficient CPU miners out there and I don't think bitcoind needs more than a very basic miner for testing and maybe educational purposes.
This PR makes the miner code less efficient but clearer.