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

jtimon commented at 10:10 am on August 30, 2014: contributor

Continues #4506. Following @laanwj ’s advice of only moving ScanHash, I found another solution to encapsulate the miner’s pow without removing the miner’s optimization, which was rejected in #4423 But I would like to slightly change the behavior. Commit “Replace proof.nNonce >= 0xffff000 with an iteration counter” should be either rejected or squashed into “proof.nNonce >= 0xffff0000 -> proof.OutOfRangeSolution()” (and of course that 1000 in the for is open for bike-shedding). “Small miner optimization” also should be rejected or rebased in “Remove testnet’s special case from miner.cpp”. I think these 2 functional changes are meaningless enough that can be safely accepted. I would also be happy to just remove the hashmeter and the resulting PR would be simpler.

jtimon force-pushed on Aug 30, 2014

jtimon force-pushed on Sep 1, 2014

jtimon commented at 7:20 pm on September 1, 2014: contributor

Closing as it is included in #4794 with only one commit more.

jtimon closed this on Sep 1, 2014

jtimon deleted the branch on Sep 1, 2014

jtimon renamed this:
~~Encapsulate miner-related pow in Proof class~~
Encapsulate miner-related pow in ScanHash
on Sep 2, 2014

jtimon restored the branch on Sep 2, 2014

jtimon commented at 11:28 am on September 2, 2014: contributor

Reopened as independent from #4506

jtimon reopened this on Sep 2, 2014

jtimon force-pushed on Sep 2, 2014

BitcoinPullTester commented at 11:42 am on September 2, 2014: none

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

in src/miner.cpp: in cf07ea708c outdated

349@@ -350,6 +350,7 @@ void IncrementExtraNonce(CBlock* pblock, CBlockIndex* pindexPrev, unsigned int&
350 //
351 double dHashesPerSec = 0.0;
352 int64_t nHPSTimerStart = 0;
353+uint32_t nOldNonce = 0;

sipa commented at 10:58 pm on October 8, 2014:

Why turn this into a global? It’s ugly, and I don’t think it will actually work in case of multiple hasher threads…

jtimon commented at 5:42 pm on October 9, 2014:

I don’t know of any other way to encapsulate the hashmeter inside the function (which will later be a method of CProof). I already asked this and nobody answered, but would anyone oppose to just removing the hashmeter? It would be simpler and cleaner.

gavinandresen commented at 6:04 pm on October 9, 2014:

Internal miner should only be used for testing, so I vote to simplify and remove the hash meter.

jtimon commented at 9:26 am on October 10, 2014: contributor

@gavinandresen that’s the reason why I wanted to get rid of the optimization (which doesn’t really help much in regtest mode) in #4423 but I’m happy removing the hashmeter.

jtimon force-pushed on Oct 10, 2014

jtimon commented at 8:34 pm on October 10, 2014: contributor

The last commit (Remove gethashespersec() from rpcmining) is completely optional and can be moved to another PR.

sipa commented at 8:47 pm on October 10, 2014: member

Concept ACK and code looks good.

We may want to deprecation period for gethashespersec() though, and just having it return 0 is not really acceptable. Having it return an error would be fine to me as nobody should really be depending on it anymore.

TheBlueMatt commented at 7:38 am on October 11, 2014: member

utACK. I dont think we need a deprecation period for anything related to the internal miner.

jtimon commented at 5:20 pm on October 11, 2014: contributor

Waiting to get more consensus before changing the rpc stuff (in case we decide to leave it as it is now). I prefer to just remove it but I have no strong opinion on this.

jtimon commented at 8:01 am on October 14, 2014: contributor

So counting opinions on the RPC stuff…

A) Just remove gethashespersec RPC call in this PR ( TheBlueMatt, jtimon ) total 2 B) Leave gethashespersec but throw an error when somebody calls it (sipa) total 1 C) Let gethashespersec return always 0 and leave the RPC for another PR (nobody) total 0

I don’t really have a strong opinion on this. I’m not in a hurry to remove the RPC call. Please, more opinions are greatly appreciated.

gmaxwell commented at 8:05 am on October 14, 2014: contributor

I think gethashespersec is more or less worthless. I don’t think I’ve heard of anyone using it in a long time. (IIRC it was even broken for a while). I don’t really have a strong opinion, but I think this is an unimportant enough RPC that removing it shoudl be fine. @jgarzik has had opinions about the internal miner before. @luke-jr often has mining opinions

Diapolo commented at 8:52 am on October 14, 2014: none

Remove it gethashespersec, as long as the internal miner is working I’m fine with that ;).

laanwj commented at 8:56 am on October 14, 2014: member

As said before, sometimes I wonder why we don’t just nuke the internal miner, instead of death by a thousand cuts.

Anyhow - yes I’m fine with removing gethashespersec.

sipa commented at 10:54 pm on October 14, 2014: member

ut ACK

jtimon commented at 1:22 am on October 22, 2014: contributor

Pushed one additional commit to avoid using boost on ScanHash(). Instead it returns false, and just after that boost::this_thread::interruption_point() was called anyway.

maaku commented at 5:36 pm on October 28, 2014: contributor

utACK

laanwj added the label Improvement on Oct 29, 2014

jtimon commented at 9:02 pm on October 29, 2014: contributor

That makes sense, I can squash it there as soon as I get some more acks for the latest commit. In fact, the commits are very separated to simplify review, but they’re all very localized so I think I wouldn’t oppose to collapse them all into a single commit once we’re ready to merge.

jtimon commented at 2:56 pm on November 14, 2014: contributor

Ping Also, should I squash already?

jtimon force-pushed on Nov 18, 2014

jtimon commented at 1:01 pm on November 18, 2014: contributor

Squashed into 2 commits. The step by step version can still be found here: https://github.com/jtimon/bitcoin/tree/proof2_old

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

jtimon force-pushed on Jan 4, 2015

jtimon commented at 8:16 pm on January 4, 2015: contributor

Squashed and rebased with some corrections, but closing at least until #5599 is merged.

jtimon closed this on Jan 4, 2015

jtimon reopened this on Jan 24, 2015

jtimon force-pushed on Jan 24, 2015

jtimon commented at 6:30 pm on January 24, 2015: contributor

After #5599 has been merged I reopened this. I also make regtest use the new generateProof (was ScanHash) function as discussed in #5275 after it was merged. This should slightly improve regtest mining performance but I haven’t tested.

jtimon renamed this:
~~Encapsulate miner-related pow in ScanHash~~
Pow: Refactor: Encapsulate miner-related pow in GenerateProof (was ScanHash) amd use it for regtest mining
on Jan 24, 2015

ghost commented at 6:38 am on January 26, 2015: none

ut ACK

jtimon force-pushed on Feb 6, 2015

jtimon commented at 5:23 am on February 6, 2015: contributor

Squashed the 2 commits. It didn’t really help much to review to have them separated.

jtimon force-pushed on Mar 26, 2015

jtimon commented at 4:25 pm on March 26, 2015: contributor

Needed rebase

jtimon renamed this:
~~Pow: Refactor: Encapsulate miner-related pow in GenerateProof (was ScanHash) amd use it for regtest mining~~
Pow: Refactor: Encapsulate miner-related pow in GenerateProof (was ScanHash) and use it for regtest mining
on Apr 6, 2015

sipa commented at 10:08 pm on April 7, 2015: member

@jtimon #5957 includes something similar to this (but goes further). I forgot you had a PR still open for this.

jtimon commented at 10:12 pm on April 8, 2015: contributor

Closing in favor of #5957, although waiting for some nits to be solved. I plan to remove the CheckProofOfWork() warnings as part of the libconsensus work to expose VerifyBlockHeader() soon.

jtimon closed this on Apr 8, 2015

jtimon reopened this on Apr 10, 2015

jtimon force-pushed on Apr 10, 2015

in src/miner.cpp: in bd8df512ee outdated

385-        // Return the nonce if the hash has at least some zero bits,
386-        // caller will check if it has enough to reach the target
387-        if (((uint16_t*)phash)[15] == 0)
388+        // Check if the hash has at least some zero bits, then fully check the proof of work 
389+        if (((uint16_t*)&hash)[15] == 0 && CheckProofOfWork(pblock->GetHash(), pblock->nBits, params)) {
390+            LogPrintf("hash: %s  \ntarget: %s\n", hash.GetHex(), arith_uint256().SetCompact(pblock->nBits).GetHex());

jtimon commented at 9:34 am on April 10, 2015:

This log could be removed too for additional regtest performance.

jtimon force-pushed on Apr 10, 2015

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

Rebased and reopened.

jtimon force-pushed on Apr 20, 2015

jtimon commented at 8:15 pm on April 20, 2015: contributor

Needed rebase

in src/miner.cpp: in 4ff179daad outdated

470@@ -478,31 +471,16 @@ void static BitcoinMiner(CWallet *pwallet)
471             // Search
472             //
473             int64_t nStart = GetTime();
474-            arith_uint256 hashTarget = arith_uint256().SetCompact(pblock->nBits);
475-            uint256 hash;
476-            uint32_t nNonce = 0;
477-            while (true) {
478+            pblock->nNonce = 0;
479+            for (int i=0; i < 1000; i++) {

laanwj commented at 2:58 pm on May 6, 2015:

What is this “1000” magic value based on?

in src/miner.cpp: in 4ff179daad outdated

487@@ -510,8 +488,7 @@ void static BitcoinMiner(CWallet *pwallet)
488                 // Regtest mode doesn't require peers
489                 if (vNodes.empty() && chainparams.MiningRequiresPeers())
490                     break;
491-                if (nNonce >= 0xffff0000)
492-                    break;

laanwj commented at 3:00 pm on May 6, 2015:

Is there any overflow check for the nonce left? Or is this how the 1000 is computed? If so, please define a constant and make the computation explicit.

in src/miner.cpp: in 4ff179daad outdated

384+        CHash256(hasher).Write((unsigned char*)&pblock->nNonce, 4).Finalize((unsigned char*)&hash);
385 
386-        // Return the nonce if the hash has at least some zero bits,
387-        // caller will check if it has enough to reach the target
388-        if (((uint16_t*)phash)[15] == 0)
389+        if (CheckProofOfWork(hash, pblock->nBits, chainparams))

laanwj commented at 3:03 pm on May 6, 2015:

Does it give a lot of overhead to call CheckProofOfWork for every nonce, instead of the ‘some zero bits’ check?

jtimon commented at 3:25 pm on May 6, 2015:

No, CheckProofOfWork is not much more expensive than ((uint16_t*)phash)[15] == 0 && UintToArith256(hash) <= arith_uint256().SetCompact(pblock->nBits), which are the checks that CheckProofOfWork is replacing. In fact, in regtest mode (the mining we care about the most, right?) those checks didn’t existed and CheckProofOfWork was already being called.

I also have commits to decouple the pow functions from util that would remove the logging of the errors in the internal checks, but I would really like to wait for #6051 (or at least #5975 and https://github.com/jtimon/bitcoin/commit/543f5b2ba0769f9e54cb667efbf74e040ea3119d ) to be merged before proposing that.

laanwj commented at 3:05 pm on May 6, 2015: member

Most of the comments in this PR do not seem to apply to the actual code change here anymore.

What is the motivation for this change?

jtimon commented at 3:37 pm on May 6, 2015: contributor

@laanwj instead of checking for the overflow of the nNonce, the magic 1000 is used. This does not check all possible values of the nonce before changing the extra nonce, but that shouldn’t be a problem. It can be make a constant, sure. I was hoping that @sipa @luke-jr or someone less lazy than me for base conversions and hexadecimal would say something like “since ScanHash returns false when pblock->nNonce & 0xfff) == 0 you can safely change the 1000 for X and we will still exhaust all the possibilities with the 32 bits of the nonce”.. We should probably make that 0xfff a constant as well. Any suggestions for the constants are welcomed.

The PR has many comments about the removal of the hashmeter, which was originally part of this, but was already merged separately. This was part of the effort to encapsulate the proof of work and it reduces the number of places in mining where nBits and nNonce are used. It should also make regtest slightly faster, but I haven’t tested that. It also makes regtest and non-regtest mining more similar.

jtimon commented at 7:25 pm on June 25, 2015: contributor

ping

Miner: Refactor: Create GenerateProof(CBlockHeader*) from ScanHash(const CBlockHeader*, uint32_t&, uint256*) and use it in regtest

Also, stop using multiple threads for mining in with regtest

7e4758aca4

jtimon force-pushed on Jul 4, 2015

jtimon commented at 10:07 am on July 4, 2015: contributor

Needed rebase. Also, I stopped using multiple threads for mining with regtest.

jgarzik commented at 6:11 pm on July 23, 2015: contributor

Closing this old PR. This was a very confusing PR as @laanwj notes, as the associated commits keep changing, garnering ACKs, getting closed, getting re-opened.

Please open a new PR with a specific focused change, and a fresh PR summary description.

jgarzik closed this on Jul 23, 2015

MarcoFalke locked this on Sep 8, 2021

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