From 491d827d5284ed984ee2b11daaee50321217eac5 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 13:26:01 -0400 Subject: [PATCH] refactor: Add ChainstateManager::m_chainstates member Use to replace m_active_chainstate, m_ibd_chainstate, and m_snapshot_chainstate members. This has several benefits: - Ensures ChainstateManager treats chainstates instances equally, making distinctions based on their attributes, not having special cases and making assumptions based on their identities. - Normalizes ChainstateManager representation so states that should be impossible to reach and validation code has no handling for (like m_snapshot_chainstate being set and m_ibd_chainstate being unset, or both being set but m_active_chainstate pointing to the m_ibd_chainstate) can no longer be represented. - Makes ChainstateManager more extensible so new chainstates can be added for different purposes, like indexing or generating and validating assumeutxo snapshots without interrupting regular node operations. With the m_chainstates member, new chainstates can be added and handled without needing to make changes all over validation code or to copy/paste/modify the existing code that's been already been written to handle m_ibd_chainstate and m_snapshot_chainstate. - Avoids terms that are confusing and misleading: - The term "active chainstate" term is confusing because multiple chainstates will be active and in use at the same time. Before a snapshot is validated, wallet code will use the snapshot chainstate, while indexes will use the IBD chainstate, and netorking code will use both chainstates, downloading snapshot blocks at higher priority, but also IBD blocks simultaneously. - The term "snapshot chainstate" is ambiguous because it could refer either to the chainstate originally loaded from a snapshot, or to the chainstate being used to validate a snapshot that was loaded, or to a chainstate being used to produce a snapshot, but it is arbitrary used to refer the first thing. The terms "most-work chainstate" or "assumed-valid chainstate" should be less ambiguous ways to refer to chainstates loaded from snapshots. - The term "IBD chainstate" is not just ambiguous but actively confusing because technically IBD ends and the node is considered synced when the snapshot chainstate finishes syncing, so in practice the IBD chainstate will mostly by synced after IBD is complete. The term "fully-validated" is a better way of describing the characteristics and purpose of this chainstate. --- src/node/chainstate.cpp | 2 +- src/validation.cpp | 100 ++++++++++++++++++---------------------- src/validation.h | 73 +++++++++++++---------------- 3 files changed, 78 insertions(+), 97 deletions(-) diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 2c27ccf464f..59d6de63e5a 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -175,7 +175,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize // Reset chainstate target to network tip instead of snapshot block. validated_cs.SetTargetBlock(nullptr); LogInfo("[snapshot] deleting snapshot chainstate due to reindexing"); - if (!chainman.DeleteSnapshotChainstate()) { + if (!chainman.DeleteChainstate(*assumeutxo_cs)) { return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Couldn't remove snapshot chainstate.")}; } assumeutxo_cs = nullptr; diff --git a/src/validation.cpp b/src/validation.cpp index 83a37d07079..0d16dc209a6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3180,7 +3180,7 @@ bool Chainstate::ConnectTip( // stop connecting blocks to this chainstate because this->ReachedTarget() // will be true and this->setBlockIndexCandidates will not have additional // blocks. - Chainstate& current_cs{*Assert(m_chainman.m_active_chainstate)}; + Chainstate& current_cs{m_chainman.CurrentChainstate()}; m_chainman.MaybeValidateSnapshot(*this, current_cs); connectTrace.BlockConnected(pindexNew, std::move(block_to_connect)); @@ -3843,7 +3843,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 == CurrentChainstate().SnapshotBase())) { + std::ranges::any_of(m_chainstates, [&](const auto& cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->SnapshotBase() == pindexNew; }))) { 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; @@ -5307,8 +5307,8 @@ void ChainstateManager::CheckBlockIndex() const if (pindex->pprev == nullptr) { // Genesis block checks. assert(pindex->GetBlockHash() == GetConsensus().hashGenesisBlock); // Genesis block's hash must match. - for (const Chainstate* c : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) { - if (c && c->m_chain.Genesis() != nullptr) { + for (const auto& c : m_chainstates) { + if (c->m_chain.Genesis() != nullptr) { assert(pindex == c->m_chain.Genesis()); // The chain's genesis block must be this block. } } @@ -5367,8 +5367,8 @@ void ChainstateManager::CheckBlockIndex() const assert((pindex->nStatus & BLOCK_FAILED_MASK) || pindex->nChainWork <= m_best_header->nChainWork); // Chainstate-specific checks on setBlockIndexCandidates - for (const Chainstate* c : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) { - if (!c || c->m_chain.Tip() == nullptr) continue; + for (const auto& c : m_chainstates) { + if (c->m_chain.Tip() == nullptr) continue; // Two main factors determine whether pindex is a candidate in // setBlockIndexCandidates: // @@ -5406,12 +5406,12 @@ void ChainstateManager::CheckBlockIndex() const // is the base of the snapshot, pindex is also a potential // candidate. if (pindexFirstMissing == nullptr || pindex == c->m_chain.Tip() || pindex == c->SnapshotBase()) { - // If this chainstate is the active chainstate, pindex - // must be in setBlockIndexCandidates. Otherwise, this - // chainstate is a background validation chainstate, and - // pindex only needs to be added if it is an ancestor of - // the snapshot that is being validated. - if (c == &ActiveChainstate() || snap_base->GetAncestor(pindex->nHeight) == pindex) { + // If this chainstate is not a historical chainstate + // targeting a specific block, pindex must be in + // setBlockIndexCandidates. Otherwise, pindex only + // needs to be added if it is an ancestor of the target + // block. + if (!c->TargetBlock() || c->TargetBlock()->GetAncestor(pindex->nHeight) == pindex) { assert(c->setBlockIndexCandidates.contains(const_cast(pindex))); } } @@ -5451,12 +5451,10 @@ void ChainstateManager::CheckBlockIndex() const // tip. // So if this block is itself better than any m_chain.Tip() and it wasn't in // setBlockIndexCandidates, then it must be in m_blocks_unlinked. - for (const Chainstate* c : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) { - if (!c) continue; - const bool is_active = c == &ActiveChainstate(); + for (const auto& c : m_chainstates) { if (!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip()) && !c->setBlockIndexCandidates.contains(const_cast(pindex))) { if (pindexFirstInvalid == nullptr) { - if (is_active || snap_base->GetAncestor(pindex->nHeight) == pindex) { + if (!c->TargetBlock() || c->TargetBlock()->GetAncestor(pindex->nHeight) == pindex) { assert(foundInUnlinked); } } @@ -5610,8 +5608,8 @@ std::vector ChainstateManager::GetAll() LOCK(::cs_main); std::vector out; - for (Chainstate* cs : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) { - if (cs && cs->m_assumeutxo != Assumeutxo::INVALID && !cs->m_target_utxohash) out.push_back(cs); + for (const auto& cs : m_chainstates) { + if (cs && cs->m_assumeutxo != Assumeutxo::INVALID && !cs->m_target_utxohash) out.push_back(cs.get()); } return out; @@ -5620,12 +5618,9 @@ std::vector ChainstateManager::GetAll() Chainstate& ChainstateManager::InitializeChainstate(CTxMemPool* mempool) { AssertLockHeld(::cs_main); - assert(!m_ibd_chainstate); - assert(!m_active_chainstate); - - m_ibd_chainstate = std::make_unique(mempool, m_blockman, *this); - m_active_chainstate = m_ibd_chainstate.get(); - return *m_active_chainstate; + assert(m_chainstates.empty()); + m_chainstates.emplace_back(std::make_unique(mempool, m_blockman, *this)); + return *m_chainstates.back(); } [[nodiscard]] static bool DeleteCoinsDBFromDisk(const fs::path db_path, bool is_snapshot) @@ -5706,7 +5701,7 @@ util::Result ChainstateManager::ActivateSnapshot( return util::Error{Untranslated("A forked headers-chain with more work than the chain with the snapshot base block header exists. Please proceed to sync without AssumeUtxo.")}; } - auto mempool{m_active_chainstate->GetMempool()}; + auto mempool{CurrentChainstate().GetMempool()}; if (mempool && mempool->size() > 0) { return util::Error{Untranslated("Can't activate a snapshot when mempool not empty")}; } @@ -6091,7 +6086,6 @@ SnapshotCompletionResult ChainstateManager::MaybeValidateSnapshot(Chainstate& va // Reset chainstate target to network tip instead of snapshot block. validated_cs.SetTargetBlock(nullptr); - m_active_chainstate = &validated_cs; unvalidated_cs.m_assumeutxo = Assumeutxo::INVALID; auto rename_result = unvalidated_cs.InvalidateCoinsDBOnDisk(); @@ -6164,14 +6158,13 @@ SnapshotCompletionResult ChainstateManager::MaybeValidateSnapshot(Chainstate& va Chainstate& ChainstateManager::ActiveChainstate() const { LOCK(::cs_main); - assert(m_active_chainstate); - return *m_active_chainstate; + return CurrentChainstate(); } void ChainstateManager::MaybeRebalanceCaches() { AssertLockHeld(::cs_main); - Chainstate& current_cs{*Assert(m_active_chainstate)}; + Chainstate& current_cs{CurrentChainstate()}; Chainstate* historical_cs{HistoricalChainstate()}; if (!historical_cs && !current_cs.m_from_snapshot_blockhash) { // Allocate everything to the IBD chainstate. This will always happen @@ -6202,9 +6195,7 @@ void ChainstateManager::MaybeRebalanceCaches() void ChainstateManager::ResetChainstates() { - m_ibd_chainstate.reset(); - m_snapshot_chainstate.reset(); - m_active_chainstate = nullptr; + m_chainstates.clear(); } /** @@ -6238,7 +6229,7 @@ ChainstateManager::~ChainstateManager() Chainstate* ChainstateManager::LoadAssumeutxoChainstate() { - assert(!m_snapshot_chainstate); + assert(!CurrentChainstate().m_from_snapshot_blockhash); std::optional path = node::FindAssumeutxoChainstateDir(m_options.datadir); if (!path) { return nullptr; @@ -6257,22 +6248,21 @@ Chainstate* ChainstateManager::LoadAssumeutxoChainstate() Chainstate& ChainstateManager::AddChainstate(std::unique_ptr chainstate) { - assert(!m_snapshot_chainstate); - m_snapshot_chainstate = std::move(chainstate); - + Chainstate& prev_chainstate{CurrentChainstate()}; + assert(prev_chainstate.m_assumeutxo == Assumeutxo::VALIDATED); // Set target block for historical chainstate to snapshot block. - assert(m_ibd_chainstate.get()); - assert(!m_ibd_chainstate->m_target_blockhash); - m_ibd_chainstate->SetTargetBlockHash(*Assert(m_snapshot_chainstate->m_from_snapshot_blockhash)); + assert(!prev_chainstate.m_target_blockhash); + prev_chainstate.m_target_blockhash = chainstate->m_from_snapshot_blockhash; + m_chainstates.push_back(std::move(chainstate)); + Chainstate& curr_chainstate{CurrentChainstate()}; + assert(&curr_chainstate == m_chainstates.back().get()); // Transfer possession of the mempool to the chainstate. // Mempool is empty at this point because we're still in IBD. - Assert(m_active_chainstate->m_mempool->size() == 0); - Assert(!m_snapshot_chainstate->m_mempool); - m_snapshot_chainstate->m_mempool = m_active_chainstate->m_mempool; - m_active_chainstate->m_mempool = nullptr; - m_active_chainstate = m_snapshot_chainstate.get(); - return *m_snapshot_chainstate; + assert(prev_chainstate.m_mempool->size() == 0); + assert(!curr_chainstate.m_mempool); + std::swap(curr_chainstate.m_mempool, prev_chainstate.m_mempool); + return curr_chainstate; } bool IsBIP30Repeat(const CBlockIndex& block_index) @@ -6319,21 +6309,21 @@ util::Result Chainstate::InvalidateCoinsDBOnDisk() return {}; } -bool ChainstateManager::DeleteSnapshotChainstate() +bool ChainstateManager::DeleteChainstate(Chainstate& chainstate) { AssertLockHeld(::cs_main); - Assert(m_snapshot_chainstate); - Assert(m_ibd_chainstate); - - fs::path snapshot_datadir = Assert(node::FindAssumeutxoChainstateDir(m_options.datadir)).value(); - if (!DeleteCoinsDBFromDisk(snapshot_datadir, /*is_snapshot=*/ true)) { + assert(!chainstate.m_coins_views); + const fs::path db_path{chainstate.StoragePath()}; + if (!DeleteCoinsDBFromDisk(db_path, /*is_snapshot=*/bool{chainstate.m_from_snapshot_blockhash})) { LogError("Deletion of %s failed. Please remove it manually to continue reindexing.", - fs::PathToString(snapshot_datadir)); + fs::PathToString(db_path)); return false; } - m_active_chainstate = m_ibd_chainstate.get(); - m_active_chainstate->m_mempool = m_snapshot_chainstate->m_mempool; - m_snapshot_chainstate.reset(); + std::unique_ptr prev_chainstate{Assert(RemoveChainstate(chainstate))}; + Chainstate& curr_chainstate{CurrentChainstate()}; + assert(prev_chainstate->m_mempool->size() == 0); + assert(!curr_chainstate.m_mempool); + std::swap(curr_chainstate.m_mempool, prev_chainstate->m_mempool); return true; } diff --git a/src/validation.h b/src/validation.h index 450c7ca887e..f330d90af4c 100644 --- a/src/validation.h +++ b/src/validation.h @@ -931,40 +931,6 @@ enum class SnapshotCompletionResult { class ChainstateManager { private: - //! The chainstate used under normal operation (i.e. "regular" IBD) or, if - //! a snapshot is in use, for background validation. - //! - //! Its contents (including on-disk data) will be deleted *upon shutdown* - //! after background validation of the snapshot has completed. We do not - //! free the chainstate contents immediately after it finishes validation - //! to cautiously avoid a case where some other part of the system is still - //! using this pointer (e.g. net_processing). - //! - //! Once this pointer is set to a corresponding chainstate, it will not - //! be reset until init.cpp:Shutdown(). - //! - //! It is important for the pointer to not be deleted until shutdown, - //! because cs_main is not always held when the pointer is accessed, for - //! example when calling ActivateBestChain, so there's no way you could - //! prevent code from using the pointer while deleting it. - std::unique_ptr m_ibd_chainstate GUARDED_BY(::cs_main); - - //! A chainstate initialized on the basis of a UTXO snapshot. If this is - //! non-null, it is always our active chainstate. - //! - //! Once this pointer is set to a corresponding chainstate, it will not - //! be reset until init.cpp:Shutdown(). - //! - //! It is important for the pointer to not be deleted until shutdown, - //! because cs_main is not always held when the pointer is accessed, for - //! example when calling ActivateBestChain, so there's no way you could - //! prevent code from using the pointer while deleting it. - std::unique_ptr m_snapshot_chainstate GUARDED_BY(::cs_main); - - //! Points to either the ibd or snapshot chainstate; indicates our - //! most-work chain. - Chainstate* m_active_chainstate GUARDED_BY(::cs_main) {nullptr}; - CBlockIndex* m_best_invalid GUARDED_BY(::cs_main){nullptr}; /** The last header for which a headerTip notification was issued. */ @@ -1128,8 +1094,7 @@ public: //! per assumeutxo chain parameters. //! - Wait for our headers chain to include the base block of the snapshot. //! - "Fast forward" the tip of the new chainstate to the base of the snapshot. - //! - Move the new chainstate to `m_snapshot_chainstate` and make it our - //! ChainstateActive(). + //! - Construct the new Chainstate and add it to m_chainstates. [[nodiscard]] util::Result ActivateSnapshot( AutoFile& coins_file, const node::SnapshotMetadata& metadata, bool in_memory); @@ -1146,14 +1111,19 @@ public: //! Return current chainstate targeting the most-work, network tip. Chainstate& CurrentChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { - return *Assert(m_active_chainstate); + for (auto& cs : m_chainstates) { + if (cs && cs->m_assumeutxo != Assumeutxo::INVALID && !cs->m_target_blockhash) return *cs; + } + abort(); } //! Return historical chainstate targeting a specific block, if any. Chainstate* HistoricalChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { - auto* cs{m_ibd_chainstate.get()}; - return cs && cs->m_target_blockhash && !cs->m_target_utxohash ? cs : nullptr; + for (auto& cs : m_chainstates) { + if (cs && cs->m_assumeutxo != Assumeutxo::INVALID && cs->m_target_blockhash && !cs->m_target_utxohash) return cs.get(); + } + return nullptr; } //! Return fully validated chainstate that should be used for indexing, to @@ -1167,6 +1137,18 @@ public: abort(); } + //! Remove a chainstate. + std::unique_ptr RemoveChainstate(Chainstate& chainstate) EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) + { + auto it{std::find_if(m_chainstates.begin(), m_chainstates.end(), [&](auto& cs) { return cs.get() == &chainstate; })}; + if (it != m_chainstates.end()) { + auto ret{std::move(*it)}; + m_chainstates.erase(it); + return ret; + } + return nullptr; + } + //! 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 @@ -1331,9 +1313,9 @@ public: void ResetChainstates() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Remove the snapshot-based chainstate and all on-disk artifacts. + //! Remove the chainstate and all on-disk artifacts. //! Used when reindex{-chainstate} is called during snapshot use. - [[nodiscard]] bool DeleteSnapshotChainstate() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + [[nodiscard]] bool DeleteChainstate(Chainstate& chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); //! If we have validated a snapshot chain during this runtime, copy its //! chainstate directory over to the main `chainstate` location, completing @@ -1363,6 +1345,15 @@ public: CCheckQueue& GetCheckQueue() { return m_script_check_queue; } ~ChainstateManager(); + + //! List of chainstates. Note: in general, it is not safe to delete + //! Chainstate objects once they are added to this list because there is no + //! mutex that can be locked to prevent Chainstate pointers from being used + //! while they are deleted. (cs_main doesn't work because it is too narrow + //! and is released in the middle of Chainstate::ActivateBestChain to let + //! notifications be processed. m_chainstate_mutex doesn't work because it + //! is not locked at other times when the chainstate is in use.) + std::vector> m_chainstates GUARDED_BY(::cs_main); }; /** Deployment* info via ChainstateManager */