Simplify mining code (rerun of #5957)

sipa commented at 10:17 am on April 10, 2015: member

This time without regtest mining slowdown.

Simplify mining code a119cfa881

in src/miner.cpp: in 8ea15d0154 outdated

87-        pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, Params().GetConsensus());
88+    pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, Params().GetConsensus());
89 }
90 
91-CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
92+CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn, CBlockIndex*& pindexPrev)

jtimon commented at 10:29 am on April 10, 2015:

Given that you’re changing the function, can you also pass const CChainParams& params ?

sipa commented at 10:50 am on April 10, 2015:

Done.

in src/miner.cpp: in 8ea15d0154 outdated

438+
439+        while (true) {
440+            if (ScanHash(pblock, pindexPrev, 0x1000)) {
441+                CValidationState state;
442+                if (ProcessNewBlock(state, NULL, pblock)) {
443+                    hash = pblock->GetHash();

jtimon commented at 10:32 am on April 10, 2015:

Is it really that important to pass uint256& hash ? Maybe better to pass CBlock *pblock directly is cleaner?

sipa commented at 10:35 am on April 10, 2015:

Not worth copying a whole block, imho…

jtimon commented at 10:42 am on April 10, 2015:

It’s not a whole block, it’s a pointer, no?

sipa commented at 10:47 am on April 10, 2015:

CreateNewBlock builds a new CBlock object. If we’d want to return it in a passed pointer, you’d have to copy it there.

Alternatively, we could return a CBlock* variable in a CBlock*& argument, but why bother? All we need is the hash…

jtimon commented at 11:04 am on April 10, 2015:

Ok, can we pass ChainParams to the newly created MineBlock() as well (CreateNewBlockWithKey is taking it).

in src/miner.cpp: in 8ea15d0154 outdated

531+
532                 // Regtest mode doesn't require peers
533                 if (vNodes.empty() && Params().MiningRequiresPeers())
534                     break;
535-                if (nNonce >= 0xffff0000)
536+                if (pblock->nNonce >= 0xffff0000)

jtimon commented at 10:37 am on April 10, 2015:

The problem wasn’t accesing the nonce inside scanhash but outside of it, so passing int64_t nMaxIter doesn’t solve it. It is this while (and the one inside mineblock that should turn in to fors). In the case of regtest, updating the extra nonce every time scanhash fails is probably good enough, as in https://github.com/bitcoin/bitcoin/pull/4793/files#diff-c2c990fee1c3381462640e80ae7db0d0R156

sipa commented at 10:50 am on April 10, 2015:

I agree it doesn’t solve it. I’ve tried other strategies, but didn’t find anything I could reasonably include in what this PR is trying to accomplish. Feel free to improve things in your PR.

jtimon commented at 11:07 am on April 10, 2015:

Well, in any case I don’t think the new int64_t nMaxIter param in ScanHash is worth it (as dicussed it will be useful for later that it takes Consensus::Params though).

in src/miner.cpp: in 8ea15d0154 outdated

391+        hash = 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+        if (UintToArith256(hash) <= hashTarget)

jtimon commented at 10:38 am on April 10, 2015:

Can you replace this with CheckProofOfWork directly? Maybe with if (((uint16_t*)&hash)[15] == 0 && CheckProofOfWork(pblock->GetHash(), pblock->nBits, params)) as in https://github.com/bitcoin/bitcoin/pull/4793/files#diff-4a59b408ad3778278c3aeffa7da33c3cR384 ?

sipa commented at 10:42 am on April 10, 2015:

No. The direct hash check makes regtest mining very slow, and calling checkproofofwork directly would cause 1000s of error lines in testnet mining.

My intent was eventually moving this function to pow.cpp, so it’s fine to have it access details of the pow construction.

jtimon commented at 10:47 am on April 10, 2015:

Ok, it can be replaced once the error logs are removed… In anycase, I was planning to move ScanHash/GenerateProof to pow too, but I’ve changed my mind since the other pow functions will move to consensus and this is not part of consensus. My current plan is to remove pow, moving most of it to consensus, a logging version of GetNextWork to miner and getBlockProof to chain. Anyway, things for other place to discuss

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

Still, can you pass a const Consensus::Params& params parameter for when CheckProofOfWork is ready?

jtimon commented at 10:49 am on April 10, 2015:

Or maybe just remove the log errors of CheckProofOfWork in this PR

in src/miner.cpp: in 8ea15d0154 outdated

539                     break;
540                 if (pindexPrev != chainActive.Tip())
541                     break;
542-
543-                // Update nTime every few seconds
544-                UpdateTime(pblock, pindexPrev);

jtimon commented at 10:39 am on April 10, 2015:

It seems you’re not moving update time anywhere, just removing it.

jtimon commented at 11:10 am on April 10, 2015:

You have to keep it here and also put it on MineBlock I believe. Maybe this is too much to ask but it taking Consensus::Params and replacing Params().AllowMinDifficultyBlocks() with params.fPowAllowMinDifficultyBlocks as in https://github.com/bitcoin/bitcoin/pull/5968/files#diff-4a59b408ad3778278c3aeffa7da33c3cR80 would be very nice as well.

jtimon commented at 11:16 am on April 10, 2015:

Oh, sorry, I didn’t see that you moved it to ScanHash, that’s actually better if regtest is going to use it as well. But why are you getting rid of the Params().AllowMinDifficultyBlocks() check inside UpdateTime?

sipa force-pushed on Apr 10, 2015

in src/test/miner_tests.cpp: in a119cfa881

62     LOCK(cs_main);
63     Checkpoints::fEnabled = false;
64 
65     // Simple block creation, nothing special yet:
66-    BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey));
67+    BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey, pindexPrev, Params()));

jtimon commented at 11:21 am on April 10, 2015:

Thank you for doing this, but if you do it, you should do it right. That is, with

0const CChainParams& params = Params(CBaseChainParams::MAIN);

at the beginning of the test case as in https://github.com/bitcoin/bitcoin/pull/5970/files#diff-5c64500485fda76388a86c95c0059585R53

laanwj added the label Improvement on May 6, 2015

sipa commented at 10:40 am on May 6, 2015: member

Closing this until some other miner/wallet things are in.

sipa closed this on May 6, 2015

MarcoFalke locked this on Sep 8, 2021

Simplify mining code (rerun of #5957) #5993