From ad442a4d5f4d5baf4f27ed64a3dafa55106719a3 Mon Sep 17 00:00:00 2001 From: Zannick Date: Sun, 9 May 2021 15:58:35 -0700 Subject: [PATCH 1/2] Give mapBlockIndex its own lock. Declare mapBlockIndex to be protected by cs_mapblockindex. Move a lot of generic lookups to LookupBlockIndex, while other more complex uses (e.g. atomic lookup+insert) are left alone. Make sure the lock is taken everywhere necessary (places found with clang thread-safety warnings) and only as tightly as possible to avoid potential lock cycles (detectable by tsan). Many uses of cs_main are replaced by cs_mapblockindex if they were only protecting mapBlockIndex, or removed (LookupBlockIndex taking the lock itself), or moved to after LookupBlockIndex calls. Rework PruneStaleBlockIndexes to do most work without the lock. --- src/chain.h | 2 +- src/index/base.cpp | 6 +- src/init.cpp | 17 +- src/interfaces/wallet.cpp | 6 +- src/miner.cpp | 12 +- src/net_processing.cpp | 45 +++-- src/qt/coincontroldialog.cpp | 29 +-- src/rest.cpp | 41 ++--- src/rpc/blockchain.cpp | 108 +++++------ src/rpc/mining.cpp | 52 ++---- src/rpc/rawtransaction.cpp | 9 - src/validation.cpp | 281 +++++++++++++++++------------ src/validation.h | 8 +- src/veil/ringct/anonwallet.cpp | 21 +-- src/veil/zerocoin/accumulators.cpp | 5 +- src/veil/zerocoin/zchain.cpp | 15 +- src/veil/zerocoin/ztracker.cpp | 7 +- src/veil/zerocoin/zwallet.cpp | 7 +- src/wallet/rpcwallet.cpp | 4 +- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 15 +- 21 files changed, 352 insertions(+), 340 deletions(-) diff --git a/src/chain.h b/src/chain.h index 82831949d6..bd81e39e49 100644 --- a/src/chain.h +++ b/src/chain.h @@ -179,7 +179,7 @@ enum BlockMemFlags: uint32_t { class CBlockIndex { public: - //! pointer to the hash of the block, if any. Memory is owned by this CBlockIndex + //! pointer to the hash of the block, if any. Memory is owned by mapBlockIndex const uint256* phashBlock; //! pointer to the index of the predecessor of this block diff --git a/src/index/base.cpp b/src/index/base.cpp index 9d274a55e7..20e9432775 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -194,11 +194,7 @@ void BaseIndex::ChainStateFlushed(const CBlockLocator& locator) } const uint256& locator_tip_hash = locator.vHave.front(); - const CBlockIndex* locator_tip_index; - { - LOCK(cs_main); - locator_tip_index = LookupBlockIndex(locator_tip_hash); - } + const CBlockIndex* locator_tip_index = LookupBlockIndex(locator_tip_hash); if (!locator_tip_index) { FatalError("%s: First block (hash=%s) in locator was not found", diff --git a/src/init.cpp b/src/init.cpp index dd6c4adbac..72014fbff5 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1602,10 +1602,13 @@ bool AppInitMain() break; } - // 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() && !LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) { - return InitError(_("Incorrect or no genesis block found. Wrong datadir for network?")); + { + LOCK(cs_mapblockindex); + // 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() && !LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) { + return InitError(_("Incorrect or no genesis block found. Wrong datadir for network?")); + } } // Check for changed -prune state. What we are concerned about is a user who has pruned blocks @@ -1835,14 +1838,12 @@ bool AppInitMain() // ********************************************************* Step 12: start node - int chain_active_height; - //// debug print { - LOCK(cs_main); + LOCK(cs_mapblockindex); LogPrintf("mapBlockIndex.size() = %u\n", mapBlockIndex.size()); - chain_active_height = chainActive.Height(); } + int chain_active_height = chainActive.Height(); LogPrintf("nBestHeight = %d\n", chain_active_height); if (gArgs.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 0980378305..b72d43c3f8 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -237,8 +237,7 @@ WalletTxStatus MakeWalletTxStatus(const CWalletTx& wtx) { LOCK(cs_main); WalletTxStatus result; - auto mi = ::mapBlockIndex.find(wtx.hashBlock); - CBlockIndex* block = mi != ::mapBlockIndex.end() ? mi->second : nullptr; + CBlockIndex* block = LookupBlockIndex(wtx.hashBlock); result.block_height = (block ? block->nHeight : std::numeric_limits::max()); result.blocks_to_maturity = wtx.GetBlocksToMaturity(); result.depth_in_main_chain = wtx.GetDepthInMainChain(); @@ -256,9 +255,8 @@ WalletTxStatus MakeWalletTxStatus(const CWalletTx& wtx) WalletTxStatus MakeWalletTxStatus(AnonWallet* pAnonWallet, const uint256 &hash, const CTransactionRecord &rtx) { WalletTxStatus result; - auto mi = ::mapBlockIndex.find(rtx.blockHash); - CBlockIndex* block = mi != ::mapBlockIndex.end() ? mi->second : nullptr; + CBlockIndex* block = LookupBlockIndex(rtx.blockHash); result.block_height = (block ? block->nHeight : std::numeric_limits::max()), result.blocks_to_maturity = 0; diff --git a/src/miner.cpp b/src/miner.cpp index 19126e3019..d64d60feb7 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -168,13 +168,13 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc CMutableTransaction txCoinStake; CBlockIndex* pindexPrev; + //Do not pass in the chain tip, because it can change. Instead pass the blockindex directly from mapblockindex, which is const. + auto pindexTip = chainActive.Tip(); + if (!pindexTip) + return nullptr; + auto hashBest = pindexTip->GetBlockHash(); { - LOCK(cs_main); - //Do not pass in the chain tip, because it can change. Instead pass the blockindex directly from mapblockindex, which is const. - auto pindexTip = chainActive.Tip(); - if (!pindexTip) - return nullptr; - auto hashBest = pindexTip->GetBlockHash(); + LOCK(cs_mapblockindex); pindexPrev = mapBlockIndex.at(hashBest); } if (fProofOfStake && pindexPrev->nHeight + 1 >= Params().HeightPoSStart()) { diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e1bbd79d62..480afaff7b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1156,21 +1156,19 @@ void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, c } bool need_activate_chain = false; - { - LOCK(cs_main); - const CBlockIndex* pindex = LookupBlockIndex(inv.hash); - if (pindex) { - if (pindex->nChainTx && !pindex->IsValid(BLOCK_VALID_SCRIPTS) && - pindex->IsValid(BLOCK_VALID_TREE)) { - // If we have the block and all of its parents, but have not yet validated it, - // we might be in the middle of connecting it (ie in the unlock of cs_main - // before ActivateBestChain but after AcceptBlock). - // In this case, we need to run ActivateBestChain prior to checking the relay - // conditions below. - need_activate_chain = true; - } + + const CBlockIndex* pindex = LookupBlockIndex(inv.hash); + if (pindex) { + if (pindex->nChainTx && !pindex->IsValid(BLOCK_VALID_SCRIPTS) && + pindex->IsValid(BLOCK_VALID_TREE)) { + // If we have the block and all of its parents, but have not yet validated it, + // we might be in the middle of connecting it + // (ie before ActivateBestChain but after AcceptBlock). + // In this case, we need to run ActivateBestChain prior to checking the relay + // conditions below. + need_activate_chain = true; } - } // release cs_main before calling ActivateBestChain + } if (need_activate_chain) { CValidationState state; if (!ActivateBestChain(state, Params(), a_recent_block)) { @@ -1178,8 +1176,9 @@ void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, c } } + pindex = LookupBlockIndex(inv.hash); + LOCK(cs_main); - const CBlockIndex* pindex = LookupBlockIndex(inv.hash); if (pindex) { send = BlockRequestAllowed(pindex, consensusParams); if (!send) { @@ -1577,7 +1576,10 @@ void ProcessStaging() continue; } - fProcessNext = mapBlockIndex.at(pblockStaged->hashPrevBlock)->nChainTx > 0; + { + LOCK(cs_mapblockindex); + fProcessNext = mapBlockIndex.at(pblockStaged->hashPrevBlock)->nChainTx > 0; + } if (!fProcessNext) { MilliSleep(100); @@ -2447,14 +2449,14 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr return true; } - LOCK(cs_main); - const CBlockIndex* pindex = LookupBlockIndex(req.blockhash); if (!pindex || !(pindex->nStatus & BLOCK_HAVE_DATA)) { LogPrint(BCLog::NET, "Peer %d sent us a getblocktxn for a block we don't have\n", pfrom->GetId()); return true; } + LOCK(cs_main); + if (pindex->nHeight < chainActive.Height() - MAX_BLOCKTXN_DEPTH) { // If an older block is requested (should never happen in practice, // but can happen in tests) send a block response instead of a @@ -2776,10 +2778,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr bool received_new_header = false; - { - LOCK(cs_main); - if (!LookupBlockIndex(cmpctblock.header.hashPrevBlock)) { + LOCK(cs_main); // Doesn't connect (or is genesis), instead of DoSing in AcceptBlockHeader, request deeper headers if (!IsInitialBlockDownload()) connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::GETHEADERS, chainActive.GetLocator(pindexBestHeader), uint256())); @@ -2789,7 +2789,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr if (!LookupBlockIndex(cmpctblock.header.GetHash())) { received_new_header = true; } - } const CBlockIndex *pindex = nullptr; CValidationState state; @@ -3105,7 +3104,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr int nHeightNext = 0; const uint256 hash(pblock->GetHash()); { - LOCK(cs_main); + LOCK2(cs_main, cs_mapblockindex); // Also always process if we requested the block explicitly, as we may // need it even though it is not a candidate for a new best tip. forceProcessing |= MarkBlockAsReceived(hash); diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index fa69b51661..67bf4f49ff 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -565,14 +565,15 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog) uint256 hashBlock; if (!GetTransaction(txid, txRef, Params().GetConsensus(), hashBlock, true)) continue; - auto it = mapBlockIndex.find(hashBlock); - if (it == mapBlockIndex.end()) + + CBlockIndex* block = LookupBlockIndex(hashBlock); + if (block == nullptr) continue; - if (!chainActive.Contains(it->second)) + if (!chainActive.Contains(block)) continue; - nDepth = chainActive.Height() - it->second->nHeight + 1; + nDepth = chainActive.Height() - block->nHeight + 1; } if (nDepth < 1) @@ -888,15 +889,15 @@ void CoinControlDialog::updateView(int nCoinType) if (!GetTransaction(txid, txRef, Params().GetConsensus(), hashBlock, true)) continue; - auto it = mapBlockIndex.find(hashBlock); - if (it == mapBlockIndex.end()) + CBlockIndex* block = LookupBlockIndex(hashBlock); + if (block == nullptr) continue; - if (!chainActive.Contains(it->second)) + if (!chainActive.Contains(block)) continue; - nDepth = chainActive.Height() - it->second->nHeight + 1; - nTimeTx = it->second->GetBlockTime(); + nDepth = chainActive.Height() - block->nHeight + 1; + nTimeTx = block->GetBlockTime(); } for (unsigned int i = 0; i < txRecord.vout.size(); i++) { @@ -1062,15 +1063,15 @@ void CoinControlDialog::updateView(int nCoinType) if (!GetTransaction(mint.txid, txRef, Params().GetConsensus(), hashBlock, true)) continue; - auto it = mapBlockIndex.find(hashBlock); - if (it == mapBlockIndex.end()) + CBlockIndex* block = LookupBlockIndex(hashBlock); + if (block == nullptr) continue; - if (!chainActive.Contains(it->second)) + if (!chainActive.Contains(block)) continue; - nDepth = chainActive.Height() - it->second->nHeight + 1; - nTimeTx = it->second->GetBlockTime(); + nDepth = chainActive.Height() - block->nHeight + 1; + nTimeTx = block->GetBlockTime(); } // increase the count for each denom we see diff --git a/src/rest.cpp b/src/rest.cpp index 566e9b1b80..4261b4a889 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -147,16 +147,14 @@ static bool rest_headers(HTTPRequest* req, const CBlockIndex* tip = nullptr; std::vector headers; headers.reserve(count); - { - LOCK(cs_main); - tip = chainActive.Tip(); - const CBlockIndex* pindex = LookupBlockIndex(hash); - while (pindex != nullptr && chainActive.Contains(pindex)) { - headers.push_back(pindex); - if (headers.size() == (unsigned long)count) - break; - pindex = chainActive.Next(pindex); - } + + tip = chainActive.Tip(); + const CBlockIndex* pindex = LookupBlockIndex(hash); + while (pindex != nullptr && chainActive.Contains(pindex)) { + headers.push_back(pindex); + if (headers.size() == (unsigned long)count) + break; + pindex = chainActive.Next(pindex); } CDataStream ssHeader(SER_NETWORK, PROTOCOL_VERSION); @@ -210,22 +208,17 @@ static bool rest_block(HTTPRequest* req, return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr); CBlock block; - CBlockIndex* pblockindex = nullptr; - CBlockIndex* tip = nullptr; - { - LOCK(cs_main); - tip = chainActive.Tip(); - pblockindex = LookupBlockIndex(hash); - if (!pblockindex) { - return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found"); - } + CBlockIndex* tip = chainActive.Tip(); + CBlockIndex* pblockindex = LookupBlockIndex(hash); + if (!pblockindex) { + return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found"); + } - if (IsBlockPruned(pblockindex)) - return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)"); + if (IsBlockPruned(pblockindex)) + return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)"); - if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus())) - return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found"); - } + if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus())) + return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found"); CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags()); ssBlock << block; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index e6dbc33b3f..2e342565e8 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -960,35 +960,27 @@ static UniValue getblockheader(const JSONRPCRequest& request) + HelpExampleRpc("getblockheader", "\"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09\"") ); - const CBlockIndex* pChainTip; - const CBlockIndex* pblockindex; - { - LOCK(cs_main); - - std::string strHash = request.params[0].get_str(); - uint256 hash(uint256S(strHash)); - - bool fVerbose = true; - if (!request.params[1].isNull()) - fVerbose = request.params[1].get_bool(); + std::string strHash = request.params[0].get_str(); + uint256 hash(uint256S(strHash)); - pblockindex = LookupBlockIndex(hash); - if (!pblockindex) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); - } + bool fVerbose = true; + if (!request.params[1].isNull()) + fVerbose = request.params[1].get_bool(); - if (!fVerbose) - { - CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION); - ssBlock << pblockindex->GetBlockHeader(); - std::string strHex = HexStr(ssBlock.begin(), ssBlock.end()); - return strHex; - } + const CBlockIndex* pblockindex = LookupBlockIndex(hash); + if (!pblockindex) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); + } - pChainTip = chainActive.Tip(); + if (!fVerbose) + { + CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION); + ssBlock << pblockindex->GetBlockHeader(); + std::string strHex = HexStr(ssBlock.begin(), ssBlock.end()); + return strHex; } - return blockheaderToJSON(pChainTip, pblockindex); + return blockheaderToJSON(chainActive.Tip(), pblockindex); } static CBlock GetBlockChecked(const CBlockIndex* pblockindex) @@ -1072,21 +1064,15 @@ static UniValue getblock(const JSONRPCRequest& request) verbosity = request.params[1].get_bool() ? 1 : 0; } - CBlock block; - const CBlockIndex* pblockindex; - const CBlockIndex* tip; - { - LOCK(cs_main); - pblockindex = LookupBlockIndex(hash); - tip = chainActive.Tip(); - - if (!pblockindex) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); - } + const CBlockIndex* pblockindex = LookupBlockIndex(hash); + const CBlockIndex* tip = chainActive.Tip(); - block = GetBlockChecked(pblockindex); + if (!pblockindex) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); } + CBlock block = GetBlockChecked(pblockindex); + if (verbosity <= 0) { CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags()); @@ -1138,10 +1124,7 @@ static bool GetUTXOStats(CCoinsView *view, CCoinsStats &stats) CHashWriter ss(SER_GETHASH, PROTOCOL_VERSION); stats.hashBlock = pcursor->GetBestBlock(); - { - LOCK(cs_main); - stats.nHeight = LookupBlockIndex(stats.hashBlock)->nHeight; - } + stats.nHeight = LookupBlockIndex(stats.hashBlock)->nHeight; ss << stats.hashBlock; uint256 prevkey; std::map outputs; @@ -1609,11 +1592,14 @@ static UniValue getchaintips(const JSONRPCRequest& request) std::set setOrphans; std::set setPrevs; - for (const std::pair& item : mapBlockIndex) { - if (!chainActive.Contains(item.second)) { - setOrphans.insert(item.second); - setPrevs.insert(item.second->pprev); + LOCK(cs_mapblockindex); + for (const std::pair& item : mapBlockIndex) + { + if (!chainActive.Contains(item.second)) { + setOrphans.insert(item.second); + setPrevs.insert(item.second->pprev); + } } } @@ -1721,14 +1707,10 @@ static UniValue preciousblock(const JSONRPCRequest& request) std::string strHash = request.params[0].get_str(); uint256 hash(uint256S(strHash)); - CBlockIndex* pblockindex; - { - LOCK(cs_main); - pblockindex = LookupBlockIndex(hash); - if (!pblockindex) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); - } + CBlockIndex* pblockindex = LookupBlockIndex(hash); + if (!pblockindex) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); } CValidationState state; @@ -1759,13 +1741,13 @@ static UniValue invalidateblock(const JSONRPCRequest& request) uint256 hash(uint256S(strHash)); CValidationState state; + CBlockIndex* pblockindex = LookupBlockIndex(hash); + if (!pblockindex) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); + } + { LOCK(cs_main); - CBlockIndex* pblockindex = LookupBlockIndex(hash); - if (!pblockindex) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); - } - InvalidateBlock(state, Params(), pblockindex); } @@ -1798,13 +1780,13 @@ static UniValue reconsiderblock(const JSONRPCRequest& request) std::string strHash = request.params[0].get_str(); uint256 hash(uint256S(strHash)); + CBlockIndex* pblockindex = LookupBlockIndex(hash); + if (!pblockindex) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); + } + { LOCK(cs_main); - CBlockIndex* pblockindex = LookupBlockIndex(hash); - if (!pblockindex) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); - } - ResetBlockFailureFlags(pblockindex); } @@ -1846,11 +1828,9 @@ static UniValue getchaintxstats(const JSONRPCRequest& request) int blockcount = 30 * 24 * 60 * 60 / Params().GetConsensus().nPowTargetSpacing; // By default: 1 month if (request.params[1].isNull()) { - LOCK(cs_main); pindex = chainActive.Tip(); } else { uint256 hash = uint256S(request.params[1].get_str()); - LOCK(cs_main); pindex = LookupBlockIndex(hash); if (!pindex) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); @@ -2008,8 +1988,6 @@ static UniValue getblockstats(const JSONRPCRequest& request) ); } - LOCK(cs_main); - CBlockIndex* pindex; if (request.params[0].isNum()) { const int height = request.params[0].get_int(); diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 73ecc9f1d7..010ce9939f 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -993,25 +993,19 @@ static UniValue pprpcsb(const JSONRPCRequest& request) { blockptr->mixHash = calculated_mix_hash; uint256 hash = blockptr->GetHash(); - { - LOCK(cs_main); - const CBlockIndex* pindex = LookupBlockIndex(hash); - if (pindex) { - if (pindex->IsValid(BLOCK_VALID_SCRIPTS)) { - return "duplicate"; - } - if (pindex->nStatus & BLOCK_FAILED_MASK) { - return "duplicate-invalid"; - } + const CBlockIndex* pindex = LookupBlockIndex(hash); + if (pindex) { + if (pindex->IsValid(BLOCK_VALID_SCRIPTS)) { + return "duplicate"; + } + if (pindex->nStatus & BLOCK_FAILED_MASK) { + return "duplicate-invalid"; } } - { - LOCK(cs_main); - const CBlockIndex* pindex = LookupBlockIndex(blockptr->hashPrevBlock); - if (pindex) { - UpdateUncommittedBlockStructures(*blockptr, pindex, Params().GetConsensus()); - } + pindex = LookupBlockIndex(blockptr->hashPrevBlock); + if (pindex) { + UpdateUncommittedBlockStructures(*blockptr, pindex, Params().GetConsensus()); } bool new_block; @@ -1069,25 +1063,19 @@ static UniValue submitblock(const JSONRPCRequest& request) } uint256 hash = block.GetHash(); - { - LOCK(cs_main); - const CBlockIndex* pindex = LookupBlockIndex(hash); - if (pindex) { - if (pindex->IsValid(BLOCK_VALID_SCRIPTS)) { - return "duplicate"; - } - if (pindex->nStatus & BLOCK_FAILED_MASK) { - return "duplicate-invalid"; - } + const CBlockIndex* pindex = LookupBlockIndex(hash); + if (pindex) { + if (pindex->IsValid(BLOCK_VALID_SCRIPTS)) { + return "duplicate"; + } + if (pindex->nStatus & BLOCK_FAILED_MASK) { + return "duplicate-invalid"; } } - { - LOCK(cs_main); - const CBlockIndex* pindex = LookupBlockIndex(block.hashPrevBlock); - if (pindex) { - UpdateUncommittedBlockStructures(block, pindex, Params().GetConsensus()); - } + pindex = LookupBlockIndex(block.hashPrevBlock); + if (pindex) { + UpdateUncommittedBlockStructures(block, pindex, Params().GetConsensus()); } bool new_block; diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 313fd934f9..f56aa61f69 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -51,8 +51,6 @@ static void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& TxToUniv(tx, uint256(), vTxRingCtInputs, entry, true, RPCSerializationFlags()); if (!hashBlock.IsNull()) { - LOCK(cs_main); - entry.pushKV("blockhash", hashBlock.GetHex()); CBlockIndex* pindex = LookupBlockIndex(hashBlock); if (pindex) { @@ -164,8 +162,6 @@ static UniValue getrawtransaction(const JSONRPCRequest& request) } if (!request.params[2].isNull()) { - LOCK(cs_main); - uint256 blockhash = ParseHashV(request.params[2], "parameter 3"); blockindex = LookupBlockIndex(blockhash); if (!blockindex) { @@ -248,7 +244,6 @@ static UniValue gettxoutproof(const JSONRPCRequest& request) CBlockIndex* pblockindex = nullptr; uint256 hashBlock; if (!request.params[1].isNull()) { - LOCK(cs_main); hashBlock = uint256S(request.params[1].get_str()); pblockindex = LookupBlockIndex(hashBlock); if (!pblockindex) { @@ -273,8 +268,6 @@ static UniValue gettxoutproof(const JSONRPCRequest& request) g_txindex->BlockUntilSyncedToCurrentChain(); } - LOCK(cs_main); - if (pblockindex == nullptr) { CTransactionRef tx; @@ -329,8 +322,6 @@ static UniValue verifytxoutproof(const JSONRPCRequest& request) //if (merkleBlock.txn.ExtractMatches(vMatch, vIndex) != merkleBlock.header.hashMerkleRoot) // return res; - LOCK(cs_main); - const CBlockIndex* pindex = LookupBlockIndex(merkleBlock.header.GetHash()); if (!pindex || !chainActive.Contains(pindex) || pindex->nTx == 0) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found in chain"); diff --git a/src/validation.cpp b/src/validation.cpp index 8a4b91fc99..50e7f36bf3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -175,6 +175,7 @@ class CChainState { public: CChain chainActive; + CCriticalSection cs_mapblockindex; BlockMap mapBlockIndex; std::multimap mapBlocksUnlinked; CBlockIndex *pindexBestInvalid = nullptr; @@ -239,7 +240,7 @@ class CChainState { CCriticalSection cs_main; - +CCriticalSection& cs_mapblockindex = g_chainstate.cs_mapblockindex; BlockMap& mapBlockIndex = g_chainstate.mapBlockIndex; CChain& chainActive = g_chainstate.chainActive; CBlockIndex *pindexBestHeader = nullptr; @@ -317,8 +318,6 @@ namespace { CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator) { - 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) { @@ -1289,7 +1288,7 @@ bool GetTransaction(const uint256& hash, CTransactionRef& txOut, const Consensus bool IsBlockHashInChain(const uint256& hashBlock, int& nHeight, const CBlockIndex* pindex) { - LOCK(cs_main); + LOCK2(cs_main, cs_mapblockindex); if (hashBlock == uint256() || !mapBlockIndex.count(hashBlock)) return false; @@ -1649,7 +1648,6 @@ bool CScriptCheck::operator()() { int GetSpendHeight(const CCoinsViewCache& inputs) { - LOCK(cs_main); CBlockIndex* pindexPrev = LookupBlockIndex(inputs.GetBestBlock()); return pindexPrev->nHeight + 1; } @@ -2283,9 +2281,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); - if (it != mapBlockIndex.end()) { - if (it->second->GetAncestor(pindex->nHeight) == pindex && + CBlockIndex* block = LookupBlockIndex(hashAssumeValid); + if (block != nullptr) { + if (block->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. @@ -3602,50 +3600,64 @@ static const unsigned int PRUNE_COUNT = 12; */ void PruneStaleBlockIndexes() { - static int64_t lastPrunedHeight = 0; + // Use the same lock for convenience. + static int64_t lastPrunedHeight GUARDED_BY(cs_mapblockindex) = 0; // This isn't going to change while we're processing, so just leave and try again later. if (!pindexBestHeader) return; - // If mapBlockIndex isn't bloated, don't bother taking the time. - if (chainActive.Height() > static_cast(mapBlockIndex.size() - PRUNE_COUNT)) { - return; - } - - LogPrintf("%s: Checking for stale indexes. Block index size=%d; Chain height=%d\n", - __func__, mapBlockIndex.size(), chainActive.Height()); - - uint32_t irrelevantIndexes = 0; - int64_t lowestPrunedHeight = lastPrunedHeight; std::set setDelete; + uint32_t irrelevantIndexes = 0; + int64_t lowestPrunedHeight = -1; + int64_t currentHeight; + std::vector> checkPrunable; + { + LOCK(cs_mapblockindex); + // This doesn't require the mapblockindex lock but may change while we wait on it. + currentHeight = chainActive.Height(); + // If mapBlockIndex isn't bloated, don't bother taking the time. + if (currentHeight > static_cast(mapBlockIndex.size() - PRUNE_COUNT)) { + return; + } - for (const auto& p : mapBlockIndex) { - const CBlockIndex* pindex = p.second; - // If we've already checked to this height, don't waste time + // Guess at the size + checkPrunable.reserve(mapBlockIndex.size() - currentHeight); + LogPrintf("%s: Checking for stale indexes. Block index size=%d; Chain height=%d\n", + __func__, mapBlockIndex.size(), currentHeight); - if (pindex->nHeight < lastPrunedHeight) - continue; + for (const auto& p : mapBlockIndex) { + const CBlockIndex* pindex = p.second; + // If we've already checked to this height, don't waste time + if (pindex->nHeight >= lastPrunedHeight) + checkPrunable.emplace_back(p); + } + } + // Determine what can be deleted from mapBlockIndex while not holding the lock for it + for (const auto& p : checkPrunable) { + const CBlockIndex* pindex = p.second; // If it's not in the active chain, and not in our best header ancestor list. if (!chainActive.Contains(pindex) && (!IsAncestor(pindexBestHeader, pindex))) { irrelevantIndexes++; // if it's also old enough, add it to the prune list. - if (chainActive.Height() > static_cast(pindex->nHeight + PRUNE_DEPTH)) { + if (currentHeight > static_cast(pindex->nHeight + PRUNE_DEPTH)) { setDelete.emplace(p.first); - // save the lowest height that we're purging - if ((pindex->nHeight < lowestPrunedHeight) || (lowestPrunedHeight <= lastPrunedHeight)) { + // save the lowest height that we're purging, even if it's lower than the previous + // lastPrunedHeight + if (pindex->nHeight < lowestPrunedHeight || lowestPrunedHeight == -1) { lowestPrunedHeight = pindex->nHeight; } } } } - if (irrelevantIndexes > 0) { + if (setDelete.size() > 0) { LogPrintf("%s: Erasing %d of %d irrelevant indexes. LastPrunedHeight now %d.\n", __func__, setDelete.size(), irrelevantIndexes, lowestPrunedHeight); + LOCK(cs_mapblockindex); // Purge the ones we flagged for (const uint256& hash : setDelete) { mapBlockIndex.erase(hash); @@ -3848,14 +3860,17 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c // back to the mempool. UpdateMempoolForReorg(disconnectpool, true); - // The resulting new best tip may not be in setBlockIndexCandidates anymore, so - // add it again. - BlockMap::iterator it = mapBlockIndex.begin(); - while (it != mapBlockIndex.end()) { - if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->nChainTx && !setBlockIndexCandidates.value_comp()(it->second, chainActive.Tip())) { - setBlockIndexCandidates.insert(it->second); + { + LOCK(cs_mapblockindex); + // The resulting new best tip may not be in setBlockIndexCandidates anymore, so + // add it again. + BlockMap::iterator it = mapBlockIndex.begin(); + while (it != mapBlockIndex.end()) { + if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->nChainTx && !setBlockIndexCandidates.value_comp()(it->second, chainActive.Tip())) { + setBlockIndexCandidates.insert(it->second); + } + it++; } - it++; } InvalidChainFound(pindex); @@ -3875,22 +3890,25 @@ 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(); - while (it != mapBlockIndex.end()) { - if (!it->second->IsValid() && it->second->GetAncestor(nHeight) == pindex) { - it->second->nStatus &= ~BLOCK_FAILED_MASK; - setDirtyBlockIndex.insert(it->second); - if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->nChainTx && setBlockIndexCandidates.value_comp()(chainActive.Tip(), it->second)) { - setBlockIndexCandidates.insert(it->second); - } - if (it->second == pindexBestInvalid) { - // Reset invalid block marker if it was pointing to one of those. - pindexBestInvalid = nullptr; + { + LOCK(cs_mapblockindex); + // Remove the invalidity flag from this block and all its descendants. + BlockMap::iterator it = mapBlockIndex.begin(); + while (it != mapBlockIndex.end()) { + if (!it->second->IsValid() && it->second->GetAncestor(nHeight) == pindex) { + it->second->nStatus &= ~BLOCK_FAILED_MASK; + setDirtyBlockIndex.insert(it->second); + if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->nChainTx && setBlockIndexCandidates.value_comp()(chainActive.Tip(), it->second)) { + setBlockIndexCandidates.insert(it->second); + } + if (it->second == 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->second); + it++; } - it++; } // Remove the invalidity flag from all ancestors too. @@ -3911,27 +3929,33 @@ void ResetBlockFailureFlags(CBlockIndex *pindex) { CBlockIndex* CChainState::AddToBlockIndex(const CBlockHeader& block, bool fProofOfStake, bool fProofOfFullNode) { AssertLockHeld(cs_main); + CBlockIndex* pindexNew; // Check for duplicate uint256 hash = block.GetHash(); - BlockMap::iterator it = mapBlockIndex.find(hash); - if (it != mapBlockIndex.end()) - return it->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 = mapBlockIndex.insert(std::make_pair(hash, pindexNew)).first; - pindexNew->phashBlock = &((*mi).first); - BlockMap::iterator miPrev = mapBlockIndex.find(block.hashPrevBlock); - if (miPrev != mapBlockIndex.end()) { - pindexNew->pprev = (*miPrev).second; - pindexNew->nHeight = pindexNew->pprev->nHeight + 1; - pindexNew->BuildSkip(); + // atomic lookup+insert + LOCK(cs_mapblockindex); + + BlockMap::iterator it = mapBlockIndex.find(hash); + if (it != mapBlockIndex.end()) + return it->second; + + // Construct new block index object + 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 = mapBlockIndex.insert(std::make_pair(hash, pindexNew)).first; + pindexNew->phashBlock = &((*mi).first); + BlockMap::iterator miPrev = mapBlockIndex.find(block.hashPrevBlock); + if (miPrev != mapBlockIndex.end()) + { + pindexNew->pprev = (*miPrev).second; + pindexNew->nHeight = pindexNew->pprev->nHeight + 1; + pindexNew->BuildSkip(); + } } pindexNew->nTimeMax = (pindexNew->pprev ? std::max(pindexNew->pprev->nTimeMax, pindexNew->nTime) : pindexNew->nTime); @@ -4695,12 +4719,11 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState& AssertLockHeld(cs_main); // Check for duplicate uint256 hash = block.GetHash(); - BlockMap::iterator miSelf = mapBlockIndex.find(hash); CBlockIndex *pindex = nullptr; if (hash != chainparams.GetConsensus().hashGenesisBlock) { - if (miSelf != mapBlockIndex.end()) { + pindex = LookupBlockIndex(hash); + if (pindex != nullptr) { // Block header is already known. - pindex = miSelf->second; if (ppindex) *ppindex = pindex; if (pindex->nStatus & BLOCK_FAILED_MASK) @@ -4717,11 +4740,9 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState& } // Get prev block index - CBlockIndex* pindexPrev = nullptr; - BlockMap::iterator mi = mapBlockIndex.find(block.hashPrevBlock); - if (mi == mapBlockIndex.end()) + CBlockIndex* pindexPrev = LookupBlockIndex(block.hashPrevBlock); + if (pindexPrev == nullptr) return state.DoS(10, error("%s: prev block not found", __func__), 0, "prev-blk-not-found"); - pindexPrev = (*mi).second; 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())) @@ -5051,7 +5072,9 @@ uint64_t CalculateCurrentUsage() /* Prune a block file (modify associated database entries)*/ void PruneOneBlockFile(const int fileNumber) { - LOCK(cs_LastBlockFile); + AssertLockHeld(cs_main); + AssertLockHeld(cs_LastBlockFile); + LOCK(cs_mapblockindex); for (const auto& entry : mapBlockIndex) { CBlockIndex* pindex = entry.second; @@ -5251,6 +5274,9 @@ CBlockIndex * CChainState::InsertBlockIndex(const uint256& hash) if (hash.IsNull()) return nullptr; + // atomic lookup+insert + LOCK(cs_mapblockindex); + // Return existing BlockMap::iterator mi = mapBlockIndex.find(hash); if (mi != mapBlockIndex.end()) @@ -5273,11 +5299,14 @@ bool CChainState::LoadBlockIndex(const Consensus::Params& consensus_params, CBlo // Calculate nChainWork std::vector > vSortedByHeight; - vSortedByHeight.reserve(mapBlockIndex.size()); - for (const std::pair& item : mapBlockIndex) { - CBlockIndex* pindex = item.second; - vSortedByHeight.push_back(std::make_pair(pindex->nHeight, pindex)); + LOCK(cs_mapblockindex); + vSortedByHeight.reserve(mapBlockIndex.size()); + for (const std::pair& item : mapBlockIndex) + { + CBlockIndex* pindex = item.second; + vSortedByHeight.push_back(std::make_pair(pindex->nHeight, pindex)); + } } sort(vSortedByHeight.begin(), vSortedByHeight.end()); for (const std::pair& item : vSortedByHeight) @@ -5342,11 +5371,14 @@ bool static LoadBlockIndexDB(const CChainParams& chainparams) EXCLUSIVE_LOCKS_RE // Check presence of blk files LogPrintf("Checking all blk files are present...\n"); std::set setBlkDataFiles; - for (const std::pair& item : mapBlockIndex) { - CBlockIndex* pindex = item.second; - if (pindex->nStatus & BLOCK_HAVE_DATA) { - setBlkDataFiles.insert(pindex->nFile); + LOCK(cs_mapblockindex); + for (const std::pair& item : mapBlockIndex) + { + CBlockIndex* pindex = item.second; + if (pindex->nStatus & BLOCK_HAVE_DATA) { + setBlkDataFiles.insert(pindex->nFile); + } } } for (std::set::iterator it = setBlkDataFiles.begin(); it != setBlkDataFiles.end(); it++) @@ -5376,21 +5408,26 @@ bool LoadChainTip(const CChainParams& chainparams) if (chainActive.Tip() && chainActive.Tip()->GetBlockHash() == pcoinsTip->GetBestBlock()) return true; - if (pcoinsTip->GetBestBlock().IsNull() && mapBlockIndex.size() == 1) { - // In case we just added the genesis block, connect it now, so - // that we always have a chainActive.Tip() when we return. - LogPrintf("%s: Connecting genesis block...\n", __func__); - CValidationState state; - if (!ActivateBestChain(state, chainparams)) { - LogPrintf("%s: failed to activate chain (%s)\n", __func__, FormatStateMessage(state)); - return false; + CBlockIndex* pindex; + { + LOCK(cs_mapblockindex); + + if (pcoinsTip->GetBestBlock().IsNull() && mapBlockIndex.size() == 1) { + // In case we just added the genesis block, connect it now, so + // that we always have a chainActive.Tip() when we return. + LogPrintf("%s: Connecting genesis block...\n", __func__); + CValidationState state; + if (!ActivateBestChain(state, chainparams)) { + LogPrintf("%s: failed to activate chain (%s)\n", __func__, FormatStateMessage(state)); + return false; + } } - } - // Load pointer to end of best chain - CBlockIndex* pindex = LookupBlockIndex(pcoinsTip->GetBestBlock()); - if (!pindex) { - return false; + // Load pointer to end of best chain + pindex = LookupBlockIndex(pcoinsTip->GetBestBlock()); + if (!pindex) { + return false; + } } chainActive.SetTip(pindex); @@ -5575,18 +5612,21 @@ 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) { - return error("ReplayBlocks(): reorganization to unknown block requested"); - } - pindexNew = mapBlockIndex[hashHeads[0]]; + { + LOCK(cs_mapblockindex); + if (mapBlockIndex.count(hashHeads[0]) == 0) { + return error("ReplayBlocks(): reorganization to unknown block requested"); + } + pindexNew = mapBlockIndex[hashHeads[0]]; - if (!hashHeads[1].IsNull()) { // The old tip is allowed to be 0, indicating it's the first flush. - if (mapBlockIndex.count(hashHeads[1]) == 0) { - return error("ReplayBlocks(): reorganization from unknown block requested"); + if (!hashHeads[1].IsNull()) { // The old tip is allowed to be 0, indicating it's the first flush. + if (mapBlockIndex.count(hashHeads[1]) == 0) { + return error("ReplayBlocks(): reorganization from unknown block requested"); + } + pindexOld = mapBlockIndex[hashHeads[1]]; + pindexFork = LastCommonAncestor(pindexOld, pindexNew); + assert(pindexFork != nullptr); } - pindexOld = mapBlockIndex[hashHeads[1]]; - pindexFork = LastCommonAncestor(pindexOld, pindexNew); - assert(pindexFork != nullptr); } // Rollback along the old branch. @@ -5666,6 +5706,7 @@ bool CChainState::RewindBlockIndex(const CChainParams& params) } } + LOCK(cs_mapblockindex); // 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. @@ -5763,10 +5804,13 @@ void UnloadBlockIndex() warningcache[b].clear(); } - for (BlockMap::value_type& entry : mapBlockIndex) { - delete entry.second; + { + LOCK(cs_mapblockindex); + for (BlockMap::value_type& entry : mapBlockIndex) { + delete entry.second; + } + mapBlockIndex.clear(); } - mapBlockIndex.clear(); fHavePruned = false; g_chainstate.UnloadBlockIndex(); @@ -5779,6 +5823,7 @@ bool LoadBlockIndex(const CChainParams& chainparams) if (!fReindex) { bool ret = LoadBlockIndexDB(chainparams); if (!ret) return false; + LOCK(cs_mapblockindex); needs_init = mapBlockIndex.empty(); } @@ -5802,8 +5847,11 @@ 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())) - return true; + { + LOCK(cs_mapblockindex); + if (mapBlockIndex.count(chainparams.GenesisBlock().GetHash())) + return true; + } try { CBlock &block = const_cast(chainparams.GenesisBlock()); @@ -5958,17 +6006,21 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams) // so we have the genesis block in mapBlockIndex but no active chain. (A few of the tests when // iterating the block tree require that chainActive has been initialized.) if (chainActive.Height() < 0) { + LOCK(cs_mapblockindex); assert(mapBlockIndex.size() <= 1); return; } // Build forward-pointing map of the entire block tree. std::multimap forward; - for (auto& entry : mapBlockIndex) { - forward.insert(std::make_pair(entry.second->pprev, entry.second)); - } + { + LOCK(cs_mapblockindex); + for (auto& entry : mapBlockIndex) { + forward.insert(std::make_pair(entry.second->pprev, entry.second)); + } - assert(forward.size() == mapBlockIndex.size()); + assert(forward.size() == mapBlockIndex.size()); + } std::pair::iterator,std::multimap::iterator> rangeGenesis = forward.equal_range(nullptr); CBlockIndex *pindex = rangeGenesis.first->second; @@ -6314,6 +6366,7 @@ class CMainCleanup CMainCleanup() {} ~CMainCleanup() { // block headers + LOCK(cs_mapblockindex); BlockMap::iterator it1 = mapBlockIndex.begin(); for (; it1 != mapBlockIndex.end(); it1++) delete (*it1).second; diff --git a/src/validation.h b/src/validation.h index 1540b69821..fc08e00ecc 100644 --- a/src/validation.h +++ b/src/validation.h @@ -154,7 +154,9 @@ extern CBlockPolicyEstimator feeEstimator; extern CTxMemPool mempool; extern std::atomic_bool g_is_mempool_loaded; typedef std::unordered_map BlockMap; -extern BlockMap& mapBlockIndex; +// Only protects the map, not any attributes of its contents. +extern CCriticalSection& cs_mapblockindex; +extern BlockMap& mapBlockIndex GUARDED_BY(cs_mapblockindex); extern uint64_t nLastBlockTx; extern uint64_t nLastBlockWeight; extern const std::string strMessageMagic; @@ -309,7 +311,7 @@ uint64_t CalculateCurrentUsage(); /** * Mark one block file as pruned. */ -void PruneOneBlockFile(const int fileNumber); +void PruneOneBlockFile(const int fileNumber) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** * Actually unlink the specified files @@ -484,7 +486,7 @@ bool ReplayBlocks(const CChainParams& params, CCoinsView* view); inline CBlockIndex* LookupBlockIndex(const uint256& hash) { - AssertLockHeld(cs_main); + LOCK(cs_mapblockindex); BlockMap::const_iterator it = mapBlockIndex.find(hash); return it == mapBlockIndex.end() ? nullptr : it->second; } diff --git a/src/veil/ringct/anonwallet.cpp b/src/veil/ringct/anonwallet.cpp index 9f04962451..30dd5755ae 100644 --- a/src/veil/ringct/anonwallet.cpp +++ b/src/veil/ringct/anonwallet.cpp @@ -910,14 +910,9 @@ int AnonWallet::GetDepthInMainChain(const uint256 &blockhash, int nIndex) const if (hashUnset(blockhash)) return 0; - AssertLockHeld(cs_main); - // Find the block it claims to be in - BlockMap::iterator mi = mapBlockIndex.find(blockhash); - if (mi == mapBlockIndex.end()) - return 0; - CBlockIndex *pindex = (*mi).second; - if (!pindex || !chainActive.Contains(pindex)) + CBlockIndex* pindex = LookupBlockIndex(blockhash); + if (pindex == nullptr || !chainActive.Contains(pindex)) return 0; //pindexRet = pindex; @@ -6227,14 +6222,12 @@ std::set AnonWallet::GetConflicts(const uint256 &txid) const void AnonWallet::MarkConflicted(const uint256 &hashBlock, const uint256 &hashTx) { - LOCK2(cs_main, pwalletParent->cs_wallet); - int conflictconfirms = 0; - BlockMap::iterator mi = mapBlockIndex.find(hashTx); - if (mi != mapBlockIndex.end()) { - if (chainActive.Contains(mi->second)) - conflictconfirms = -(chainActive.Height() - mi->second->nHeight + 1); + CBlockIndex* block = LookupBlockIndex(hashTx); + if (block != nullptr) { + if (chainActive.Contains(block)) + conflictconfirms = -(chainActive.Height() - block->nHeight + 1); } // If number of conflict confirms cannot be determined, this means @@ -6244,6 +6237,8 @@ void AnonWallet::MarkConflicted(const uint256 &hashBlock, const uint256 &hashTx) if (conflictconfirms >= 0) return; + LOCK2(cs_main, pwalletParent->cs_wallet); + // Do not flush the wallet here for performance reasons AnonWalletDB walletdb(*walletDatabase, "r+", false); diff --git a/src/veil/zerocoin/accumulators.cpp b/src/veil/zerocoin/accumulators.cpp index 77326bbf48..a2c3037cbf 100644 --- a/src/veil/zerocoin/accumulators.cpp +++ b/src/veil/zerocoin/accumulators.cpp @@ -396,7 +396,10 @@ bool GenerateAccumulatorWitness(const PublicCoin &coin, Accumulator& accumulator if (!IsBlockHashInChain(hashBlock, nHeightTest)) return error("%s: mint tx %s is not in chain", __func__, txid.GetHex()); - nHeightMintAdded = mapBlockIndex[hashBlock]->nHeight; + { + LOCK(cs_mapblockindex); + nHeightMintAdded = mapBlockIndex[hashBlock]->nHeight; + } //get the checkpoint added at the next multiple of 10 int nHeightCheckpoint = nHeightMintAdded + (10 - (nHeightMintAdded % 10)); diff --git a/src/veil/zerocoin/zchain.cpp b/src/veil/zerocoin/zchain.cpp index 479d6c9c9f..759f935181 100644 --- a/src/veil/zerocoin/zchain.cpp +++ b/src/veil/zerocoin/zchain.cpp @@ -203,9 +203,14 @@ void FindMints(std::vector vMintsToFind, std::vector& vMin continue; } - if (!mapBlockIndex.count(hashBlock)) { - vMissingMints.push_back(meta); - continue; + int nHeight; + { + LOCK(cs_mapblockindex); + if (!mapBlockIndex.count(hashBlock)) { + vMissingMints.push_back(meta); + continue; + } + nHeight = mapBlockIndex[hashBlock]->nHeight; } //see if this mint is spent @@ -245,12 +250,12 @@ void FindMints(std::vector vMintsToFind, std::vector& vMin } // if meta data is correct, then no need to update - if (meta.txid == txHash && meta.nHeight == mapBlockIndex[hashBlock]->nHeight && meta.isUsed == fSpent) + if (meta.txid == txHash && meta.nHeight == nHeight && meta.isUsed == fSpent) continue; //mark this mint for update meta.txid = txHash; - meta.nHeight = mapBlockIndex[hashBlock]->nHeight; + meta.nHeight = nHeight; meta.isUsed = fSpent; LogPrintf("%s: found updates for pubcoinhash = %s\n", __func__, meta.hashPubcoin.GetHex()); diff --git a/src/veil/zerocoin/ztracker.cpp b/src/veil/zerocoin/ztracker.cpp index c64669854e..0c2349e65a 100644 --- a/src/veil/zerocoin/ztracker.cpp +++ b/src/veil/zerocoin/ztracker.cpp @@ -434,7 +434,12 @@ bool CzTracker::UpdateStatusInternal(const std::set& setMempool, const } // An orphan tx if hashblock is in mapBlockIndex but not in chain active - if (mapBlockIndex.count(hashBlock) && !chainActive.Contains(mapBlockIndex.at(hashBlock))) { + bool orphaned; + { + LOCK(cs_mapblockindex); + orphaned = mapBlockIndex.count(hashBlock) && !chainActive.Contains(mapBlockIndex.at(hashBlock)); + } + if (orphaned) { LogPrintf("%s : Found orphaned mint txid=%s\n", __func__, mint.txid.GetHex()); mint.isUsed = false; mint.nHeight = 0; diff --git a/src/veil/zerocoin/zwallet.cpp b/src/veil/zerocoin/zwallet.cpp index 1904145424..4794b0afe3 100644 --- a/src/veil/zerocoin/zwallet.cpp +++ b/src/veil/zerocoin/zwallet.cpp @@ -252,8 +252,11 @@ void CzWallet::SyncWithChain(bool fGenerateMintPool) } CBlockIndex* pindex = nullptr; - if (mapBlockIndex.count(hashBlock)) - pindex = mapBlockIndex.at(hashBlock); + { + LOCK(cs_mapblockindex); + if (mapBlockIndex.count(hashBlock)) + pindex = mapBlockIndex.at(hashBlock); + } if (!setAddedTx.count(txHash)) { CBlock block; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index facc2c8afa..14e3cbc283 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2392,8 +2392,6 @@ static UniValue listsinceblock(const JSONRPCRequest& request) // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - LOCK2(cs_main, pwallet->cs_wallet); - const CBlockIndex* pindex = nullptr; // Block index of the specified block or the common ancestor, if the block provided was in a deactivated chain. const CBlockIndex* paltindex = nullptr; // Block index of the specified block, even if it's in a deactivated chain. int target_confirms = 1; @@ -2433,6 +2431,8 @@ static UniValue listsinceblock(const JSONRPCRequest& request) UniValue transactions(UniValue::VARR); + LOCK2(cs_main, pwallet->cs_wallet); + for (const std::pair& pairWtx : pwallet->mapWallet) { CWalletTx tx = pairWtx.second; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 22b0ac2d9d..a2ffe8c013 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -204,7 +204,7 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64 SetMockTime(mockTime); CBlockIndex* block = nullptr; if (blockTime > 0) { - LOCK(cs_main); + LOCK(cs_mapblockindex); auto inserted = mapBlockIndex.emplace(GetRandHash(), new CBlockIndex); assert(inserted.second); const uint256& hash = inserted.first->first; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7295ef5139..ad1786a69b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1303,8 +1303,6 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) { - LOCK2(cs_main, cs_wallet); - int conflictconfirms = 0; CBlockIndex* pindex = LookupBlockIndex(hashBlock); if (pindex && chainActive.Contains(pindex)) { @@ -1317,6 +1315,8 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) if (conflictconfirms >= 0) return; + LOCK2(cs_main, cs_wallet); + // Do not flush the wallet here for performance reasons WalletBatch batch(*database, "r+", false); @@ -5604,8 +5604,6 @@ int CMerkleTx::GetDepthInMainChain() const if (hashUnset()) return 0; - AssertLockHeld(cs_main); - // Find the block it claims to be in CBlockIndex* pindex = LookupBlockIndex(hashBlock); if (!pindex || !chainActive.Contains(pindex)) @@ -6498,9 +6496,12 @@ bool CWallet::CreateZerocoinSpendTransaction(CAmount nValue, int nSecurityLevel, if (!GetTransaction(mint.GetTxHash(), txMint, Params().GetConsensus(), hashBlock)) { receipt.SetStatus("Unable to find transaction containing mint", nStatus); fArchive = true; - } else if (!mapBlockIndex.count(hashBlock) || !chainActive.Contains(mapBlockIndex.at(hashBlock))) { - receipt.SetStatus("Mint did not make it into blockchain", nStatus); - fArchive = true; + } else { + LOCK(cs_mapblockindex); + if (!mapBlockIndex.count(hashBlock) || !chainActive.Contains(mapBlockIndex.at(hashBlock))) { + receipt.SetStatus("Mint did not make it into blockchain", nStatus); + fArchive = true; + } } // archive this mint as an orphan From 13dd0256d2b66762beedbe32af2d3ba76700ef8b Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 16 Oct 2019 12:37:25 -0400 Subject: [PATCH 2/2] chain: Set all CBlockIndex members to null, remove SetNull helper === Veil: cannot presently remove SetNull helper due to other use, however, this is still worthwhile due to the initialization changes. --- src/chain.h | 113 ++++++++++++++++++++++++++-------------------------- 1 file changed, 57 insertions(+), 56 deletions(-) diff --git a/src/chain.h b/src/chain.h index bd81e39e49..d57e70663e 100644 --- a/src/chain.h +++ b/src/chain.h @@ -180,92 +180,104 @@ class CBlockIndex { public: //! pointer to the hash of the block, if any. Memory is owned by mapBlockIndex - const uint256* phashBlock; + const uint256* phashBlock{nullptr}; //! pointer to the index of the predecessor of this block - CBlockIndex* pprev; + CBlockIndex* pprev{nullptr}; //! pointer to the index of some further predecessor of this block - CBlockIndex* pskip; + CBlockIndex* pskip{nullptr}; //! height of the entry in the chain. The genesis block has height 0 - int nHeight; + int nHeight{0}; //! money supply tracking - int64_t nMoneySupply; + int64_t nMoneySupply{0}; //! zerocoin mint supply tracking - int64_t nMint; + int64_t nMint{0}; //! Which # file this block is stored in (blk?????.dat) - int nFile; + int nFile{0}; //! Byte offset within blk?????.dat where this block's data is stored - unsigned int nDataPos; + unsigned int nDataPos{0}; //! Byte offset within rev?????.dat where this block's undo data is stored - unsigned int nUndoPos; + unsigned int nUndoPos{0}; //! (memory only) Total amount of work (expected number of hashes) in the chain up to and including this block - arith_uint256 nChainWork; + arith_uint256 nChainWork{}; //! (memory only) Total amount of work (only looking at PoW) in the chain up to and including this block - arith_uint256 nChainPoW; + arith_uint256 nChainPoW{}; //! Number of transactions in this block. //! Note: in a potential headers-first mode, this number cannot be relied upon - unsigned int nTx; + unsigned int nTx{0}; //! (memory only) Number of transactions in the chain up to and including this block. //! This value will be non-zero only if and only if transactions for this block and all its parents are available. //! Change to 64-bit type when necessary; won't happen before 2030 - unsigned int nChainTx; + unsigned int nChainTx{0}; - int64_t nAnonOutputs; // last index + int64_t nAnonOutputs{0}; // last index //! Verification status of this block. See enum BlockStatus - uint32_t nStatus; + uint32_t nStatus{0}; - bool fProofOfStake; - bool fProofOfFullNode; + bool fProofOfStake{false}; + bool fProofOfFullNode{false}; //! Funds sent into the network to serve as an additional reward to stakers and miners - CAmount nNetworkRewardReserve; + CAmount nNetworkRewardReserve{0}; //! block header - int32_t nVersion; - uint256 hashVeilData; - uint32_t nTime; - uint32_t nBits; - uint32_t nNonce; + int32_t nVersion{0}; + uint256 hashVeilData{}; + uint32_t nTime{0}; + uint32_t nBits{0}; + uint32_t nNonce{0}; //! ProgPow Header items // Height was already in the CBlockIndex - uint64_t nNonce64; - uint256 mixHash; + uint64_t nNonce64{0}; + uint256 mixHash{}; //! (memory only) Sequential id assigned to distinguish order in which blocks are received. - int32_t nSequenceId; + int32_t nSequenceId{0}; //! zerocoin specific fields std::map mapZerocoinSupply; std::vector vMintDenominationsInBlock; //! (memory only) Maximum nTime in the chain up to and including this block. - unsigned int nTimeMax; + unsigned int nTimeMax{0}; //! Hash value for the accumulator. Can be used to access the zerocoindb for the accumulator value std::map mapAccumulatorHashes; - uint256 hashMerkleRoot; - uint256 hashWitnessMerkleRoot; - uint256 hashPoFN; - uint256 hashAccumulators; + uint256 hashMerkleRoot{}; + uint256 hashWitnessMerkleRoot{}; + uint256 hashPoFN{}; + uint256 hashAccumulators{}; //! vector that holds a proof of stake proof hash if the block has one. If not, its empty and has less memory //! overhead than an empty uint256 std::vector vHashProof; + void ResetMaps() + { + for (auto& denom : libzerocoin::zerocoinDenomList) { + mapAccumulatorHashes[denom] = uint256(); + } + + // Start supply of each denomination with 0s + for (auto& denom : libzerocoin::zerocoinDenomList) { + mapZerocoinSupply.insert(std::make_pair(denom, 0)); + } + } + void SetNull() { phashBlock = nullptr; @@ -301,14 +313,7 @@ class CBlockIndex hashWitnessMerkleRoot = uint256(); hashAccumulators = uint256(); - for (auto& denom : libzerocoin::zerocoinDenomList) { - mapAccumulatorHashes[denom] = uint256(); - } - - // Start supply of each denomination with 0s - for (auto& denom : libzerocoin::zerocoinDenomList) { - mapZerocoinSupply.insert(std::make_pair(denom, 0)); - } + ResetMaps(); vMintDenominationsInBlock.clear(); @@ -327,27 +332,23 @@ class CBlockIndex CBlockIndex() { - SetNull(); + ResetMaps(); } explicit CBlockIndex(const CBlockHeader& block) + : nHeight{static_cast(block.nHeight)}, + nVersion{block.nVersion}, + hashVeilData{block.hashVeilData}, + nTime{block.nTime}, + nBits{block.nBits}, + nNonce{block.nNonce}, + nNonce64{block.nNonce64}, + mixHash{block.mixHash}, + hashMerkleRoot{block.hashMerkleRoot}, + hashWitnessMerkleRoot{block.hashWitnessMerkleRoot}, + hashAccumulators{block.hashAccumulators} { - SetNull(); - - nVersion = block.nVersion; - hashVeilData = block.hashVeilData; - hashMerkleRoot = block.hashMerkleRoot; - hashWitnessMerkleRoot = block.hashWitnessMerkleRoot; - hashAccumulators = block.hashAccumulators; - nTime = block.nTime; - nBits = block.nBits; - nNonce = block.nNonce; - nMint = 0; - - //ProgPow - nNonce64 = block.nNonce64; - mixHash = block.mixHash; - nHeight = block.nHeight; + ResetMaps(); } CDiskBlockPos GetBlockPos() const {