stop using pointer to map key; it could be volatile

src/chain.h

@@ -171,7 +171,7 @@ class CBlockIndex
 {
 public:
     //! pointer to the hash of the block, if any. Memory is owned by this CBlockIndex
-    const uint256* phashBlock;
+    const uint256 hash;
 
     //! pointer to the index of the predecessor of this block
     CBlockIndex* pprev;
@@ -222,7 +222,6 @@ public:
 
     void SetNull()
     {
-        phashBlock = nullptr;
         pprev = nullptr;
         pskip = nullptr;
         nHeight = 0;
@@ -244,12 +243,12 @@ public:
         nNonce         = 0;
     }
 
-    CBlockIndex()
+    CBlockIndex(const uint256& blockHash) : hash(blockHash)
     {
         SetNull();
     }
 
-    explicit CBlockIndex(const CBlockHeader& block)
+    explicit CBlockIndex(const CBlockHeader& block) : hash(block.GetHash())
     {
         SetNull();
 
@@ -295,7 +294,7 @@ public:
 
     uint256 GetBlockHash() const
     {
-        return *phashBlock;
+        return hash;
     }
 
     uint256 GetBlockPoWHash() const

src/rpc/blockchain.cpp

@@ -1351,9 +1351,9 @@ static UniValue getchaintips(const JSONRPCRequest& request)
 
     for (const auto& item : g_chainstate.mapBlockIndex)
     {
-        if (!chainActive.Contains(item.second)) {
+        if (!chainActive.Contains(item)) {
-            setOrphans.insert(item.second);
+            setOrphans.insert(item);
-            setPrevs.insert(item.second->pprev);
+            setPrevs.insert(item->pprev);
         }
     }
 
@@ -1373,7 +1373,7 @@ static UniValue getchaintips(const JSONRPCRequest& request)
     {
         UniValue obj(UniValue::VOBJ);
         obj.pushKV("height", block->nHeight);
-        obj.pushKV("hash", block->phashBlock->GetHex());
+        obj.pushKV("hash", block->hash.GetHex());
 
         // not use ForkAt method because we need the previous one as well
         const CBlockIndex *forkAt = block, *forkPrev = block;
@@ -1385,8 +1385,8 @@ static UniValue getchaintips(const JSONRPCRequest& request)
         const int branchLen = block->nHeight - forkAt->nHeight;
         obj.pushKV("branchlen", branchLen);
         if (forkAt != forkPrev) {
-            obj.pushKV("branchhash", forkAt->phashBlock->GetHex());
+            obj.pushKV("branchhash", forkAt->hash.GetHex());
-            obj.pushKV("branchhashNext", forkPrev->phashBlock->GetHex());
+            obj.pushKV("branchhashNext", forkPrev->hash.GetHex());
         }
 
         std::string status;

src/test/blockchain_tests.cpp

@@ -1,7 +1,5 @@
 #include <boost/test/unit_test.hpp>
 
-#include <stdlib.h>
-
 #include <rpc/blockchain.h>
 #include <test/test_bitcoin.h>
 #include <txdb.h>
@@ -16,7 +14,7 @@ static bool DoubleEquals(double a, double b, double epsilon)
 
 static CBlockIndex* CreateBlockIndexWithNbits(uint32_t nbits)
 {
-    CBlockIndex* block_index = new CBlockIndex();
+    CBlockIndex* block_index = new CBlockIndex((uint256()));
     block_index->nHeight = 46367;
     block_index->nTime = 1269211443;
     block_index->nBits = nbits;

src/test/miner_tests.cpp

@@ -76,7 +76,7 @@ const unsigned int nonces[] = {
 
 static CBlockIndex CreateBlockIndex(int nHeight)
 {
-    CBlockIndex index;
+    CBlockIndex index((uint256()));
     index.nHeight = nHeight;
     index.pprev = chainActive.Tip();
     return index;
@@ -376,8 +376,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     // Create an actual 209999-long block chain (without valid blocks).
     while (chainActive.Tip()->nHeight < 209999) {
         CBlockIndex* prev = chainActive.Tip();
-        CBlockIndex* next = new CBlockIndex();
+        CBlockIndex* next = new CBlockIndex(InsecureRand256());
-        next->phashBlock = new uint256(InsecureRand256());
         next->hashClaimTrie = pblocktemplate->block.hashClaimTrie;
         pcoinsTip->SetBestBlock(next->GetBlockHash());
         next->pprev = prev;
@@ -389,8 +388,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     // Extend to a 210000-long block chain.
     while (chainActive.Tip()->nHeight < 210000) {
         CBlockIndex* prev = chainActive.Tip();
-        CBlockIndex* next = new CBlockIndex();
+        CBlockIndex* next = new CBlockIndex(InsecureRand256());
-        next->phashBlock = new uint256(InsecureRand256());
         next->hashClaimTrie = pblocktemplate->block.hashClaimTrie;
         pcoinsTip->SetBestBlock(next->GetBlockHash());
         next->pprev = prev;
@@ -423,7 +421,6 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
         CBlockIndex* del = chainActive.Tip();
         chainActive.SetTip(del->pprev);
         pcoinsTip->SetBestBlock(del->pprev->GetBlockHash());
-        delete del->phashBlock;
         delete del;
     }
 

src/test/pow_tests.cpp

@@ -18,7 +18,7 @@ BOOST_AUTO_TEST_CASE(get_next_work)
 {
     const auto chainParams = CreateChainParams(CBaseChainParams::MAIN);
     int64_t nLastRetargetTime = 1261130161; // Block #30240
-    CBlockIndex pindexLast;
+    CBlockIndex pindexLast((uint256()));
     pindexLast.nHeight = 32255;
     pindexLast.nTime = 1262152739;  // Block #32255
     pindexLast.nBits = 0x1d00ffff;
@@ -30,7 +30,7 @@ BOOST_AUTO_TEST_CASE(get_next_work_pow_limit)
 {
     const auto chainParams = CreateChainParams(CBaseChainParams::MAIN);
     int64_t nLastRetargetTime = 1231006505; // Block #0
-    CBlockIndex pindexLast;
+    CBlockIndex pindexLast((uint256()));
     pindexLast.nHeight = 2015;
     pindexLast.nTime = 1233061996;  // Block #2015
     pindexLast.nBits = 0x1d00ffff;
@@ -42,7 +42,7 @@ BOOST_AUTO_TEST_CASE(get_next_work_lower_limit_actual)
 {
     const auto chainParams = CreateChainParams(CBaseChainParams::MAIN);
     int64_t nLastRetargetTime = 1279008237; // Block #66528
-    CBlockIndex pindexLast;
+    CBlockIndex pindexLast((uint256()));
     pindexLast.nHeight = 68543;
     pindexLast.nTime = 1279297671;  // Block #68543
     pindexLast.nBits = 0x1c05a3f4;
@@ -54,7 +54,7 @@ BOOST_AUTO_TEST_CASE(get_next_work_upper_limit_actual)
 {
     const auto chainParams = CreateChainParams(CBaseChainParams::MAIN);
     int64_t nLastRetargetTime = 1263163443; // NOTE: Not an actual block time
-    CBlockIndex pindexLast;
+    CBlockIndex pindexLast((uint256()));
     pindexLast.nHeight = 46367;
     pindexLast.nTime = 1269211443;  // Block #46367
     pindexLast.nBits = 0x1c387f6f;
@@ -64,8 +64,10 @@ BOOST_AUTO_TEST_CASE(get_next_work_upper_limit_actual)
 BOOST_AUTO_TEST_CASE(GetBlockProofEquivalentTime_test)
 {
     const auto chainParams = CreateChainParams(CBaseChainParams::MAIN);
-    std::vector<CBlockIndex> blocks(10000);
+    std::vector<CBlockIndex> blocks;
+    blocks.reserve(10000);
     for (int i = 0; i < 10000; i++) {
+        blocks.emplace_back(uint256());
         blocks[i].pprev = i ? &blocks[i - 1] : nullptr;
         blocks[i].nHeight = i;
         blocks[i].nTime = 1269211443 + i * chainParams->GetConsensus().nPowTargetSpacing;

src/test/skiplist_tests.cpp

@@ -16,9 +16,11 @@ BOOST_FIXTURE_TEST_SUITE(skiplist_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(skiplist_test)
 {
-    std::vector<CBlockIndex> vIndex(SKIPLIST_LENGTH);
+    std::vector<CBlockIndex> vIndex;
+    vIndex.reserve(SKIPLIST_LENGTH);
 
     for (int i=0; i<SKIPLIST_LENGTH; i++) {
+        vIndex.emplace_back(uint256());
         vIndex[i].nHeight = i;
         vIndex[i].pprev = (i == 0) ? nullptr : &vIndex[i - 1];
         vIndex[i].BuildSkip();
@@ -47,12 +49,13 @@ BOOST_AUTO_TEST_CASE(getlocator_test)
 {
     // Build a main chain 100000 blocks long.
     std::vector<uint256> vHashMain(100000);
-    std::vector<CBlockIndex> vBlocksMain(100000);
+    std::vector<CBlockIndex> vBlocksMain;
-    for (unsigned int i=0; i<vBlocksMain.size(); i++) {
+    vBlocksMain.reserve(100000);
+    for (unsigned int i=0; i<vHashMain.size(); i++) {
         vHashMain[i] = ArithToUint256(i); // Set the hash equal to the height, so we can quickly check the distances.
+        vBlocksMain.emplace_back(vHashMain[i]);
         vBlocksMain[i].nHeight = i;
         vBlocksMain[i].pprev = i ? &vBlocksMain[i - 1] : nullptr;
-        vBlocksMain[i].phashBlock = &vHashMain[i];
         vBlocksMain[i].BuildSkip();
         BOOST_CHECK_EQUAL((int)UintToArith256(vBlocksMain[i].GetBlockHash()).GetLow64(), vBlocksMain[i].nHeight);
         BOOST_CHECK(vBlocksMain[i].pprev == nullptr || vBlocksMain[i].nHeight == vBlocksMain[i].pprev->nHeight + 1);
@@ -60,12 +63,13 @@ BOOST_AUTO_TEST_CASE(getlocator_test)
 
     // Build a branch that splits off at block 49999, 50000 blocks long.
     std::vector<uint256> vHashSide(50000);
-    std::vector<CBlockIndex> vBlocksSide(50000);
+    std::vector<CBlockIndex> vBlocksSide;
-    for (unsigned int i=0; i<vBlocksSide.size(); i++) {
+    vBlocksSide.reserve(50000);
+    for (unsigned int i=0; i<vHashSide.size(); i++) {
         vHashSide[i] = ArithToUint256(i + 50000 + (arith_uint256(1) << 128)); // Add 1<<128 to the hashes, so GetLow64() still returns the height.
+        vBlocksSide.emplace_back(vHashSide[i]);
         vBlocksSide[i].nHeight = i + 50000;
         vBlocksSide[i].pprev = i ? &vBlocksSide[i - 1] : (vBlocksMain.data()+49999);
-        vBlocksSide[i].phashBlock = &vHashSide[i];
         vBlocksSide[i].BuildSkip();
         BOOST_CHECK_EQUAL((int)UintToArith256(vBlocksSide[i].GetBlockHash()).GetLow64(), vBlocksSide[i].nHeight);
         BOOST_CHECK(vBlocksSide[i].pprev == nullptr || vBlocksSide[i].nHeight == vBlocksSide[i].pprev->nHeight + 1);
@@ -102,12 +106,13 @@ BOOST_AUTO_TEST_CASE(getlocator_test)
 BOOST_AUTO_TEST_CASE(findearliestatleast_test)
 {
     std::vector<uint256> vHashMain(100000);
-    std::vector<CBlockIndex> vBlocksMain(100000);
+    std::vector<CBlockIndex> vBlocksMain;
-    for (unsigned int i=0; i<vBlocksMain.size(); i++) {
+    vBlocksMain.reserve(100000);
+    for (unsigned int i=0; i<vHashMain.size(); i++) {
         vHashMain[i] = ArithToUint256(i); // Set the hash equal to the height
+        vBlocksMain.emplace_back(vHashMain[i]);
         vBlocksMain[i].nHeight = i;
         vBlocksMain[i].pprev = i ? &vBlocksMain[i - 1] : nullptr;
-        vBlocksMain[i].phashBlock = &vHashMain[i];
         vBlocksMain[i].BuildSkip();
         if (i < 10) {
             vBlocksMain[i].nTime = i;
@@ -148,7 +153,7 @@ BOOST_AUTO_TEST_CASE(findearliestatleast_edge_test)
     std::list<CBlockIndex> blocks;
     for (unsigned int timeMax : {100, 100, 100, 200, 200, 200, 300, 300, 300}) {
         CBlockIndex* prev = blocks.empty() ? nullptr : &blocks.back();
-        blocks.emplace_back();
+        blocks.emplace_back(uint256());
         blocks.back().nHeight = prev ? prev->nHeight + 1 : 0;
         blocks.back().pprev = prev;
         blocks.back().BuildSkip();

src/test/versionbits_tests.cpp

@@ -76,7 +76,7 @@ public:
 
     VersionBitsTester& Mine(unsigned int height, int32_t nTime, int32_t nVersion) {
         while (vpblock.size() < height) {
-            CBlockIndex* pindex = new CBlockIndex();
+            CBlockIndex* pindex = new CBlockIndex((uint256()));
             pindex->nHeight = vpblock.size();
             pindex->pprev = vpblock.size() > 0 ? vpblock.back() : nullptr;
             pindex->nTime = nTime;

src/validation.cpp

@@ -220,7 +220,7 @@ bool CheckSequenceLocks(const CTransaction &tx, int flags, LockPoints* lp, bool
     CBlockIndex* tip = chainActive.Tip();
     assert(tip != nullptr);
 
-    CBlockIndex index;
+    CBlockIndex index((uint256()));
     index.pprev = tip;
     // CheckSequenceLocks() uses chainActive.Height()+1 to evaluate
     // height based locks because when SequenceLocks() is called within
@@ -1151,14 +1151,14 @@ static void CheckForkWarningConditions()
         if (!GetfLargeWorkForkFound() && pindexBestForkBase)
         {
             std::string warning = std::string("'Warning: Large-work fork detected, forking after block ") +
-                pindexBestForkBase->phashBlock->ToString() + std::string("'");
+                pindexBestForkBase->hash.ToString() + std::string("'");
             AlertNotify(warning);
         }
         if (pindexBestForkTip && pindexBestForkBase)
         {
             LogPrintf("%s: Warning: Large valid fork found\n  forking the chain at height %d (%s)\n  lasting to height %d (%s).\nChain state database corruption likely.\n", __func__,
-                   pindexBestForkBase->nHeight, pindexBestForkBase->phashBlock->ToString(),
+                   pindexBestForkBase->nHeight, pindexBestForkBase->hash.ToString(),
-                   pindexBestForkTip->nHeight, pindexBestForkTip->phashBlock->ToString());
+                   pindexBestForkTip->nHeight, pindexBestForkTip->hash.ToString());
             SetfLargeWorkForkFound(true);
         }
         else
@@ -1713,8 +1713,8 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
     // mainnet and testnet), so for simplicity, always leave P2SH
     // on except for the one violating block.
     if (consensusparams.BIP16Exception.IsNull() || // no bip16 exception on this chain
-        pindex->phashBlock == nullptr || // this is a new candidate block, eg from TestBlockValidity()
+        pindex->hash.IsNull() || // this is a new candidate block, eg from TestBlockValidity()
-        *pindex->phashBlock != consensusparams.BIP16Exception) // this block isn't the historical exception
+        pindex->hash != consensusparams.BIP16Exception) // this block isn't the historical exception
     {
         flags |= SCRIPT_VERIFY_P2SH;
     }
@@ -1766,7 +1766,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
 {
     AssertLockHeld(cs_main);
     assert(pindex);
-    assert(*pindex->phashBlock == block.GetHash());
+    assert(pindex->hash == block.GetHash());
     int64_t nTimeStart = GetTimeMicros();
 
     // Check it again in case a previous version let a bad block in
@@ -1822,9 +1822,9 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
         //  relative to a piece of software is an objective fact these defaults can be easily reviewed.
         // This setting doesn't force the selection of any particular chain but makes validating some faster by
         //  effectively caching the result of part of the verification.
-        BlockMap::const_iterator  it = mapBlockIndex.find(hashAssumeValid);
+        auto it = mapBlockIndex.find(hashAssumeValid);
         if (it != mapBlockIndex.end()) {
-            if (it->second->GetAncestor(pindex->nHeight) == pindex &&
+            if ((*it)->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.
@@ -2095,7 +2095,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
         setDirtyBlockIndex.insert(pindex);
     }
 
-    assert(pindex->phashBlock);
+    assert(!pindex->hash.IsNull());
     // add this block to the view's block chain
     view.SetBestBlock(pindex->GetBlockHash());
 
@@ -2892,12 +2892,12 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c
 
     // The resulting new best tip may not be in setBlockIndexCandidates anymore, so
     // add it again.
-    BlockMap::iterator it = mapBlockIndex.begin();
+    auto it = mapBlockIndex.begin();
     while (it != mapBlockIndex.end()) {
-        if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->nChainTx && !setBlockIndexCandidates.value_comp()(it->second, chainActive.Tip())) {
+        if ((*it)->IsValid(BLOCK_VALID_TRANSACTIONS) && (*it)->nChainTx && !setBlockIndexCandidates.value_comp()(*it, chainActive.Tip())) {
-            setBlockIndexCandidates.insert(it->second);
+            setBlockIndexCandidates.insert(*it);
         }
-        it++;
+        ++it;
     }
 
     InvalidChainFound(pindex);
@@ -2918,21 +2918,21 @@ void CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) {
     int nHeight = pindex->nHeight;
 
     // Remove the invalidity flag from this block and all its descendants.
-    BlockMap::iterator it = mapBlockIndex.begin();
+    auto it = mapBlockIndex.begin();
     while (it != mapBlockIndex.end()) {
-        if (!it->second->IsValid() && it->second->GetAncestor(nHeight) == pindex) {
+        if (!(*it)->IsValid() && (*it)->GetAncestor(nHeight) == pindex) {
-            it->second->nStatus &= ~BLOCK_FAILED_MASK;
+            (*it)->nStatus &= ~BLOCK_FAILED_MASK;
-            setDirtyBlockIndex.insert(it->second);
+            setDirtyBlockIndex.insert(*it);
-            if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->nChainTx && setBlockIndexCandidates.value_comp()(chainActive.Tip(), it->second)) {
+            if ((*it)->IsValid(BLOCK_VALID_TRANSACTIONS) && (*it)->nChainTx && setBlockIndexCandidates.value_comp()(chainActive.Tip(), *it)) {
-                setBlockIndexCandidates.insert(it->second);
+                setBlockIndexCandidates.insert(*it);
             }
-            if (it->second == pindexBestInvalid) {
+            if (*it == pindexBestInvalid) {
                 // Reset invalid block marker if it was pointing to one of those.
                 pindexBestInvalid = nullptr;
             }
-            m_failed_blocks.erase(it->second);
+            m_failed_blocks.erase(*it);
         }
-        it++;
+        ++it;
     }
 
     // Remove the invalidity flag from all ancestors too.
@@ -2956,9 +2956,9 @@ CBlockIndex* CChainState::AddToBlockIndex(const CBlockHeader& block)
 
     // Check for duplicate
     uint256 hash = block.GetHash();
-    BlockMap::iterator it = mapBlockIndex.find(hash);
+    auto it = mapBlockIndex.find(hash);
     if (it != mapBlockIndex.end())
-        return it->second;
+        return *it;
 
     // Construct new block index object
     CBlockIndex* pindexNew = new CBlockIndex(block);
@@ -2966,12 +2966,11 @@ CBlockIndex* CChainState::AddToBlockIndex(const CBlockHeader& block)
     // to avoid miners withholding blocks but broadcasting headers, to get a
     // competitive advantage.
     pindexNew->nSequenceId = 0;
-    BlockMap::iterator mi = mapBlockIndex.insert(std::make_pair(hash, pindexNew)).first;
+    mapBlockIndex.insert(pindexNew);
-    pindexNew->phashBlock = &((*mi).first);
     BlockMap::iterator miPrev = mapBlockIndex.find(block.hashPrevBlock);
     if (miPrev != mapBlockIndex.end())
     {
-        pindexNew->pprev = (*miPrev).second;
+        pindexNew->pprev = *miPrev;
         pindexNew->nHeight = pindexNew->pprev->nHeight + 1;
         pindexNew->BuildSkip();
     }
@@ -3401,12 +3400,12 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState&
     AssertLockHeld(cs_main);
     // Check for duplicate
     uint256 hash = block.GetHash();
-    BlockMap::iterator miSelf = mapBlockIndex.find(hash);
+    auto miSelf = mapBlockIndex.find(hash);
     CBlockIndex *pindex = nullptr;
     if (hash != chainparams.GetConsensus().hashGenesisBlock) {
         if (miSelf != mapBlockIndex.end()) {
             // Block header is already known.
-            pindex = miSelf->second;
+            pindex = *miSelf;
             if (ppindex)
                 *ppindex = pindex;
             if (pindex->nStatus & BLOCK_FAILED_MASK)
@@ -3419,10 +3418,10 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState&
 
         // Get prev block index
         CBlockIndex* pindexPrev = nullptr;
-        BlockMap::iterator mi = mapBlockIndex.find(block.hashPrevBlock);
+        auto mi = mapBlockIndex.find(block.hashPrevBlock);
         if (mi == mapBlockIndex.end())
             return state.DoS(10, error("%s: prev block not found", __func__), 0, "prev-blk-not-found");
-        pindexPrev = (*mi).second;
+        pindexPrev = *mi;
         if (pindexPrev->nStatus & BLOCK_FAILED_MASK)
             return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk");
         if (!ContextualCheckBlockHeader(block, state, chainparams, pindexPrev, GetAdjustedTime()))
@@ -3617,11 +3616,9 @@ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams,
     assert(pindexPrev && pindexPrev == chainActive.Tip());
     CCoinsViewCache viewNew(pcoinsTip.get());
     CClaimTrieCache trieCache(pclaimTrie);
-    uint256 block_hash(block.GetHash());
     CBlockIndex indexDummy(block);
     indexDummy.pprev = pindexPrev;
     indexDummy.nHeight = pindexPrev->nHeight + 1;
-    indexDummy.phashBlock = &block_hash;
 
     // NOTE: CheckBlockHeader is called by CheckBlock
     if (!ContextualCheckBlockHeader(block, state, chainparams, pindexPrev, GetAdjustedTime()))
@@ -3659,7 +3656,7 @@ void PruneOneBlockFile(const int fileNumber)
     LOCK(cs_LastBlockFile);
 
     for (const auto& entry : g_chainstate.mapBlockIndex) {
-        CBlockIndex* pindex = entry.second;
+        CBlockIndex* pindex = entry;
         if (pindex->nFile == fileNumber) {
             pindex->nStatus &= ~BLOCK_HAVE_DATA;
             pindex->nStatus &= ~BLOCK_HAVE_UNDO;
@@ -3856,15 +3853,14 @@ CBlockIndex * CChainState::InsertBlockIndex(const uint256& hash)
     // Return existing
     auto mi = mapBlockIndex.find(hash);
     if (mi != mapBlockIndex.end())
-        return mi->second;
+        return *mi;
 
     if (hash.IsNull())
         return nullptr;
 
     // Create new
-    CBlockIndex* pindexNew = new CBlockIndex();
+    CBlockIndex* pindexNew = new CBlockIndex(hash);
-    mi = mapBlockIndex.insert(std::make_pair(hash, pindexNew)).first;
+    mapBlockIndex.insert(pindexNew);
-    pindexNew->phashBlock = &(mi->first);
 
     return pindexNew;
 }
@@ -3879,9 +3875,8 @@ bool CChainState::LoadBlockIndex(const Consensus::Params& consensus_params, CBlo
     // Calculate nChainWork
     std::vector<std::pair<int, CBlockIndex*> > vSortedByHeight;
     vSortedByHeight.reserve(mapBlockIndex.size());
-    for (const std::pair<const uint256, CBlockIndex*>& item : mapBlockIndex)
+    for (const auto& pindex : mapBlockIndex)
     {
-        CBlockIndex* pindex = item.second;
         vSortedByHeight.push_back(std::make_pair(pindex->nHeight, pindex));
     }
     sort(vSortedByHeight.begin(), vSortedByHeight.end());
@@ -3946,9 +3941,8 @@ bool static LoadBlockIndexDB(const CChainParams& chainparams) EXCLUSIVE_LOCKS_RE
     // 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 : g_chainstate.mapBlockIndex)
+    for (const auto& pindex : g_chainstate.mapBlockIndex)
     {
-        CBlockIndex* pindex = item.second;
         if (pindex->nStatus & BLOCK_HAVE_DATA) {
             setBlkDataFiles.insert(pindex->nFile);
         }
@@ -4135,10 +4129,7 @@ bool CChainState::RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& i
 
 bool CChainState::ReplayBlocks(const CChainParams& params, CCoinsView* view)
 {
-    LOCK(cs_main);
+    AssertLockHeld(cs_main);
-
-    CCoinsViewCache cache(view);
-    CClaimTrieCache trieCache(pclaimTrie);
 
     std::vector<uint256> hashHeads = view->GetHeadBlocks();
     if (hashHeads.empty()) return true; // We're already in a consistent state.
@@ -4151,20 +4142,25 @@ bool CChainState::ReplayBlocks(const CChainParams& params, CCoinsView* view)
     const CBlockIndex* pindexNew;            // New tip during the interrupted flush.
     const CBlockIndex* pindexFork = nullptr; // Latest block common to both the old and the new tip.
 
-    if (mapBlockIndex.count(hashHeads[0]) == 0) {
+    auto it = mapBlockIndex.find(hashHeads[0]);
+    if (it == mapBlockIndex.end()) {
         return error("ReplayBlocks(): reorganization to unknown block requested");
     }
-    pindexNew = mapBlockIndex[hashHeads[0]];
+    pindexNew = *it;
 
     if (!hashHeads[1].IsNull()) { // The old tip is allowed to be 0, indicating it's the first flush.
-        if (mapBlockIndex.count(hashHeads[1]) == 0) {
+        it = mapBlockIndex.find(hashHeads[1]);
+        if (it == mapBlockIndex.end()) {
             return error("ReplayBlocks(): reorganization from unknown block requested");
         }
-        pindexOld = mapBlockIndex[hashHeads[1]];
+        pindexOld = *it;
         pindexFork = LastCommonAncestor(pindexOld, pindexNew);
         assert(pindexFork != nullptr);
     }
 
+    CCoinsViewCache cache(view);
+    CClaimTrieCache trieCache(pclaimTrie);
+
     // Rollback along the old branch.
     while (pindexOld != pindexFork) {
         if (pindexOld->nHeight > 0) { // Never disconnect the genesis block.
@@ -4246,8 +4242,7 @@ bool CChainState::RewindBlockIndex(const CChainParams& params)
     // Reduce validity flag and have-data flags.
     // We do this after actual disconnecting, otherwise we'll end up writing the lack of data
     // to disk before writing the chainstate, resulting in a failure to continue if interrupted.
-    for (const auto& entry : mapBlockIndex) {
+    for (const auto& pindexIter : mapBlockIndex) {
-        CBlockIndex* pindexIter = entry.second;
 
         // Note: If we encounter an insufficiently validated block that
         // is on chainActive, it must be because we are a pruning node, and
@@ -4340,8 +4335,8 @@ void UnloadBlockIndex()
         warningcache[b].clear();
     }
 
-    for (BlockMap::value_type& entry : g_chainstate.mapBlockIndex) {
+    for (auto& entry : g_chainstate.mapBlockIndex) {
-        delete entry.second;
+        delete entry;
     }
     g_chainstate.mapBlockIndex.clear();
     fHavePruned = false;
@@ -4389,7 +4384,7 @@ bool CChainState::LoadGenesisBlock(const CChainParams& chainparams)
     // 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(chainparams.GenesisBlock().GetHash()))
+    if (mapBlockIndex.find(chainparams.GenesisBlock().GetHash()) != mapBlockIndex.end())
         return true;
 
     try {
@@ -4552,7 +4547,7 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
     // Build forward-pointing map of the entire block tree.
     std::multimap<CBlockIndex*,CBlockIndex*> forward;
     for (auto& entry : mapBlockIndex) {
-        forward.insert(std::make_pair(entry.second->pprev, entry.second));
+        forward.insert(std::make_pair(entry->pprev, entry));
     }
 
     assert(forward.size() == mapBlockIndex.size());
@@ -4898,7 +4893,7 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin
 CBlockIndex *LookupBlockIndex(const uint256 &hash) {
     AssertLockHeld(cs_main);
     auto it = g_chainstate.mapBlockIndex.find(hash);
-    return it == g_chainstate.mapBlockIndex.end() ? nullptr : it->second;
+    return it == g_chainstate.mapBlockIndex.end() ? nullptr : *it;
 }
 
 class CMainCleanup
@@ -4909,7 +4904,7 @@ public:
         // block headers
         auto it1 = g_chainstate.mapBlockIndex.begin();
         for (; it1 != g_chainstate.mapBlockIndex.end(); ++it1)
-            delete (*it1).second;
+            delete *it1;
         g_chainstate.mapBlockIndex.clear();
     }
 } instance_of_cmaincleanup;

src/validation.h

@@ -144,7 +144,25 @@ extern CCriticalSection cs_main;
 extern CBlockPolicyEstimator feeEstimator;
 extern CTxMemPool mempool;
 extern std::atomic_bool g_is_mempool_loaded;
-typedef std::map<uint256, CBlockIndex*> BlockMap;
+
+struct BlockIndexPointerCompare {
+    inline bool operator() (const CBlockIndex* lhs, const CBlockIndex* rhs) const {
+        return lhs->hash < rhs->hash;
+    }
+};
+
+struct BlockMap : public std::set<CBlockIndex*, BlockIndexPointerCompare> {
+    inline iterator find(const uint256& blockHash) {
+        CBlockIndex temp(blockHash);
+        return std::set<CBlockIndex*, BlockIndexPointerCompare>::find(&temp);
+    }
+
+    inline const_iterator find(const uint256& blockHash) const {
+        CBlockIndex temp(blockHash);
+        return std::set<CBlockIndex*, BlockIndexPointerCompare>::find(&temp);
+    }
+};
+
 extern uint64_t nLastBlockTx;
 extern uint64_t nLastBlockWeight;
 extern const std::string strMessageMagic;

src/wallet/test/wallet_tests.cpp

@@ -207,12 +207,9 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64
     CBlockIndex* block = nullptr;
     if (blockTime > 0) {
         LOCK(cs_main);
-        auto inserted = g_chainstate.mapBlockIndex.emplace(GetRandHash(), new CBlockIndex);
+        auto inserted = g_chainstate.mapBlockIndex.insert(new CBlockIndex(GetRandHash()));
-        assert(inserted.second);
+        block = *inserted.first;
-        const uint256& hash = inserted.first->first;
-        block = inserted.first->second;
         block->nTime = blockTime;
-        block->phashBlock = &hash;
     }
 
     CWalletTx wtx(&wallet, MakeTransactionRef(tx));