Avoid crash on start in TestBlockValidity with gen=1. #6079

pull gmaxwell wants to merge 1 commits into bitcoin:master from gmaxwell:createnewblock_precheckpointfail changing 1 files +14 −2

gmaxwell commented at 12:05 AM on April 29, 2015: contributor

When the internal miner is enabled at the start of a new node, there is an near instant assert in TestBlockValidity because its attempting to mine a block before the top checkpoint.

Also avoids a data race around vNodes.

Avoid crash on start in TestBlockValidity with gen=1.

When the internal miner is enabled at the start of a new node, there
 is an near instant assert in TestBlockValidity because its attempting
 to mine a block before the top checkpoint.

Also avoids a data race around vNodes.

504e9bb9af

gmaxwell commented at 12:08 AM on April 29, 2015: contributor

This was reported on IRC by smccully; either of the two internal checks fix my reproduction of his problem.

An implication of this is that it makes it harder to intentionally fork testnet-- due to requiring getting it past IsInitialBlockDownload, which may be undesirable. (Though you should be able to syncup then invalidate block and mine ahead).
laanwj added the label Bug on Apr 29, 2015
laanwj commented at 7:24 AM on April 29, 2015: member

This may be solved by Bugfix: Fix testnet-in-a-box use case #5987 as well (or at least overlaps in scope).

in src/miner.cpp:None in 504e9bb9af

  90 | @@ -91,6 +91,11 @@ void UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams,
  91 |  
  92 |  CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
  93 |  {
  94 | +    LOCK(cs_main);
  95 | +    // Are we in sync enough to even try creating a block? Otherwise TestBlockValidity will fail.
  96 | +    if (chainActive.Height() < Checkpoints::GetTotalBlocksEstimate())

laanwj commented at 10:10 AM on May 11, 2015:

I've reproduced the issue to verify: the underlying problem appears to be that the std::runtime_exception raised by CreateNewBlock in case TestBlockValidity fails is not caught by the internal miner. The other uses of this function in RPC are protected.

terminate called after throwing an instance of 'std::runtime_error'
  what():  CreateNewBlock() : TestBlockValidity failed

Adding an extra check here at the beginning is redundant - you'd add a check to avoid raising an exception later in the same funcion :) better to change the calling code in BitcoinMiner to catch and report this exception.

laanwj commented at 10:35 AM on May 11, 2015: member

See here: https://github.com/laanwj/bitcoin/commit/89388eb36fe3e220605b9f4ffc46f4ad7e8fadb7

I've tested it with and without the change to BitcoinMiner, it catches the same problem.
gmaxwell commented at 9:52 PM on May 11, 2015: contributor

That looks good to me.
laanwj commented at 8:58 AM on May 12, 2015: member

Ok, submitted that as pull, closing in favor of #6123
laanwj closed this on May 12, 2015
ghost commented at 3:14 PM on May 18, 2015: none

Great. Adding a reference to #5742 for tracking purposes.
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6123 Avoid crash on start in TestBlockValidity with gen=1 (rework)