From eda888e57352037ab2e60f6ef90098b3ce23a157 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 6 Jul 2017 19:57:20 -0400 Subject: [PATCH 1/5] Fix some LoadChainTip-related init-order bugs. * Move the writing of fTxIndex to LoadBlockIndex - this fixes a bug introduced in d6af06d68aae985436cbc942f0d11078041d121b where InitBlockIndex was writing to fTxIndex which had not yet been checked (because LoadChainTip hadn't yet initialized the chainActive, which would otherwise have resulted in InitBlockIndex being a NOP), allowing you to modify -txindex without reindex, potentially corrupting your chainstate! * Rename InitBlockIndex to LoadGenesisBlock, which is now a more natural name for it. Also check mapBlockIndex instead of chainActive, fixing a bug where we'd write the genesis block out on every start. --- src/init.cpp | 4 +-- src/test/test_bitcoin.cpp | 2 +- src/validation.cpp | 67 +++++++++++++++++++++++---------------- src/validation.h | 7 ++-- 4 files changed, 47 insertions(+), 33 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 1e8564201..b1fe8e7d3 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -646,7 +646,7 @@ void ThreadImport(std::vector vImportFiles) fReindex = false; LogPrintf("Reindexing finished\n"); // To avoid ending up in a situation without genesis block, re-try initializing (no-op if reindexing worked): - InitBlockIndex(chainparams); + LoadGenesisBlock(chainparams); } // hardcoded $DATADIR/bootstrap.dat @@ -1419,7 +1419,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) return InitError(_("Incorrect or no genesis block found. Wrong datadir for network?")); // Initialize the block index (no-op if non-empty database was already loaded) - if (!InitBlockIndex(chainparams)) { + 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 3ba81ed17..8ad955e7a 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -72,7 +72,7 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha pblocktree = new CBlockTreeDB(1 << 20, true); pcoinsdbview = new CCoinsViewDB(1 << 23, true); pcoinsTip = new CCoinsViewCache(pcoinsdbview); - if (!InitBlockIndex(chainparams)) { + if (!LoadGenesisBlock(chainparams)) { throw std::runtime_error("InitBlockIndex failed."); } { diff --git a/src/validation.cpp b/src/validation.cpp index d7d880d24..ebbdb7e65 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3864,42 +3864,55 @@ void UnloadBlockIndex() bool LoadBlockIndex(const CChainParams& chainparams) { // Load block index from databases - if (!fReindex && !LoadBlockIndexDB(chainparams)) - return false; + bool needs_init = fReindex; + if (!fReindex) { + bool ret = LoadBlockIndexDB(chainparams); + if (!ret) return false; + needs_init = mapBlockIndex.empty(); + } + + if (needs_init) { + // Everything here is for *new* reindex/DBs. Thus, though + // LoadBlockIndexDB may have set fReindex if we shut down + // mid-reindex previously, we don't check fReindex and + // instead only check it prior to LoadBlockIndexDB to set + // needs_init. + + LogPrintf("Initializing databases...\n"); + // Use the provided setting for -txindex in the new database + fTxIndex = GetBoolArg("-txindex", DEFAULT_TXINDEX); + pblocktree->WriteFlag("txindex", fTxIndex); + } return true; } -bool InitBlockIndex(const CChainParams& chainparams) +bool LoadGenesisBlock(const CChainParams& chainparams) { LOCK(cs_main); - // Check whether we're already initialized - if (chainActive.Genesis() != NULL) + // Check whether we're already initialized by checking for genesis in + // mapBlockIndex. Note that we can't use chainActive here, since it is + // set based on the coins db, not the block index db, which is the only + // thing loaded at this point. + if (mapBlockIndex.count(chainparams.GenesisBlock().GetHash())) return true; - // Use the provided setting for -txindex in the new database - fTxIndex = GetBoolArg("-txindex", DEFAULT_TXINDEX); - pblocktree->WriteFlag("txindex", fTxIndex); - LogPrintf("Initializing databases...\n"); - // Only add the genesis block if not reindexing (in which case we reuse the one already on disk) - if (!fReindex) { - try { - CBlock &block = const_cast(chainparams.GenesisBlock()); - // Start new block file - unsigned int nBlockSize = ::GetSerializeSize(block, SER_DISK, CLIENT_VERSION); - CDiskBlockPos blockPos; - CValidationState state; - if (!FindBlockPos(state, blockPos, nBlockSize+8, 0, block.GetBlockTime())) - return error("LoadBlockIndex(): FindBlockPos failed"); - if (!WriteBlockToDisk(block, blockPos, chainparams.MessageStart())) - return error("LoadBlockIndex(): writing genesis block to disk failed"); - CBlockIndex *pindex = AddToBlockIndex(block); - if (!ReceivedBlockTransactions(block, state, pindex, blockPos, chainparams.GetConsensus())) - return error("LoadBlockIndex(): genesis block not accepted"); - } catch (const std::runtime_error& e) { - return error("LoadBlockIndex(): failed to initialize block database: %s", e.what()); - } + try { + CBlock &block = const_cast(chainparams.GenesisBlock()); + // Start new block file + unsigned int nBlockSize = ::GetSerializeSize(block, SER_DISK, CLIENT_VERSION); + CDiskBlockPos blockPos; + CValidationState state; + if (!FindBlockPos(state, blockPos, nBlockSize+8, 0, block.GetBlockTime())) + return error("%s: FindBlockPos failed", __func__); + if (!WriteBlockToDisk(block, blockPos, chainparams.MessageStart())) + return error("%s: writing genesis block to disk failed", __func__); + CBlockIndex *pindex = AddToBlockIndex(block); + if (!ReceivedBlockTransactions(block, state, pindex, blockPos, chainparams.GetConsensus())) + return error("%s: genesis block not accepted", __func__); + } catch (const std::runtime_error& e) { + return error("%s: failed to write genesis block: %s", __func__, e.what()); } return true; diff --git a/src/validation.h b/src/validation.h index a9f995abb..3ce5023f0 100644 --- a/src/validation.h +++ b/src/validation.h @@ -256,9 +256,10 @@ FILE* OpenBlockFile(const CDiskBlockPos &pos, bool fReadOnly = false); fs::path GetBlockPosFilename(const CDiskBlockPos &pos, const char *prefix); /** Import blocks from an external file */ bool LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, CDiskBlockPos *dbp = NULL); -/** Initialize a new block tree database + block data on disk */ -bool InitBlockIndex(const CChainParams& chainparams); -/** Load the block tree and coins database from disk */ +/** Ensures we have a genesis block in the block tree, possibly writing one to disk. */ +bool LoadGenesisBlock(const CChainParams& chainparams); +/** Load the block tree and coins database from disk, + * initializing state if we're running with -reindex. */ bool LoadBlockIndex(const CChainParams& chainparams); /** Update the chain tip based on database information. */ void LoadChainTip(const CChainParams& chainparams); From b0f32497b873cd1eaf0be86f8e265355aa86174f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 6 Jul 2017 20:00:11 -0400 Subject: [PATCH 2/5] More user-friendly error message if UTXO DB runs ahead of block DB This gives LoadChainTip a return value - allowing it to indicate that the UTXO DB ran ahead of the block DB. This just provides a nicer error message instead of the previous mysterious assert(!setBlockIndexCandidates.empty()) error. This also calls ActivateBestChain in case we just loaded the genesis block in LoadChainTip, avoiding relying on the ActivateBestChain in ThreadImport before continuing init process. --- src/init.cpp | 10 +++++++++- src/validation.cpp | 17 ++++++++++++++--- src/validation.h | 2 +- 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index b1fe8e7d3..8fec69c2b 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1442,7 +1442,15 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) break; } pcoinsTip = new CCoinsViewCache(pcoinscatcher); - LoadChainTip(chainparams); + + if (!fReindex && !fReindexChainState) { + // LoadChainTip sets chainActive based on pcoinsTip's best block + if (!LoadChainTip(chainparams)) { + strLoadError = _("Error initializing block database"); + break; + } + assert(chainActive.Tip() != NULL); + } if (!fReindex && chainActive.Tip() != NULL) { uiInterface.InitMessage(_("Rewinding blocks...")); diff --git a/src/validation.cpp b/src/validation.cpp index ebbdb7e65..91982ead9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3538,14 +3538,24 @@ bool static LoadBlockIndexDB(const CChainParams& chainparams) return true; } -void LoadChainTip(const CChainParams& chainparams) +bool LoadChainTip(const CChainParams& chainparams) { - if (chainActive.Tip() && chainActive.Tip()->GetBlockHash() == pcoinsTip->GetBestBlock()) return; + if (chainActive.Tip() && chainActive.Tip()->GetBlockHash() == pcoinsTip->GetBestBlock()) return true; + + if (pcoinsTip->GetBestBlock().IsNull() && mapBlockIndex.size() == 1) { + // In case we just added the genesis block, connect it now, so + // that we always have a chainActive.Tip() when we return. + LogPrintf("%s: Connecting genesis block...\n", __func__); + CValidationState state; + if (!ActivateBestChain(state, chainparams)) { + return false; + } + } // Load pointer to end of best chain BlockMap::iterator it = mapBlockIndex.find(pcoinsTip->GetBestBlock()); if (it == mapBlockIndex.end()) - return; + return false; chainActive.SetTip(it->second); PruneBlockIndexCandidates(); @@ -3554,6 +3564,7 @@ void LoadChainTip(const CChainParams& chainparams) chainActive.Tip()->GetBlockHash().ToString(), chainActive.Height(), DateTimeStrFormat("%Y-%m-%d %H:%M:%S", chainActive.Tip()->GetBlockTime()), GuessVerificationProgress(chainparams.TxData(), chainActive.Tip())); + return true; } CVerifyDB::CVerifyDB() diff --git a/src/validation.h b/src/validation.h index 3ce5023f0..b1d482cbf 100644 --- a/src/validation.h +++ b/src/validation.h @@ -262,7 +262,7 @@ bool LoadGenesisBlock(const CChainParams& chainparams); * initializing state if we're running with -reindex. */ bool LoadBlockIndex(const CChainParams& chainparams); /** Update the chain tip based on database information. */ -void LoadChainTip(const CChainParams& chainparams); +bool LoadChainTip(const CChainParams& chainparams); /** Unload database information */ void UnloadBlockIndex(); /** Run an instance of the script checking thread */ From ff3a21919d97f7500978b4160199336e4b50b36a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 6 Jul 2017 21:29:46 -0400 Subject: [PATCH 3/5] Call RewindBlockIndex even if we're about to run -reindex-chainstate RewindBlockIndex works over both chainActive - disconnecting blocks from the tip that need witness verification - and mapBlockIndex - requiring redownload of blocks missing witness data. It should never have been the case that the second half is skipped if we're about to run -reindex-chainstate. --- src/init.cpp | 5 ++++- src/validation.cpp | 17 +++++++++++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 8fec69c2b..2ea778a5b 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1452,7 +1452,10 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) assert(chainActive.Tip() != NULL); } - if (!fReindex && chainActive.Tip() != NULL) { + if (!fReindex) { + // 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. uiInterface.InitMessage(_("Rewinding blocks...")); if (!RewindBlockIndex(chainparams)) { strLoadError = _("Unable to rewind the database to a pre-fork state. You will need to redownload the blockchain"); diff --git a/src/validation.cpp b/src/validation.cpp index 91982ead9..7ec77406e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3763,6 +3763,8 @@ bool RewindBlockIndex(const CChainParams& params) { LOCK(cs_main); + // Note that during -reindex-chainstate we are called with an empty chainActive! + int nHeight = 1; while (nHeight <= chainActive.Height()) { if (IsWitnessEnabled(chainActive[nHeight - 1], params.GetConsensus()) && !(chainActive[nHeight]->nStatus & BLOCK_OPT_WITNESS)) { @@ -3832,12 +3834,19 @@ bool RewindBlockIndex(const CChainParams& params) } } - PruneBlockIndexCandidates(); + if (chainActive.Tip() != NULL) { + // We can't prune block index candidates based on our tip if we have + // no tip due to chainActive being empty! + PruneBlockIndexCandidates(); - CheckBlockIndex(params.GetConsensus()); + CheckBlockIndex(params.GetConsensus()); - if (!FlushStateToDisk(params, state, FLUSH_STATE_ALWAYS)) { - return false; + // FlushStateToDisk can possibly read chainActive. Be conservative + // and skip it here, we're about to -reindex-chainstate anyway, so + // it'll get called a bunch real soon. + if (!FlushStateToDisk(params, state, FLUSH_STATE_ALWAYS)) { + return false; + } } return true; From 138569722cae09bb9e3bc31bbae4b1886b904bb5 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 6 Jul 2017 21:32:08 -0400 Subject: [PATCH 4/5] Order chainstate init more logically. * Order chainstate init more logically - first all of the blocktree-related loading, then coinsdb, then pcoinsTip/chainActive. Only create objects as needed. * More clearly document exactly what is and isn't called in -reindex and -reindex-chainstate both with comments noting calls as no-ops and by adding if guards. * Move LoadGenesisBlock further down in init. This is a more logical location for it, as it is after all of the blockindex-related loading and checking, but before any of the UTXO-related loading and checking. * Move all of the VerifyDB()-related stuff into a -reindex + -reindex-chainstate if guard. It couldn't do anything useful as chainActive.Tip() would be null at this point anyway. --- src/init.cpp | 82 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 49 insertions(+), 33 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 2ea778a5b..5b7379e16 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1391,23 +1391,19 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) delete pblocktree; pblocktree = new CBlockTreeDB(nBlockTreeDBCache, false, fReindex); - pcoinsdbview = new CCoinsViewDB(nCoinDBCache, false, fReindex || fReindexChainState); - pcoinscatcher = new CCoinsViewErrorCatcher(pcoinsdbview); if (fReindex) { pblocktree->WriteReindexing(true); //If we're reindexing in prune mode, wipe away unusable block files and all undo data files if (fPruneMode) CleanupBlockRevFiles(); - } else { - // If necessary, upgrade from older database format. - if (!pcoinsdbview->Upgrade()) { - strLoadError = _("Error upgrading chainstate database"); - break; - } } + if (fRequestShutdown) break; + // 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. if (!LoadBlockIndex(chainparams)) { strLoadError = _("Error loading block database"); break; @@ -1418,12 +1414,6 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) if (!mapBlockIndex.empty() && mapBlockIndex.count(chainparams.GetConsensus().hashGenesisBlock) == 0) return InitError(_("Incorrect or no genesis block found. Wrong datadir for network?")); - // Initialize the block index (no-op if non-empty database was already loaded) - if (!fReindex && !LoadGenesisBlock(chainparams)) { - strLoadError = _("Error initializing block database"); - break; - } - // Check for changed -txindex state if (fTxIndex != GetBoolArg("-txindex", DEFAULT_TXINDEX)) { strLoadError = _("You need to rebuild the database using -reindex-chainstate to change -txindex"); @@ -1437,10 +1427,34 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) break; } + // 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 (!fReindex && !LoadGenesisBlock(chainparams)) { + strLoadError = _("Error initializing block database"); + break; + } + + // 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); + pcoinscatcher = new CCoinsViewErrorCatcher(pcoinsdbview); + + // If necessary, upgrade from older database format. + // This is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate + if (!pcoinsdbview->Upgrade()) { + strLoadError = _("Error upgrading chainstate database"); + break; + } + + // ReplayBlocks is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate if (!ReplayBlocks(chainparams, pcoinsdbview)) { strLoadError = _("Unable to replay blocks. You will need to rebuild the database using -reindex-chainstate."); break; } + + // The on-disk coinsdb is now in a good state, create the cache pcoinsTip = new CCoinsViewCache(pcoinscatcher); if (!fReindex && !fReindexChainState) { @@ -1463,28 +1477,30 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler) } } - 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", - MIN_BLOCKS_TO_KEEP); - } + if (!fReindex && !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", + MIN_BLOCKS_TO_KEEP); + } - { - LOCK(cs_main); - CBlockIndex* tip = chainActive.Tip(); - RPCNotifyBlockChange(true, tip); - if (tip && tip->nTime > GetAdjustedTime() + 2 * 60 * 60) { - strLoadError = _("The block database contains a block which appears to be from the future. " - "This may be due to your computer's date and time being set incorrectly. " - "Only rebuild the block database if you are sure that your computer's date and time are correct"); - break; + { + LOCK(cs_main); + CBlockIndex* tip = chainActive.Tip(); + RPCNotifyBlockChange(true, tip); + if (tip && tip->nTime > GetAdjustedTime() + 2 * 60 * 60) { + strLoadError = _("The block database contains a block which appears to be from the future. " + "This may be due to your computer's date and time being set incorrectly. " + "Only rebuild the block database if you are sure that your computer's date and time are correct"); + break; + } } - } - if (!CVerifyDB().VerifyDB(chainparams, pcoinsdbview, GetArg("-checklevel", DEFAULT_CHECKLEVEL), - GetArg("-checkblocks", DEFAULT_CHECKBLOCKS))) { - strLoadError = _("Corrupted block database detected"); - break; + if (!CVerifyDB().VerifyDB(chainparams, pcoinsdbview, GetArg("-checklevel", DEFAULT_CHECKLEVEL), + GetArg("-checkblocks", DEFAULT_CHECKBLOCKS))) { + strLoadError = _("Corrupted block database detected"); + break; + } } } catch (const std::exception& e) { LogPrintf("%s\n", e.what()); From c0025d0a92e438155901438580e4d2ebb376b951 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 15 Jul 2017 20:03:11 -0400 Subject: [PATCH 5/5] Fix segfault when shutting down before fully loading This was introduced by 3192975f1d177aa9f0bbd823c6387cfbfa943610. It can be triggered easily when canceling DB upgrade from pre-per-utxo. --- src/init.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index 5b7379e16..2ca3ec17a 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -216,7 +216,9 @@ void Shutdown() } // FlushStateToDisk generates a SetBestChain callback, which we should avoid missing - FlushStateToDisk(); + if (pcoinsTip != nullptr) { + FlushStateToDisk(); + } // After there are no more peers/RPC left to give us new data which may generate // CValidationInterface callbacks, flush them...