Skip to content

Simplify mining code (rerun of #5957) #5993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Simplify mining code (rerun of #5957) #5993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 60 additions & 68 deletions src/miner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,10 @@ class TxPriorityCompare
void UpdateTime(CBlockHeader* pblock, const CBlockIndex* pindexPrev)
{
pblock->nTime = std::max(pindexPrev->GetMedianTimePast()+1, GetAdjustedTime());

// Updating time can change work required on testnet:
if (Params().AllowMinDifficultyBlocks())
pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, Params().GetConsensus());
pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, Params().GetConsensus());
}

CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn, CBlockIndex*& pindexPrev, const CChainParams& params)
{
// Create new block
auto_ptr<CBlockTemplate> pblocktemplate(new CBlockTemplate());
Expand All @@ -97,7 +94,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)

// -regtest only: allow overriding block.nVersion with
// -blockversion=N to test forking scenarios
if (Params().MineBlocksOnDemand())
if (params.MineBlocksOnDemand())
pblock->nVersion = GetArg("-blockversion", pblock->nVersion);

// Create coinbase tx
Expand Down Expand Up @@ -132,7 +129,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)

{
LOCK2(cs_main, mempool.cs);
CBlockIndex* pindexPrev = chainActive.Tip();
pindexPrev = chainActive.Tip();
const int nHeight = pindexPrev->nHeight + 1;
CCoinsViewCache view(pcoinsTip);

Expand Down Expand Up @@ -326,7 +323,6 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
// Fill in header
pblock->hashPrevBlock = pindexPrev->GetBlockHash();
UpdateTime(pblock, pindexPrev);
pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, Params().GetConsensus());
pblock->nNonce = 0;
pblocktemplate->vTxSigOps[0] = GetLegacySigOpCount(pblock->vtx[0]);

Expand Down Expand Up @@ -364,46 +360,36 @@ void IncrementExtraNonce(CBlock* pblock, CBlockIndex* pindexPrev, unsigned int&
//

//
// ScanHash scans nonces looking for a hash with at least some zero bits.
// The nonce is usually preserved between calls, but periodically or if the
// nonce is 0xffff0000 or above, the block is rebuilt and nNonce starts over at
// zero.
// ScanHash iterates up to nMaxIter nonces in order to find a block with
// valid proof of work. It returns false after that.
//
bool static ScanHash(const CBlockHeader *pblock, uint32_t& nNonce, uint256 *phash)
bool static ScanHash(CBlockHeader *pblock, CBlockIndex *pindexPrev, int64_t nMaxIter)
{
// Write the first 76 bytes of the block header to a double-SHA256 state.
CHash256 hasher;
CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
ss << *pblock;
assert(ss.size() == 80);
hasher.Write((unsigned char*)&ss[0], 76);
UpdateTime(pblock, pindexPrev);

while (true) {
nNonce++;
uint256 hash;
arith_uint256 hashTarget = arith_uint256().SetCompact(pblock->nBits);

// Write the last 4 bytes of the block header (the nonce) to a copy of
// the double-SHA256 state, and compute the result.
CHash256(hasher).Write((unsigned char*)&nNonce, 4).Finalize((unsigned char*)phash);
while (nMaxIter--) {
pblock->nNonce++;
hash = 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)
if (UintToArith256(hash) <= hashTarget)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return true;

// If nothing found after trying for a while, return -1
if ((nNonce & 0xfff) == 0)
return false;
}

// If nothing found after trying for a while, return false.
return false;
}

CBlockTemplate* CreateNewBlockWithKey(CReserveKey& reservekey)
CBlockTemplate* CreateNewBlockWithKey(CReserveKey& reservekey, CBlockIndex*& pindexPrev, const CChainParams& params)
{
CPubKey pubkey;
if (!reservekey.GetReservedKey(pubkey))
return NULL;

CScript scriptPubKey = CScript() << ToByteVector(pubkey) << OP_CHECKSIG;
return CreateNewBlock(scriptPubKey);
return CreateNewBlock(scriptPubKey, pindexPrev, params);
}

static bool ProcessBlockFound(CBlock* pblock, CWallet& wallet, CReserveKey& reservekey)
Expand Down Expand Up @@ -435,6 +421,36 @@ static bool ProcessBlockFound(CBlock* pblock, CWallet& wallet, CReserveKey& rese
return true;
}

bool MineBlock(CReserveKey& reservekey, uint256& hash)
{
unsigned int nExtraNonce = 0;

while (true) {
CBlockIndex *pindexPrev = NULL;

auto_ptr<CBlockTemplate> pblocktemplate(CreateNewBlockWithKey(reservekey, pindexPrev, Params()));
if (!pblocktemplate.get()) {
return false;
}

CBlock *pblock = &pblocktemplate->block;
IncrementExtraNonce(pblock, pindexPrev, nExtraNonce);

while (true) {
if (ScanHash(pblock, pindexPrev, 0x1000)) {
CValidationState state;
if (ProcessNewBlock(state, NULL, pblock)) {
hash = pblock->GetHash();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not worth copying a whole block, imho...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return true;
}
}
boost::this_thread::interruption_point();
if (pblock->nNonce >= 0xffff0000)
break;
}
}
}

void static BitcoinMiner(CWallet *pwallet)
{
LogPrintf("BitcoinMiner started\n");
Expand All @@ -458,9 +474,9 @@ void static BitcoinMiner(CWallet *pwallet)
// Create new block
//
unsigned int nTransactionsUpdatedLast = mempool.GetTransactionsUpdated();
CBlockIndex* pindexPrev = chainActive.Tip();
CBlockIndex* pindexPrev = NULL;

auto_ptr<CBlockTemplate> pblocktemplate(CreateNewBlockWithKey(reservekey));
auto_ptr<CBlockTemplate> pblocktemplate(CreateNewBlockWithKey(reservekey, pindexPrev, Params()));
if (!pblocktemplate.get())
{
LogPrintf("Error in BitcoinMiner: Keypool ran out, please call keypoolrefill before restarting the mining thread\n");
Expand All @@ -476,52 +492,28 @@ void static BitcoinMiner(CWallet *pwallet)
// Search
//
int64_t nStart = GetTime();
arith_uint256 hashTarget = arith_uint256().SetCompact(pblock->nBits);
uint256 hash;
uint32_t nNonce = 0;
while (true) {
// Check if something found
if (ScanHash(pblock, nNonce, &hash))
{
if (UintToArith256(hash) <= hashTarget)
{
// Found a solution
pblock->nNonce = nNonce;
assert(hash == pblock->GetHash());

SetThreadPriority(THREAD_PRIORITY_NORMAL);
LogPrintf("BitcoinMiner:\n");
LogPrintf("proof-of-work found \n hash: %s \ntarget: %s\n", hash.GetHex(), hashTarget.GetHex());
ProcessBlockFound(pblock, *pwallet, reservekey);
SetThreadPriority(THREAD_PRIORITY_LOWEST);

// In regression test mode, stop mining after a block is found.
if (Params().MineBlocksOnDemand())
throw boost::thread_interrupted();
if (ScanHash(pblock, pindexPrev, 0x1000)) {
SetThreadPriority(THREAD_PRIORITY_NORMAL);
LogPrintf("BitcoinMiner:\n");
LogPrintf("proof-of-work found \n");
ProcessBlockFound(pblock, *pwallet, reservekey);
SetThreadPriority(THREAD_PRIORITY_LOWEST);

break;
}
break;
}

// Check for stop or if block needs to be rebuilt
boost::this_thread::interruption_point();

// Regtest mode doesn't require peers
if (vNodes.empty() && Params().MiningRequiresPeers())
break;
if (nNonce >= 0xffff0000)
if (pblock->nNonce >= 0xffff0000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

break;
if (mempool.GetTransactionsUpdated() != nTransactionsUpdatedLast && GetTime() - nStart > 60)
break;
if (pindexPrev != chainActive.Tip())
break;

// Update nTime every few seconds
UpdateTime(pblock, pindexPrev);
if (Params().AllowMinDifficultyBlocks())
{
// Changing pblock->nTime can change work required on testnet:
hashTarget.SetCompact(pblock->nBits);
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Expand Down
6 changes: 4 additions & 2 deletions src/miner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#define BITCOIN_MINER_H

#include "primitives/block.h"
#include "chainparams.h"

#include <stdint.h>

Expand All @@ -25,8 +26,9 @@ struct CBlockTemplate
/** Run the miner threads */
void GenerateBitcoins(bool fGenerate, CWallet* pwallet, int nThreads);
/** Generate a new block, without valid proof-of-work */
CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn);
CBlockTemplate* CreateNewBlockWithKey(CReserveKey& reservekey);
CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn, CBlockIndex*& pindexPrev, const CChainParams& params);
CBlockTemplate* CreateNewBlockWithKey(CReserveKey& reservekey, CBlockIndex*& pindexPrev, const CChainParams& params);
bool MineBlock(CReserveKey& key, uint256& hash);
/** Modify the extranonce in a block */
void IncrementExtraNonce(CBlock* pblock, CBlockIndex* pindexPrev, unsigned int& nExtraNonce);
void UpdateTime(CBlockHeader* block, const CBlockIndex* pindexPrev);
Expand Down
23 changes: 5 additions & 18 deletions src/rpcmining.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,28 +145,15 @@ Value generate(const Array& params, bool fHelp)
nHeight = nHeightStart;
nHeightEnd = nHeightStart+nGenerate;
}
unsigned int nExtraNonce = 0;
Array blockHashes;
while (nHeight < nHeightEnd)
{
auto_ptr<CBlockTemplate> pblocktemplate(CreateNewBlockWithKey(reservekey));
if (!pblocktemplate.get())
uint256 hash;
if (!MineBlock(reservekey, hash)) {
throw JSONRPCError(RPC_INTERNAL_ERROR, "Wallet keypool empty");
CBlock *pblock = &pblocktemplate->block;
{
LOCK(cs_main);
IncrementExtraNonce(pblock, chainActive.Tip(), nExtraNonce);
}
while (!CheckProofOfWork(pblock->GetHash(), pblock->nBits, Params().GetConsensus())) {
// Yes, there is a chance every nonce could fail to satisfy the -regtest
// target -- 1 in 2^(2^32). That ain't gonna happen.
++pblock->nNonce;
}
CValidationState state;
if (!ProcessNewBlock(state, NULL, pblock))
throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted");
++nHeight;
blockHashes.push_back(pblock->GetHash().GetHex());
blockHashes.push_back(hash.GetHex());
}
return blockHashes;
}
Expand Down Expand Up @@ -494,7 +481,7 @@ Value getblocktemplate(const Array& params, bool fHelp)

// Store the pindexBest used before CreateNewBlock, to avoid races
nTransactionsUpdatedLast = mempool.GetTransactionsUpdated();
CBlockIndex* pindexPrevNew = chainActive.Tip();
CBlockIndex* pindexPrevNew = NULL;
nStart = GetTime();

// Create new block
Expand All @@ -504,7 +491,7 @@ Value getblocktemplate(const Array& params, bool fHelp)
pblocktemplate = NULL;
}
CScript scriptDummy = CScript() << OP_TRUE;
pblocktemplate = CreateNewBlock(scriptDummy);
pblocktemplate = CreateNewBlock(scriptDummy, pindexPrevNew, Params());
if (!pblocktemplate)
throw JSONRPCError(RPC_OUT_OF_MEMORY, "Out of memory");

Expand Down
Loading