diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 7a38f715dbfc2e..6e2a9ec25981b2 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -37,11 +37,18 @@ static FILE* OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false); static FlatFileSeq BlockFileSeq(); static FlatFileSeq UndoFileSeq(); -CBlockIndex* BlockManager::LookupBlockIndex(const uint256& hash) const +CBlockIndex* BlockManager::LookupBlockIndex(const uint256& hash) +{ + AssertLockHeld(cs_main); + BlockMap::iterator it = m_block_index.find(hash); + return it == m_block_index.end() ? nullptr : &it->second; +} + +const CBlockIndex* BlockManager::LookupBlockIndex(const uint256& hash) const { AssertLockHeld(cs_main); BlockMap::const_iterator it = m_block_index.find(hash); - return it == m_block_index.end() ? nullptr : it->second; + return it == m_block_index.end() ? nullptr : &it->second; } CBlockIndex* BlockManager::AddToBlockIndex(const CBlockHeader& block, const uint256& hash, enum BlockStatus nStatus) @@ -49,23 +56,21 @@ CBlockIndex* BlockManager::AddToBlockIndex(const CBlockHeader& block, const uint assert(!(nStatus & BLOCK_FAILED_MASK)); // no failed blocks allowed AssertLockHeld(cs_main); - // Check for duplicate - BlockMap::iterator it = m_block_index.find(hash); - if (it != m_block_index.end()) { - return it->second; + auto [mi, inserted] = m_block_index.try_emplace(hash, block); + if (!inserted) { + return &mi->second; } + CBlockIndex* pindexNew = &(*mi).second; - // Construct new block index object - CBlockIndex* pindexNew = new CBlockIndex(block); // We assign the sequence id to blocks only when the full data is available, // to avoid miners withholding blocks but broadcasting headers, to get a // competitive advantage. pindexNew->nSequenceId = 0; - BlockMap::iterator mi = m_block_index.insert(std::make_pair(hash, pindexNew)).first; + pindexNew->phashBlock = &((*mi).first); BlockMap::iterator miPrev = m_block_index.find(block.hashPrevBlock); if (miPrev != m_block_index.end()) { - pindexNew->pprev = (*miPrev).second; + pindexNew->pprev = &(*miPrev).second; pindexNew->nHeight = pindexNew->pprev->nHeight + 1; pindexNew->BuildSkip(); } @@ -96,8 +101,8 @@ void BlockManager::PruneOneBlockFile(const int fileNumber) AssertLockHeld(cs_main); LOCK(cs_LastBlockFile); - for (const auto& entry : m_block_index) { - CBlockIndex* pindex = entry.second; + for (auto& entry : m_block_index) { + CBlockIndex* pindex = &entry.second; if (pindex->nFile == fileNumber) { pindex->nStatus &= ~BLOCK_HAVE_DATA; pindex->nStatus &= ~BLOCK_HAVE_UNDO; @@ -215,18 +220,13 @@ CBlockIndex* BlockManager::InsertBlockIndex(const uint256& hash) return nullptr; } - // Return existing - BlockMap::iterator mi = m_block_index.find(hash); - if (mi != m_block_index.end()) { - return (*mi).second; + // Return existing or create new + auto [mi, inserted] = m_block_index.try_emplace(hash); + CBlockIndex* pindex = &(*mi).second; + if (inserted) { + pindex->phashBlock = &((*mi).first); } - - // Create new - CBlockIndex* pindexNew = new CBlockIndex(); - mi = m_block_index.insert(std::make_pair(hash, pindexNew)).first; - pindexNew->phashBlock = &((*mi).first); - - return pindexNew; + return pindex; } bool BlockManager::LoadBlockIndex( @@ -240,8 +240,8 @@ bool BlockManager::LoadBlockIndex( // Calculate nChainWork std::vector> vSortedByHeight; vSortedByHeight.reserve(m_block_index.size()); - for (const std::pair& item : m_block_index) { - CBlockIndex* pindex = item.second; + for (auto& [_, block_index] : m_block_index) { + CBlockIndex* pindex = &block_index; vSortedByHeight.push_back(std::make_pair(pindex->nHeight, pindex)); // build m_blockman.m_prev_block_index @@ -348,10 +348,6 @@ void BlockManager::Unload() { m_blocks_unlinked.clear(); - for (const BlockMap::value_type& entry : m_block_index) { - delete entry.second; - } - m_block_index.clear(); m_prev_block_index.clear(); @@ -407,8 +403,8 @@ bool BlockManager::LoadBlockIndexDB(ChainstateManager& chainman) // Check presence of blk files LogPrintf("Checking all blk files are present...\n"); std::set setBlkDataFiles; - for (const std::pair& item : m_block_index) { - CBlockIndex* pindex = item.second; + for (const auto& [_, block_index] : m_block_index) { + const CBlockIndex* pindex = &block_index; if (pindex->nStatus & BLOCK_HAVE_DATA) { setBlkDataFiles.insert(pindex->nFile); } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 1f980da515ce9c..daba5d64038b6f 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_NODE_BLOCKSTORAGE_H #define BITCOIN_NODE_BLOCKSTORAGE_H +#include #include #include // For CMessageHeader::MessageStartChars #include @@ -20,7 +21,6 @@ class ArgsManager; class BlockValidationState; class CBlock; class CBlockFileInfo; -class CBlockIndex; class CBlockUndo; class CChain; class CChainParams; @@ -63,8 +63,12 @@ extern bool fTimestampIndex; /** True if we're running in -spentindex mode. */ extern bool fSpentIndex; -typedef std::unordered_map BlockMap; -typedef std::unordered_multimap PrevBlockMap; +// Because validation code takes pointers to the map's CBlockIndex objects, if +// we ever switch to another associative container, we need to either use a +// container that has stable addressing (true of all std associative +// containers), or make the key a `std::unique_ptr` +using BlockMap = std::unordered_map; +using PrevBlockMap = std::unordered_multimap; struct CBlockIndexWorkComparator { bool operator()(const CBlockIndex* pa, const CBlockIndex* pb) const; @@ -157,7 +161,8 @@ class BlockManager //! Mark one block file as pruned (modify associated database entries) void PruneOneBlockFile(const int fileNumber) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - CBlockIndex* LookupBlockIndex(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); + CBlockIndex* LookupBlockIndex(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + const CBlockIndex* LookupBlockIndex(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Get block file info entry for one block file */ CBlockFileInfo* GetBlockFileInfo(size_t n); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 7517333b3a5c0f..ebf7c22c5162f2 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1879,10 +1879,10 @@ static RPCHelpMan getchaintips() std::set setOrphans; std::set setPrevs; - for (const std::pair& item : chainman.BlockIndex()) { - if (!active_chain.Contains(item.second)) { - setOrphans.insert(item.second); - setPrevs.insert(item.second->pprev); + for (const auto& [_, block_index] : chainman.BlockIndex()) { + if (!active_chain.Contains(&block_index)) { + setOrphans.insert(&block_index); + setPrevs.insert(block_index.pprev); } } diff --git a/src/rpc/evo.cpp b/src/rpc/evo.cpp index 66f435dafdc950..b6cfe6d59defea 100644 --- a/src/rpc/evo.cpp +++ b/src/rpc/evo.cpp @@ -1558,7 +1558,7 @@ static RPCHelpMan protx_info() g_txindex->BlockUntilSyncedToCurrentChain(); } - CBlockIndex* pindex{nullptr}; + const CBlockIndex* pindex{nullptr}; uint256 proTxHash(ParseHashV(request.params[0], "proTxHash")); diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp index 923cdc09932316..f79cabf32c87fc 100644 --- a/src/rpc/masternode.cpp +++ b/src/rpc/masternode.cpp @@ -370,7 +370,7 @@ static RPCHelpMan masternode_payments() const NodeContext& node = EnsureAnyNodeContext(request.context); const ChainstateManager& chainman = EnsureChainman(node); - CBlockIndex* pindex{nullptr}; + const CBlockIndex* pindex{nullptr}; if (g_txindex) { g_txindex->BlockUntilSyncedToCurrentChain(); diff --git a/src/rpc/quorums.cpp b/src/rpc/quorums.cpp index a45cfa808f6071..e042a9007eadb6 100644 --- a/src/rpc/quorums.cpp +++ b/src/rpc/quorums.cpp @@ -925,7 +925,7 @@ static RPCHelpMan verifychainlock() const ChainstateManager& chainman = EnsureChainman(node); int nBlockHeight; - CBlockIndex* pIndex{nullptr}; + const CBlockIndex* pIndex{nullptr}; if (request.params[2].isNull()) { pIndex = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(nBlockHash)); if (pIndex == nullptr) { diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index fa33c0fbb049d0..8d5a55db18490e 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -93,7 +93,7 @@ void TxToJSON(const CTransaction& tx, const uint256 hashBlock, CTxMemPool& mempo bool chainLock = false; if (!hashBlock.IsNull()) { entry.pushKV("blockhash", hashBlock.GetHex()); - CBlockIndex* pindex = active_chainstate.m_blockman.LookupBlockIndex(hashBlock); + const CBlockIndex* pindex = active_chainstate.m_blockman.LookupBlockIndex(hashBlock); if (pindex) { if (active_chainstate.m_chain.Contains(pindex)) { entry.pushKV("height", pindex->nHeight); @@ -324,7 +324,7 @@ static RPCHelpMan getrawtransactionmulti() { const uint256 blockhash{uint256S(blockhash_str)}; const UniValue txids = transactions[blockhash_str].get_array(); - CBlockIndex* blockindex{blockhash.IsNull() ? nullptr : WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(blockhash))}; + const CBlockIndex* blockindex{blockhash.IsNull() ? nullptr : WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(blockhash))}; if (blockindex == nullptr && !blockhash.IsNull()) { for (const auto idx : irange::range(txids.size())) { result.pushKV(txids[idx].get_str(), "None"); diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index 79dbac9645cafd..420e5b9d65c5ab 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -77,9 +77,6 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches) // The view cache should be empty since we had to destruct to downsize. BOOST_CHECK(!c1.CoinsTip().HaveCoinInCache(outpoint)); } - - // Avoid triggering the address sanitizer. - WITH_LOCK(::cs_main, manager.Unload()); } //! Test UpdateTip behavior for both active and background chainstates. diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index cc45fd0d028484..d3374dcdc636a8 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -114,8 +114,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) SyncWithValidationInterfaceQueue(); DashTestSetupClose(m_node); - - WITH_LOCK(::cs_main, manager.Unload()); } //! Test rebalancing the caches associated with each chainstate. diff --git a/src/validation.cpp b/src/validation.cpp index 5d68b438625107..93f976ff2db056 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1906,7 +1906,7 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, // effectively caching the result of part of the verification. BlockMap::const_iterator it = m_blockman.m_block_index.find(hashAssumeValid); if (it != m_blockman.m_block_index.end()) { - if (it->second->GetAncestor(pindex->nHeight) == pindex && + if (it->second.GetAncestor(pindex->nHeight) == pindex && pindexBestHeader->GetAncestor(pindex->nHeight) == pindex && pindexBestHeader->nChainWork >= nMinimumChainWork) { // This block is a member of the assumed verified chain and an ancestor of the best header. @@ -3116,8 +3116,8 @@ bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pind { LOCK(cs_main); - for (const auto& entry : m_blockman.m_block_index) { - CBlockIndex *candidate = entry.second; + for (auto& entry : m_blockman.m_block_index) { + CBlockIndex* candidate = &entry.second; // We don't need to put anything in our active chain into the // multimap, because those candidates will be found and considered // as we disconnect. @@ -3225,12 +3225,10 @@ bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pind // it up here, this should be an essentially unobservable error. // Loop back over all block index entries and add any missing entries // to setBlockIndexCandidates. - BlockMap::iterator it = m_blockman.m_block_index.begin(); - while (it != m_blockman.m_block_index.end()) { - if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && !(it->second->nStatus & BLOCK_CONFLICT_CHAINLOCK) && it->second->HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(it->second, m_chain.Tip())) { - setBlockIndexCandidates.insert(it->second); + for (auto& [_, block_index] : m_blockman.m_block_index) { + if (block_index.IsValid(BLOCK_VALID_TRANSACTIONS) && !(block_index.nStatus & BLOCK_CONFLICT_CHAINLOCK) && block_index.HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(&block_index, m_chain.Tip())) { + setBlockIndexCandidates.insert(&block_index); } - it++; } InvalidChainFound(to_mark_failed); @@ -3329,8 +3327,8 @@ bool CChainState::MarkConflictingBlock(BlockValidationState& state, CBlockIndex // add it again. BlockMap::iterator it = m_blockman.m_block_index.begin(); while (it != m_blockman.m_block_index.end()) { - if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && !(it->second->nStatus & BLOCK_CONFLICT_CHAINLOCK) && it->second->HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(it->second, m_chain.Tip())) { - setBlockIndexCandidates.insert(it->second); + if (it->second.IsValid(BLOCK_VALID_TRANSACTIONS) && !(it->second.nStatus & BLOCK_CONFLICT_CHAINLOCK) && it->second.HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(&it->second, m_chain.Tip())) { + setBlockIndexCandidates.insert(&it->second); } it++; } @@ -3362,21 +3360,19 @@ void CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) { int nHeight = pindex->nHeight; // Remove the invalidity flag from this block and all its descendants. - BlockMap::iterator it = m_blockman.m_block_index.begin(); - while (it != m_blockman.m_block_index.end()) { - if (!it->second->IsValid() && it->second->GetAncestor(nHeight) == pindex) { - it->second->nStatus &= ~BLOCK_FAILED_MASK; - m_blockman.m_dirty_blockindex.insert(it->second); - if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && !(it->second->nStatus & BLOCK_CONFLICT_CHAINLOCK) && it->second->HaveTxsDownloaded() && setBlockIndexCandidates.value_comp()(m_chain.Tip(), it->second)) { - setBlockIndexCandidates.insert(it->second); + for (auto& [_, block_index] : m_blockman.m_block_index) { + if (!block_index.IsValid() && block_index.GetAncestor(nHeight) == pindex) { + block_index.nStatus &= ~BLOCK_FAILED_MASK; + m_blockman.m_dirty_blockindex.insert(&block_index); + if (block_index.IsValid(BLOCK_VALID_TRANSACTIONS) && !(block_index.nStatus & BLOCK_CONFLICT_CHAINLOCK) && block_index.HaveTxsDownloaded() && setBlockIndexCandidates.value_comp()(m_chain.Tip(), &block_index)) { + setBlockIndexCandidates.insert(&block_index); } - if (it->second == m_chainman.m_best_invalid) { + if (&block_index == m_chainman.m_best_invalid) { // Reset invalid block marker if it was pointing to one of those. m_chainman.m_best_invalid = nullptr; } - m_chainman.m_failed_blocks.erase(it->second); + m_chainman.m_failed_blocks.erase(&block_index); } - it++; } // Remove the invalidity flag from all ancestors too. @@ -3687,7 +3683,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida if (hash != chainparams.GetConsensus().hashGenesisBlock) { if (miSelf != m_blockman.m_block_index.end()) { // Block header is already known. - CBlockIndex* pindex = miSelf->second; + CBlockIndex* pindex = &(miSelf->second); if (ppindex) *ppindex = pindex; if (pindex->nStatus & BLOCK_FAILED_MASK) { @@ -3713,7 +3709,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida LogPrintf("ERROR: %s: prev block not found\n", __func__); return state.Invalid(BlockValidationResult::BLOCK_MISSING_PREV, "prev-blk-not-found"); } - pindexPrev = (*mi).second; + pindexPrev = &((*mi).second); assert(pindexPrev); if (pindexPrev->nStatus & BLOCK_FAILED_MASK) { @@ -4298,13 +4294,13 @@ bool CChainState::ReplayBlocks() if (m_blockman.m_block_index.count(hashHeads[0]) == 0) { return error("ReplayBlocks(): reorganization to unknown block requested"); } - pindexNew = m_blockman.m_block_index[hashHeads[0]]; + pindexNew = &(m_blockman.m_block_index[hashHeads[0]]); if (!hashHeads[1].IsNull()) { // The old tip is allowed to be 0, indicating it's the first flush. if (m_blockman.m_block_index.count(hashHeads[1]) == 0) { return error("ReplayBlocks(): reorganization from unknown block requested"); } - pindexOld = m_blockman.m_block_index[hashHeads[1]]; + pindexOld = &(m_blockman.m_block_index[hashHeads[1]]); pindexFork = LastCommonAncestor(pindexOld, pindexNew); assert(pindexFork != nullptr); const bool fDIP0003Active = pindexOld->nHeight >= m_params.GetConsensus().DIP0003Height; @@ -4590,8 +4586,8 @@ void CChainState::CheckBlockIndex() // Build forward-pointing map of the entire block tree. std::multimap forward; - for (const std::pair& entry : m_blockman.m_block_index) { - forward.insert(std::make_pair(entry.second->pprev, entry.second)); + for (auto& [_, block_index] : m_blockman.m_block_index) { + forward.emplace(block_index.pprev, &block_index); } assert(forward.size() == m_blockman.m_block_index.size()); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 8f4df7ea025b09..4df20533f933db 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -437,10 +437,10 @@ static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lock CBlockIndex* block = nullptr; if (blockTime > 0) { LOCK(cs_main); - auto inserted = chainman.BlockIndex().emplace(GetRandHash(), new CBlockIndex); + auto inserted = chainman.BlockIndex().emplace(std::piecewise_construct, std::make_tuple(GetRandHash()), std::make_tuple()); assert(inserted.second); const uint256& hash = inserted.first->first; - block = inserted.first->second; + block = &inserted.first->second; block->nTime = blockTime; block->phashBlock = &hash; confirm = {CWalletTx::Status::CONFIRMED, block->nHeight, hash, 0};