diff options
author | MarcoFalke <falke.marco@gmail.com> | 2022-03-07 13:15:22 +0100 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2022-03-07 13:15:27 +0100 |
commit | 5e49b2a2529adc737175ebdcbf312635c52b05f6 (patch) | |
tree | a057dc284b6fa99e25e8145bdd312b749e7d341e /src/node | |
parent | b9894a13083c6ad7085d735c9c514edf3c8e6471 (diff) | |
parent | 6c23c415613d8b847e6f6a2f872be893da9f4384 (diff) | |
download | bitcoin-5e49b2a2529adc737175ebdcbf312635c52b05f6.tar.xz |
Merge bitcoin/bitcoin#24050: validation: Give `m_block_index` ownership of `CBlockIndex`s
6c23c415613d8b847e6f6a2f872be893da9f4384 refactor: Rewrite AddToBlockIndex with try_emplace (Carl Dong)
c05cf7aa1e1c15089753897a10c14762027d4b99 style: Modernize range-based loops over m_block_index (Carl Dong)
c2a1655799c5d5dab9b14bd2a6b2d2296efd6964 style-only: Use using instead of typedef for BlockMap (Carl Dong)
dd79dad17545424d145e846026518d70da594380 refactor: Rewrite InsertBlockIndex with try_emplace (Carl Dong)
531dce034718523967808a89c18ba69a1e3e5a1f tests: Remove now-unnecessary manual Unload's (Carl Dong)
bec86ae32683ac56b4e6ba9c9b7d21cfbdf4ac03 blockstorage: Make m_block_index own CBlockIndex's (Carl Dong)
Pull request description:
Part of: #24303
Split off from: #22564
```
Instead of having CBlockIndex's live on the heap, which requires manual
memory management, have them be owned by m_block_index. This means that
they will live and die with BlockManager.
```
The second commit demonstrates how this makes calls to `Unload()` to satisfy the address sanitizer unnecessary.
ACKs for top commit:
ajtowns:
ACK 6c23c415613d8b847e6f6a2f872be893da9f4384
MarcoFalke:
re-ACK 6c23c415613d8b847e6f6a2f872be893da9f4384 🎨
Tree-SHA512: 81b2b5119be27cc0f8a9457b11da60cc60930315d2a5be36be89fe253d32073ffe622348ff153114b9b3212197bddbc791810913a43811b33cc58e7162bd105b
Diffstat (limited to 'src/node')
-rw-r--r-- | src/node/blockstorage.cpp | 59 | ||||
-rw-r--r-- | src/node/blockstorage.h | 11 |
2 files changed, 35 insertions, 35 deletions
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 8a99130fd0..7392830261 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -32,35 +32,39 @@ 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) { AssertLockHeld(cs_main); - // Check for duplicate - uint256 hash = block.GetHash(); - 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(block.GetHash(), 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(); } @@ -80,8 +84,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; @@ -199,18 +203,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( @@ -224,8 +223,8 @@ bool BlockManager::LoadBlockIndex( // Calculate nChainWork std::vector<std::pair<int, CBlockIndex*>> vSortedByHeight; vSortedByHeight.reserve(m_block_index.size()); - for (const std::pair<const uint256, CBlockIndex*>& 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)); } sort(vSortedByHeight.begin(), vSortedByHeight.end()); @@ -327,10 +326,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_blockfile_info.clear(); @@ -386,8 +381,8 @@ bool BlockManager::LoadBlockIndexDB(ChainstateManager& chainman) // Check presence of blk files LogPrintf("Checking all blk files are present...\n"); std::set<int> setBlkDataFiles; - for (const std::pair<const uint256, CBlockIndex*>& 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 42e46797d2..12224f7a5d 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 <chain.h> #include <fs.h> #include <protocol.h> // For CMessageHeader::MessageStartChars #include <sync.h> @@ -20,7 +21,6 @@ class ArgsManager; class BlockValidationState; class CBlock; class CBlockFileInfo; -class CBlockIndex; class CBlockUndo; class CChain; class CChainParams; @@ -52,7 +52,11 @@ extern bool fPruneMode; /** Number of MiB of block files that we're trying to stay below. */ extern uint64_t nPruneTarget; -typedef std::unordered_map<uint256, CBlockIndex*, BlockHasher> BlockMap; +// 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<CBlockIndex>` +using BlockMap = std::unordered_map<uint256, CBlockIndex, BlockHasher>; struct CBlockIndexWorkComparator { bool operator()(const CBlockIndex* pa, const CBlockIndex* pb) const; @@ -144,7 +148,8 @@ public: //! 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); |