From efac91e65452dd89a9b891e79bdd191208d2b44d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 23 Jul 2017 13:29:06 -0400 Subject: [PATCH 1/4] Always wait for threadGroup to exit in bitcoind shutdown This resolves a possible-assert-on-shutdown race introduced in 1f668b646806f94acd851acdbd9939c24e0492d3 when early shutdown occurs. Previously this was not done to avoid any cases where the threadGroup might not exit due to a blocking thread, but at this point the threadGroup isn't used all that much, plus Qt already does this, and its good to keep their init/shutdown consistent. For those curious, the threadGroup is only used in a few places: * Its used to run the CCheckQueues in script validation, but these use the boost mutex/condition variable primitives, so they respect the interrupt pretty trivially. * Its used for the import thread, which should exit rather quickly as mostly it just calls LoadExternalBlockFile, which has an interruption_point right before each block loaded. * Its used in the scheduler thread, which is only used for: * validationinterface has an effectively-dummy reference to it. * wallet compaction, which should not last long * addr/banlist dumping from CConnman, which should also be fast --- src/bitcoind.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index f3844e9d4..ff61b9065 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -176,9 +176,7 @@ bool AppInit(int argc, char* argv[]) if (!fRet) { Interrupt(threadGroup); - // threadGroup.join_all(); was left out intentionally here, because we didn't re-test all of - // the startup-failure cases to make sure they don't result in a hang due to some - // thread-blocking-waiting-for-another-thread-during-startup case + threadGroup.join_all(); } else { WaitForShutdown(&threadGroup); } From fce3f4f4920714187616e76fdecbec306e8d7a8f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 23 Jul 2017 18:08:50 -0400 Subject: [PATCH 2/4] Fix resume-of-reindex-after-restart This more clearly uses fReindex vs fReset to make sure we're not clearing our coinsdb needlessly when restarting after a reindex. It also makes it so that restarting after shutting down mid-reindex isn't treates specially at all during txdb loading code, as it shouldn't be. --- src/init.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 0ee828ce9..5cc45c52f 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1400,9 +1400,9 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) delete pcoinscatcher; delete pblocktree; - pblocktree = new CBlockTreeDB(nBlockTreeDBCache, false, fReindex); + pblocktree = new CBlockTreeDB(nBlockTreeDBCache, false, fReset); - if (fReindex) { + if (fReset) { pblocktree->WriteReindexing(true); //If we're reindexing in prune mode, wipe away unusable block files and all undo data files if (fPruneMode) @@ -1414,6 +1414,8 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) // LoadBlockIndex will load fTxIndex from the db, or set it if // we're reindexing. It will also load fHavePruned if we've // ever removed a block file from disk. + // Note that it also sets fReindex based on the disk flag! + // From here on out fReindex and fReset mean something different! if (!LoadBlockIndex(chainparams)) { strLoadError = _("Error loading block database"); break; @@ -1448,7 +1450,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) // At this point we're either in reindex or we've loaded a useful // block tree into mapBlockIndex! - pcoinsdbview = new CCoinsViewDB(nCoinDBCache, false, fReindex || fReindexChainState); + pcoinsdbview = new CCoinsViewDB(nCoinDBCache, false, fReset || fReindexChainState); pcoinscatcher = new CCoinsViewErrorCatcher(pcoinsdbview); // If necessary, upgrade from older database format. @@ -1467,7 +1469,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) // The on-disk coinsdb is now in a good state, create the cache pcoinsTip = new CCoinsViewCache(pcoinscatcher); - if (!fReindex && !fReindexChainState) { + if (!fReset && !fReindexChainState) { // LoadChainTip sets chainActive based on pcoinsTip's best block if (!LoadChainTip(chainparams)) { strLoadError = _("Error initializing block database"); @@ -1476,7 +1478,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) assert(chainActive.Tip() != NULL); } - if (!fReindex) { + if (!fReset) { // Note that RewindBlockIndex MUST run even if we're about to -reindex-chainstate. // It both disconnects blocks based on chainActive, and drops block data in // mapBlockIndex based on lack of available witness data. @@ -1487,7 +1489,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) } } - if (!fReindex && !fReindexChainState) { + if (!fReset && !fReindexChainState) { uiInterface.InitMessage(_("Verifying blocks...")); if (fHavePruned && GetArg("-checkblocks", DEFAULT_CHECKBLOCKS) > MIN_BLOCKS_TO_KEEP) { LogPrintf("Prune: pruned datadir may not have more than %d blocks; only checking available blocks", From 13ab353829bfcf928a0dd88cc9841e6515eea111 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 26 Jul 2017 14:41:50 -0400 Subject: [PATCH 3/4] Check for empty coinsview instead of just-reset coinsview in init This fixes a few cases where we should be treating a restart-after- coinsviewdb-reset identically to a just-reset-coinsviewdb. Thanks to @morcos for identifying the bug. --- src/init.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 5cc45c52f..ae84e49cd 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1469,7 +1469,8 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) // The on-disk coinsdb is now in a good state, create the cache pcoinsTip = new CCoinsViewCache(pcoinscatcher); - if (!fReset && !fReindexChainState) { + bool is_coinsview_empty = fReset || fReindexChainState || pcoinsTip->GetBestBlock().IsNull(); + if (!is_coinsview_empty) { // LoadChainTip sets chainActive based on pcoinsTip's best block if (!LoadChainTip(chainparams)) { strLoadError = _("Error initializing block database"); @@ -1489,7 +1490,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) } } - if (!fReset && !fReindexChainState) { + if (!is_coinsview_empty) { uiInterface.InitMessage(_("Verifying blocks...")); if (fHavePruned && GetArg("-checkblocks", DEFAULT_CHECKBLOCKS) > MIN_BLOCKS_TO_KEEP) { LogPrintf("Prune: pruned datadir may not have more than %d blocks; only checking available blocks", From e7539f864984740b80efc44e1a8970f4353ff066 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 1 Aug 2017 17:02:10 -0400 Subject: [PATCH 4/4] Fix some broken init-time prints/constants --- src/init.cpp | 5 +++-- src/test/test_bitcoin.cpp | 2 +- src/validation.cpp | 1 - 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index ae84e49cd..ca62d3e7c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1440,8 +1440,9 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) } // At this point blocktree args are consistent with what's on disk. - // If we're not mid-reindex (based on disk + args), add a genesis block on disk. - // This is called again in ThreadImport in the reindex completes. + // If we're not mid-reindex (based on disk + args), add a genesis block on disk + // (otherwise we use the one already on disk). + // This is called again in ThreadImport after the reindex completes. if (!fReindex && !LoadGenesisBlock(chainparams)) { strLoadError = _("Error initializing block database"); break; diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index 545e56983..e2e0a9668 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -75,7 +75,7 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha pcoinsdbview = new CCoinsViewDB(1 << 23, true); pcoinsTip = new CCoinsViewCache(pcoinsdbview); if (!LoadGenesisBlock(chainparams)) { - throw std::runtime_error("InitBlockIndex failed."); + throw std::runtime_error("LoadGenesisBlock failed."); } { CValidationState state; diff --git a/src/validation.cpp b/src/validation.cpp index babf6f152..405ff356f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3916,7 +3916,6 @@ bool LoadGenesisBlock(const CChainParams& chainparams) if (mapBlockIndex.count(chainparams.GenesisBlock().GetHash())) return true; - // Only add the genesis block if not reindexing (in which case we reuse the one already on disk) try { CBlock &block = const_cast(chainparams.GenesisBlock()); // Start new block file