Several mining-related improvements #5957

sipa · 2015-04-01T03:38:23Z

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 · 2015-04-01T03:56:38Z

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

fanquake · 2015-04-01T07:32:55Z

Travis failure

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

sipa · 2015-04-01T20:03:18Z

@fanquake Fixed.

laanwj · 2015-04-08T10:33:04Z

src/rpcmining.cpp

-        mapArgs ["-genproclimit"] = itostr(nGenProcLimit);
-        GenerateBitcoins(fGenerate, pwalletMain, nGenProcLimit);
-    }
+    mapArgs["-gen"] = (fGenerate ? "1" : "0");


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.

laanwj · 2015-04-08T10:34:55Z

This is obviously the right way to go, utACK

Edit: tested ACK

jtimon · 2015-04-08T16:19:29Z

src/miner.cpp

-        // the double-SHA256 state, and compute the result.
-        CHash256(hasher).Write((unsigned char*)&nNonce, 4).Finalize((unsigned char*)phash);
+        pblock->nNonce++;
+        *phash = pblock->GetHash();

        // Return the nonce if the hash has at least some zero bits,
        // caller will check if it has enough to reach the target
        if (((uint16_t*)phash)[15] == 0)
            return true;


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()] ?

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.

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.

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

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.

Wait, I misunderstood. Yes, that seems fine.

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.

e2edf95 Bugfix: make CreateNewBlock return pindexPrev (Pieter Wuille) 6b04508 Introduce separate 'generate' RPC call (Pieter Wuille) 0df67f1 Simplify hash loop code (Pieter Wuille)

gavinandresen · 2015-04-09T15:07:48Z

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.

New wallet tests have been added in the meantime and need to be updated to use `generate` instead of `setgenerate`.

This reverts commit e2edf95 6b04508 0df67f1, except the changes to the RPC tests. A `generate` RPC call is introduced based on the old code.

48265f3 Revert mining changes in #5957 (Wladimir J. van der Laan)

laanwj · 2015-04-10T06:32:36Z

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 · 2015-04-10T09:33:01Z

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 · 2015-04-11T11:36:23Z

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 · 2015-04-11T12:46:59Z

@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 · 2015-04-11T13:11:50Z

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 · 2015-04-11T13:24:32Z

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

msgilligan · 2015-04-14T17:13:20Z

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.

Ever since bitcoin#5957 there has been the problem that older RPC test cases (as can be found plenty in open pulls) use setgenerate() on regtest, assuming a different interpretation of the arguments. Directly generating a number of blocks has been split off into a new method `generate` - however using `setgenerate` with the previous arguments will result in spawning an unreasonable number of threads, and well, simply not work as expected without clear indication of the error. Add an error to point the user at the right method.

Do not translate -help-debug options, Many technical terms, and only a very small audience, so is unnecessary stress to translators. Brings the code up to date with translation string policy in `doc/translation_strings_policy.md`. Also remove no-longer-relevant "In this mode -genproclimit controls how many blocks are generated immediately." (as of bitcoin#5957) from regtest help.

No longer relevant after bitcoin#5957. This hack existed because of another hack where the numthreads parameter, on regtest, doubled as how many blocks to generate.

No longer relevant after #5957. This hack existed because of another hack where the numthreads parameter, on regtest, doubled as how many blocks to generate. Conflicts: src/miner.cpp

Simplify hash loop code

0df67f1

sipa force-pushed the simplehash branch 2 times, most recently from 1f5872d to 6f25142 Compare April 1, 2015 18:09

sipa added 2 commits April 1, 2015 11:47

Introduce separate 'generate' RPC call

6b04508

Bugfix: make CreateNewBlock return pindexPrev

e2edf95

sipa force-pushed the simplehash branch from 6f25142 to e2edf95 Compare April 1, 2015 18:47

laanwj added the Mining label Apr 2, 2015

sipa mentioned this pull request Apr 7, 2015

Pow: Refactor: Encapsulate miner-related pow in GenerateProof (was ScanHash) and use it for regtest mining #4793

Closed

laanwj reviewed Apr 8, 2015
View reviewed changes

jtimon reviewed Apr 8, 2015
View reviewed changes

laanwj merged commit e2edf95 into bitcoin:master Apr 9, 2015

laanwj added a commit that referenced this pull request Apr 9, 2015

Merge pull request #5957

57026a2

e2edf95 Bugfix: make CreateNewBlock return pindexPrev (Pieter Wuille) 6b04508 Introduce separate 'generate' RPC call (Pieter Wuille) 0df67f1 Simplify hash loop code (Pieter Wuille)

sdaftuar mentioned this pull request Apr 9, 2015

Replace setgenerate with generate in wallet.py #5988

Closed

laanwj added a commit that referenced this pull request Apr 9, 2015

Fix travis after merging #5957

4ac79f9

New wallet tests have been added in the meantime and need to be updated to use `generate` instead of `setgenerate`.

laanwj added a commit to laanwj/bitcoin that referenced this pull request Apr 10, 2015

Revert mining changes in bitcoin#5957

48265f3

This reverts commit e2edf95 6b04508 0df67f1, except the changes to the RPC tests. A `generate` RPC call is introduced based on the old code.

laanwj added a commit that referenced this pull request Apr 10, 2015

Merge pull request #5991

c8a1350

48265f3 Revert mining changes in #5957 (Wladimir J. van der Laan)

jtimon mentioned this pull request Apr 20, 2015

Consensus: Consensus version of pow functions #6037

Closed

laanwj mentioned this pull request Apr 21, 2015

tests: Error when setgenerate is used on regtest #6039

Merged

jonasschnelli mentioned this pull request Apr 21, 2015

[REST] getutxos REST command (based on Bip64) #5420

Merged

laanwj mentioned this pull request Jun 10, 2015

Remove translation for -help-debug options #6264

Merged

laanwj mentioned this pull request Jul 3, 2015

Remove ChainParams::DefaultMinerThreads #6370

Merged

daira mentioned this pull request Feb 26, 2017

mapArgs is modified by setgenerate RPC without locking zcash/zcash#2132

Open

laanwj mentioned this pull request Oct 16, 2018

Deprecate wallet generate RPC method #14299

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Several mining-related improvements #5957

Several mining-related improvements #5957

Uh oh!

Conversation

sipa commented Apr 1, 2015

Uh oh!

jgarzik commented Apr 1, 2015

Uh oh!

fanquake commented Apr 1, 2015

Uh oh!

sipa commented Apr 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Apr 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavinandresen commented Apr 9, 2015

Uh oh!

laanwj commented Apr 10, 2015

Uh oh!

jtimon commented Apr 10, 2015

Uh oh!

dexX7 commented Apr 11, 2015

Uh oh!

sipa commented Apr 11, 2015 via email

Uh oh!

dexX7 commented Apr 11, 2015

Uh oh!

sipa commented Apr 11, 2015 via email

Uh oh!

msgilligan commented Apr 14, 2015

Uh oh!

Uh oh!