From fa3d62cf7b3501a056b34c5458c14d2fe6a55bd7 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 23 Nov 2021 18:07:00 +0100 Subject: [PATCH 1/4] Move FindForkInGlobalIndex from BlockManager to CChainState The helper was moved in commit b026e318c39f59a06e29f1b25c7f577e01b25ccb, which also mentioned that it could be moved to CChainState. So do that, as the functionality is not block-storage related. This also allows to drop one function argument. --- src/index/base.cpp | 2 +- src/net_processing.cpp | 4 ++-- src/node/interfaces.cpp | 4 ++-- src/validation.cpp | 13 +++++++------ src/validation.h | 6 +++--- 5 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index 8525dcbfa02..419b6fa9829 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -65,7 +65,7 @@ bool BaseIndex::Init() if (locator.IsNull()) { m_best_block_index = nullptr; } else { - m_best_block_index = m_chainstate->m_blockman.FindForkInGlobalIndex(active_chain, locator); + m_best_block_index = m_chainstate->FindForkInGlobalIndex(locator); } m_synced = m_best_block_index.load() == active_chain.Tip(); if (!m_synced) { diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2a30afdb5b2..d4f0e940563 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3083,7 +3083,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, LOCK(cs_main); // Find the last block the caller has in the main chain - const CBlockIndex* pindex = m_chainman.m_blockman.FindForkInGlobalIndex(m_chainman.ActiveChain(), locator); + const CBlockIndex* pindex = m_chainman.ActiveChainstate().FindForkInGlobalIndex(locator); // Send the rest of the chain if (pindex) @@ -3203,7 +3203,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, else { // Find the last block the caller has in the main chain - pindex = m_chainman.m_blockman.FindForkInGlobalIndex(m_chainman.ActiveChain(), locator); + pindex = m_chainman.ActiveChainstate().FindForkInGlobalIndex(locator); if (pindex) pindex = m_chainman.ActiveChain().Next(pindex); } diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 075842ef73f..8109dce2c0a 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -494,8 +494,8 @@ class ChainImpl : public Chain std::optional findLocatorFork(const CBlockLocator& locator) override { LOCK(cs_main); - const CChain& active = Assert(m_node.chainman)->ActiveChain(); - if (CBlockIndex* fork = m_node.chainman->m_blockman.FindForkInGlobalIndex(active, locator)) { + const CChainState& active = Assert(m_node.chainman)->ActiveChainstate(); + if (CBlockIndex* fork = active.FindForkInGlobalIndex(locator)) { return fork->nHeight; } return std::nullopt; diff --git a/src/validation.cpp b/src/validation.cpp index 1aac71fb0fb..c2d86d89c5b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -155,23 +155,24 @@ CBlockIndex* BlockManager::LookupBlockIndex(const uint256& hash) const return it == m_block_index.end() ? nullptr : it->second; } -CBlockIndex* BlockManager::FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator) +CBlockIndex* CChainState::FindForkInGlobalIndex(const CBlockLocator& locator) const { AssertLockHeld(cs_main); // Find the latest block common to locator and chain - we expect that // locator.vHave is sorted descending by height. for (const uint256& hash : locator.vHave) { - CBlockIndex* pindex = LookupBlockIndex(hash); + CBlockIndex* pindex{m_blockman.LookupBlockIndex(hash)}; if (pindex) { - if (chain.Contains(pindex)) + if (m_chain.Contains(pindex)) { return pindex; - if (pindex->GetAncestor(chain.Height()) == chain.Tip()) { - return chain.Tip(); + } + if (pindex->GetAncestor(m_chain.Height()) == m_chain.Tip()) { + return m_chain.Tip(); } } } - return chain.Genesis(); + return m_chain.Genesis(); } bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, diff --git a/src/validation.h b/src/validation.h index 534df9bc87c..27b8f4e906f 100644 --- a/src/validation.h +++ b/src/validation.h @@ -466,9 +466,6 @@ class BlockManager CBlockIndex* LookupBlockIndex(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); - /** Find the last common block between the parameter chain and a locator. */ - CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - //! Returns last CBlockIndex* that is a checkpoint CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -756,6 +753,9 @@ class CChainState /** Check whether we are doing an initial block download (synchronizing from disk or network) */ bool IsInitialBlockDownload() const; + /** Find the last common block of this chain and a locator. */ + CBlockIndex* FindForkInGlobalIndex(const CBlockLocator& locator) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); + /** * Make various assertions about the state of the block index. * From fa47b5c100f81c65c15b5a6afaf6c91bc0861264 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 23 Nov 2021 20:05:07 +0100 Subject: [PATCH 2/4] Move AcceptBlockHeader to ChainstateManager This is needed for the next commit. --- src/validation.cpp | 21 ++++++++++----------- src/validation.h | 21 +++++++++++---------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index c2d86d89c5b..60c01ad7b61 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3260,14 +3260,14 @@ static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& stat return true; } -bool BlockManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex) +bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex) { AssertLockHeld(cs_main); // Check for duplicate uint256 hash = block.GetHash(); - BlockMap::iterator miSelf = m_block_index.find(hash); + BlockMap::iterator miSelf{m_blockman.m_block_index.find(hash)}; if (hash != chainparams.GetConsensus().hashGenesisBlock) { - if (miSelf != m_block_index.end()) { + if (miSelf != m_blockman.m_block_index.end()) { // Block header is already known. CBlockIndex* pindex = miSelf->second; if (ppindex) @@ -3286,8 +3286,8 @@ bool BlockManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationS // Get prev block index CBlockIndex* pindexPrev = nullptr; - BlockMap::iterator mi = m_block_index.find(block.hashPrevBlock); - if (mi == m_block_index.end()) { + BlockMap::iterator mi{m_blockman.m_block_index.find(block.hashPrevBlock)}; + if (mi == m_blockman.m_block_index.end()) { LogPrint(BCLog::VALIDATION, "%s: %s prev block not found\n", __func__, hash.ToString()); return state.Invalid(BlockValidationResult::BLOCK_MISSING_PREV, "prev-blk-not-found"); } @@ -3296,7 +3296,7 @@ bool BlockManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationS LogPrint(BCLog::VALIDATION, "%s: %s prev block invalid\n", __func__, hash.ToString()); return state.Invalid(BlockValidationResult::BLOCK_INVALID_PREV, "bad-prevblk"); } - if (!ContextualCheckBlockHeader(block, state, *this, chainparams, pindexPrev, GetAdjustedTime())) { + if (!ContextualCheckBlockHeader(block, state, m_blockman, chainparams, pindexPrev, GetAdjustedTime())) { LogPrint(BCLog::VALIDATION, "%s: Consensus::ContextualCheckBlockHeader: %s, %s\n", __func__, hash.ToString(), state.ToString()); return false; } @@ -3325,7 +3325,7 @@ bool BlockManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationS // hasn't been validated up to BLOCK_VALID_SCRIPTS. This is a performance // optimization, in the common case of adding a new block to the tip, // we don't need to iterate over the failed blocks list. - for (const CBlockIndex* failedit : m_failed_blocks) { + for (const CBlockIndex* failedit : m_blockman.m_failed_blocks) { if (pindexPrev->GetAncestor(failedit->nHeight) == failedit) { assert(failedit->nStatus & BLOCK_FAILED_VALID); CBlockIndex* invalid_walk = pindexPrev; @@ -3340,7 +3340,7 @@ bool BlockManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationS } } } - CBlockIndex* pindex = AddToBlockIndex(block); + CBlockIndex* pindex{m_blockman.AddToBlockIndex(block)}; if (ppindex) *ppindex = pindex; @@ -3356,8 +3356,7 @@ bool ChainstateManager::ProcessNewBlockHeaders(const std::vector& LOCK(cs_main); for (const CBlockHeader& header : headers) { CBlockIndex *pindex = nullptr; // Use a temp pindex instead of ppindex to avoid a const_cast - bool accepted = m_blockman.AcceptBlockHeader( - header, state, chainparams, &pindex); + bool accepted{AcceptBlockHeader(header, state, chainparams, &pindex)}; ActiveChainstate().CheckBlockIndex(); if (!accepted) { @@ -3387,7 +3386,7 @@ bool CChainState::AcceptBlock(const std::shared_ptr& pblock, Block CBlockIndex *pindexDummy = nullptr; CBlockIndex *&pindex = ppindex ? *ppindex : pindexDummy; - bool accepted_header = m_blockman.AcceptBlockHeader(block, state, m_params, &pindex); + bool accepted_header{m_chainman.AcceptBlockHeader(block, state, m_params, &pindex)}; CheckBlockIndex(); if (!accepted_header) diff --git a/src/validation.h b/src/validation.h index 27b8f4e906f..dd29abc607b 100644 --- a/src/validation.h +++ b/src/validation.h @@ -454,16 +454,6 @@ class BlockManager //! Mark one block file as pruned (modify associated database entries) void PruneOneBlockFile(const int fileNumber) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - /** - * If a block header hasn't already been seen, call CheckBlockHeader on it, ensure - * that it doesn't descend from an invalid block, and then add it to m_block_index. - */ - bool AcceptBlockHeader( - const CBlockHeader& block, - BlockValidationState& state, - const CChainParams& chainparams, - CBlockIndex** ppindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - CBlockIndex* LookupBlockIndex(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); //! Returns last CBlockIndex* that is a checkpoint @@ -902,6 +892,17 @@ class ChainstateManager CAutoFile& coins_file, const SnapshotMetadata& metadata); + /** + * If a block header hasn't already been seen, call CheckBlockHeader on it, ensure + * that it doesn't descend from an invalid block, and then add it to m_block_index. + */ + bool AcceptBlockHeader( + const CBlockHeader& block, + BlockValidationState& state, + const CChainParams& chainparams, + CBlockIndex** ppindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + friend CChainState; + public: std::thread m_load_block; //! A single BlockManager instance is shared across each constructed From facd2137eceacb95e1f71c87ddc704d752b37272 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 23 Nov 2021 19:44:38 +0100 Subject: [PATCH 3/4] Move m_failed_blocks to ChainstateManager The member is unrelated to block storage (BlockManager). It is related to validation. Fix the confusion by moving it. Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space --- src/validation.cpp | 12 ++++++------ src/validation.h | 41 +++++++++++++++++++++-------------------- 2 files changed, 27 insertions(+), 26 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 60c01ad7b61..ba6b9e853a5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1291,7 +1291,7 @@ void CChainState::InvalidBlockFound(CBlockIndex* pindex, const BlockValidationSt { if (state.GetResult() != BlockValidationResult::BLOCK_MUTATED) { pindex->nStatus |= BLOCK_FAILED_VALID; - m_blockman.m_failed_blocks.insert(pindex); + m_chainman.m_failed_blocks.insert(pindex); setDirtyBlockIndex.insert(pindex); setBlockIndexCandidates.erase(pindex); InvalidChainFound(pindex); @@ -2844,7 +2844,7 @@ bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pind to_mark_failed->nStatus |= BLOCK_FAILED_VALID; setDirtyBlockIndex.insert(to_mark_failed); setBlockIndexCandidates.erase(to_mark_failed); - m_blockman.m_failed_blocks.insert(to_mark_failed); + m_chainman.m_failed_blocks.insert(to_mark_failed); // If any new blocks somehow arrived while we were disconnecting // (above), then the pre-calculation of what should go into @@ -2889,7 +2889,7 @@ void CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) { // Reset invalid block marker if it was pointing to one of those. pindexBestInvalid = nullptr; } - m_blockman.m_failed_blocks.erase(it->second); + m_chainman.m_failed_blocks.erase(it->second); } it++; } @@ -2899,7 +2899,7 @@ void CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) { if (pindex->nStatus & BLOCK_FAILED_MASK) { pindex->nStatus &= ~BLOCK_FAILED_MASK; setDirtyBlockIndex.insert(pindex); - m_blockman.m_failed_blocks.erase(pindex); + m_chainman.m_failed_blocks.erase(pindex); } pindex = pindex->pprev; } @@ -3325,7 +3325,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida // hasn't been validated up to BLOCK_VALID_SCRIPTS. This is a performance // optimization, in the common case of adding a new block to the tip, // we don't need to iterate over the failed blocks list. - for (const CBlockIndex* failedit : m_blockman.m_failed_blocks) { + for (const CBlockIndex* failedit : m_failed_blocks) { if (pindexPrev->GetAncestor(failedit->nHeight) == failedit) { assert(failedit->nStatus & BLOCK_FAILED_VALID); CBlockIndex* invalid_walk = pindexPrev; @@ -3804,7 +3804,6 @@ bool BlockManager::LoadBlockIndex( } void BlockManager::Unload() { - m_failed_blocks.clear(); m_blocks_unlinked.clear(); for (const BlockMap::value_type& entry : m_block_index) { @@ -5119,6 +5118,7 @@ void ChainstateManager::Unload() chainstate->UnloadBlockIndex(); } + m_failed_blocks.clear(); m_blockman.Unload(); } diff --git a/src/validation.h b/src/validation.h index dd29abc607b..3a702b4fa63 100644 --- a/src/validation.h +++ b/src/validation.h @@ -405,26 +405,6 @@ class BlockManager public: BlockMap m_block_index GUARDED_BY(cs_main); - /** In order to efficiently track invalidity of headers, we keep the set of - * blocks which we tried to connect and found to be invalid here (ie which - * were set to BLOCK_FAILED_VALID since the last restart). We can then - * walk this set and check if a new header is a descendant of something in - * this set, preventing us from having to walk m_block_index when we try - * to connect a bad block and fail. - * - * While this is more complicated than marking everything which descends - * from an invalid block as invalid at the time we discover it to be - * invalid, doing so would require walking all of m_block_index to find all - * descendants. Since this case should be very rare, keeping track of all - * BLOCK_FAILED_VALID blocks in a set should be just fine and work just as - * well. - * - * Because we already walk m_block_index in height-order at startup, we go - * ahead and mark descendants of invalid blocks as FAILED_CHILD at that time, - * instead of putting things in this set. - */ - std::set m_failed_blocks; - /** * All pairs A->B, where A (or one of its ancestors) misses transactions, but B has transactions. * Pruned nodes may have entries where B is missing data. @@ -909,6 +889,27 @@ class ChainstateManager //! chainstate to avoid duplicating block metadata. BlockManager m_blockman GUARDED_BY(::cs_main); + /** + * In order to efficiently track invalidity of headers, we keep the set of + * blocks which we tried to connect and found to be invalid here (ie which + * were set to BLOCK_FAILED_VALID since the last restart). We can then + * walk this set and check if a new header is a descendant of something in + * this set, preventing us from having to walk m_block_index when we try + * to connect a bad block and fail. + * + * While this is more complicated than marking everything which descends + * from an invalid block as invalid at the time we discover it to be + * invalid, doing so would require walking all of m_block_index to find all + * descendants. Since this case should be very rare, keeping track of all + * BLOCK_FAILED_VALID blocks in a set should be just fine and work just as + * well. + * + * Because we already walk m_block_index in height-order at startup, we go + * ahead and mark descendants of invalid blocks as FAILED_CHILD at that time, + * instead of putting things in this set. + */ + std::set m_failed_blocks; + //! The total number of bytes available for us to use across all in-memory //! coins caches. This will be split somehow across chainstates. int64_t m_total_coinstip_cache{0}; From fab6d6b2d154893ab422dda87f3535d42c3e06f4 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 23 Nov 2021 19:49:48 +0100 Subject: [PATCH 4/4] Move pindexBestInvalid to ChainstateManager A private member is better than a global. --- src/validation.cpp | 28 +++++++++++++--------------- src/validation.h | 3 +++ 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index ba6b9e853a5..27af75245f9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -133,11 +133,6 @@ arith_uint256 nMinimumChainWork; CFeeRate minRelayTxFee = CFeeRate(DEFAULT_MIN_RELAY_TX_FEE); -// Internal stuff -namespace { - CBlockIndex* pindexBestInvalid = nullptr; -} // namespace - // Internal stuff from blockstorage ... extern RecursiveMutex cs_LastBlockFile; extern std::vector vinfoBlockFile; @@ -1257,7 +1252,7 @@ void CChainState::CheckForkWarningConditions() return; } - if (pindexBestInvalid && pindexBestInvalid->nChainWork > m_chain.Tip()->nChainWork + (GetBlockProof(*m_chain.Tip()) * 6)) { + if (m_chainman.m_best_invalid && m_chainman.m_best_invalid->nChainWork > m_chain.Tip()->nChainWork + (GetBlockProof(*m_chain.Tip()) * 6)) { LogPrintf("%s: Warning: Found invalid chain at least ~6 blocks longer than our best chain.\nChain state database corruption likely.\n", __func__); SetfLargeWorkInvalidChainFound(true); } else { @@ -1268,8 +1263,9 @@ void CChainState::CheckForkWarningConditions() // Called both upon regular invalid block discovery *and* InvalidateBlock void CChainState::InvalidChainFound(CBlockIndex* pindexNew) { - if (!pindexBestInvalid || pindexNew->nChainWork > pindexBestInvalid->nChainWork) - pindexBestInvalid = pindexNew; + if (!m_chainman.m_best_invalid || pindexNew->nChainWork > m_chainman.m_best_invalid->nChainWork) { + m_chainman.m_best_invalid = pindexNew; + } if (pindexBestHeader != nullptr && pindexBestHeader->GetAncestor(pindexNew->nHeight) == pindexNew) { pindexBestHeader = m_chain.Tip(); } @@ -2432,8 +2428,9 @@ CBlockIndex* CChainState::FindMostWorkChain() { bool fMissingData = !(pindexTest->nStatus & BLOCK_HAVE_DATA); if (fFailedChain || fMissingData) { // Candidate chain is not usable (either invalid or missing data) - if (fFailedChain && (pindexBestInvalid == nullptr || pindexNew->nChainWork > pindexBestInvalid->nChainWork)) - pindexBestInvalid = pindexNew; + if (fFailedChain && (m_chainman.m_best_invalid == nullptr || pindexNew->nChainWork > m_chainman.m_best_invalid->nChainWork)) { + m_chainman.m_best_invalid = pindexNew; + } CBlockIndex *pindexFailed = pindexNew; // Remove the entire chain from the set. while (pindexTest != pindexFailed) { @@ -2885,9 +2882,9 @@ void CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) { if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->HaveTxsDownloaded() && setBlockIndexCandidates.value_comp()(m_chain.Tip(), it->second)) { setBlockIndexCandidates.insert(it->second); } - if (it->second == pindexBestInvalid) { + if (it->second == m_chainman.m_best_invalid) { // Reset invalid block marker if it was pointing to one of those. - pindexBestInvalid = nullptr; + m_chainman.m_best_invalid = nullptr; } m_chainman.m_failed_blocks.erase(it->second); } @@ -3792,8 +3789,9 @@ bool BlockManager::LoadBlockIndex( } } } - if (pindex->nStatus & BLOCK_FAILED_MASK && (!pindexBestInvalid || pindex->nChainWork > pindexBestInvalid->nChainWork)) - pindexBestInvalid = pindex; + if (pindex->nStatus & BLOCK_FAILED_MASK && (!chainman.m_best_invalid || pindex->nChainWork > chainman.m_best_invalid->nChainWork)) { + chainman.m_best_invalid = pindex; + } if (pindex->pprev) pindex->BuildSkip(); if (pindex->IsValid(BLOCK_VALID_TREE) && (pindexBestHeader == nullptr || CBlockIndexWorkComparator()(pindexBestHeader, pindex))) @@ -4141,7 +4139,6 @@ void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman) { LOCK(cs_main); chainman.Unload(); - pindexBestInvalid = nullptr; pindexBestHeader = nullptr; if (mempool) mempool->clear(); vinfoBlockFile.clear(); @@ -5120,6 +5117,7 @@ void ChainstateManager::Unload() m_failed_blocks.clear(); m_blockman.Unload(); + m_best_invalid = nullptr; } void ChainstateManager::Reset() diff --git a/src/validation.h b/src/validation.h index 3a702b4fa63..784ff0b3bc3 100644 --- a/src/validation.h +++ b/src/validation.h @@ -866,6 +866,9 @@ class ChainstateManager //! by the background validation chainstate. bool m_snapshot_validated{false}; + CBlockIndex* m_best_invalid; + friend bool BlockManager::LoadBlockIndex(const Consensus::Params&, ChainstateManager&); + //! Internal helper for ActivateSnapshot(). [[nodiscard]] bool PopulateAndValidateSnapshot( CChainState& snapshot_chainstate,