From 9bcc942a01662883bcae2eae2fc1c3721cddfe5b Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 2 Mar 2021 10:37:09 +0100 Subject: [PATCH 1/5] Fix some LoadChainTip-related init-order bugs. >>> adapts bitcoin/bitcoin@eda888e57352037ab2e60f6ef90098b3ce23a157 * Move the writing of fTxIndex to LoadBlockIndex - this fixes a bug introduced in d6af06d 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_pivx.cpp | 2 +- src/validation.cpp | 71 ++++++++++++++++++++++++------------------ src/validation.h | 7 +++-- 4 files changed, 48 insertions(+), 36 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index cc2c917397f6..a179ea5bf73c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -688,7 +688,7 @@ void ThreadImport(const 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(); + LoadGenesisBlock(); } // hardcoded $DATADIR/bootstrap.dat @@ -1620,7 +1620,7 @@ bool AppInitMain() return UIError(_("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()) { + if (!LoadGenesisBlock()) { strLoadError = _("Error initializing block database"); break; } diff --git a/src/test/test_pivx.cpp b/src/test/test_pivx.cpp index d1aa0e6baca5..5e009205a53a 100644 --- a/src/test/test_pivx.cpp +++ b/src/test/test_pivx.cpp @@ -72,7 +72,7 @@ TestingSetup::TestingSetup() pblocktree = new CBlockTreeDB(1 << 20, true); pcoinsdbview = new CCoinsViewDB(1 << 23, true); pcoinsTip = new CCoinsViewCache(pcoinsdbview); - InitBlockIndex(); + LoadGenesisBlock(); { CValidationState state; bool ok = ActivateBestChain(state); diff --git a/src/validation.cpp b/src/validation.cpp index b18919d97158..5e882b3b21ed 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3743,45 +3743,56 @@ void UnloadBlockIndex() bool LoadBlockIndex(std::string& strError) { - // Load block index from databases - if (!fReindex && !LoadBlockIndexDB(strError)) - return false; + bool needs_init = fReindex; + if (!fReindex) { + if (!LoadBlockIndexDB(strError)) + 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 = gArgs.GetBoolArg("-txindex", DEFAULT_TXINDEX); + pblocktree->WriteFlag("txindex", fTxIndex); + } return true; } -bool InitBlockIndex() +bool LoadGenesisBlock() { 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(Params().GenesisBlock().GetHash())) return true; - // Use the provided setting for -txindex in the new database - fTxIndex = gArgs.GetBoolArg("-txindex", true); - 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(Params().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)) - return error("LoadBlockIndex() : writing genesis block to disk failed"); - CBlockIndex* pindex = AddToBlockIndex(block); - if (!ReceivedBlockTransactions(block, state, pindex, blockPos)) - 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(Params().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)) + return error("%s: writing genesis block to disk failed", __func__); + CBlockIndex *pindex = AddToBlockIndex(block); + if (!ReceivedBlockTransactions(block, state, pindex, blockPos)) + 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 407455208ba9..d0ff2c1d9f3f 100644 --- a/src/validation.h +++ b/src/validation.h @@ -183,9 +183,10 @@ FILE* OpenUndoFile(const CDiskBlockPos& pos, bool fReadOnly = false); fs::path GetBlockPosFilename(const CDiskBlockPos& pos, const char* prefix); /** Import blocks from an external file */ bool LoadExternalBlockFile(FILE* fileIn, CDiskBlockPos* dbp = NULL); -/** Initialize a new block tree database + block data on disk */ -bool InitBlockIndex(); -/** 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(); +/** Load the block tree and coins database from disk, + * initializing state if we're running with -reindex. */ bool LoadBlockIndex(std::string& strError); /** Update the chain tip based on database information. */ void LoadChainTip(const CChainParams& chainparams); From 9b87537d59f5aefd6d3206e1318c045411213c15 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 | 12 +++++++++++- src/validation.cpp | 20 ++++++++++++++++---- src/validation.h | 2 +- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index a179ea5bf73c..f2a54b1f2cd9 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1636,7 +1636,17 @@ bool AppInitMain() break; } pcoinsTip = new CCoinsViewCache(pcoinscatcher); - LoadChainTip(chainparams); + + // !TODO: after enabling reindex-chainstate + // if (!fReindex && !fReindexChainState) { + if (!fReindex) { + // LoadChainTip sets chainActive based on pcoinsTip's best block + if (!LoadChainTip(chainparams)) { + strLoadError = _("Error initializing block database"); + break; + } + assert(chainActive.Tip() != NULL); + } // Populate list of invalid/fraudulent outpoints that are banned from the chain invalid_out::LoadOutpoints(); diff --git a/src/validation.cpp b/src/validation.cpp index 5e882b3b21ed..dcee17ea8a0e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3517,14 +3517,25 @@ bool static LoadBlockIndexDB(std::string& strError) 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)) { + return false; + } + } // Load pointer to end of best chain BlockMap::iterator it = mapBlockIndex.find(pcoinsTip->GetBestBlock()); - if (it == mapBlockIndex.end()) - return; + if (it == mapBlockIndex.end()) { + return false; + } chainActive.SetTip(it->second); PruneBlockIndexCandidates(); @@ -3535,6 +3546,7 @@ void LoadChainTip(const CChainParams& chainparams) pChainTip->GetBlockHash().GetHex(), pChainTip->nHeight, DateTimeStrFormat("%Y-%m-%d %H:%M:%S", pChainTip->GetBlockTime()), Checkpoints::GuessVerificationProgress(pChainTip)); + return true; } CVerifyDB::CVerifyDB() diff --git a/src/validation.h b/src/validation.h index d0ff2c1d9f3f..d6f37cfc6ffb 100644 --- a/src/validation.h +++ b/src/validation.h @@ -189,7 +189,7 @@ bool LoadGenesisBlock(); * initializing state if we're running with -reindex. */ bool LoadBlockIndex(std::string& strError); /** Update the chain tip based on database information. */ -void LoadChainTip(const CChainParams& chainparams); +bool LoadChainTip(const CChainParams& chainparams); /** Unload database information */ void UnloadBlockIndex(); /** See whether the protocol update is enforced for connected nodes */ From 5f1f014f14ff532eb1714ee8236ad056b0a4187a Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 2 Mar 2021 11:03:10 +0100 Subject: [PATCH 3/5] Order chainstate init more logically. >>> adapts bitcoin/bitcoin@138569722cae09bb9e3bc31bbae4b1886b904bb5 * 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 | 44 ++++++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index f2a54b1f2cd9..d00ba56ede4b 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1581,18 +1581,9 @@ bool AppInitMain() pSporkDB = new CSporkDB(0, false, false); pblocktree = new CBlockTreeDB(nBlockTreeDBCache, false, fReindex); - pcoinsdbview = new CCoinsViewDB(nCoinDBCache, false, fReindex); - pcoinscatcher = new CCoinsViewErrorCatcher(pcoinsdbview); if (fReindex) { pblocktree->WriteReindexing(true); - } else { - uiInterface.InitMessage(_("Upgrading coins database...")); - // If necessary, upgrade from older database format. - if (!pcoinsdbview->Upgrade()) { - strLoadError = _("Error upgrading chainstate database"); - break; - } } // End loop if shutdown was requested @@ -1602,6 +1593,9 @@ bool AppInitMain() uiInterface.InitMessage(_("Loading sporks...")); sporkManager.LoadSporksFromDB(); + // 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. uiInterface.InitMessage(_("Loading block index...")); std::string strBlockIndexError; if (!LoadBlockIndex(strBlockIndexError)) { @@ -1619,22 +1613,42 @@ bool AppInitMain() if (!mapBlockIndex.empty() && mapBlockIndex.count(consensus.hashGenesisBlock) == 0) return UIError(_("Incorrect or no genesis block found. Wrong datadir for network?")); - // Initialize the block index (no-op if non-empty database was already loaded) - if (!LoadGenesisBlock()) { + // Check for changed -txindex state + if (fTxIndex != gArgs.GetBoolArg("-txindex", DEFAULT_TXINDEX)) { + strLoadError = _("You need to rebuild the database using -reindex to change -txindex"); + 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()) { strLoadError = _("Error initializing block database"); break; } - // Check for changed -txindex state - if (fTxIndex != gArgs.GetBoolArg("-txindex", DEFAULT_TXINDEX)) { - strLoadError = _("You need to rebuild the database using -reindex to change -txindex"); + // At this point we're either in reindex or we've loaded a useful + // block tree into mapBlockIndex! + + pcoinsdbview = new CCoinsViewDB(nCoinDBCache, false, fReindex); + 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 !TODO) + uiInterface.InitMessage(_("Upgrading coins database if needed...")); + // If necessary, upgrade from older database format. + if (!pcoinsdbview->Upgrade()) { + strLoadError = _("Error upgrading chainstate database"); break; } + // ReplayBlocks is a no-op if we cleared the coinsviewdb with -reindex (or -reindex-chainstate !TODO) if (!ReplayBlocks(chainparams, pcoinsdbview)) { strLoadError = _("Unable to replay blocks. You will need to rebuild the database using -reindex."); break; } + + // The on-disk coinsdb is now in a good state, create the cache pcoinsTip = new CCoinsViewCache(pcoinscatcher); // !TODO: after enabling reindex-chainstate @@ -1674,6 +1688,8 @@ bool AppInitMain() } } + // !TODO: after enabling reindex-chainstate + // if (!fReindex && !fReindexChainState) { if (!fReindex) { uiInterface.InitMessage(_("Verifying blocks...")); From 43cc880a9b28e3ebfd4af687cc100e9947c8708c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 15 Jul 2017 20:03:11 -0400 Subject: [PATCH 4/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 d00ba56ede4b..d32d7e6b4778 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -263,7 +263,9 @@ void PrepareShutdown() } // 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... From 4749d5288990a884ea3b695eb673fb036c87e06a Mon Sep 17 00:00:00 2001 From: random-zebra Date: Tue, 2 Mar 2021 23:35:37 +0100 Subject: [PATCH 5/5] [Refactoring][BUG] Unchecked LoadGenesisBlock return value --- src/init.cpp | 4 +++- src/test/test_pivx.cpp | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index d32d7e6b4778..74bd55904af5 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -690,7 +690,9 @@ void ThreadImport(const 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): - LoadGenesisBlock(); + if (!LoadGenesisBlock()) { + throw std::runtime_error("Error initializing block database"); + } } // hardcoded $DATADIR/bootstrap.dat diff --git a/src/test/test_pivx.cpp b/src/test/test_pivx.cpp index 5e009205a53a..2d4b2c3b5423 100644 --- a/src/test/test_pivx.cpp +++ b/src/test/test_pivx.cpp @@ -72,7 +72,9 @@ TestingSetup::TestingSetup() pblocktree = new CBlockTreeDB(1 << 20, true); pcoinsdbview = new CCoinsViewDB(1 << 23, true); pcoinsTip = new CCoinsViewCache(pcoinsdbview); - LoadGenesisBlock(); + if (!LoadGenesisBlock()) { + throw std::runtime_error("Error initializing block database"); + } { CValidationState state; bool ok = ActivateBestChain(state);