From 3f5b5f3f6db0e5716911b3fba1460ce327e8a845 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Tue, 25 Aug 2020 19:38:23 -0400 Subject: [PATCH] validation: Move FindFilesToPrune{,Manual} to BlockManager [META] No behaviour change is intended in this commit. [META] This commit should be followed up by removing the comments and assertions meant only to show that the change is correct. Also stop FindFilesToPrune{,Manual} from unnecessary reaching for ::ChainActive() by passing in the necessary information. --- src/validation.cpp | 66 +++++++++++++++++++++++++++++++++++----------- src/validation.h | 10 ++++++- 2 files changed, 60 insertions(+), 16 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 76f8bcba7..389faa99a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -197,9 +197,6 @@ CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& loc std::unique_ptr pblocktree; -// See definition for documentation -static void FindFilesToPruneManual(ChainstateManager& chainman, std::set& setFilesToPrune, int nManualPruneHeight); -static void FindFilesToPrune(ChainstateManager& chainman, std::set& setFilesToPrune, uint64_t nPruneAfterHeight); bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks = nullptr); static FILE* OpenUndoFile(const FlatFilePos &pos, bool fReadOnly = false); static FlatFileSeq BlockFileSeq(); @@ -2284,14 +2281,53 @@ bool CChainState::FlushStateToDisk( CoinsCacheSizeState cache_state = GetCoinsCacheSizeState(&m_mempool); LOCK(cs_LastBlockFile); if (fPruneMode && (fCheckForPruning || nManualPruneHeight > 0) && !fReindex) { + // Previously, we called the global function ::ChainActive() in + // FindFilesToPrune{,Manual} to get the tip height and to determine + // whether or not a tip even exists. Now, we are simply passing in + // m_chain.Height() (which returns -1 if the tip doesn't exist). To + // make sure we're not changing behaviour, let's check that + // ::ChainActive() is the same object as m_chain (not just + // identical). + // + // This comment and the following assert will be removed in a + // subsequent commit, as they're just meant to demonstrate + // correctness (you can run tests against it and see that nothing + // exit unexpectedly). + assert(std::addressof(::ChainActive()) == std::addressof(m_chain)); if (nManualPruneHeight > 0) { LOG_TIME_MILLIS_WITH_CATEGORY("find files to prune (manual)", BCLog::BENCH); - FindFilesToPruneManual(g_chainman, setFilesToPrune, nManualPruneHeight); + m_blockman.FindFilesToPruneManual(setFilesToPrune, nManualPruneHeight, m_chain.Height()); } else { LOG_TIME_MILLIS_WITH_CATEGORY("find files to prune", BCLog::BENCH); - FindFilesToPrune(g_chainman, setFilesToPrune, chainparams.PruneAfterHeight()); + // Previously, we called the global function + // ::ChainstateActive() in FindFilesToPrune{,Manual} to get the + // IBD status. Now, we are simply passing in + // IsInitialBlockDownload(). To make sure we're not changing + // behaviour, let's check that ::ChainstateActive() is the same + // object as *this (not just identical). + // + // This comment and the following assert will be removed in a + // subsequent commit, as they're just meant to demonstrate + // correctness (you can run tests against it and see that + // nothing exit unexpectedly). + assert(std::addressof(::ChainstateActive()) == std::addressof(*this)); + + // Previously, we called PruneOneBlockFile on g_chainman's + // m_blockman in FindFilesToPrune{,Manual}. Now, we are instead + // calling PruneOneBlockFile on _our_ m_blockman. To make sure + // we're not changing behaviour, let's check that + // g_chainman.m_blockman is the same object as _our_ m_blockman + // (not just identical). + // + // This comment and the following assert will be removed in a + // subsequent commit, as they're just meant to demonstrate + // correctness (you can run tests against it and see that + // nothing exit unexpectedly). + assert(std::addressof(g_chainman.m_blockman) == std::addressof(m_blockman)); + + m_blockman.FindFilesToPrune(setFilesToPrune, chainparams.PruneAfterHeight(), m_chain.Height(), IsInitialBlockDownload()); fCheckForPruning = false; } if (!setFilesToPrune.empty()) { @@ -3958,21 +3994,21 @@ void UnlinkPrunedFiles(const std::set& setFilesToPrune) } /* Calculate the block/rev files to delete based on height specified by user with RPC command pruneblockchain */ -static void FindFilesToPruneManual(ChainstateManager& chainman, std::set& setFilesToPrune, int nManualPruneHeight) +void BlockManager::FindFilesToPruneManual(std::set& setFilesToPrune, int nManualPruneHeight, int chain_tip_height) { assert(fPruneMode && nManualPruneHeight > 0); LOCK2(cs_main, cs_LastBlockFile); - if (::ChainActive().Tip() == nullptr) + if (chain_tip_height < 0) return; // last block to prune is the lesser of (user-specified height, MIN_BLOCKS_TO_KEEP from the tip) - unsigned int nLastBlockWeCanPrune = std::min((unsigned)nManualPruneHeight, ::ChainActive().Tip()->nHeight - MIN_BLOCKS_TO_KEEP); + unsigned int nLastBlockWeCanPrune = std::min((unsigned)nManualPruneHeight, chain_tip_height - MIN_BLOCKS_TO_KEEP); int count=0; for (int fileNumber = 0; fileNumber < nLastBlockFile; fileNumber++) { if (vinfoBlockFile[fileNumber].nSize == 0 || vinfoBlockFile[fileNumber].nHeightLast > nLastBlockWeCanPrune) continue; - chainman.m_blockman.PruneOneBlockFile(fileNumber); + PruneOneBlockFile(fileNumber); setFilesToPrune.insert(fileNumber); count++; } @@ -4005,17 +4041,17 @@ void PruneBlockFilesManual(int nManualPruneHeight) * * @param[out] setFilesToPrune The set of file indices that can be unlinked will be returned */ -static void FindFilesToPrune(ChainstateManager& chainman, std::set& setFilesToPrune, uint64_t nPruneAfterHeight) +void BlockManager::FindFilesToPrune(std::set& setFilesToPrune, uint64_t nPruneAfterHeight, int chain_tip_height, bool is_ibd) { LOCK2(cs_main, cs_LastBlockFile); - if (::ChainActive().Tip() == nullptr || nPruneTarget == 0) { + if (chain_tip_height < 0 || nPruneTarget == 0) { return; } - if ((uint64_t)::ChainActive().Tip()->nHeight <= nPruneAfterHeight) { + if ((uint64_t)chain_tip_height <= nPruneAfterHeight) { return; } - unsigned int nLastBlockWeCanPrune = ::ChainActive().Tip()->nHeight - MIN_BLOCKS_TO_KEEP; + unsigned int nLastBlockWeCanPrune = chain_tip_height - MIN_BLOCKS_TO_KEEP; uint64_t nCurrentUsage = CalculateCurrentUsage(); // We don't check to prune until after we've allocated new space for files // So we should leave a buffer under our target to account for another allocation @@ -4029,7 +4065,7 @@ static void FindFilesToPrune(ChainstateManager& chainman, std::set& setFile // To avoid excessive prune events negating the benefit of high dbcache // values, we should not prune too rapidly. // So when pruning in IBD, increase the buffer a bit to avoid a re-prune too soon. - if (::ChainstateActive().IsInitialBlockDownload()) { + if (is_ibd) { // Since this is only relevant during IBD, we use a fixed 10% nBuffer += nPruneTarget / 10; } @@ -4047,7 +4083,7 @@ static void FindFilesToPrune(ChainstateManager& chainman, std::set& setFile if (vinfoBlockFile[fileNumber].nHeightLast > nLastBlockWeCanPrune) continue; - chainman.m_blockman.PruneOneBlockFile(fileNumber); + PruneOneBlockFile(fileNumber); // Queue up the files for removal setFilesToPrune.insert(fileNumber); nCurrentUsage -= nBytesToPrune; diff --git a/src/validation.h b/src/validation.h index 40611fa61..94bf16968 100644 --- a/src/validation.h +++ b/src/validation.h @@ -356,7 +356,15 @@ struct CBlockIndexWorkComparator * This data is used mostly in `CChainState` - information about, e.g., * candidate tips is not maintained here. */ -class BlockManager { +class BlockManager +{ + friend CChainState; + +private: + // See definition for documentation + void FindFilesToPruneManual(std::set& setFilesToPrune, int nManualPruneHeight, int chain_tip_height); + void FindFilesToPrune(std::set& setFilesToPrune, uint64_t nPruneAfterHeight, int chain_tip_height, bool is_ibd); + public: BlockMap m_block_index GUARDED_BY(cs_main);