From 5be9ee3c54dcb396ff52fc8c8b7e4e6e39ec4a3b Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 2 Mar 2022 15:42:57 +1000 Subject: [PATCH 1/7] refactor: more const annotations for uses of CBlockIndex* --- src/index/coinstatsindex.cpp | 2 +- src/node/blockstorage.cpp | 4 ++-- src/node/blockstorage.h | 2 +- src/node/interfaces.cpp | 4 ++-- src/node/miner.cpp | 2 +- src/rest.cpp | 4 ++-- src/rpc/blockchain.cpp | 17 +++++++++-------- src/rpc/rawtransaction.cpp | 6 +++--- src/validation.cpp | 8 ++++---- src/validation.h | 2 +- 10 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 386eb67ce9..69078708f9 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -277,7 +277,7 @@ bool CoinStatsIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* n { LOCK(cs_main); - CBlockIndex* iter_tip{m_chainstate->m_blockman.LookupBlockIndex(current_tip->GetBlockHash())}; + const CBlockIndex* iter_tip{m_chainstate->m_blockman.LookupBlockIndex(current_tip->GetBlockHash())}; const auto& consensus_params{Params().GetConsensus()}; do { diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 7392830261..c35d2a51ce 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -408,13 +408,13 @@ bool BlockManager::LoadBlockIndexDB(ChainstateManager& chainman) return true; } -CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data) +const CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data) { const MapCheckpoints& checkpoints = data.mapCheckpoints; for (const MapCheckpoints::value_type& i : reverse_iterate(checkpoints)) { const uint256& hash = i.second; - CBlockIndex* pindex = LookupBlockIndex(hash); + const CBlockIndex* pindex = LookupBlockIndex(hash); if (pindex) { return pindex; } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 12224f7a5d..69aed28cab 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -163,7 +163,7 @@ public: uint64_t CalculateCurrentUsage(); //! Returns last CBlockIndex* that is a checkpoint - CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + const CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main); ~BlockManager() { diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index cb063ae9f8..74d53d2062 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -490,7 +490,7 @@ public: { LOCK(cs_main); const CChainState& active = Assert(m_node.chainman)->ActiveChainstate(); - if (CBlockIndex* fork = active.FindForkInGlobalIndex(locator)) { + if (const CBlockIndex* fork = active.FindForkInGlobalIndex(locator)) { return fork->nHeight; } return std::nullopt; @@ -557,7 +557,7 @@ public: // used to limit the range, and passing min_height that's too low or // max_height that's too high will not crash or change the result. LOCK(::cs_main); - if (CBlockIndex* block = chainman().m_blockman.LookupBlockIndex(block_hash)) { + if (const CBlockIndex* block = chainman().m_blockman.LookupBlockIndex(block_hash)) { if (max_height && block->nHeight >= *max_height) block = block->GetAncestor(*max_height); for (; block->nStatus & BLOCK_HAVE_DATA; block = block->pprev) { // Check pprev to not segfault if min_height is too low diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 7fe10ecabc..fbfa88c170 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -50,7 +50,7 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman) tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block)); block.vtx.at(0) = MakeTransactionRef(tx); - CBlockIndex* prev_block = WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock)); + const CBlockIndex* prev_block = WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock)); GenerateCoinbaseCommitment(block, prev_block, Params().GetConsensus()); block.hashMerkleRoot = BlockMerkleRoot(block); diff --git a/src/rest.cpp b/src/rest.cpp index 063872b47a..29a16fadb5 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -283,8 +283,8 @@ static bool rest_block(const std::any& context, return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr); CBlock block; - CBlockIndex* pblockindex = nullptr; - CBlockIndex* tip = nullptr; + const CBlockIndex* pblockindex = nullptr; + const CBlockIndex* tip = nullptr; { ChainstateManager* maybe_chainman = GetChainman(context, req); if (!maybe_chainman) return false; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 86dfbbae35..641c8dca45 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -110,7 +110,8 @@ static int ComputeNextBlockAndDepth(const CBlockIndex* tip, const CBlockIndex* b return blockindex == tip ? 1 : -1; } -CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateManager& chainman) { +static const CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateManager& chainman) +{ LOCK(::cs_main); CChain& active_chain = chainman.ActiveChain(); @@ -127,7 +128,7 @@ CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateManager& chainma return active_chain[height]; } else { const uint256 hash{ParseHashV(param, "hash_or_height")}; - CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash); + const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash); if (!pindex) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); @@ -854,7 +855,7 @@ static RPCHelpMan getblockhash() if (nHeight < 0 || nHeight > active_chain.Height()) throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of range"); - CBlockIndex* pblockindex = active_chain[nHeight]; + const CBlockIndex* pblockindex = active_chain[nHeight]; return pblockindex->GetBlockHash().GetHex(); }, }; @@ -1132,7 +1133,7 @@ static RPCHelpMan pruneblockchain() // too low to be a block time (corresponds to timestamp from Sep 2001). if (heightParam > 1000000000) { // Add a 2 hour buffer to include blocks which might have had old timestamps - CBlockIndex* pindex = active_chain.FindEarliestAtLeast(heightParam - TIMESTAMP_WINDOW, 0); + const CBlockIndex* pindex = active_chain.FindEarliestAtLeast(heightParam - TIMESTAMP_WINDOW, 0); if (!pindex) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Could not find block with at least the specified timestamp."); } @@ -1226,7 +1227,7 @@ static RPCHelpMan gettxoutsetinfo() { UniValue ret(UniValue::VOBJ); - CBlockIndex* pindex{nullptr}; + const CBlockIndex* pindex{nullptr}; const CoinStatsHashType hash_type{request.params[0].isNull() ? CoinStatsHashType::HASH_SERIALIZED : ParseHashType(request.params[0].get_str())}; CCoinsStats stats{hash_type}; stats.index_requested = request.params[2].isNull() || request.params[2].get_bool(); @@ -2177,7 +2178,7 @@ static RPCHelpMan getblockstats() { ChainstateManager& chainman = EnsureAnyChainman(request.context); LOCK(cs_main); - CBlockIndex* pindex{ParseHashOrHeight(request.params[0], chainman)}; + const CBlockIndex* pindex{ParseHashOrHeight(request.params[0], chainman)}; CHECK_NONFATAL(pindex != nullptr); std::set stats; @@ -2572,7 +2573,7 @@ static RPCHelpMan scantxoutset() g_should_abort_scan = false; int64_t count = 0; std::unique_ptr pcursor; - CBlockIndex* tip; + const CBlockIndex* tip; NodeContext& node = EnsureAnyNodeContext(request.context); { ChainstateManager& chainman = EnsureChainman(node); @@ -2760,7 +2761,7 @@ UniValue CreateUTXOSnapshot( { std::unique_ptr pcursor; CCoinsStats stats{CoinStatsHashType::HASH_SERIALIZED}; - CBlockIndex* tip; + const CBlockIndex* tip; { // We need to lock cs_main to ensure that the coinsdb isn't written to diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 6272a7c8cf..1ef531b293 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -67,7 +67,7 @@ static void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& LOCK(cs_main); entry.pushKV("blockhash", hashBlock.GetHex()); - CBlockIndex* pindex = active_chainstate.m_blockman.LookupBlockIndex(hashBlock); + const CBlockIndex* pindex = active_chainstate.m_blockman.LookupBlockIndex(hashBlock); if (pindex) { if (active_chainstate.m_chain.Contains(pindex)) { entry.pushKV("confirmations", 1 + active_chainstate.m_chain.Height() - pindex->nHeight); @@ -207,7 +207,7 @@ static RPCHelpMan getrawtransaction() bool in_active_chain = true; uint256 hash = ParseHashV(request.params[0], "parameter 1"); - CBlockIndex* blockindex = nullptr; + const CBlockIndex* blockindex = nullptr; if (hash == Params().GenesisBlock().hashMerkleRoot) { // Special exception for the genesis block coinbase transaction @@ -302,7 +302,7 @@ static RPCHelpMan gettxoutproof() } } - CBlockIndex* pblockindex = nullptr; + const CBlockIndex* pblockindex = nullptr; uint256 hashBlock; ChainstateManager& chainman = EnsureAnyChainman(request.context); if (!request.params[1].isNull()) { diff --git a/src/validation.cpp b/src/validation.cpp index d80e2576d2..eccc3c07aa 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -152,14 +152,14 @@ arith_uint256 nMinimumChainWork; CFeeRate minRelayTxFee = CFeeRate(DEFAULT_MIN_RELAY_TX_FEE); -CBlockIndex* CChainState::FindForkInGlobalIndex(const CBlockLocator& locator) const +const CBlockIndex* CChainState::FindForkInGlobalIndex(const CBlockLocator& locator) const { 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) { - CBlockIndex* pindex{m_blockman.LookupBlockIndex(hash)}; + const CBlockIndex* pindex{m_blockman.LookupBlockIndex(hash)}; if (pindex) { if (m_chain.Contains(pindex)) { return pindex; @@ -3373,7 +3373,7 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio // Don't accept any forks from the main chain prior to last checkpoint. // GetLastCheckpoint finds the last checkpoint in MapCheckpoints that's in our // BlockIndex(). - CBlockIndex* pcheckpoint = blockman.GetLastCheckpoint(params.Checkpoints()); + const CBlockIndex* pcheckpoint = blockman.GetLastCheckpoint(params.Checkpoints()); if (pcheckpoint && nHeight < pcheckpoint->nHeight) { LogPrintf("ERROR: %s: forked chain older than last checkpoint (height %d)\n", __func__, nHeight); return state.Invalid(BlockValidationResult::BLOCK_CHECKPOINT, "bad-fork-prior-to-checkpoint"); @@ -4186,7 +4186,7 @@ void CChainState::LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp) } // process in case the block isn't known yet - CBlockIndex* pindex = m_blockman.LookupBlockIndex(hash); + const CBlockIndex* pindex = m_blockman.LookupBlockIndex(hash); if (!pindex || (pindex->nStatus & BLOCK_HAVE_DATA) == 0) { BlockValidationState state; if (AcceptBlock(pblock, state, nullptr, true, dbp, nullptr)) { diff --git a/src/validation.h b/src/validation.h index cc2247239f..28887909ad 100644 --- a/src/validation.h +++ b/src/validation.h @@ -693,7 +693,7 @@ public: bool IsInitialBlockDownload() const; /** Find the last common block of this chain and a locator. */ - CBlockIndex* FindForkInGlobalIndex(const CBlockLocator& locator) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); + const CBlockIndex* FindForkInGlobalIndex(const CBlockLocator& locator) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** * Make various assertions about the state of the block index. From 3bbb6fea051f4e19bd2448e401a6c4e9b4cc7a41 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 3 Mar 2022 15:05:15 -0500 Subject: [PATCH 2/7] style-only: Various blockstorage.cpp cleanups --- src/node/blockstorage.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index c35d2a51ce..f948a6cbd7 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -203,8 +203,7 @@ CBlockIndex* BlockManager::InsertBlockIndex(const uint256& hash) return nullptr; } - // Return existing or create new - auto [mi, inserted] = m_block_index.try_emplace(hash); + const auto [mi, inserted]{m_block_index.try_emplace(hash)}; CBlockIndex* pindex = &(*mi).second; if (inserted) { pindex->phashBlock = &((*mi).first); @@ -224,8 +223,7 @@ bool BlockManager::LoadBlockIndex( std::vector> vSortedByHeight; vSortedByHeight.reserve(m_block_index.size()); for (auto& [_, block_index] : m_block_index) { - CBlockIndex* pindex = &block_index; - vSortedByHeight.push_back(std::make_pair(pindex->nHeight, pindex)); + vSortedByHeight.push_back(std::make_pair(block_index.nHeight, &block_index)); } sort(vSortedByHeight.begin(), vSortedByHeight.end()); @@ -382,9 +380,8 @@ bool BlockManager::LoadBlockIndexDB(ChainstateManager& chainman) LogPrintf("Checking all blk files are present...\n"); std::set setBlkDataFiles; for (const auto& [_, block_index] : m_block_index) { - const CBlockIndex* pindex = &block_index; - if (pindex->nStatus & BLOCK_HAVE_DATA) { - setBlkDataFiles.insert(pindex->nFile); + if (block_index.nStatus & BLOCK_HAVE_DATA) { + setBlkDataFiles.insert(block_index.nFile); } } for (std::set::iterator it = setBlkDataFiles.begin(); it != setBlkDataFiles.end(); it++) { From 42e56d9b188f97c077ed2269e24acc0be35ece17 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Mon, 7 Mar 2022 21:32:12 -0500 Subject: [PATCH 3/7] style-only: No need for std::pair for vSortedByHeight ...since the height information in already in CBlockIndex* and we can use an easy custom sorter. --- src/node/blockstorage.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index f948a6cbd7..29ad8285ec 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -220,17 +220,20 @@ bool BlockManager::LoadBlockIndex( } // Calculate nChainWork - std::vector> vSortedByHeight; + std::vector vSortedByHeight; vSortedByHeight.reserve(m_block_index.size()); for (auto& [_, block_index] : m_block_index) { - vSortedByHeight.push_back(std::make_pair(block_index.nHeight, &block_index)); + vSortedByHeight.push_back(&block_index); } - sort(vSortedByHeight.begin(), vSortedByHeight.end()); + sort(vSortedByHeight.begin(), vSortedByHeight.end(), + [](const CBlockIndex* pa, const CBlockIndex* pb) { + return pa->nHeight < pb->nHeight; + }); // Find start of assumed-valid region. int first_assumed_valid_height = std::numeric_limits::max(); - for (const auto& [height, block] : vSortedByHeight) { + for (const CBlockIndex* block : vSortedByHeight) { if (block->IsAssumedValid()) { auto chainstates = chainman.GetAll(); @@ -242,14 +245,13 @@ bool BlockManager::LoadBlockIndex( assert(any_chain([](auto chainstate) { return chainstate->reliesOnAssumedValid(); })); assert(any_chain([](auto chainstate) { return !chainstate->reliesOnAssumedValid(); })); - first_assumed_valid_height = height; + first_assumed_valid_height = block->nHeight; break; } } - for (const std::pair& item : vSortedByHeight) { + for (CBlockIndex* pindex : vSortedByHeight) { if (ShutdownRequested()) return false; - CBlockIndex* pindex = item.second; pindex->nChainWork = (pindex->pprev ? pindex->pprev->nChainWork : 0) + GetBlockProof(*pindex); pindex->nTimeMax = (pindex->pprev ? std::max(pindex->pprev->nTimeMax, pindex->nTime) : pindex->nTime); From c600ee38168a460d3026eae0e289c976194aad14 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Mon, 7 Mar 2022 21:42:27 -0500 Subject: [PATCH 4/7] Only load BlockMan in BlockMan member functions This commit effectively splits the "load block index itself" logic from "derive Chainstate variables from loaded block index" logic. This means that BlockManager::LoadBlockIndex{,DB} will only load what's relevant to the BlockManager. I strongly recommend reviewing with the following git-diff flags: --color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change --- src/node/blockstorage.cpp | 65 ++-------------------------------- src/node/blockstorage.h | 7 ++-- src/validation.cpp | 74 ++++++++++++++++++++++++++++++++++++++- src/validation.h | 2 +- 4 files changed, 80 insertions(+), 68 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 29ad8285ec..5610f6348f 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -211,9 +211,7 @@ CBlockIndex* BlockManager::InsertBlockIndex(const uint256& hash) return pindex; } -bool BlockManager::LoadBlockIndex( - const Consensus::Params& consensus_params, - ChainstateManager& chainman) +bool BlockManager::LoadBlockIndex(const Consensus::Params& consensus_params) { if (!m_block_tree_db->LoadBlockIndexGuts(consensus_params, [this](const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return this->InsertBlockIndex(hash); })) { return false; @@ -230,26 +228,6 @@ bool BlockManager::LoadBlockIndex( return pa->nHeight < pb->nHeight; }); - // Find start of assumed-valid region. - int first_assumed_valid_height = std::numeric_limits::max(); - - for (const CBlockIndex* block : vSortedByHeight) { - if (block->IsAssumedValid()) { - auto chainstates = chainman.GetAll(); - - // If we encounter an assumed-valid block index entry, ensure that we have - // one chainstate that tolerates assumed-valid entries and another that does - // not (i.e. the background validation chainstate), since assumed-valid - // entries should always be pending validation by a fully-validated chainstate. - auto any_chain = [&](auto fnc) { return std::any_of(chainstates.cbegin(), chainstates.cend(), fnc); }; - assert(any_chain([](auto chainstate) { return chainstate->reliesOnAssumedValid(); })); - assert(any_chain([](auto chainstate) { return !chainstate->reliesOnAssumedValid(); })); - - first_assumed_valid_height = block->nHeight; - break; - } - } - for (CBlockIndex* pindex : vSortedByHeight) { if (ShutdownRequested()) return false; pindex->nChainWork = (pindex->pprev ? pindex->pprev->nChainWork : 0) + GetBlockProof(*pindex); @@ -275,43 +253,6 @@ bool BlockManager::LoadBlockIndex( pindex->nStatus |= BLOCK_FAILED_CHILD; m_dirty_blockindex.insert(pindex); } - if (pindex->IsAssumedValid() || - (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && - (pindex->HaveTxsDownloaded() || pindex->pprev == nullptr))) { - - // Fill each chainstate's block candidate set. Only add assumed-valid - // blocks to the tip candidate set if the chainstate is allowed to rely on - // assumed-valid blocks. - // - // If all setBlockIndexCandidates contained the assumed-valid blocks, the - // background chainstate's ActivateBestChain() call would add assumed-valid - // blocks to the chain (based on how FindMostWorkChain() works). Obviously - // we don't want this since the purpose of the background validation chain - // is to validate assued-valid blocks. - // - // Note: This is considering all blocks whose height is greater or equal to - // the first assumed-valid block to be assumed-valid blocks, and excluding - // them from the background chainstate's setBlockIndexCandidates set. This - // does mean that some blocks which are not technically assumed-valid - // (later blocks on a fork beginning before the first assumed-valid block) - // might not get added to the background chainstate, but this is ok, - // because they will still be attached to the active chainstate if they - // actually contain more work. - // - // Instead of this height-based approach, an earlier attempt was made at - // detecting "holistically" whether the block index under consideration - // relied on an assumed-valid ancestor, but this proved to be too slow to - // be practical. - for (CChainState* chainstate : chainman.GetAll()) { - if (chainstate->reliesOnAssumedValid() || - pindex->nHeight < first_assumed_valid_height) { - chainstate->setBlockIndexCandidates.insert(pindex); - } - } - } - if (pindex->nStatus & BLOCK_FAILED_MASK && (!chainman.m_best_invalid || pindex->nChainWork > chainman.m_best_invalid->nChainWork)) { - chainman.m_best_invalid = pindex; - } if (pindex->pprev) { pindex->BuildSkip(); } @@ -355,9 +296,9 @@ bool BlockManager::WriteBlockIndexDB() return true; } -bool BlockManager::LoadBlockIndexDB(ChainstateManager& chainman) +bool BlockManager::LoadBlockIndexDB() { - if (!LoadBlockIndex(::Params().GetConsensus(), chainman)) { + if (!LoadBlockIndex(::Params().GetConsensus())) { return false; } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 69aed28cab..d230bdd24b 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -127,16 +127,15 @@ public: std::unique_ptr m_block_tree_db GUARDED_BY(::cs_main); bool WriteBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - bool LoadBlockIndexDB(ChainstateManager& chainman) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + bool LoadBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** * Load the blocktree off disk and into memory. Populate certain metadata * per index entry (nStatus, nChainWork, nTimeMax, etc.) as well as peripheral * collections like m_dirty_blockindex. */ - bool LoadBlockIndex( - const Consensus::Params& consensus_params, - ChainstateManager& chainman) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool LoadBlockIndex(const Consensus::Params& consensus_params) + EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Clear all data members. */ void Unload() EXCLUSIVE_LOCKS_REQUIRED(cs_main); diff --git a/src/validation.cpp b/src/validation.cpp index eccc3c07aa..ba8925322f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4083,8 +4083,80 @@ bool ChainstateManager::LoadBlockIndex() // Load block index from databases bool needs_init = fReindex; if (!fReindex) { - bool ret = m_blockman.LoadBlockIndexDB(*this); + bool ret = m_blockman.LoadBlockIndexDB(); if (!ret) return false; + + std::vector vSortedByHeight; + vSortedByHeight.reserve(m_blockman.m_block_index.size()); + for (auto& [_, block_index] : m_blockman.m_block_index) { + vSortedByHeight.push_back(&block_index); + } + sort(vSortedByHeight.begin(), vSortedByHeight.end(), + [](const CBlockIndex* pa, const CBlockIndex* pb) { + return pa->nHeight < pb->nHeight; + }); + + // Find start of assumed-valid region. + int first_assumed_valid_height = std::numeric_limits::max(); + + for (const CBlockIndex* block : vSortedByHeight) { + if (block->IsAssumedValid()) { + auto chainstates = GetAll(); + + // If we encounter an assumed-valid block index entry, ensure that we have + // one chainstate that tolerates assumed-valid entries and another that does + // not (i.e. the background validation chainstate), since assumed-valid + // entries should always be pending validation by a fully-validated chainstate. + auto any_chain = [&](auto fnc) { return std::any_of(chainstates.cbegin(), chainstates.cend(), fnc); }; + assert(any_chain([](auto chainstate) { return chainstate->reliesOnAssumedValid(); })); + assert(any_chain([](auto chainstate) { return !chainstate->reliesOnAssumedValid(); })); + + first_assumed_valid_height = block->nHeight; + break; + } + } + + for (CBlockIndex* pindex : vSortedByHeight) { + if (ShutdownRequested()) return false; + if (pindex->IsAssumedValid() || + (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && + (pindex->HaveTxsDownloaded() || pindex->pprev == nullptr))) { + + // Fill each chainstate's block candidate set. Only add assumed-valid + // blocks to the tip candidate set if the chainstate is allowed to rely on + // assumed-valid blocks. + // + // If all setBlockIndexCandidates contained the assumed-valid blocks, the + // background chainstate's ActivateBestChain() call would add assumed-valid + // blocks to the chain (based on how FindMostWorkChain() works). Obviously + // we don't want this since the purpose of the background validation chain + // is to validate assued-valid blocks. + // + // Note: This is considering all blocks whose height is greater or equal to + // the first assumed-valid block to be assumed-valid blocks, and excluding + // them from the background chainstate's setBlockIndexCandidates set. This + // does mean that some blocks which are not technically assumed-valid + // (later blocks on a fork beginning before the first assumed-valid block) + // might not get added to the background chainstate, but this is ok, + // because they will still be attached to the active chainstate if they + // actually contain more work. + // + // Instead of this height-based approach, an earlier attempt was made at + // detecting "holistically" whether the block index under consideration + // relied on an assumed-valid ancestor, but this proved to be too slow to + // be practical. + for (CChainState* chainstate : GetAll()) { + if (chainstate->reliesOnAssumedValid() || + pindex->nHeight < first_assumed_valid_height) { + chainstate->setBlockIndexCandidates.insert(pindex); + } + } + } + if (pindex->nStatus & BLOCK_FAILED_MASK && (!m_best_invalid || pindex->nChainWork > m_best_invalid->nChainWork)) { + m_best_invalid = pindex; + } + } + needs_init = m_blockman.m_block_index.empty(); } diff --git a/src/validation.h b/src/validation.h index 28887909ad..8833a4813e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -836,7 +836,7 @@ private: bool m_snapshot_validated{false}; CBlockIndex* m_best_invalid; - friend bool node::BlockManager::LoadBlockIndex(const Consensus::Params&, ChainstateManager&); + friend bool node::BlockManager::LoadBlockIndex(const Consensus::Params&); //! Internal helper for ActivateSnapshot(). [[nodiscard]] bool PopulateAndValidateSnapshot( From 12eb05df63f930969115af6dc66e2e5d02f2a517 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Tue, 15 Mar 2022 19:17:36 -0400 Subject: [PATCH 5/7] move-only: Move CBlockIndexWorkComparator to blockstorage ...it's declared in blockstorage.h --- src/node/blockstorage.cpp | 19 +++++++++++++++++++ src/validation.cpp | 18 ------------------ 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 5610f6348f..bbdd240692 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -28,6 +28,25 @@ bool fHavePruned = false; bool fPruneMode = false; uint64_t nPruneTarget = 0; +bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIndex* pb) const +{ + // First sort by most total work, ... + if (pa->nChainWork > pb->nChainWork) return false; + if (pa->nChainWork < pb->nChainWork) return true; + + // ... then by earliest time received, ... + if (pa->nSequenceId < pb->nSequenceId) return false; + if (pa->nSequenceId > pb->nSequenceId) return true; + + // Use pointer address as tie breaker (should only happen with blocks + // loaded from disk, as those all have id 0). + if (pa < pb) return false; + if (pa > pb) return true; + + // Identical blocks. + return false; +} + static FILE* OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false); static FlatFileSeq BlockFileSeq(); static FlatFileSeq UndoFileSeq(); diff --git a/src/validation.cpp b/src/validation.cpp index ba8925322f..eebb6773d4 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -107,24 +107,6 @@ const std::vector CHECKLEVEL_DOC { "each level includes the checks of the previous levels", }; -bool CBlockIndexWorkComparator::operator()(const CBlockIndex *pa, const CBlockIndex *pb) const { - // First sort by most total work, ... - if (pa->nChainWork > pb->nChainWork) return false; - if (pa->nChainWork < pb->nChainWork) return true; - - // ... then by earliest time received, ... - if (pa->nSequenceId < pb->nSequenceId) return false; - if (pa->nSequenceId > pb->nSequenceId) return true; - - // Use pointer address as tie breaker (should only happen with blocks - // loaded from disk, as those all have id 0). - if (pa < pb) return false; - if (pa > pb) return true; - - // Identical blocks. - return false; -} - /** * Mutex to guard access to validation specific variables, such as reading * or changing the chainstate. From 28ba0313eac37e4a900b7e97af7169ce999c4024 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Tue, 15 Mar 2022 19:19:58 -0400 Subject: [PATCH 6/7] Add and use CBlockIndexHeightOnlyComparator ...also use std::sort for clarity --- src/node/blockstorage.cpp | 11 +++++++---- src/node/blockstorage.h | 5 +++++ src/validation.cpp | 17 ++++++++--------- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index bbdd240692..73bc4e0072 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -47,6 +47,11 @@ bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIn return false; } +bool CBlockIndexHeightOnlyComparator::operator()(const CBlockIndex* pa, const CBlockIndex* pb) const +{ + return pa->nHeight < pb->nHeight; +} + static FILE* OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false); static FlatFileSeq BlockFileSeq(); static FlatFileSeq UndoFileSeq(); @@ -242,10 +247,8 @@ bool BlockManager::LoadBlockIndex(const Consensus::Params& consensus_params) for (auto& [_, block_index] : m_block_index) { vSortedByHeight.push_back(&block_index); } - sort(vSortedByHeight.begin(), vSortedByHeight.end(), - [](const CBlockIndex* pa, const CBlockIndex* pb) { - return pa->nHeight < pb->nHeight; - }); + std::sort(vSortedByHeight.begin(), vSortedByHeight.end(), + CBlockIndexHeightOnlyComparator()); for (CBlockIndex* pindex : vSortedByHeight) { if (ShutdownRequested()) return false; diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index d230bdd24b..0fd87ddaf1 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -62,6 +62,11 @@ struct CBlockIndexWorkComparator { bool operator()(const CBlockIndex* pa, const CBlockIndex* pb) const; }; +struct CBlockIndexHeightOnlyComparator { + /* Only compares the height of two block indices, doesn't try to tie-break */ + bool operator()(const CBlockIndex* pa, const CBlockIndex* pb) const; +}; + /** * Maintains a tree of blocks (stored in `m_block_index`) which is consulted * to determine where the most-work tip is. diff --git a/src/validation.cpp b/src/validation.cpp index eebb6773d4..2828b4ae98 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -65,21 +65,22 @@ using node::BLOCKFILE_CHUNK_SIZE; using node::BlockManager; using node::BlockMap; +using node::CBlockIndexHeightOnlyComparator; using node::CBlockIndexWorkComparator; using node::CCoinsStats; using node::CoinStatsHashType; +using node::fHavePruned; +using node::fImporting; +using node::fPruneMode; +using node::fReindex; using node::GetUTXOStats; +using node::nPruneTarget; using node::OpenBlockFile; using node::ReadBlockFromDisk; using node::SnapshotMetadata; using node::UNDOFILE_CHUNK_SIZE; using node::UndoReadFromDisk; using node::UnlinkPrunedFiles; -using node::fHavePruned; -using node::fImporting; -using node::fPruneMode; -using node::fReindex; -using node::nPruneTarget; #define MICRO 0.000001 #define MILLI 0.001 @@ -4073,10 +4074,8 @@ bool ChainstateManager::LoadBlockIndex() for (auto& [_, block_index] : m_blockman.m_block_index) { vSortedByHeight.push_back(&block_index); } - sort(vSortedByHeight.begin(), vSortedByHeight.end(), - [](const CBlockIndex* pa, const CBlockIndex* pb) { - return pa->nHeight < pb->nHeight; - }); + std::sort(vSortedByHeight.begin(), vSortedByHeight.end(), + CBlockIndexHeightOnlyComparator()); // Find start of assumed-valid region. int first_assumed_valid_height = std::numeric_limits::max(); From f865cf8ded2b2fbc82a6fbc41226d991909a6880 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Tue, 15 Mar 2022 19:28:46 -0400 Subject: [PATCH 7/7] Add and use BlockManager::GetAllBlockIndices --- src/node/blockstorage.cpp | 17 ++++++++++++----- src/node/blockstorage.h | 2 ++ src/validation.cpp | 6 +----- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 73bc4e0072..763fd29744 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -56,6 +56,17 @@ static FILE* OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false); static FlatFileSeq BlockFileSeq(); static FlatFileSeq UndoFileSeq(); +std::vector BlockManager::GetAllBlockIndices() +{ + AssertLockHeld(cs_main); + std::vector rv; + rv.reserve(m_block_index.size()); + for (auto& [_, block_index] : m_block_index) { + rv.push_back(&block_index); + } + return rv; +} + CBlockIndex* BlockManager::LookupBlockIndex(const uint256& hash) { AssertLockHeld(cs_main); @@ -242,11 +253,7 @@ bool BlockManager::LoadBlockIndex(const Consensus::Params& consensus_params) } // Calculate nChainWork - std::vector vSortedByHeight; - vSortedByHeight.reserve(m_block_index.size()); - for (auto& [_, block_index] : m_block_index) { - vSortedByHeight.push_back(&block_index); - } + std::vector vSortedByHeight{GetAllBlockIndices()}; std::sort(vSortedByHeight.begin(), vSortedByHeight.end(), CBlockIndexHeightOnlyComparator()); diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 0fd87ddaf1..a051e90808 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -123,6 +123,8 @@ private: public: BlockMap m_block_index GUARDED_BY(cs_main); + std::vector GetAllBlockIndices() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + /** * All pairs A->B, where A (or one of its ancestors) misses transactions, but B has transactions. * Pruned nodes may have entries where B is missing data. diff --git a/src/validation.cpp b/src/validation.cpp index 2828b4ae98..58488be3a7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4069,11 +4069,7 @@ bool ChainstateManager::LoadBlockIndex() bool ret = m_blockman.LoadBlockIndexDB(); if (!ret) return false; - std::vector vSortedByHeight; - vSortedByHeight.reserve(m_blockman.m_block_index.size()); - for (auto& [_, block_index] : m_blockman.m_block_index) { - vSortedByHeight.push_back(&block_index); - } + std::vector vSortedByHeight{m_blockman.GetAllBlockIndices()}; std::sort(vSortedByHeight.begin(), vSortedByHeight.end(), CBlockIndexHeightOnlyComparator());