Remove miner optimization

jtimon commented at 10:44 pm on June 26, 2014: contributor

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.

sipa commented at 11:08 pm on June 26, 2014: member

2014-06-26 23:07:19 ERROR: CheckProofOfWork() : hash doesn’t match nBits

in src/miner.cpp: in 5bdf5df64d outdated

447-                nOldNonce = nNonce;
448 
449+                pblock->nNonce++;
450                 // Check if something found
451-                if (fFound)
452+                if (!CheckProofOfWork(pblock->GetHash(), pblock->nBits))

sipa commented at 11:28 pm on June 26, 2014:

Remove the negation. Did you test this?

sipa commented at 11:30 pm on June 26, 2014: member

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

laanwj commented at 6:30 am on June 27, 2014: member

Could as well remove the internal miner completely, instead of crippling it first…

Diapolo commented at 7:28 am on June 27, 2014: none

Still NACK on removing.

laanwj commented at 7:33 am on June 27, 2014: member

Anyhow, I don’t feel like this discussion again, ACK if the pulltester OK.

in src/miner.cpp: in 5bdf5df64d outdated

496@@ -551,11 +497,6 @@ void static BitcoinMiner(CWallet *pwallet)
497 
498                 // Update nTime every few seconds
499                 UpdateTime(*pblock, pindexPrev);
500-                if (Params().AllowMinDifficultyBlocks())

Diapolo commented at 9:10 am on June 27, 2014:

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?

jtimon commented at 10:14 am on June 27, 2014:

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

jtimon commented at 10:12 am on June 27, 2014: contributor

Made changes suggested by @sipa @laanwj isn’t it already crippled (when compared to other miners)? We still need a minimal miner for testing.

Remove miner optimizations fa5da46938

BitcoinPullTester commented at 11:05 am on June 27, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4423_fa5da469388ad5ef4520eed59c279ce220dca4b5/ 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.

jgarzik commented at 1:44 pm on June 27, 2014: contributor

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.

luke-jr commented at 1:50 pm on June 27, 2014: member

(there’s also BFGMiner and libblkmaker’s GBT example which uses libgcrypt for hashing)

Diapolo commented at 2:03 pm on June 27, 2014: none

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!

sipa commented at 2:12 pm on June 27, 2014: member

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.

gavinandresen commented at 4:10 pm on June 27, 2014: contributor

Yes, the regression tests use the internal miner to create chains.

So NACK from me on removing (unless replaced by something better). And NACK on making the internal miner slower. As jgarzik says, see contrib/pyminer for a slow-but-easy-to-understand miner.

laanwj commented at 4:17 pm on June 27, 2014: member

Ok, closing.

laanwj closed this on Jun 27, 2014

jtimon deleted the branch on Jul 7, 2014

jtimon commented at 1:17 pm on July 28, 2014: contributor

Did anyone tested how much slower this is for the regtest case (the one we’re using and want to keep)? Do we really need to print the hashes per second? Removing that would also make it simpler to refactor the POW generation out to pow.h as in #4377

sipa commented at 1:20 pm on July 28, 2014: member

I benchamrked your earlier naive miner (which just looped and performed CheckProofOfWork on each solution); IIRC it was 2-3x slower.

jtimon restored the branch on Aug 28, 2014

jtimon commented at 2:59 pm on August 28, 2014: contributor

@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? It is plausible that is even faster.

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. The most obvious solution was to move the whole optimization to the Proof class, which @laanwj and I dislike. I’m open to code several possibilities to help people chose what they like most.

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. Would anybody oppose to the removal of the hashmeter? Users of bitcoind’s miner hashmeter, show yourselves. Oh, wait, @sipa were you using that for the benchmark? Mhmm, if so, it is not a fair comparison since my version works more for the hashmeter, look https://github.com/bitcoin/bitcoin/pull/4423/files#diff-4a59b408ad3778278c3aeffa7da33c3cL519

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. I promise that if my regtest-only argument it’s not convincing, I won’t insist like I did with CTxDestination.

sipa commented at 5:46 pm on October 10, 2014: member

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.

jtimon referenced this in commit 18379875bf on Nov 27, 2014

MarcoFalke locked this on Sep 8, 2021

Remove miner optimization #4423