From a229cb9477e6622087241be7a105551d1329503b Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 12:03:47 -0400 Subject: [PATCH] refactor: Add ChainstateManager::CurrentChainstate() method CurrentChainstate() is basically the same as ActiveChainstate() except it requires cs_main to be locked when it is called, instead of locking cs_main internally. The name "current" should also be less confusing than "active" because multiple chainstates can be active, and CurrentChainstate() returns the chainstate targeting the current network tip, regardless of what chainstates are being downloaded or how they are used. --- src/net_processing.cpp | 2 +- src/rpc/blockchain.cpp | 11 +++++------ src/validation.cpp | 11 +++-------- src/validation.h | 14 +++++++++++--- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6e60ce824fe..09b2f221c1e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1387,7 +1387,7 @@ void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int co // When we sync with AssumeUtxo and discover the snapshot is not in the peer's best chain, abort: // We can't reorg to this chain due to missing undo data until the background sync has finished, // so downloading blocks from it would be futile. - const CBlockIndex* snap_base{m_chainman.GetSnapshotBaseBlock()}; + const CBlockIndex* snap_base{m_chainman.CurrentChainstate().SnapshotBase()}; if (snap_base && state->pindexBestKnownBlock->GetAncestor(snap_base->nHeight) != snap_base) { LogDebug(BCLog::NET, "Not downloading blocks from peer=%d, which doesn't have the snapshot block in its best chain.\n", peer.m_id); return; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 8f283ff4f03..78dbafecbdb 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -3432,7 +3432,7 @@ return RPCHelpMan{ ChainstateManager& chainman = EnsureAnyChainman(request.context); - auto make_chain_data = [&](const Chainstate& cs, bool validated) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + auto make_chain_data = [&](const Chainstate& cs) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); UniValue data(UniValue::VOBJ); if (!cs.m_chain.Tip()) { @@ -3452,17 +3452,16 @@ return RPCHelpMan{ if (cs.m_from_snapshot_blockhash) { data.pushKV("snapshot_blockhash", cs.m_from_snapshot_blockhash->ToString()); } - data.pushKV("validated", validated); + data.pushKV("validated", cs.m_assumeutxo == Assumeutxo::VALIDATED); return data; }; obj.pushKV("headers", chainman.m_best_header ? chainman.m_best_header->nHeight : -1); - - const auto& chainstates = chainman.GetAll(); UniValue obj_chainstates{UniValue::VARR}; - for (Chainstate* cs : chainstates) { - obj_chainstates.push_back(make_chain_data(*cs, !cs->m_from_snapshot_blockhash || chainstates.size() == 1)); + if (const Chainstate * cs{chainman.HistoricalChainstate()}) { + obj_chainstates.push_back(make_chain_data(*cs)); } + obj_chainstates.push_back(make_chain_data(chainman.CurrentChainstate())); obj.pushKV("chainstates", std::move(obj_chainstates)); return obj; } diff --git a/src/validation.cpp b/src/validation.cpp index a9bf12b5495..fabff80c587 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3841,7 +3841,7 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd // m_chain_tx_count value is not zero, assert that value is actually correct. auto prev_tx_sum = [](CBlockIndex& block) { return block.nTx + (block.pprev ? block.pprev->m_chain_tx_count : 0); }; if (!Assume(pindexNew->m_chain_tx_count == 0 || pindexNew->m_chain_tx_count == prev_tx_sum(*pindexNew) || - pindexNew == GetSnapshotBaseBlock())) { + pindexNew == CurrentChainstate().SnapshotBase())) { LogWarning("Internal bug detected: block %d has unexpected m_chain_tx_count %i that should be %i (%s %s). Please report this issue here: %s\n", pindexNew->nHeight, pindexNew->m_chain_tx_count, prev_tx_sum(*pindexNew), CLIENT_NAME, FormatFullVersion(), CLIENT_BUGREPORT); pindexNew->m_chain_tx_count = 0; @@ -4974,7 +4974,7 @@ bool ChainstateManager::LoadBlockIndex() // VALID_TRANSACTIONS (eg if we haven't yet downloaded the block), // so we special-case the snapshot block as a potential candidate // here. - if (pindex == GetSnapshotBaseBlock() || + if (pindex == CurrentChainstate().SnapshotBase() || (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && (pindex->HaveNumChainTxs() || pindex->pprev == nullptr))) { @@ -5263,7 +5263,7 @@ void ChainstateManager::CheckBlockIndex() const // to earlier blocks that have not been downloaded or validated yet, so // checks for later blocks can assume the earlier blocks were validated and // be stricter, testing for more requirements. - const CBlockIndex* snap_base{GetSnapshotBaseBlock()}; + const CBlockIndex* snap_base{CurrentChainstate().SnapshotBase()}; const CBlockIndex *snap_first_missing{}, *snap_first_notx{}, *snap_first_notv{}, *snap_first_nocv{}, *snap_first_nosv{}; auto snap_update_firsts = [&] { if (pindex == snap_base) { @@ -6357,11 +6357,6 @@ ChainstateRole Chainstate::GetRole() const return m_target_blockhash ? ChainstateRole::BACKGROUND : m_assumeutxo == Assumeutxo::UNVALIDATED ? ChainstateRole::ASSUMEDVALID : ChainstateRole::NORMAL; } -const CBlockIndex* ChainstateManager::GetSnapshotBaseBlock() const -{ - return m_active_chainstate ? m_active_chainstate->SnapshotBase() : nullptr; -} - void ChainstateManager::RecalculateBestHeader() { AssertLockHeld(cs_main); diff --git a/src/validation.h b/src/validation.h index 37c6051e5b6..cafe24934be 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1140,8 +1140,11 @@ public: //! if it does happen, it is a fatal error. SnapshotCompletionResult MaybeValidateSnapshot(Chainstate& validated_cs, Chainstate& unvalidated_cs) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Returns nullptr if no snapshot has been loaded. - const CBlockIndex* GetSnapshotBaseBlock() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! Return current chainstate targeting the most-work, network tip. + Chainstate& CurrentChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) + { + return *Assert(m_active_chainstate); + } //! Return historical chainstate targeting a specific block, if any. Chainstate* HistoricalChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) @@ -1150,11 +1153,16 @@ public: return cs && cs->m_target_blockhash && !cs->m_target_utxohash ? cs : nullptr; } - //! The most-work chain. + //! Alternatives to CurrentChainstate() used by older code to query latest + //! chainstate information without locking cs_main. Newer code should avoid + //! querying ChainstateManager and use Chainstate objects directly, or + //! should use CurrentChainstate() instead. + //! @{ Chainstate& ActiveChainstate() const; CChain& ActiveChain() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChainstate().m_chain; } int ActiveHeight() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Height(); } CBlockIndex* ActiveTip() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Tip(); } + //! @} node::BlockMap& BlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {