Merge #2715: [Refactor] Add LookupBlockIndex

dc930d4 net_processing: Move pindexBestKnownBlock and pindexBestHeader NULL usage to nullptr. (furszy)
58779a0 Add LookupBlockIndex function (furszy)
94f96a5 Assert cs_main is held when accessing mapBlockIndex (João Barbosa)

Pull request description:

  Adaptation of 92fabcd for our sources.

  > Replacing all mapBlockIndex lookups with a new `LookupBlockIndex()`. In some cases it avoids a second lookup.

ACKs for top commit:
  random-zebra:
    utACK dc930d4
  Fuzzbawls:
    ACK dc930d4

Tree-SHA512: bbda36ca6794579b700dd1cc57c7288536e2b2489019b118e9f203631b1e6162645ca9aa6fed11ee5ed249fbe2a44b6186a2a3ce68877d09f34cbd27ada3d6b2

src/budget/budgetmanager.cpp

@@ -1677,13 +1677,10 @@ bool CheckCollateral(const uint256& nTxCollateralHash, const uint256& nExpectedH
     int nProposalHeight = 0;
     {
         LOCK(cs_main);
-        BlockMap::iterator mi = mapBlockIndex.find(nBlockHash);
-        if (mi != mapBlockIndex.end() && (*mi).second) {
-            CBlockIndex* pindex = (*mi).second;
-            if (chainActive.Contains(pindex)) {
-                nProposalHeight = pindex->nHeight;
-                nTime = pindex->nTime;
-            }
+        CBlockIndex* pindex = LookupBlockIndex(nBlockHash);
+        if (pindex && chainActive.Contains(pindex)) {
+            nProposalHeight = pindex->nHeight;
+            nTime = pindex->nTime;
         }
     }
 

src/budget/finalizedbudget.cpp

@@ -311,8 +311,8 @@ bool CFinalizedBudget::IsPaidAlready(const uint256& nProposalHash, const uint256
     // -> reject transaction so it gets paid to a masternode instead
     if (nBlockHash != nPaidBlockHash) {
         LOCK(cs_main);
-        auto it = mapBlockIndex.find(nPaidBlockHash);
-        return it != mapBlockIndex.end() && chainActive.Contains(it->second);
+        CBlockIndex* pindex = LookupBlockIndex(nPaidBlockHash);
+        return pindex && chainActive.Contains(pindex);
     }
 
     // Re-checking same block. Not a double payment.

src/evo/specialtx_validation.cpp

@@ -442,11 +442,10 @@ bool VerifyLLMQCommitment(const llmq::CFinalCommitment& qfc, const CBlockIndex*
 
     if (pindexPrev) {
         // Get quorum index
-        auto it = mapBlockIndex.find(qfc.quorumHash);
-        if (it == mapBlockIndex.end()) {
+        CBlockIndex* pindexQuorum = LookupBlockIndex(qfc.quorumHash);
+        if (!pindexQuorum) {
             return state.DoS(100, false, REJECT_INVALID, "bad-qc-quorum-hash-not-found");
         }
-        const CBlockIndex* pindexQuorum = it->second;
 
         // Check height
         if (pindexQuorum->nHeight % params->dkgInterval != 0) {

src/init.cpp

@@ -1508,8 +1508,9 @@ bool AppInitMain()
 
                 // If the loaded chain has a wrong genesis, bail out immediately
                 // (we're likely using a testnet datadir, or the other way around).
-                if (!mapBlockIndex.empty() && mapBlockIndex.count(consensus.hashGenesisBlock) == 0)
+                if (!mapBlockIndex.empty() && !LookupBlockIndex(consensus.hashGenesisBlock)) {
                     return UIError(_("Incorrect or no genesis block found. Wrong datadir for network?"));
+                }
 
                 // Check for changed -txindex state
                 if (fTxIndex != gArgs.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {

src/net_processing.cpp

@@ -223,9 +223,9 @@ struct CNodeState {
         fCurrentlyConnected = false;
         nMisbehavior = 0;
         fShouldBan = false;
-        pindexBestKnownBlock = NULL;
+        pindexBestKnownBlock = nullptr;
         hashLastUnknownBlock.SetNull();
-        pindexLastCommonBlock = NULL;
+        pindexLastCommonBlock = nullptr;
         fSyncStarted = false;
         nStallingSince = 0;
         nBlocksInFlight = 0;
@@ -310,7 +310,7 @@ void MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex*
     // Make sure it's not listed somewhere already.
     MarkBlockAsReceived(hash);
 
-    QueuedBlock newentry = {hash, pindex, GetTimeMicros(), nQueuedValidatedHeaders, pindex != NULL};
+    QueuedBlock newentry = {hash, pindex, GetTimeMicros(), nQueuedValidatedHeaders, pindex != nullptr};
     nQueuedValidatedHeaders += newentry.fValidatedHeaders;
     std::list<QueuedBlock>::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(), newentry);
     state->nBlocksInFlight++;
@@ -324,10 +324,10 @@ void ProcessBlockAvailability(NodeId nodeid)
     assert(state != nullptr);
 
     if (!state->hashLastUnknownBlock.IsNull()) {
-        BlockMap::iterator itOld = mapBlockIndex.find(state->hashLastUnknownBlock);
-        if (itOld != mapBlockIndex.end() && itOld->second->nChainWork > 0) {
-            if (state->pindexBestKnownBlock == NULL || itOld->second->nChainWork >= state->pindexBestKnownBlock->nChainWork)
-                state->pindexBestKnownBlock = itOld->second;
+        CBlockIndex* pindex = LookupBlockIndex(state->hashLastUnknownBlock);
+        if (pindex && pindex->nChainWork > 0) {
+            if (!state->pindexBestKnownBlock || pindex->nChainWork >= state->pindexBestKnownBlock->nChainWork)
+                state->pindexBestKnownBlock = pindex;
             state->hashLastUnknownBlock.SetNull();
         }
     }
@@ -341,11 +341,11 @@ void UpdateBlockAvailability(NodeId nodeid, const uint256& hash)
 
     ProcessBlockAvailability(nodeid);
 
-    BlockMap::iterator it = mapBlockIndex.find(hash);
-    if (it != mapBlockIndex.end() && it->second->nChainWork > 0) {
+    CBlockIndex* pindex = LookupBlockIndex(hash);
+    if (pindex && pindex->nChainWork > 0) {
         // An actually better block was announced.
-        if (state->pindexBestKnownBlock == NULL || it->second->nChainWork >= state->pindexBestKnownBlock->nChainWork)
-            state->pindexBestKnownBlock = it->second;
+        if (!state->pindexBestKnownBlock || pindex->nChainWork >= state->pindexBestKnownBlock->nChainWork)
+            state->pindexBestKnownBlock = pindex;
     } else {
         // An unknown block was announced; just assume that the latest one is the best one.
         state->hashLastUnknownBlock = hash;
@@ -366,12 +366,12 @@ void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector<con
     // Make sure pindexBestKnownBlock is up to date, we'll need it.
     ProcessBlockAvailability(nodeid);
 
-    if (state->pindexBestKnownBlock == NULL || state->pindexBestKnownBlock->nChainWork < chainActive.Tip()->nChainWork) {
+    if (!state->pindexBestKnownBlock || state->pindexBestKnownBlock->nChainWork < chainActive.Tip()->nChainWork) {
         // This peer has nothing interesting.
         return;
     }
 
-    if (state->pindexLastCommonBlock == NULL) {
+    if (!state->pindexLastCommonBlock) {
         // Bootstrap quickly by guessing a parent of our best tip is the forking point.
         // Guessing wrong in either direction is not a problem.
         state->pindexLastCommonBlock = chainActive[std::min(state->pindexBestKnownBlock->nHeight, chainActive.Height())];
@@ -475,7 +475,7 @@ bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats)
 {
     LOCK(cs_main);
     CNodeState* state = State(nodeid);
-    if (state == NULL)
+    if (!state)
         return false;
     stats.nMisbehavior = state->nMisbehavior;
     stats.nSyncHeight = state->pindexBestKnownBlock ? state->pindexBestKnownBlock->nHeight : -1;
@@ -637,9 +637,9 @@ static void CheckBlockSpam(NodeId nodeId, const uint256& hashBlock)
         nodestate = State(nodeId);
         if (!nodestate) { return; }
 
-        const auto it = mapBlockIndex.find(hashBlock);
-        if (it == mapBlockIndex.end()) { return; }
-        blockReceivedHeight = it->second->nHeight;
+        CBlockIndex* pindex = LookupBlockIndex(hashBlock);
+        if (!pindex) { return; }
+        blockReceivedHeight = pindex->nHeight;
     }
 
     nodestate->nodeBlocks.onBlockReceived(blockReceivedHeight);
@@ -782,7 +782,7 @@ bool static AlreadyHave(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
     }
 
     case MSG_BLOCK:
-        return mapBlockIndex.count(inv.hash);
+        return LookupBlockIndex(inv.hash) != nullptr;
     case MSG_TXLOCK_REQUEST:
         // deprecated
         return true;
@@ -972,26 +972,26 @@ void static ProcessGetBlockData(CNode* pfrom, const CInv& inv, CConnman* connman
     CNetMsgMaker msgMaker(pfrom->GetSendVersion());
 
     bool send = false;
-    BlockMap::iterator mi = mapBlockIndex.find(inv.hash);
-    if (mi != mapBlockIndex.end()) {
-        if (chainActive.Contains(mi->second)) {
+    CBlockIndex* pindex = LookupBlockIndex(inv.hash);
+    if (pindex) {
+        if (chainActive.Contains(pindex)) {
             send = true;
         } else {
             // To prevent fingerprinting attacks, only send blocks outside of the active
             // chain if they are valid, and no more than a max reorg depth than the best header
             // chain we know about.
-            send = mi->second->IsValid(BLOCK_VALID_SCRIPTS) && (pindexBestHeader != NULL) &&
-                   (chainActive.Height() - mi->second->nHeight < gArgs.GetArg("-maxreorg", DEFAULT_MAX_REORG_DEPTH));
+            send = pindex->IsValid(BLOCK_VALID_SCRIPTS) && pindexBestHeader &&
+                   (chainActive.Height() - pindex->nHeight < gArgs.GetArg("-maxreorg", DEFAULT_MAX_REORG_DEPTH));
             if (!send) {
                 LogPrint(BCLog::NET, "ProcessGetData(): ignoring request from peer=%i for old block that isn't in the main chain\n", pfrom->GetId());
             }
         }
     }
     // Don't send not-validated blocks
-    if (send && (mi->second->nStatus & BLOCK_HAVE_DATA)) {
+    if (send && (pindex->nStatus & BLOCK_HAVE_DATA)) {
         // Send block from disk
         CBlock block;
-        if (!ReadBlockFromDisk(block, (*mi).second))
+        if (!ReadBlockFromDisk(block, pindex))
             assert(!"cannot load block from disk");
         if (inv.type == MSG_BLOCK)
             connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::BLOCK, block));
@@ -1588,13 +1588,12 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
         if (IsInitialBlockDownload())
             return true;
 
-        CBlockIndex* pindex = NULL;
+        CBlockIndex* pindex = nullptr;
         if (locator.IsNull()) {
             // If locator is null, return the hashStop block
-            BlockMap::iterator mi = mapBlockIndex.find(hashStop);
-            if (mi == mapBlockIndex.end())
+            CBlockIndex* pindex = LookupBlockIndex(hashStop);
+            if (!pindex)
                 return true;
-            pindex = (*mi).second;
         } else {
             // Find the last block the caller has in the main chain
             pindex = FindForkInGlobalIndex(chainActive, locator);
@@ -1789,10 +1788,10 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
             // Nothing interesting. Stop asking this peers for more headers.
             return true;
         }
-        CBlockIndex* pindexLast = NULL;
+        CBlockIndex* pindexLast = nullptr;
         for (const CBlockHeader& header : headers) {
             CValidationState state;
-            if (pindexLast != NULL && header.hashPrevBlock != pindexLast->GetBlockHash()) {
+            if (pindexLast && header.hashPrevBlock != pindexLast->GetBlockHash()) {
                 Misbehaving(pfrom->GetId(), 20, "non-continuous headers sequence");
                 return false;
             }
@@ -2188,7 +2187,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& inter
     } catch (const std::exception& e) {
         PrintExceptionContinue(&e, "ProcessMessages()");
     } catch (...) {
-        PrintExceptionContinue(NULL, "ProcessMessages()");
+        PrintExceptionContinue(nullptr, "ProcessMessages()");
     }
 
     if (!fRet) {
@@ -2306,7 +2305,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto, std::atomic<bool>& interruptM
         }
 
         // Start block sync
-        if (pindexBestHeader == NULL)
+        if (!pindexBestHeader)
             pindexBestHeader = chainActive.Tip();
         bool fFetch = state.fPreferredDownload || (nPreferredDownload == 0 && !pto->fClient && !pto->fOneShot); // Download if this is a nice peer, or we have no nice peers and this one might do.
         if (!state.fSyncStarted && !pto->fClient && !fImporting && !fReindex && pto->CanRelay()) {

src/rest.cpp

@@ -131,8 +131,7 @@ static bool rest_headers(HTTPRequest* req,
     headers.reserve(count);
     {
         LOCK(cs_main);
-        BlockMap::const_iterator it = mapBlockIndex.find(hash);
-        const CBlockIndex *pindex = (it != mapBlockIndex.end()) ? it->second : NULL;
+        CBlockIndex* pindex = LookupBlockIndex(hash);
         while (pindex != NULL && chainActive.Contains(pindex)) {
             headers.push_back(pindex);
             if (headers.size() == (unsigned long)count)

src/rpc/blockchain.cpp

@@ -733,7 +733,7 @@ static bool GetUTXOStats(CCoinsView *view, CCoinsStats &stats)
     stats.hashBlock = pcursor->GetBestBlock();
     {
         LOCK(cs_main);
-        stats.nHeight = mapBlockIndex.find(stats.hashBlock)->second->nHeight;
+        stats.nHeight = LookupBlockIndex(stats.hashBlock)->nHeight;
     }
     ss << stats.hashBlock;
     uint256 prevkey;
@@ -863,8 +863,7 @@ UniValue gettxout(const JSONRPCRequest& request)
         }
     }
 
-    BlockMap::iterator it = mapBlockIndex.find(pcoinsTip->GetBestBlock());
-    CBlockIndex* pindex = it->second;
+    CBlockIndex* pindex = LookupBlockIndex(pcoinsTip->GetBestBlock());
     assert(pindex != nullptr);
     ret.pushKV("bestblock", pindex->GetBlockHash().GetHex());
     if (coin.nHeight == MEMPOOL_HEIGHT) {

src/rpc/mining.cpp

@@ -548,9 +548,8 @@ UniValue getblocktemplate(const JSONRPCRequest& request)
                 throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block decode failed");
 
             uint256 hash = block.GetHash();
-            BlockMap::iterator mi = mapBlockIndex.find(hash);
-            if (mi != mapBlockIndex.end()) {
-                CBlockIndex* pindex = mi->second;
+            CBlockIndex* pindex = LookupBlockIndex(hash);
+            if (pindex) {
                 if (pindex->IsValid(BLOCK_VALID_SCRIPTS))
                     return "duplicate";
                 if (pindex->nStatus & BLOCK_FAILED_MASK)
@@ -761,9 +760,8 @@ UniValue submitblock(const JSONRPCRequest& request)
     bool fBlockPresent = false;
     {
         LOCK(cs_main);
-        BlockMap::iterator mi = mapBlockIndex.find(hash);
-        if (mi != mapBlockIndex.end()) {
-            CBlockIndex* pindex = mi->second;
+        CBlockIndex* pindex = LookupBlockIndex(hash);
+        if (pindex) {
             if (pindex->IsValid(BLOCK_VALID_SCRIPTS))
                 return "duplicate";
             if (pindex->nStatus & BLOCK_FAILED_MASK)

src/rpc/rawtransaction.cpp

@@ -99,9 +99,8 @@ void TxToJSON(CWallet* const pwallet, const CTransaction& tx, const uint256 hash
 
     if (!hashBlock.IsNull()) {
         entry.pushKV("blockhash", hashBlock.GetHex());
-        BlockMap::iterator mi = mapBlockIndex.find(hashBlock);
-        if (mi != mapBlockIndex.end() && (*mi).second) {
-            CBlockIndex* pindex = (*mi).second;
+        CBlockIndex* pindex = LookupBlockIndex(hashBlock);
+        if (pindex) {
             if (chainActive.Contains(pindex)) {
                 entry.pushKV("confirmations", 1 + chainActive.Height() - pindex->nHeight);
                 entry.pushKV("time", pindex->GetBlockTime());
@@ -236,11 +235,10 @@ UniValue getrawtransaction(const JSONRPCRequest& request)
 
     if (!request.params[2].isNull()) {
         uint256 blockhash = ParseHashV(request.params[2], "parameter 3");
-        BlockMap::iterator it = mapBlockIndex.find(blockhash);
-        if (it == mapBlockIndex.end()) {
+        blockindex = LookupBlockIndex(blockhash);
+        if (!blockindex) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block hash not found");
         }
-        blockindex = it->second;
         in_active_chain = chainActive.Contains(blockindex);
     }