From 1c06a9b03ce7c6989d500972a6a0aade18723529 Mon Sep 17 00:00:00 2001 From: Gavin Andresen Date: Tue, 1 Mar 2016 12:45:43 -0500 Subject: [PATCH 1/2] Make CreateNewBlock more robust to coding errors If a coding bug of some sort let a bad transaction enter the memory pool, CreateNewBlock would throw a runtime_error which would cause bitcoind to crash. It is better for miners if CreateNewBlock is robust against programming errors, and this pull request makes it more robust. If an invalid block is created, all of the transactions that went into the block are removed from the memory pool and then CreateNewBlock is tried again. --- src/miner.cpp | 25 +++++++++++++++++++++++-- src/test/miner_tests.cpp | 27 +++++++++++++++++++-------- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index 3b17b0157fa..f956a284f04 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -111,6 +111,7 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s unsigned int nBlockSigOps = 100; int lastFewTxs = 0; CAmount nFees = 0; + bool fCreatedValidBlock = false; { LOCK2(cs_main, mempool.cs); @@ -303,11 +304,31 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s pblocktemplate->vTxSigOps[0] = GetLegacySigOpCount(pblock->vtx[0]); CValidationState state; - if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, false, false)) { - throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, FormatStateMessage(state))); + fCreatedValidBlock = TestBlockValidity(state, chainparams, *pblock, pindexPrev, false, false); + if (!fCreatedValidBlock) { + if (pblock->vtx.size() <= 1) { + // This should REALLY never happen! Empty block that is invalid. + throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", + __func__, FormatStateMessage(state))); + } + // This should also never happen... but if an invalid transaction somehow entered + // the mempool due to a bug, remove all the transactions in the block + // and try again (it is not worth trying to figure out which transaction(s) + // are causing the block to be invalid). + LogPrintf("%s: TestBlockValidity failed: %s, retrying with smaller mempool", + __func__, FormatStateMessage(state)); + std::list unused; + BOOST_REVERSE_FOREACH(const CTransaction& tx, pblock->vtx) { + mempool.remove(tx, unused, true); + } } } + if (!fCreatedValidBlock) { + pblocktemplate.reset(); + return CreateNewBlock(chainparams, scriptPubKeyIn); // recurse with smaller mempool + } + return pblocktemplate.release(); } diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 71b52409b33..816e67c4c96 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -124,7 +124,10 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) mempool.addUnchecked(hash, entry.Fee(1000000).Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); tx.vin[0].prevout.hash = hash; } - BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); + BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); + BOOST_CHECK(!mempool.exists(hash)); + BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 1); + delete pblocktemplate; mempool.clear(); tx.vin[0].prevout.hash = txFirst[0]->GetHash(); @@ -139,6 +142,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vin[0].prevout.hash = hash; } BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); + BOOST_CHECK(pblocktemplate->block.vtx.size() > 1); delete pblocktemplate; mempool.clear(); @@ -163,10 +167,12 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) delete pblocktemplate; mempool.clear(); - // orphan in mempool, template creation fails + // orphan in mempool not mined hash = tx.GetHash(); mempool.addUnchecked(hash, entry.Fee(1000000).Time(GetTime()).FromTx(tx)); - BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); + BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); + BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 1); + delete pblocktemplate; mempool.clear(); // child with higher priority than parent @@ -195,10 +201,12 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) hash = tx.GetHash(); // give it a fee so it'll get mined mempool.addUnchecked(hash, entry.Fee(100000).Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); - BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); + BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); + BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 1); + delete pblocktemplate; mempool.clear(); - // invalid (pre-p2sh) txn in mempool, template creation fails + // invalid (pre-p2sh) txn in mempool, don't mine tx.vin[0].prevout.hash = txFirst[0]->GetHash(); tx.vin[0].prevout.n = 0; tx.vin[0].scriptSig = CScript() << OP_1; @@ -212,10 +220,11 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vout[0].nValue -= 1000000; hash = tx.GetHash(); mempool.addUnchecked(hash, entry.Fee(1000000).Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); - BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); + BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); + BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 1); // Just coinbase mempool.clear(); - // double spend txn pair in mempool, template creation fails + // double spend txn pair in mempool, don't mine tx.vin[0].prevout.hash = txFirst[0]->GetHash(); tx.vin[0].scriptSig = CScript() << OP_1; tx.vout[0].nValue = 4900000000LL; @@ -225,7 +234,9 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vout[0].scriptPubKey = CScript() << OP_2; hash = tx.GetHash(); mempool.addUnchecked(hash, entry.Fee(100000000L).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); - BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); + BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); + BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 1); // Just coinbase + delete pblocktemplate; mempool.clear(); // subsidy changing From 0910bf4b9ba3949220adbc74f9a5503f4c034d25 Mon Sep 17 00:00:00 2001 From: TomZ Date: Wed, 9 Mar 2016 15:22:48 +0000 Subject: [PATCH 2/2] Update CLI help --- src/init.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index c5e60343f33..16c11b6a51c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -493,7 +493,7 @@ std::string HelpMessage(HelpMessageMode mode) strUsage += HelpMessageGroup(_("Block creation options:")); strUsage += HelpMessageOpt("-blockminsize=", strprintf(_("Set minimum block size in bytes (default: %u)"), DEFAULT_BLOCK_MIN_SIZE)); - strUsage += HelpMessageOpt("-blockmaxsize=", strprintf(_("Set maximum block size in bytes (default: %d)"), DEFAULT_BLOCK_MAX_SIZE)); + strUsage += HelpMessageOpt("-blockmaxsize=", _("Set maximum block size in bytes (default is equal to network max-block-size)")); strUsage += HelpMessageOpt("-blockprioritysize=", strprintf(_("Set maximum size of high-priority/low-fee transactions in bytes (default: %d)"), DEFAULT_BLOCK_PRIORITY_SIZE)); if (showDebug) strUsage += HelpMessageOpt("-blockversion=", strprintf("Override block version to test forking scenarios (default: %d)", (int)CBlock::CURRENT_VERSION));