Several mining-related improvements

sipa commented at 3:38 am on April 1, 2015: member

Get rid of the optimized hashing code (which wasn’t even been used for regtest, which is the only thing for which it matters…).
Introduce a separate ‘generate’ RPC call for the regtest-specific ‘setgenerate’ behaviour.
Make regtest mining faster by using the mining loops from miner.cpp.
Avoid the 1000s of errors when mining long regtest chains.
Abstract out some of the mining code.
Fix a bug where chainActive.Tip() was accessed outside of cs_main.

jgarzik commented at 3:56 am on April 1, 2015: contributor

Nice.. I’ve wanted something like ‘generate’ Passes quick-glance review. Will test.

fanquake commented at 7:32 am on April 1, 2015: member

Travis failure

 0Running testscript wallet.py...
 1Initializing test directory /tmp/testFqH6I5
 2Mining blocks...
 3JSONRPC error: value is type 
 4  File "/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/test_framework.py", line 117, in main
 5    self.run_test()
 6  File "/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/wallet.py", line 43, in run_test
 7    self.nodes[0].generate(1)
 8  File "/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/python-bitcoinrpc/bitcoinrpc/authproxy.py", line 126, in __call__
 9    raise JSONRPCException(response['error'])
10Cleaning up
11Failed

Simplify hash loop code 0df67f1f7a

sipa force-pushed on Apr 1, 2015

Introduce separate 'generate' RPC call 6b04508e37

Bugfix: make CreateNewBlock return pindexPrev e2edf95cd3

sipa force-pushed on Apr 1, 2015

sipa commented at 8:03 pm on April 1, 2015: member

@fanquake Fixed.

laanwj added the label Mining on Apr 2, 2015

in src/rpcmining.cpp: in e2edf95cd3

234-    {
235-        mapArgs["-gen"] = (fGenerate ? "1" : "0");
236-        mapArgs ["-genproclimit"] = itostr(nGenProcLimit);
237-        GenerateBitcoins(fGenerate, pwalletMain, nGenProcLimit);
238-    }
239+    mapArgs["-gen"] = (fGenerate ? "1" : "0");

laanwj commented at 10:33 am on April 8, 2015:

We should probably change this at some point too to not update the mapArgs. Throughout the program we assume that mapArgs is read-only after initialization. This kind of r/w access would need proper locking around everything. No change needed for this pull but I’m just reminded of it.

sipa commented at 6:23 pm on April 8, 2015:

Agree.

laanwj commented at 10:34 am on April 8, 2015: member

This is obviously the right way to go, utACK

Edit: tested ACK

in src/miner.cpp: in e2edf95cd3

391+        *phash = pblock->GetHash();
392 
393         // Return the nonce if the hash has at least some zero bits,
394         // caller will check if it has enough to reach the target
395         if (((uint16_t*)phash)[15] == 0)
396             return true;

jtimon commented at 4:19 pm on April 8, 2015:

Can’t we directly return CheckProofOfWork() here instead of true and remove the later UintToArith256(hash) <= hashTarget check in ScanLoop() [which means you can also remove the uint256 *phash param in ScanHash()] ?

jtimon commented at 4:36 pm on April 8, 2015:

Even without CheckProofOfWork, that check belongs inside ScanHash so you can save the hash parameter and the hash parameter and you can reuse the arith_uint256 hashTarget = arith_uint256().SetCompact(pblock->nBits); instead of repeating the conversion every time ((uint16_t*)phash)[15] == 0. At that point, I’m not sure it’s worth it to remove the optimization but I’m still not opposed to removing it. And of course it can be && in the condition rather than returning.

sipa commented at 6:12 pm on April 8, 2015:

Yeah, just CheckProofOfWork would be fine, if we removed its error reporting (which imho we should in any case…), as now regtest mining beyond the first 2016 blocks prints 1000s of error messages to the log for each miner block as a result.

jtimon commented at 9:05 pm on April 8, 2015:

Can you at least move the check in ScanLoop inside ScanHash for now?

sipa commented at 10:20 pm on April 8, 2015:

I’m not doing that as long as it produces this much debug output - that makes it effectively unusable for mining a >2016 long block chain. That error output should probably be discussed elsewhere.

sipa commented at 10:36 pm on April 8, 2015:

Wait, I misunderstood. Yes, that seems fine.

jtimon commented at 10:45 pm on April 8, 2015:

Yes, as said in the other PR I plan to solve the warning problem soon as part of the consensus work for blockheader. At that point that check can be directly replaced by CheckProofOfWork, I’m not proposing to do it now.

in src/miner.cpp: in e2edf95cd3

466+            if (ScanLoop(pblock, pindexPrev, pwallet, reservekey)) {
467+                hash = pblock->GetHash();
468+                return true;
469+            }
470+            boost::this_thread::interruption_point();
471+            if (pblock->nNonce >= 0xffff0000)

jtimon commented at 4:20 pm on April 8, 2015:

Isn’t this included here https://github.com/bitcoin/bitcoin/pull/5957/files#diff-4a59b408ad3778278c3aeffa7da33c3cR383 ?

sipa commented at 6:09 pm on April 8, 2015:

This is about resetting the nonce if it’s about to overflow. The other check is about occasionally interrupting the mining loop.

jtimon commented at 9:50 pm on April 8, 2015:

Sorry, you can prevent the overflow by replacing the middle while(true) loop [the one that first checks if ScanLoop()] with a for loop. Then you can remove this check (from the two places it appears in) and this access to CBlockHeader::nNonce is not required. By the way, you’re incrementing CBlockHeader::nNonce in ScanHash, but where are you resetting it to 0?

sipa commented at 10:35 pm on April 8, 2015:

The nonce is reset when a new block attempt is created by calling CreateNewBlockWithKey.

I don’t understand the rest of the comment, but I’ll try to rework it a bit.

jtimon commented at 10:51 pm on April 8, 2015:

I mean, it’s just a nit and probably the more irrational one of the 3, I would just like to minimize the accesses to nBits and nNonce in the code so that the strictly-pow stuff doesn’t take much to read. If you don’t make anything about this, thank you for the try.

in src/miner.cpp: in e2edf95cd3

543-                }
544+                if (ScanLoop(pblock, pindexPrev, pwallet, reservekey))
545+                    break;
546 
547                 // Check for stop or if block needs to be rebuilt
548                 boost::this_thread::interruption_point();

jtimon commented at 9:56 pm on April 8, 2015:

This can be moved inside ScanLoop to avoid code duplication.

sipa commented at 10:33 pm on April 8, 2015:

ack

laanwj merged this on Apr 9, 2015

laanwj closed this on Apr 9, 2015

laanwj referenced this in commit 57026a29bc on Apr 9, 2015

gavinandresen commented at 3:07 pm on April 9, 2015: contributor

This makes -regtest block generation MUCH slower.

Before: cd qa/rpc-tests; rm -rf cache; time mempool_resurrect_test.py –> 26 seconds.

After: –> 2 minutes 10 seconds.

laanwj referenced this in commit 4ac79f99b0 on Apr 9, 2015

laanwj referenced this in commit 48265f3cf4 on Apr 10, 2015

laanwj referenced this in commit c8a1350119 on Apr 10, 2015

laanwj commented at 6:32 am on April 10, 2015: member

Apparently I merged this too soon, sorry. @sdaftuar noticed that this was giving problems with travis due to the slowdown @gavinandresen remarks on. I’ve thus reverted the changes to the mining code for now (but not the generate RPC call) in 48265f3.

jtimon commented at 9:33 am on April 10, 2015: contributor

Reopened #4793, which also does some code simplifications in miner.cpp. I haven’t measured regtest performance but it shouldn’t make it slower, in fact I expect to make it slightly faster apart from saving some logs produced by CheckProofOfWork (though not all of them yet). @gavinandresen can you test it?

dexX7 commented at 11:36 am on April 11, 2015: contributor

While it’s sort of easy to mitigate, changing the API of Bitcoin Core (setgenerate true n) introduces the risk of breaking system that currently use that API. Imho, similar to the depreciation of accounting system, it would be more gentle, to provide some backwards compability and clearly tag setgenerate (regtest) as depreciated, for one version.

sipa commented at 12:46 pm on April 11, 2015: member

@dexX7 I agree for any production usage of the RPC system, but this is regtest specific. Do you have any evidence of the regtest-specific setgenerate being used elsewhere? If so, we can of course temporarily maintain compatibility.

dexX7 commented at 1:11 pm on April 11, 2015: contributor

I see your point, and I don’t have specific use-cases, but there are a few references to setgenerate, e.g. in the Bitcoin developer guide, the Bitcoin wiki or BitcoinJ’s “how to test applications”, which may serve as indicator.

This change may be announced on the mailing list, to enquire additional feedback from actual users, which may, or may not be out there.

sipa commented at 1:24 pm on April 11, 2015: member

Thanks for pointing that out. I was not aware that the dev guide mentioned this.

msgilligan commented at 5:13 pm on April 14, 2015: none

Here’s the line in BitcoinClient.java in the OmniJ / bitcoin-spock project and something we are planning on contributing to bitcoinj to allow people to easily test their apps in RegTest mode:

https://github.com/OmniLayer/OmniJ/blob/master/bitcoin-rpc/src/main/java/com/msgilligan/bitcoin/rpc/BitcoinClient.java#L195

If we are the only external use-case we can handle this change when it happens, but we may not be the only case.

laanwj referenced this in commit 27ce808fb5 on Apr 21, 2015

laanwj referenced this in commit 9b5659d1c4 on Jun 10, 2015

laanwj referenced this in commit 3902c40823 on Jul 3, 2015

laanwj referenced this in commit e6c8ed3066 on Jul 3, 2015

laanwj referenced this in commit 6ebac0782f on Jul 3, 2015

AllanDoensen referenced this in commit f0b42f626f on Apr 23, 2017

MarcoFalke locked this on Sep 8, 2021

Several mining-related improvements #5957