From de00e87548f7ddd623355b7094924b0387a36280 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 31 May 2024 06:53:50 -0400 Subject: [PATCH 01/17] test: Fix broken chainstatemanager_snapshot_init check The following test code never checked anything because the if statement was always false: if (cs != &chainman_restarted.ActiveChainstate()) { BOOST_CHECK_EQUAL(cs->m_chain.Height(), 109); } Also, the height of the background chainstate it was intending to check is 110, not 109. Fix both problems by rewriting the check. --- .../validation_chainstatemanager_tests.cpp | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index bf440ca6397..c6ae6088ec2 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -557,7 +557,8 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.size(), num_indexes - last_assumed_valid_idx + 1); } -//! Ensure that snapshot chainstates initialize properly when found on disk. +//! Ensure that snapshot chainstate can be loaded when found on disk after a +//! restart, and that new blocks can be connected to both chainstates. BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) { ChainstateManager& chainman = *Assert(m_node.chainman); @@ -591,8 +592,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) BOOST_CHECK_EQUAL(bg_chainstate.m_chain.Height(), 109); // Test that simulating a shutdown (resetting ChainstateManager) and then performing - // chainstate reinitializing successfully cleans up the background-validation - // chainstate data, and we end up with a single chainstate that is at tip. + // chainstate reinitializing successfully reloads both chainstates. ChainstateManager& chainman_restarted = this->SimulateNodeRestart(); BOOST_TEST_MESSAGE("Performing Load/Verify/Activate of chainstate"); @@ -600,9 +600,18 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) // This call reinitializes the chainstates. this->LoadVerifyActivateChainstate(); + std::vector chainstates; { LOCK(chainman_restarted.GetMutex()); - BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 2); + chainstates = chainman_restarted.GetAll(); + BOOST_CHECK_EQUAL(chainstates.size(), 2); + // Background chainstate has height of 109 not 110 here due to a quirk + // of the LoadVerifyActivate only calling ActivateBestChain on one + // chainstate. The height would be 110 after a real restart, but it's + // fine for this test which is focused on the snapshot chainstate. + BOOST_CHECK_EQUAL(chainstates[0]->m_chain.Height(), 109); + BOOST_CHECK_EQUAL(chainstates[1]->m_chain.Height(), 210); + BOOST_CHECK(chainman_restarted.IsSnapshotActive()); BOOST_CHECK(!chainman_restarted.IsSnapshotValidated()); @@ -618,12 +627,10 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) BOOST_CHECK_EQUAL(chainman_restarted.ActiveHeight(), 220); // Background chainstate should be unaware of new blocks on the snapshot - // chainstate. - for (Chainstate* cs : chainman_restarted.GetAll()) { - if (cs != &chainman_restarted.ActiveChainstate()) { - BOOST_CHECK_EQUAL(cs->m_chain.Height(), 109); - } - } + // chainstate, but the block disconnected above is now reattached. + BOOST_CHECK_EQUAL(chainstates[0]->m_chain.Height(), 110); + BOOST_CHECK_EQUAL(chainstates[1]->m_chain.Height(), 220); + BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 1); } } From 6082c84713f42f5fa66f9a76baef17e8ed231633 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 29 May 2024 20:27:27 -0400 Subject: [PATCH 02/17] refactor: Add Chainstate::m_target_blockhash member Make Chainstate objects aware of what block they are targeting. This makes Chainstate objects more self contained, so it's possible for validation code to look at one Chainstate object and know what blocks to connect to it without needing to consider global validation state or look at other Chainstate objects. The motivation for this change is to make validation and networking code more readable, so understanding it just requires knowing about chains and blocks, not reasoning about assumeutxo download states. This change also enables simplifications to the ChainstateManager interface in subsequent commits, and could make it easier to implement new features like creating new Chainstate objects to generate UTXO snapshots or index UTXO data. Note that behavior of the MaybeCompleteSnapshotValidation function is not changing here but some checks that were previously impossible to trigger like the BASE_BLOCKHASH_MISMATCH case have been turned into asserts. --- src/init.cpp | 2 +- src/net_processing.cpp | 15 +-- src/node/chainstate.cpp | 4 +- src/test/validation_chainstate_tests.cpp | 11 +-- src/validation.cpp | 116 ++++++++++++++--------- src/validation.h | 53 ++++++++--- 6 files changed, 125 insertions(+), 76 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 09e7523e329..593467e3439 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1880,7 +1880,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } } else { // Prior to setting NODE_NETWORK, check if we can provide historical blocks. - if (!WITH_LOCK(chainman.GetMutex(), return chainman.BackgroundSyncInProgress())) { + if (!WITH_LOCK(chainman.GetMutex(), return chainman.HistoricalChainstate())) { LogInfo("Setting NODE_NETWORK in non-prune mode"); g_local_services = ServiceFlags(g_local_services | NODE_NETWORK); } else { diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c8bc4a846df..6e60ce824fe 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5921,18 +5921,19 @@ bool PeerManagerImpl::SendMessages(CNode* pto) return std::max(0, MAX_BLOCKS_IN_TRANSIT_PER_PEER - static_cast(state.vBlocksInFlight.size())); }; - // If a snapshot chainstate is in use, we want to find its next blocks - // before the background chainstate to prioritize getting to network tip. + // If there are multiple chainstates, download blocks for the + // current chainstate first, to prioritize getting to network tip + // before downloading historical blocks. FindNextBlocksToDownload(*peer, get_inflight_budget(), vToDownload, staller); - if (m_chainman.BackgroundSyncInProgress() && !IsLimitedPeer(*peer)) { - // If the background tip is not an ancestor of the snapshot block, + auto historical_blocks{m_chainman.GetHistoricalBlockRange()}; + if (historical_blocks && !IsLimitedPeer(*peer)) { + // If the first needed historical block is not an ancestor of the last, // we need to start requesting blocks from their last common ancestor. - const CBlockIndex *from_tip = LastCommonAncestor(m_chainman.GetBackgroundSyncTip(), m_chainman.GetSnapshotBaseBlock()); + const CBlockIndex* from_tip = LastCommonAncestor(historical_blocks->first, historical_blocks->second); TryDownloadingHistoricalBlocks( *peer, get_inflight_budget(), - vToDownload, from_tip, - Assert(m_chainman.GetSnapshotBaseBlock())); + vToDownload, from_tip, historical_blocks->second); } for (const CBlockIndex *pindex : vToDownload) { uint32_t nFetchFlags = GetFetchFlags(*peer); diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 1f0cf805895..0cebcad336f 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -166,12 +166,14 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize chainman.m_total_coinsdb_cache = cache_sizes.coins_db; // Load the fully validated chainstate. - chainman.InitializeChainstate(options.mempool); + Chainstate& validated_cs{chainman.InitializeChainstate(options.mempool)}; // Load a chain created from a UTXO snapshot, if any exist. bool has_snapshot = chainman.DetectSnapshotChainstate(); if (has_snapshot && options.wipe_chainstate_db) { + // 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()) { return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Couldn't remove snapshot chainstate.")}; diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index 7204f1688dd..b4aeb5b3370 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -107,16 +107,7 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) curr_tip = get_notify_tip(); - BOOST_CHECK_EQUAL(chainman.GetAll().size(), 2); - - Chainstate& background_cs{*Assert([&]() -> Chainstate* { - for (Chainstate* cs : chainman.GetAll()) { - if (cs != &chainman.ActiveChainstate()) { - return cs; - } - } - return nullptr; - }())}; + Chainstate& background_cs{*Assert(WITH_LOCK(::cs_main, return chainman.HistoricalChainstate()))}; // Append the first block to the background chain. BlockValidationState state; diff --git a/src/validation.cpp b/src/validation.cpp index 3464e588456..0831b07ccd9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1891,6 +1891,29 @@ const CBlockIndex* Chainstate::SnapshotBase() const return m_cached_snapshot_base; } +const CBlockIndex* Chainstate::TargetBlock() const +{ + if (!m_target_blockhash) return nullptr; + if (!m_cached_target_block) m_cached_target_block = Assert(m_chainman.m_blockman.LookupBlockIndex(*m_target_blockhash)); + return m_cached_target_block; +} + +void Chainstate::SetTargetBlock(CBlockIndex* block) +{ + if (block) { + m_target_blockhash = block->GetBlockHash(); + } else { + m_target_blockhash.reset(); + } + m_cached_target_block = block; +} + +void Chainstate::SetTargetBlockHash(uint256 block_hash) +{ + m_target_blockhash = block_hash; + m_cached_target_block = nullptr; +} + void Chainstate::InitCoinsDB( size_t cache_size_bytes, bool in_memory, @@ -3144,8 +3167,6 @@ bool Chainstate::ConnectTip( // If we are the background validation chainstate, check to see if we are done // validating the snapshot (i.e. our tip has reached the snapshot's base block). if (this != &m_chainman.ActiveChainstate()) { - // This call may set `m_disabled`, which is referenced immediately afterwards in - // ActivateBestChain, so that we stop connecting blocks past the snapshot base. m_chainman.MaybeCompleteSnapshotValidation(); } @@ -3439,12 +3460,8 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< } } - // This will have been toggled in - // ActivateBestChainStep -> ConnectTip -> MaybeCompleteSnapshotValidation, - // if at all, so we should catch it here. - // - // Break this do-while to ensure we don't advance past the base snapshot. - if (m_disabled) { + // Break this do-while to ensure we don't advance past the target block. + if (ReachedTarget()) { break; } } while (!m_chain.Tip() || (starting_tip && CBlockIndexWorkComparator()(m_chain.Tip(), starting_tip))); @@ -3486,7 +3503,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< } // release cs_main // When we reach this point, we switched to a new tip (stored in pindexNewTip). - bool disabled{false}; + bool reached_target; { LOCK(m_chainman.GetMutex()); if (exited_ibd) { @@ -3500,15 +3517,14 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< return false; } - disabled = m_disabled; + reached_target = ReachedTarget(); } - if (disabled) { - // Background chainstate has reached the snapshot base block, so exit. - - // Restart indexes to resume indexing for all blocks unique to the snapshot - // chain. This resumes indexing "in order" from where the indexing on the - // background validation chain left off. + if (reached_target) { + // Chainstate has reached the target block, so exit. + // + // Restart indexes so indexes can resync and index new blocks after + // the target block. // // This cannot be done while holding cs_main (within // MaybeCompleteSnapshotValidation) or a cs_main deadlock will occur. @@ -3789,17 +3805,15 @@ void Chainstate::TryAddBlockIndexCandidate(CBlockIndex* pindex) return; } - bool is_active_chainstate = this == &m_chainman.ActiveChainstate(); - if (is_active_chainstate) { - // The active chainstate should always add entries that have more + const CBlockIndex* target_block{TargetBlock()}; + if (!target_block) { + // If no specific target block, add all entries that have more // work than the tip. setBlockIndexCandidates.insert(pindex); - } else if (!m_disabled) { - // For the background chainstate, we only consider connecting blocks - // towards the snapshot base (which can't be nullptr or else we'll - // never make progress). - const CBlockIndex* snapshot_base{Assert(m_chainman.GetSnapshotBaseBlock())}; - if (snapshot_base->GetAncestor(pindex->nHeight) == pindex) { + } else { + // If there is a target block, only consider connecting blocks + // towards the target block. + if (target_block->GetAncestor(pindex->nHeight) == pindex) { setBlockIndexCandidates.insert(pindex); } } @@ -4483,7 +4497,7 @@ bool ChainstateManager::ProcessNewBlock(const std::shared_ptr& blo return false; } - Chainstate* bg_chain{WITH_LOCK(cs_main, return BackgroundSyncInProgress() ? m_ibd_chainstate.get() : nullptr)}; + Chainstate* bg_chain{WITH_LOCK(cs_main, return HistoricalChainstate())}; BlockValidationState bg_state; if (bg_chain && !bg_chain->ActivateBestChain(bg_state, block)) { LogError("%s: [background] ActivateBestChain failed (%s)\n", __func__, bg_state.ToString()); @@ -5787,6 +5801,11 @@ util::Result ChainstateManager::ActivateSnapshot( const bool chaintip_loaded = m_snapshot_chainstate->LoadChainTip(); assert(chaintip_loaded); + // Set snapshot block as the target block for the historical chainstate. + assert(m_ibd_chainstate.get()); + assert(!m_ibd_chainstate->m_target_blockhash); + m_ibd_chainstate->SetTargetBlockHash(base_blockhash); + // Transfer possession of the mempool to the snapshot chainstate. // Mempool is empty at this point because we're still in IBD. Assert(m_active_chainstate->m_mempool->size() == 0); @@ -6048,23 +6067,25 @@ util::Result ChainstateManager::PopulateAndValidateSnapshot( SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() { AssertLockHeld(cs_main); + + // If a snapshot does not need to be validated... if (m_ibd_chainstate.get() == &this->ActiveChainstate() || + // Or if either chainstate is unusable... !this->IsUsable(m_snapshot_chainstate.get()) || !this->IsUsable(m_ibd_chainstate.get()) || - !m_ibd_chainstate->m_chain.Tip()) { - // Nothing to do - this function only applies to the background - // validation chainstate. + !m_snapshot_chainstate->m_from_snapshot_blockhash || + !m_ibd_chainstate->m_chain.Tip() || + // Or the validated chainstate is not targeting the snapshot block... + !m_ibd_chainstate->m_target_blockhash || + *m_ibd_chainstate->m_target_blockhash != *m_snapshot_chainstate->m_from_snapshot_blockhash || + // Or the validated chainstate has not reached the snapshot block yet... + !m_ibd_chainstate->ReachedTarget()) { + // Then a snapshot cannot be validated and there is nothing to do. return SnapshotCompletionResult::SKIPPED; } const int snapshot_tip_height = this->ActiveHeight(); const int snapshot_base_height = *Assert(this->GetSnapshotBaseHeight()); const CBlockIndex& index_new = *Assert(m_ibd_chainstate->m_chain.Tip()); - - if (index_new.nHeight < snapshot_base_height) { - // Background IBD not complete yet. - return SnapshotCompletionResult::SKIPPED; - } - assert(SnapshotBlockhash()); uint256 snapshot_blockhash = *Assert(SnapshotBlockhash()); @@ -6087,6 +6108,9 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() LogError("[snapshot] !!! %s\n", user_error.original); LogError("[snapshot] deleting snapshot, reverting to validated chain, and stopping node\n"); + // Reset chainstate target to network tip instead of snapshot block. + m_ibd_chainstate->SetTargetBlock(nullptr); + m_active_chainstate = m_ibd_chainstate.get(); m_snapshot_chainstate->m_disabled = true; assert(!this->IsUsable(m_snapshot_chainstate.get())); @@ -6100,14 +6124,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() GetNotifications().fatalError(user_error); }; - if (index_new.GetBlockHash() != snapshot_blockhash) { - LogWarning("[snapshot] supposed base block %s does not match the " - "snapshot base block %s (height %d). Snapshot is not valid.", - index_new.ToString(), snapshot_blockhash.ToString(), snapshot_base_height); - handle_invalid_snapshot(); - return SnapshotCompletionResult::BASE_BLOCKHASH_MISMATCH; - } - + assert(index_new.GetBlockHash() == snapshot_blockhash); assert(index_new.nHeight == snapshot_base_height); int curr_height = m_ibd_chainstate->m_chain.Height(); @@ -6286,6 +6303,12 @@ Chainstate& ChainstateManager::ActivateExistingSnapshot(uint256 base_blockhash) std::make_unique(nullptr, m_blockman, *this, base_blockhash); LogInfo("[snapshot] switching active chainstate to %s", m_snapshot_chainstate->ToString()); + // 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(base_blockhash); + + // 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); @@ -6522,3 +6545,10 @@ std::pair ChainstateManager::GetPruneRange(const Chainstate& chainstat return {prune_start, prune_end}; } + +std::optional> ChainstateManager::GetHistoricalBlockRange() const +{ + const Chainstate* chainstate{HistoricalChainstate()}; + if (!chainstate) return {}; + return std::make_pair(chainstate->m_chain.Tip(), chainstate->TargetBlock()); +} diff --git a/src/validation.h b/src/validation.h index cd448f3ca9e..e547f88c674 100644 --- a/src/validation.h +++ b/src/validation.h @@ -561,6 +561,9 @@ protected: //! Cached result of LookupBlockIndex(*m_from_snapshot_blockhash) mutable const CBlockIndex* m_cached_snapshot_base GUARDED_BY(::cs_main){nullptr}; + //! Cached result of LookupBlockIndex(*m_target_blockhash) + mutable const CBlockIndex* m_cached_target_block GUARDED_BY(::cs_main){nullptr}; + std::optional m_last_script_check_reason_logged GUARDED_BY(::cs_main){}; public: @@ -620,6 +623,12 @@ public: */ const std::optional m_from_snapshot_blockhash; + //! Target block for this chainstate. If this is not set, chainstate will + //! target the most-work, valid block. If this is set, ChainstateManager + //! considers this a "historical" chainstate since it will only contain old + //! blocks up to the target block, not newer blocks. + std::optional m_target_blockhash GUARDED_BY(::cs_main); + /** * The base of the snapshot this chainstate was created from. * @@ -627,6 +636,26 @@ public: */ const CBlockIndex* SnapshotBase() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! Return target block which chainstate tip is expected to reach, if this + //! is a historic chainstate being used to validate a snapshot, or null if + //! chainstate targets the most-work block. + const CBlockIndex* TargetBlock() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! Set target block for this chainstate. If null, chainstate will target + //! the most-work valid block. If non-null chainstate will be a historic + //! chainstate and target the specified block. + void SetTargetBlock(CBlockIndex* block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! Set target block for this chainstate using just a block hash. Useful + //! when the block database has not been loaded yet. + void SetTargetBlockHash(uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + + //! Return true if chainstate reached target block. + bool ReachedTarget() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) + { + const CBlockIndex* target_block{TargetBlock()}; + assert(!target_block || target_block->GetAncestor(m_chain.Height()) == m_chain.Tip()); + return target_block && target_block == m_chain.Tip(); + } + /** * The set of all CBlockIndex entries that have as much work as our current * tip or more, and transaction data needed to be validated (with @@ -862,10 +891,6 @@ enum class SnapshotCompletionResult { // The UTXO set hash of the background validation chainstate does not match // the one expected by assumeutxo chainparams. HASH_MISMATCH, - - // The blockhash of the current tip of the background validation chainstate does - // not match the one expected by the snapshot chainstate. - BASE_BLOCKHASH_MISMATCH, }; /** @@ -1121,22 +1146,19 @@ public: //! Returns nullptr if no snapshot has been loaded. const CBlockIndex* GetSnapshotBaseBlock() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! Return historical chainstate targeting a specific block, if any. + Chainstate* HistoricalChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) + { + auto* cs{m_ibd_chainstate.get()}; + return IsUsable(cs) && cs->m_target_blockhash ? cs : nullptr; + } + //! The most-work chain. 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(); } - //! The state of a background sync (for net processing) - bool BackgroundSyncInProgress() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { - return IsUsable(m_snapshot_chainstate.get()) && IsUsable(m_ibd_chainstate.get()); - } - - //! The tip of the background sync chain - const CBlockIndex* GetBackgroundSyncTip() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { - return BackgroundSyncInProgress() ? m_ibd_chainstate->m_chain.Tip() : nullptr; - } - node::BlockMap& BlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); @@ -1337,6 +1359,9 @@ public: //! nullopt. std::optional GetSnapshotBaseHeight() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! Get range of historical blocks to download. + std::optional> GetHistoricalBlockRange() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! If, due to invalidation / reconsideration of blocks, the previous //! best header is no longer valid / guaranteed to be the most-work //! header in our block-index not known to be invalid, recalculate it. From 9fe927b6d654e752dac82156e209e45d31b75779 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 16 Apr 2025 11:53:44 -0400 Subject: [PATCH 03/17] refactor: Add Chainstate m_assumeutxo and m_target_utxohash members Get rid of m_disabled/IsUsable members. Instead of marking chains disabled for different reasons, store chainstate assumeutxo status explicitly and use that information to determine how chains should be treated. --- doc/design/assumeutxo.md | 5 ++- src/validation.cpp | 70 ++++++++++++++++++---------------------- src/validation.h | 45 ++++++++++++-------------- 3 files changed, 54 insertions(+), 66 deletions(-) diff --git a/doc/design/assumeutxo.md b/doc/design/assumeutxo.md index 123c02ac138..f1559cf4975 100644 --- a/doc/design/assumeutxo.md +++ b/doc/design/assumeutxo.md @@ -97,14 +97,13 @@ sequentially. ### Background chainstate hits snapshot base block Once the tip of the background chainstate hits the base block of the snapshot -chainstate, we stop use of the background chainstate by setting `m_disabled`, in -`MaybeCompleteSnapshotValidation()`, which is checked in `ActivateBestChain()`). We hash the +chainstate, we hash the background chainstate's UTXO set contents and ensure it matches the compiled value in `CMainParams::m_assumeutxo_data`. | | | | ---------- | ----------- | -| number of chainstates | 2 (ibd has `m_disabled=true`) | +| number of chainstates | 2 | | active chainstate | snapshot | The background chainstate data lingers on disk until the program is restarted. diff --git a/src/validation.cpp b/src/validation.cpp index 0831b07ccd9..4c6432de84c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1882,6 +1882,7 @@ Chainstate::Chainstate( : m_mempool(mempool), m_blockman(blockman), m_chainman(chainman), + m_assumeutxo(from_snapshot_blockhash ? Assumeutxo::UNVALIDATED : Assumeutxo::VALIDATED), m_from_snapshot_blockhash(from_snapshot_blockhash) {} const CBlockIndex* Chainstate::SnapshotBase() const @@ -3393,12 +3394,11 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< // we use m_chainstate_mutex to enforce mutual exclusion so that only one caller may execute this function at a time LOCK(m_chainstate_mutex); - // Belt-and-suspenders check that we aren't attempting to advance the background - // chainstate past the snapshot base block. - if (WITH_LOCK(::cs_main, return m_disabled)) { - LogError("m_disabled is set - this chainstate should not be in operation. " - "Please report this as a bug. %s", CLIENT_BUGREPORT); - return false; + // Belt-and-suspenders check that we aren't attempting to advance the + // chainstate past the target block. + if (WITH_LOCK(::cs_main, return m_target_utxohash)) { + LogError("%s", STR_INTERNAL_BUG("m_target_utxohash is set - this chainstate should not be in operation.")); + return Assume(false); } CBlockIndex *pindexMostWork = nullptr; @@ -5609,7 +5609,7 @@ std::vector ChainstateManager::GetAll() std::vector out; for (Chainstate* cs : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) { - if (this->IsUsable(cs)) out.push_back(cs); + if (cs && cs->m_assumeutxo != Assumeutxo::INVALID && !cs->m_target_utxohash) out.push_back(cs); } return out; @@ -6071,9 +6071,11 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() // If a snapshot does not need to be validated... if (m_ibd_chainstate.get() == &this->ActiveChainstate() || // Or if either chainstate is unusable... - !this->IsUsable(m_snapshot_chainstate.get()) || - !this->IsUsable(m_ibd_chainstate.get()) || + !m_snapshot_chainstate || + m_snapshot_chainstate->m_assumeutxo != Assumeutxo::UNVALIDATED || !m_snapshot_chainstate->m_from_snapshot_blockhash || + !m_ibd_chainstate || + m_ibd_chainstate->m_assumeutxo != Assumeutxo::VALIDATED || !m_ibd_chainstate->m_chain.Tip() || // Or the validated chainstate is not targeting the snapshot block... !m_ibd_chainstate->m_target_blockhash || @@ -6112,9 +6114,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() m_ibd_chainstate->SetTargetBlock(nullptr); m_active_chainstate = m_ibd_chainstate.get(); - m_snapshot_chainstate->m_disabled = true; - assert(!this->IsUsable(m_snapshot_chainstate.get())); - assert(this->IsUsable(m_ibd_chainstate.get())); + m_snapshot_chainstate->m_assumeutxo = Assumeutxo::INVALID; auto rename_result = m_snapshot_chainstate->InvalidateCoinsDBOnDisk(); if (!rename_result) { @@ -6131,7 +6131,6 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() assert(snapshot_base_height == curr_height); assert(snapshot_base_height == index_new.nHeight); - assert(this->IsUsable(m_snapshot_chainstate.get())); assert(this->GetAll().size() == 2); CCoinsViewDB& ibd_coins_db = m_ibd_chainstate->CoinsDB(); @@ -6187,7 +6186,8 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() LogInfo("[snapshot] snapshot beginning at %s has been fully validated", snapshot_blockhash.ToString()); - m_ibd_chainstate->m_disabled = true; + m_snapshot_chainstate->m_assumeutxo = Assumeutxo::VALIDATED; + m_ibd_chainstate->m_target_utxohash = AssumeutxoHash{ibd_stats.hashSerialized}; this->MaybeRebalanceCaches(); return SnapshotCompletionResult::SUCCESS; @@ -6209,34 +6209,30 @@ bool ChainstateManager::IsSnapshotActive() const void ChainstateManager::MaybeRebalanceCaches() { AssertLockHeld(::cs_main); - bool ibd_usable = this->IsUsable(m_ibd_chainstate.get()); - bool snapshot_usable = this->IsUsable(m_snapshot_chainstate.get()); - assert(ibd_usable || snapshot_usable); - - if (ibd_usable && !snapshot_usable) { + Chainstate& current_cs{*Assert(m_active_chainstate)}; + Chainstate* historical_cs{HistoricalChainstate()}; + if (!historical_cs && !current_cs.m_from_snapshot_blockhash) { // Allocate everything to the IBD chainstate. This will always happen // when we are not using a snapshot. - m_ibd_chainstate->ResizeCoinsCaches(m_total_coinstip_cache, m_total_coinsdb_cache); - } - else if (snapshot_usable && !ibd_usable) { + current_cs.ResizeCoinsCaches(m_total_coinstip_cache, m_total_coinsdb_cache); + } else if (!historical_cs) { // If background validation has completed and snapshot is our active chain... LogInfo("[snapshot] allocating all cache to the snapshot chainstate"); // Allocate everything to the snapshot chainstate. - m_snapshot_chainstate->ResizeCoinsCaches(m_total_coinstip_cache, m_total_coinsdb_cache); - } - else if (ibd_usable && snapshot_usable) { + current_cs.ResizeCoinsCaches(m_total_coinstip_cache, m_total_coinsdb_cache); + } else { // If both chainstates exist, determine who needs more cache based on IBD status. // // Note: shrink caches first so that we don't inadvertently overwhelm available memory. if (IsInitialBlockDownload()) { - m_ibd_chainstate->ResizeCoinsCaches( + historical_cs->ResizeCoinsCaches( m_total_coinstip_cache * 0.05, m_total_coinsdb_cache * 0.05); - m_snapshot_chainstate->ResizeCoinsCaches( + current_cs.ResizeCoinsCaches( m_total_coinstip_cache * 0.95, m_total_coinsdb_cache * 0.95); } else { - m_snapshot_chainstate->ResizeCoinsCaches( + current_cs.ResizeCoinsCaches( m_total_coinstip_cache * 0.05, m_total_coinsdb_cache * 0.05); - m_ibd_chainstate->ResizeCoinsCaches( + historical_cs->ResizeCoinsCaches( m_total_coinstip_cache * 0.95, m_total_coinsdb_cache * 0.95); } } @@ -6394,12 +6390,7 @@ bool ChainstateManager::DeleteSnapshotChainstate() ChainstateRole Chainstate::GetRole() const { - if (m_chainman.GetAll().size() <= 1) { - return ChainstateRole::NORMAL; - } - return (this != &m_chainman.ActiveChainstate()) ? - ChainstateRole::BACKGROUND : - ChainstateRole::ASSUMEDVALID; + return m_target_blockhash ? ChainstateRole::BACKGROUND : m_assumeutxo == Assumeutxo::UNVALIDATED ? ChainstateRole::ASSUMEDVALID : ChainstateRole::NORMAL; } const CBlockIndex* ChainstateManager::GetSnapshotBaseBlock() const @@ -6526,10 +6517,11 @@ std::pair ChainstateManager::GetPruneRange(const Chainstate& chainstat } int prune_start{0}; - if (this->GetAll().size() > 1 && m_snapshot_chainstate.get() == &chainstate) { - // Leave the blocks in the background IBD chain alone if we're pruning - // the snapshot chain. - prune_start = *Assert(GetSnapshotBaseHeight()) + 1; + if (chainstate.m_from_snapshot_blockhash && chainstate.m_assumeutxo != Assumeutxo::VALIDATED) { + // Only prune blocks _after_ the snapshot if this is a snapshot chain + // that has not been fully validated yet. The earlier blocks need to be + // kept to validate the snapshot + prune_start = Assert(chainstate.SnapshotBase())->nHeight + 1; } int max_prune = std::max( diff --git a/src/validation.h b/src/validation.h index e547f88c674..d2d9e4196d2 100644 --- a/src/validation.h +++ b/src/validation.h @@ -514,6 +514,16 @@ constexpr int64_t LargeCoinsCacheThreshold(int64_t total_space) noexcept total_space - MAX_BLOCK_COINSDB_USAGE_BYTES); } +//! Chainstate assumeutxo validity. +enum class Assumeutxo { + //! Every block in the chain has been validated. + VALIDATED, + //! Blocks after an assumeutxo snapshot have been validated but the snapshot itself has not been validated. + UNVALIDATED, + //! The assumeutxo snapshot failed validation. + INVALID, +}; + /** * Chainstate stores and provides an API to update our local knowledge of the * current best chain. @@ -545,19 +555,6 @@ protected: //! Manages the UTXO set, which is a reflection of the contents of `m_chain`. std::unique_ptr m_coins_views; - //! This toggle exists for use when doing background validation for UTXO - //! snapshots. - //! - //! In the expected case, it is set once the background validation chain reaches the - //! same height as the base of the snapshot and its UTXO set is found to hash to - //! the expected assumeutxo value. It signals that we should no longer connect - //! blocks to the background chainstate. When set on the background validation - //! chainstate, it signifies that we have fully validated the snapshot chainstate. - //! - //! In the unlikely case that the snapshot chainstate is found to be invalid, this - //! is set to true on the snapshot chainstate. - bool m_disabled GUARDED_BY(::cs_main) {false}; - //! Cached result of LookupBlockIndex(*m_from_snapshot_blockhash) mutable const CBlockIndex* m_cached_snapshot_base GUARDED_BY(::cs_main){nullptr}; @@ -616,6 +613,11 @@ public: //! @see CChain, CBlockIndex. CChain m_chain; + //! Assumeutxo state indicating whether all blocks in the chain were + //! validated, or if the chainstate is based on an assumeutxo snapshot and + //! the snapshot has not been validated. + Assumeutxo m_assumeutxo GUARDED_BY(::cs_main); + /** * The blockhash which is the base of the snapshot this chainstate was created from. * @@ -629,6 +631,10 @@ public: //! blocks up to the target block, not newer blocks. std::optional m_target_blockhash GUARDED_BY(::cs_main); + //! Hash of the UTXO set at the target block, computed when the chainstate + //! reaches the target block, and null before then. + std::optional m_target_utxohash GUARDED_BY(::cs_main); + /** * The base of the snapshot this chainstate was created from. * @@ -993,15 +999,6 @@ private: /** Most recent headers presync progress update, for rate-limiting. */ MockableSteadyClock::time_point m_last_presync_update GUARDED_BY(GetMutex()){}; - //! Return true if a chainstate is considered usable. - //! - //! This is false when a background validation chainstate has completed its - //! validation of an assumed-valid chainstate, or when a snapshot - //! chainstate has been found to be invalid. - bool IsUsable(const Chainstate* const cs) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { - return cs && !cs->m_disabled; - } - //! A queue for script verifications that have to be performed by worker threads. CCheckQueue m_script_check_queue; @@ -1150,7 +1147,7 @@ public: Chainstate* HistoricalChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { auto* cs{m_ibd_chainstate.get()}; - return IsUsable(cs) && cs->m_target_blockhash ? cs : nullptr; + return cs && cs->m_target_blockhash && !cs->m_target_utxohash ? cs : nullptr; } //! The most-work chain. @@ -1179,7 +1176,7 @@ public: //! Is there a snapshot in use and has it been fully validated? bool IsSnapshotValidated() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { - return m_snapshot_chainstate && m_ibd_chainstate && m_ibd_chainstate->m_disabled; + return m_snapshot_chainstate && m_snapshot_chainstate->m_assumeutxo == Assumeutxo::VALIDATED; } /** Check whether we are doing an initial block download (synchronizing from disk or network) */ From 1598a15aedb9fd9c4e4a671785ebebf56fc1e072 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 09:42:03 -0400 Subject: [PATCH 04/17] refactor: Deduplicate Chainstate activation code Move duplicate code from ChainstateManager::ActivateSnapshot and ChainstateManager::ActivateExistingSnapshot methods to a new ChainstateManager::AddChainstate method. The "AddChainstate" method name doesn't mention snapshots even though it is only used to add snapshot chainstates now, because it becomes more generalized in a later commit in this PR ("refactor: Add ChainstateManager::m_chainstates member") --- .../validation_chainstatemanager_tests.cpp | 7 ++-- src/validation.cpp | 34 ++++++------------- src/validation.h | 7 ++-- 3 files changed, 16 insertions(+), 32 deletions(-) diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index c6ae6088ec2..03cec59c3dc 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -69,7 +69,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup) // Create a snapshot-based chainstate. // const uint256 snapshot_blockhash = active_tip->GetBlockHash(); - Chainstate& c2 = WITH_LOCK(::cs_main, return manager.ActivateExistingSnapshot(snapshot_blockhash)); + Chainstate& c2{WITH_LOCK(::cs_main, return manager.AddChainstate(std::make_unique(nullptr, manager.m_blockman, manager, snapshot_blockhash)))}; chainstates.push_back(&c2); c2.InitCoinsDB( /*cache_size_bytes=*/1 << 23, /*in_memory=*/true, /*should_wipe=*/false); @@ -135,7 +135,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_rebalance_caches, TestChain100Setup) // Create a snapshot-based chainstate. // CBlockIndex* snapshot_base{WITH_LOCK(manager.GetMutex(), return manager.ActiveChain()[manager.ActiveChain().Height() / 2])}; - Chainstate& c2 = WITH_LOCK(cs_main, return manager.ActivateExistingSnapshot(*snapshot_base->phashBlock)); + Chainstate& c2{WITH_LOCK(::cs_main, return manager.AddChainstate(std::make_unique(nullptr, manager.m_blockman, manager, *snapshot_base->phashBlock)))}; chainstates.push_back(&c2); c2.InitCoinsDB( /*cache_size_bytes=*/1 << 23, /*in_memory=*/true, /*should_wipe=*/false); @@ -489,8 +489,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) } // Note: cs2's tip is not set when ActivateExistingSnapshot is called. - Chainstate& cs2 = WITH_LOCK(::cs_main, - return chainman.ActivateExistingSnapshot(*assumed_base->phashBlock)); + Chainstate& cs2{WITH_LOCK(::cs_main, return chainman.AddChainstate(std::make_unique(nullptr, chainman.m_blockman, chainman, *assumed_base->phashBlock)))}; // Set tip of the fully validated chain to be the validated tip cs1.m_chain.SetTip(*validated_tip); diff --git a/src/validation.cpp b/src/validation.cpp index 4c6432de84c..9e27da58dfd 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5796,28 +5796,14 @@ util::Result ChainstateManager::ActivateSnapshot( } } - assert(!m_snapshot_chainstate); - m_snapshot_chainstate.swap(snapshot_chainstate); - const bool chaintip_loaded = m_snapshot_chainstate->LoadChainTip(); + Chainstate& chainstate{AddChainstate(std::move(snapshot_chainstate))}; + const bool chaintip_loaded{chainstate.LoadChainTip()}; assert(chaintip_loaded); - - // Set snapshot block as the target block for the historical chainstate. - assert(m_ibd_chainstate.get()); - assert(!m_ibd_chainstate->m_target_blockhash); - m_ibd_chainstate->SetTargetBlockHash(base_blockhash); - - // Transfer possession of the mempool to the snapshot 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(); - m_blockman.m_snapshot_height = this->GetSnapshotBaseHeight(); + m_blockman.m_snapshot_height = Assert(chainstate.SnapshotBase())->nHeight; LogInfo("[snapshot] successfully activated snapshot %s", base_blockhash.ToString()); LogInfo("[snapshot] (%.2f MB)", - m_snapshot_chainstate->CoinsTip().DynamicMemoryUsage() / (1000 * 1000)); + chainstate.CoinsTip().DynamicMemoryUsage() / (1000 * 1000)); this->MaybeRebalanceCaches(); return snapshot_start_block; @@ -6288,21 +6274,21 @@ bool ChainstateManager::DetectSnapshotChainstate() LogInfo("[snapshot] detected active snapshot chainstate (%s) - loading", fs::PathToString(*path)); - this->ActivateExistingSnapshot(*base_blockhash); + auto snapshot_chainstate{std::make_unique(nullptr, m_blockman, *this, base_blockhash)}; + LogInfo("[snapshot] switching active chainstate to %s", snapshot_chainstate->ToString()); + this->AddChainstate(std::move(snapshot_chainstate)); return true; } -Chainstate& ChainstateManager::ActivateExistingSnapshot(uint256 base_blockhash) +Chainstate& ChainstateManager::AddChainstate(std::unique_ptr chainstate) { assert(!m_snapshot_chainstate); - m_snapshot_chainstate = - std::make_unique(nullptr, m_blockman, *this, base_blockhash); - LogInfo("[snapshot] switching active chainstate to %s", m_snapshot_chainstate->ToString()); + m_snapshot_chainstate = std::move(chainstate); // 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(base_blockhash); + m_ibd_chainstate->SetTargetBlockHash(*Assert(m_snapshot_chainstate->m_from_snapshot_blockhash)); // Transfer possession of the mempool to the chainstate. // Mempool is empty at this point because we're still in IBD. diff --git a/src/validation.h b/src/validation.h index d2d9e4196d2..5e6d11d1c28 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1315,16 +1315,15 @@ public: //! snapshot that is in the process of being validated. bool DetectSnapshotChainstate() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! Add new chainstate. + Chainstate& AddChainstate(std::unique_ptr chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + void ResetChainstates() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); //! Remove the snapshot-based chainstate and all on-disk artifacts. //! Used when reindex{-chainstate} is called during snapshot use. [[nodiscard]] bool DeleteSnapshotChainstate() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Switch the active chainstate to one based on a UTXO snapshot that was loaded - //! previously. - Chainstate& ActivateExistingSnapshot(uint256 base_blockhash) 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 //! validation of the snapshot. From 840bd2ef230ed0582fe33a90ec2636bfefa21709 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 31 May 2024 09:17:10 -0400 Subject: [PATCH 05/17] refactor: Pass chainstate parameters to MaybeCompleteSnapshotValidation Remove hardcoded references to m_ibd_chainstate and m_snapshot_chainstate so MaybeCompleteSnapshotValidation function can be simpler and focus on validating the snapshot without dealing with internal ChainstateManager states. This is a step towards being able to validate the snapshot outside of ActivateBestChain loop so cs_main is not locked for minutes when the snapshot block is connected. --- doc/design/assumeutxo.md | 2 +- src/node/chainstate.cpp | 9 +- src/node/utxo_snapshot.cpp | 2 +- src/node/utxo_snapshot.h | 2 +- src/test/fuzz/utxo_snapshot.cpp | 2 +- .../validation_chainstatemanager_tests.cpp | 20 +-- src/validation.cpp | 140 ++++++++---------- src/validation.h | 26 ++-- 8 files changed, 93 insertions(+), 110 deletions(-) diff --git a/doc/design/assumeutxo.md b/doc/design/assumeutxo.md index f1559cf4975..9f04a52be4e 100644 --- a/doc/design/assumeutxo.md +++ b/doc/design/assumeutxo.md @@ -63,7 +63,7 @@ chainstate and a sync to tip begins. A new chainstate directory is created in th datadir for the snapshot chainstate called `chainstate_snapshot`. When this directory is present in the datadir, the snapshot chainstate will be detected -and loaded as active on node startup (via `DetectSnapshotChainstate()`). +and loaded as active on node startup (via `LoadAssumeutxoChainstate()`). A special file is created within that directory, `base_blockhash`, which contains the serialized `uint256` of the base block of the snapshot. This is used to reinitialize diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 0cebcad336f..5a03e581626 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -169,15 +169,16 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize Chainstate& validated_cs{chainman.InitializeChainstate(options.mempool)}; // Load a chain created from a UTXO snapshot, if any exist. - bool has_snapshot = chainman.DetectSnapshotChainstate(); + Chainstate* assumeutxo_cs{chainman.LoadAssumeutxoChainstate()}; - if (has_snapshot && options.wipe_chainstate_db) { + if (assumeutxo_cs && options.wipe_chainstate_db) { // 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()) { return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Couldn't remove snapshot chainstate.")}; } + assumeutxo_cs = nullptr; } auto [init_status, init_error] = CompleteChainstateInitialization(chainman, options); @@ -193,7 +194,9 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize // snapshot is actually validated? Because this entails unusual // filesystem operations to move leveldb data directories around, and that seems // too risky to do in the middle of normal runtime. - auto snapshot_completion = chainman.MaybeCompleteSnapshotValidation(); + auto snapshot_completion{assumeutxo_cs + ? chainman.MaybeValidateSnapshot(validated_cs, *assumeutxo_cs) + : SnapshotCompletionResult::SKIPPED}; if (snapshot_completion == SnapshotCompletionResult::SKIPPED) { // do nothing; expected case diff --git a/src/node/utxo_snapshot.cpp b/src/node/utxo_snapshot.cpp index d6e878ae7e5..e159d9e0213 100644 --- a/src/node/utxo_snapshot.cpp +++ b/src/node/utxo_snapshot.cpp @@ -81,7 +81,7 @@ std::optional ReadSnapshotBaseBlockhash(fs::path chaindir) return base_blockhash; } -std::optional FindSnapshotChainstateDir(const fs::path& data_dir) +std::optional FindAssumeutxoChainstateDir(const fs::path& data_dir) { fs::path possible_dir = data_dir / fs::u8path(strprintf("chainstate%s", SNAPSHOT_CHAINSTATE_SUFFIX)); diff --git a/src/node/utxo_snapshot.h b/src/node/utxo_snapshot.h index e4eb6d60ad8..cc7caccc6ed 100644 --- a/src/node/utxo_snapshot.h +++ b/src/node/utxo_snapshot.h @@ -125,7 +125,7 @@ constexpr std::string_view SNAPSHOT_CHAINSTATE_SUFFIX = "_snapshot"; //! Return a path to the snapshot-based chainstate dir, if one exists. -std::optional FindSnapshotChainstateDir(const fs::path& data_dir); +std::optional FindAssumeutxoChainstateDir(const fs::path& data_dir); } // namespace node diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp index 56d3615570f..5b088af4f30 100644 --- a/src/test/fuzz/utxo_snapshot.cpp +++ b/src/test/fuzz/utxo_snapshot.cpp @@ -191,7 +191,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer) const auto* index{chainman.m_blockman.LookupBlockIndex(block->GetHash())}; Assert(index); Assert(index->nTx == 0); - if (index->nHeight == chainman.GetSnapshotBaseHeight()) { + if (index->nHeight == chainman.ActiveChainstate().SnapshotBase()->nHeight) { auto params{chainman.GetParams().AssumeutxoForHeight(index->nHeight)}; Assert(params.has_value()); Assert(params.value().m_chain_tx_count == index->m_chain_tx_count); diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 03cec59c3dc..036089c30f6 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -187,7 +187,7 @@ struct SnapshotTestSetup : TestChain100Setup { { LOCK(::cs_main); BOOST_CHECK(!chainman.IsSnapshotValidated()); - BOOST_CHECK(!node::FindSnapshotChainstateDir(chainman.m_options.datadir)); + BOOST_CHECK(!node::FindAssumeutxoChainstateDir(chainman.m_options.datadir)); } size_t initial_size; @@ -240,7 +240,7 @@ struct SnapshotTestSetup : TestChain100Setup { auto_infile >> coin; })); - BOOST_CHECK(!node::FindSnapshotChainstateDir(chainman.m_options.datadir)); + BOOST_CHECK(!node::FindAssumeutxoChainstateDir(chainman.m_options.datadir)); BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot( this, [](AutoFile& auto_infile, SnapshotMetadata& metadata) { @@ -264,7 +264,7 @@ struct SnapshotTestSetup : TestChain100Setup { })); BOOST_REQUIRE(CreateAndActivateUTXOSnapshot(this)); - BOOST_CHECK(fs::exists(*node::FindSnapshotChainstateDir(chainman.m_options.datadir))); + BOOST_CHECK(fs::exists(*node::FindAssumeutxoChainstateDir(chainman.m_options.datadir))); // Ensure our active chain is the snapshot chainstate. BOOST_CHECK(!chainman.ActiveChainstate().m_from_snapshot_blockhash->IsNull()); @@ -277,7 +277,7 @@ struct SnapshotTestSetup : TestChain100Setup { { LOCK(::cs_main); - fs::path found = *node::FindSnapshotChainstateDir(chainman.m_options.datadir); + fs::path found = *node::FindAssumeutxoChainstateDir(chainman.m_options.datadir); // Note: WriteSnapshotBaseBlockhash() is implicitly tested above. BOOST_CHECK_EQUAL( @@ -565,7 +565,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) this->SetupSnapshot(); - fs::path snapshot_chainstate_dir = *node::FindSnapshotChainstateDir(chainman.m_options.datadir); + fs::path snapshot_chainstate_dir = *node::FindAssumeutxoChainstateDir(chainman.m_options.datadir); BOOST_CHECK(fs::exists(snapshot_chainstate_dir)); BOOST_CHECK_EQUAL(snapshot_chainstate_dir, gArgs.GetDataDirNet() / "chainstate_snapshot"); @@ -639,13 +639,14 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup ChainstateManager& chainman = *Assert(m_node.chainman); Chainstate& active_cs = chainman.ActiveChainstate(); + Chainstate& validated_cs{*Assert(WITH_LOCK(cs_main, return chainman.HistoricalChainstate()))}; auto tip_cache_before_complete = active_cs.m_coinstip_cache_size_bytes; auto db_cache_before_complete = active_cs.m_coinsdb_cache_size_bytes; SnapshotCompletionResult res; m_node.notifications->m_shutdown_on_fatal_error = false; - fs::path snapshot_chainstate_dir = *node::FindSnapshotChainstateDir(chainman.m_options.datadir); + fs::path snapshot_chainstate_dir = *node::FindAssumeutxoChainstateDir(chainman.m_options.datadir); BOOST_CHECK(fs::exists(snapshot_chainstate_dir)); BOOST_CHECK_EQUAL(snapshot_chainstate_dir, gArgs.GetDataDirNet() / "chainstate_snapshot"); @@ -653,7 +654,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup const uint256 snapshot_tip_hash = WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip()->GetBlockHash()); - res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation()); + res = WITH_LOCK(::cs_main, return chainman.MaybeValidateSnapshot(validated_cs, active_cs)); BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SUCCESS); WITH_LOCK(::cs_main, BOOST_CHECK(chainman.IsSnapshotValidated())); @@ -669,7 +670,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup BOOST_CHECK_EQUAL(all_chainstates[0], &active_cs); // Trying completion again should return false. - res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation()); + res = WITH_LOCK(::cs_main, return chainman.MaybeValidateSnapshot(validated_cs, active_cs)); BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SKIPPED); // The invalid snapshot path should not have been used. @@ -720,6 +721,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna { auto chainstates = this->SetupSnapshot(); Chainstate& validation_chainstate = *std::get<0>(chainstates); + Chainstate& unvalidated_cs = *std::get<1>(chainstates); ChainstateManager& chainman = *Assert(m_node.chainman); SnapshotCompletionResult res; m_node.notifications->m_shutdown_on_fatal_error = false; @@ -740,7 +742,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna { ASSERT_DEBUG_LOG("failed to validate the -assumeutxo snapshot state"); - res = WITH_LOCK(::cs_main, return chainman.MaybeCompleteSnapshotValidation()); + res = WITH_LOCK(::cs_main, return chainman.MaybeValidateSnapshot(validation_chainstate, unvalidated_cs)); BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::HASH_MISMATCH); } diff --git a/src/validation.cpp b/src/validation.cpp index 9e27da58dfd..15981708f58 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3165,11 +3165,17 @@ bool Chainstate::ConnectTip( Ticks(m_chainman.time_total), Ticks(m_chainman.time_total) / m_chainman.num_blocks_total); - // If we are the background validation chainstate, check to see if we are done - // validating the snapshot (i.e. our tip has reached the snapshot's base block). - if (this != &m_chainman.ActiveChainstate()) { - m_chainman.MaybeCompleteSnapshotValidation(); - } + // See if this chainstate has reached a target block and can be used to + // validate an assumeutxo snapshot. If it can, hashing the UTXO database + // will be slow, and cs_main could remain locked here for several minutes. + // If the snapshot is validated, the UTXO hash will be saved to + // this->m_target_utxohash, causing HistoricalChainstate() to return null + // and this chainstate to no longer be used. ActivateBestChain() will also + // 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)}; + m_chainman.MaybeValidateSnapshot(*this, current_cs); connectTrace.BlockConnected(pindexNew, std::move(block_to_connect)); return true; @@ -3527,7 +3533,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< // the target block. // // This cannot be done while holding cs_main (within - // MaybeCompleteSnapshotValidation) or a cs_main deadlock will occur. + // MaybeValidateSnapshot) or a cs_main deadlock will occur. if (m_chainman.snapshot_download_completed) { m_chainman.snapshot_download_completed(); } @@ -5761,7 +5767,7 @@ util::Result ChainstateManager::ActivateSnapshot( // PopulateAndValidateSnapshot can return (in error) before the leveldb datadir // has been created, so only attempt removal if we got that far. - if (auto snapshot_datadir = node::FindSnapshotChainstateDir(m_options.datadir)) { + if (auto snapshot_datadir = node::FindAssumeutxoChainstateDir(m_options.datadir)) { // We have to destruct leveldb::DB in order to release the db lock, otherwise // DestroyDB() (in DeleteCoinsDBFromDisk()) will fail. See `leveldb::~DBImpl()`. // Destructing the chainstate (and so resetting the coinsviews object) does this. @@ -6037,45 +6043,35 @@ util::Result ChainstateManager::PopulateAndValidateSnapshot( } // Currently, this function holds cs_main for its duration, which could be for -// multiple minutes due to the ComputeUTXOStats call. This hold is necessary -// because we need to avoid advancing the background validation chainstate -// farther than the snapshot base block - and this function is also invoked -// from within ConnectTip, i.e. from within ActivateBestChain, so cs_main is -// held anyway. +// multiple minutes due to the ComputeUTXOStats call. Holding cs_main used to be +// necessary (before d43a1f1a2fa3) to avoid advancing validated_cs farther than +// its target block. Now it should be possible to avoid this, but simply +// releasing cs_main here would not be possible because this function is invoked +// by ConnectTip within ActivateBestChain. // -// Eventually (TODO), we could somehow separate this function's runtime from -// maintenance of the active chain, but that will either require -// -// (i) setting `m_disabled` immediately and ensuring all chainstate accesses go -// through IsUsable() checks, or -// -// (ii) giving each chainstate its own lock instead of using cs_main for everything. -SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() +// Eventually (TODO) it would be better to call this function outside of +// ActivateBestChain, on a separate thread that should not require cs_main to +// hash, because the UTXO set is only hashed after the historical chainstate +// reaches its target block and is no longer changing. +SnapshotCompletionResult ChainstateManager::MaybeValidateSnapshot(Chainstate& validated_cs, Chainstate& unvalidated_cs) { AssertLockHeld(cs_main); - // If a snapshot does not need to be validated... - if (m_ibd_chainstate.get() == &this->ActiveChainstate() || + // If the snapshot does not need to be validated... + if (unvalidated_cs.m_assumeutxo != Assumeutxo::UNVALIDATED || // Or if either chainstate is unusable... - !m_snapshot_chainstate || - m_snapshot_chainstate->m_assumeutxo != Assumeutxo::UNVALIDATED || - !m_snapshot_chainstate->m_from_snapshot_blockhash || - !m_ibd_chainstate || - m_ibd_chainstate->m_assumeutxo != Assumeutxo::VALIDATED || - !m_ibd_chainstate->m_chain.Tip() || + !unvalidated_cs.m_from_snapshot_blockhash || + validated_cs.m_assumeutxo != Assumeutxo::VALIDATED || + !validated_cs.m_chain.Tip() || // Or the validated chainstate is not targeting the snapshot block... - !m_ibd_chainstate->m_target_blockhash || - *m_ibd_chainstate->m_target_blockhash != *m_snapshot_chainstate->m_from_snapshot_blockhash || + !validated_cs.m_target_blockhash || + *validated_cs.m_target_blockhash != *unvalidated_cs.m_from_snapshot_blockhash || // Or the validated chainstate has not reached the snapshot block yet... - !m_ibd_chainstate->ReachedTarget()) { - // Then a snapshot cannot be validated and there is nothing to do. + !validated_cs.ReachedTarget()) { + // Then the snapshot cannot be validated and there is nothing to do. return SnapshotCompletionResult::SKIPPED; } - const int snapshot_tip_height = this->ActiveHeight(); - const int snapshot_base_height = *Assert(this->GetSnapshotBaseHeight()); - const CBlockIndex& index_new = *Assert(m_ibd_chainstate->m_chain.Tip()); - assert(SnapshotBlockhash()); - uint256 snapshot_blockhash = *Assert(SnapshotBlockhash()); + assert(validated_cs.TargetBlock() == validated_cs.m_chain.Tip()); auto handle_invalid_snapshot = [&]() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { bilingual_str user_error = strprintf(_( @@ -6090,19 +6086,20 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() "Please report this incident to %s, including how you obtained the snapshot. " "The invalid snapshot chainstate will be left on disk in case it is " "helpful in diagnosing the issue that caused this error."), - CLIENT_NAME, snapshot_tip_height, snapshot_base_height, snapshot_base_height, CLIENT_BUGREPORT - ); + CLIENT_NAME, unvalidated_cs.m_chain.Height(), + validated_cs.m_chain.Height(), + validated_cs.m_chain.Height(), CLIENT_BUGREPORT); LogError("[snapshot] !!! %s\n", user_error.original); LogError("[snapshot] deleting snapshot, reverting to validated chain, and stopping node\n"); // Reset chainstate target to network tip instead of snapshot block. - m_ibd_chainstate->SetTargetBlock(nullptr); + validated_cs.SetTargetBlock(nullptr); - m_active_chainstate = m_ibd_chainstate.get(); - m_snapshot_chainstate->m_assumeutxo = Assumeutxo::INVALID; + m_active_chainstate = &validated_cs; + unvalidated_cs.m_assumeutxo = Assumeutxo::INVALID; - auto rename_result = m_snapshot_chainstate->InvalidateCoinsDBOnDisk(); + auto rename_result = unvalidated_cs.InvalidateCoinsDBOnDisk(); if (!rename_result) { user_error += Untranslated("\n") + util::ErrorString(rename_result); } @@ -6110,34 +6107,25 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() GetNotifications().fatalError(user_error); }; - assert(index_new.GetBlockHash() == snapshot_blockhash); - assert(index_new.nHeight == snapshot_base_height); + CCoinsViewDB& validated_coins_db = validated_cs.CoinsDB(); + validated_cs.ForceFlushStateToDisk(); - int curr_height = m_ibd_chainstate->m_chain.Height(); - - assert(snapshot_base_height == curr_height); - assert(snapshot_base_height == index_new.nHeight); - assert(this->GetAll().size() == 2); - - CCoinsViewDB& ibd_coins_db = m_ibd_chainstate->CoinsDB(); - m_ibd_chainstate->ForceFlushStateToDisk(); - - const auto& maybe_au_data = m_options.chainparams.AssumeutxoForHeight(curr_height); + const auto& maybe_au_data = m_options.chainparams.AssumeutxoForHeight(validated_cs.m_chain.Height()); if (!maybe_au_data) { LogWarning("[snapshot] assumeutxo data not found for height " - "(%d) - refusing to validate snapshot", curr_height); + "(%d) - refusing to validate snapshot", validated_cs.m_chain.Height()); handle_invalid_snapshot(); return SnapshotCompletionResult::MISSING_CHAINPARAMS; } const AssumeutxoData& au_data = *maybe_au_data; - std::optional maybe_ibd_stats; + std::optional validated_cs_stats; LogInfo("[snapshot] computing UTXO stats for background chainstate to validate " "snapshot - this could take a few minutes"); try { - maybe_ibd_stats = ComputeUTXOStats( + validated_cs_stats = ComputeUTXOStats( CoinStatsHashType::HASH_SERIALIZED, - &ibd_coins_db, + &validated_coins_db, m_blockman, [&interrupt = m_interrupt] { SnapshotUTXOHashBreakpoint(interrupt); }); } catch (StopHashingException const&) { @@ -6145,7 +6133,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() } // XXX note that this function is slow and will hold cs_main for potentially minutes. - if (!maybe_ibd_stats) { + if (!validated_cs_stats) { LogWarning("[snapshot] failed to generate stats for validation coins db"); // While this isn't a problem with the snapshot per se, this condition // prevents us from validating the snapshot, so we should shut down and let the @@ -6153,7 +6141,6 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() handle_invalid_snapshot(); return SnapshotCompletionResult::STATS_FAILED; } - const auto& ibd_stats = *maybe_ibd_stats; // Compare the background validation chainstate's UTXO set hash against the hard-coded // assumeutxo hash we expect. @@ -6161,19 +6148,19 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation() // TODO: For belt-and-suspenders, we could cache the UTXO set // hash for the snapshot when it's loaded in its chainstate's leveldb. We could then // reference that here for an additional check. - if (AssumeutxoHash{ibd_stats.hashSerialized} != au_data.hash_serialized) { + if (AssumeutxoHash{validated_cs_stats->hashSerialized} != au_data.hash_serialized) { LogWarning("[snapshot] hash mismatch: actual=%s, expected=%s", - ibd_stats.hashSerialized.ToString(), + validated_cs_stats->hashSerialized.ToString(), au_data.hash_serialized.ToString()); handle_invalid_snapshot(); return SnapshotCompletionResult::HASH_MISMATCH; } LogInfo("[snapshot] snapshot beginning at %s has been fully validated", - snapshot_blockhash.ToString()); + unvalidated_cs.m_from_snapshot_blockhash->ToString()); - m_snapshot_chainstate->m_assumeutxo = Assumeutxo::VALIDATED; - m_ibd_chainstate->m_target_utxohash = AssumeutxoHash{ibd_stats.hashSerialized}; + unvalidated_cs.m_assumeutxo = Assumeutxo::VALIDATED; + validated_cs.m_target_utxohash = AssumeutxoHash{validated_cs_stats->hashSerialized}; this->MaybeRebalanceCaches(); return SnapshotCompletionResult::SUCCESS; @@ -6260,24 +6247,23 @@ ChainstateManager::~ChainstateManager() m_versionbitscache.Clear(); } -bool ChainstateManager::DetectSnapshotChainstate() +Chainstate* ChainstateManager::LoadAssumeutxoChainstate() { assert(!m_snapshot_chainstate); - std::optional path = node::FindSnapshotChainstateDir(m_options.datadir); + std::optional path = node::FindAssumeutxoChainstateDir(m_options.datadir); if (!path) { - return false; + return nullptr; } std::optional base_blockhash = node::ReadSnapshotBaseBlockhash(*path); if (!base_blockhash) { - return false; + return nullptr; } LogInfo("[snapshot] detected active snapshot chainstate (%s) - loading", fs::PathToString(*path)); auto snapshot_chainstate{std::make_unique(nullptr, m_blockman, *this, base_blockhash)}; LogInfo("[snapshot] switching active chainstate to %s", snapshot_chainstate->ToString()); - this->AddChainstate(std::move(snapshot_chainstate)); - return true; + return &this->AddChainstate(std::move(snapshot_chainstate)); } Chainstate& ChainstateManager::AddChainstate(std::unique_ptr chainstate) @@ -6337,7 +6323,7 @@ util::Result Chainstate::InvalidateCoinsDBOnDisk() // The invalid snapshot datadir is simply moved and not deleted because we may // want to do forensics later during issue investigation. The user is instructed - // accordingly in MaybeCompleteSnapshotValidation(). + // accordingly in MaybeValidateSnapshot(). try { fs::rename(snapshot_datadir, invalid_path); } catch (const fs::filesystem_error& e) { @@ -6362,7 +6348,7 @@ bool ChainstateManager::DeleteSnapshotChainstate() Assert(m_snapshot_chainstate); Assert(m_ibd_chainstate); - fs::path snapshot_datadir = Assert(node::FindSnapshotChainstateDir(m_options.datadir)).value(); + fs::path snapshot_datadir = Assert(node::FindAssumeutxoChainstateDir(m_options.datadir)).value(); if (!DeleteCoinsDBFromDisk(snapshot_datadir, /*is_snapshot=*/ true)) { LogError("Deletion of %s failed. Please remove it manually to continue reindexing.", fs::PathToString(snapshot_datadir)); @@ -6384,12 +6370,6 @@ const CBlockIndex* ChainstateManager::GetSnapshotBaseBlock() const return m_active_chainstate ? m_active_chainstate->SnapshotBase() : nullptr; } -std::optional ChainstateManager::GetSnapshotBaseHeight() const -{ - const CBlockIndex* base = this->GetSnapshotBaseBlock(); - return base ? std::make_optional(base->nHeight) : std::nullopt; -} - void ChainstateManager::RecalculateBestHeader() { AssertLockHeld(cs_main); diff --git a/src/validation.h b/src/validation.h index 5e6d11d1c28..6fe31117034 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1131,14 +1131,15 @@ public: [[nodiscard]] util::Result ActivateSnapshot( AutoFile& coins_file, const node::SnapshotMetadata& metadata, bool in_memory); - //! Once the background validation chainstate has reached the height which - //! is the base of the UTXO snapshot in use, compare its coins to ensure - //! they match those expected by the snapshot. - //! - //! If the coins match (expected), then mark the validation chainstate for - //! deletion and continue using the snapshot chainstate as active. - //! Otherwise, revert to using the ibd chainstate and shutdown. - SnapshotCompletionResult MaybeCompleteSnapshotValidation() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! Try to validate an assumeutxo snapshot by using a validated historical + //! chainstate targeted at the snapshot block. When the target block is + //! reached, the UTXO hash is computed and saved to + //! `validated_cs.m_target_utxohash`, and `unvalidated_cs.m_assumeutxo` will + //! be updated from UNVALIDATED to either VALIDATED or INVALID depending on + //! whether the hash matches. The INVALID case should not happen in practice + //! because the software should refuse to load unrecognized snapshots, but + //! 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); @@ -1312,8 +1313,9 @@ public: void ReportHeadersPresync(const arith_uint256& work, int64_t height, int64_t timestamp); //! When starting up, search the datadir for a chainstate based on a UTXO - //! snapshot that is in the process of being validated. - bool DetectSnapshotChainstate() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! snapshot that is in the process of being validated and load it if found. + //! Return pointer to the Chainstate if it is loaded. + Chainstate* LoadAssumeutxoChainstate() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); //! Add new chainstate. Chainstate& AddChainstate(std::unique_ptr chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); @@ -1351,10 +1353,6 @@ public: std::pair GetPruneRange( const Chainstate& chainstate, int last_height_can_prune) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Return the height of the base block of the snapshot in use, if one exists, else - //! nullopt. - std::optional GetSnapshotBaseHeight() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Get range of historical blocks to download. std::optional> GetHistoricalBlockRange() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); From a9b7f5614c24fe6f386448604c325ec4fa6c98a5 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 11:25:04 -0400 Subject: [PATCH 06/17] refactor: Add Chainstate::StoragePath() method Use to simplify code determining the chainstate leveldb paths. New method is the now the only code that needs to figure out the storage path, so the path doesn't need to be constructed multiple places and backed out of leveldb. --- src/dbwrapper.cpp | 2 +- src/dbwrapper.h | 14 ------ src/node/chainstate.cpp | 2 +- src/node/utxo_snapshot.cpp | 2 +- src/test/util/chainstate.h | 2 +- src/txdb.h | 3 -- src/validation.cpp | 98 ++++++++++++++------------------------ src/validation.h | 11 ++--- 8 files changed, 45 insertions(+), 89 deletions(-) diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp index 608f61f07c6..b3f08cb20d6 100644 --- a/src/dbwrapper.cpp +++ b/src/dbwrapper.cpp @@ -214,7 +214,7 @@ struct LevelDBContext { }; CDBWrapper::CDBWrapper(const DBParams& params) - : m_db_context{std::make_unique()}, m_name{fs::PathToString(params.path.stem())}, m_path{params.path}, m_is_memory{params.memory_only} + : m_db_context{std::make_unique()}, m_name{fs::PathToString(params.path.stem())} { DBContext().penv = nullptr; DBContext().readoptions.verify_checksums = true; diff --git a/src/dbwrapper.h b/src/dbwrapper.h index c770df8e206..b2ce67c7c2e 100644 --- a/src/dbwrapper.h +++ b/src/dbwrapper.h @@ -191,12 +191,6 @@ private: //! obfuscation key storage key, null-prefixed to avoid collisions inline static const std::string OBFUSCATION_KEY{"\000obfuscate_key", 14}; // explicit size to avoid truncation at leading \0 - //! path to filesystem storage - const fs::path m_path; - - //! whether or not the database resides in memory - bool m_is_memory; - std::optional ReadImpl(std::span key) const; bool ExistsImpl(std::span key) const; size_t EstimateSizeImpl(std::span key1, std::span key2) const; @@ -237,14 +231,6 @@ public: WriteBatch(batch, fSync); } - //! @returns filesystem path to the on-disk data. - std::optional StoragePath() { - if (m_is_memory) { - return {}; - } - return m_path; - } - template bool Exists(const K& key) const { diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 5a03e581626..bba518d1825 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -202,7 +202,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize // do nothing; expected case } else if (snapshot_completion == SnapshotCompletionResult::SUCCESS) { LogInfo("[snapshot] cleaning up unneeded background chainstate, then reinitializing"); - if (!chainman.ValidatedSnapshotCleanup()) { + if (!chainman.ValidatedSnapshotCleanup(validated_cs, *assumeutxo_cs)) { return {ChainstateLoadStatus::FAILURE_FATAL, Untranslated("Background chainstate cleanup failed unexpectedly.")}; } diff --git a/src/node/utxo_snapshot.cpp b/src/node/utxo_snapshot.cpp index e159d9e0213..abd7bcc19b6 100644 --- a/src/node/utxo_snapshot.cpp +++ b/src/node/utxo_snapshot.cpp @@ -25,7 +25,7 @@ bool WriteSnapshotBaseBlockhash(Chainstate& snapshot_chainstate) AssertLockHeld(::cs_main); assert(snapshot_chainstate.m_from_snapshot_blockhash); - const std::optional chaindir = snapshot_chainstate.CoinsDB().StoragePath(); + const std::optional chaindir = snapshot_chainstate.StoragePath(); assert(chaindir); // Sanity check that chainstate isn't in-memory. const fs::path write_to = *chaindir / node::SNAPSHOT_BLOCKHASH_FILENAME; diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h index aac7816e0e3..2d5919c2a8f 100644 --- a/src/test/util/chainstate.h +++ b/src/test/util/chainstate.h @@ -81,7 +81,7 @@ CreateAndActivateUTXOSnapshot( Chainstate& chain = node.chainman->ActiveChainstate(); Assert(chain.LoadGenesisBlock()); // These cache values will be corrected shortly in `MaybeRebalanceCaches`. - chain.InitCoinsDB(1 << 20, true, false, ""); + chain.InitCoinsDB(1 << 20, /*in_memory=*/true, /*should_wipe=*/false); chain.InitCoinsCache(1 << 20); chain.CoinsTip().SetBestBlock(gen_hash); chain.setBlockIndexCandidates.insert(node.chainman->m_blockman.LookupBlockIndex(gen_hash)); diff --git a/src/txdb.h b/src/txdb.h index ea0cf9d77e5..89e94473333 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -53,9 +53,6 @@ public: //! Dynamically alter the underlying leveldb cache size. void ResizeCache(size_t new_cache_size) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - - //! @returns filesystem path to on-disk storage or std::nullopt if in memory. - std::optional StoragePath() { return m_db->StoragePath(); } }; #endif // BITCOIN_TXDB_H diff --git a/src/validation.cpp b/src/validation.cpp index 15981708f58..a9bf12b5495 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1885,6 +1885,15 @@ Chainstate::Chainstate( m_assumeutxo(from_snapshot_blockhash ? Assumeutxo::UNVALIDATED : Assumeutxo::VALIDATED), m_from_snapshot_blockhash(from_snapshot_blockhash) {} +fs::path Chainstate::StoragePath() const +{ + fs::path path{m_chainman.m_options.datadir / "chainstate"}; + if (m_from_snapshot_blockhash) { + path += node::SNAPSHOT_CHAINSTATE_SUFFIX; + } + return path; +} + const CBlockIndex* Chainstate::SnapshotBase() const { if (!m_from_snapshot_blockhash) return nullptr; @@ -1918,16 +1927,11 @@ void Chainstate::SetTargetBlockHash(uint256 block_hash) void Chainstate::InitCoinsDB( size_t cache_size_bytes, bool in_memory, - bool should_wipe, - fs::path leveldb_name) + bool should_wipe) { - if (m_from_snapshot_blockhash) { - leveldb_name += node::SNAPSHOT_CHAINSTATE_SUFFIX; - } - m_coins_views = std::make_unique( DBParams{ - .path = m_chainman.m_options.datadir / leveldb_name, + .path = StoragePath(), .cache_bytes = cache_size_bytes, .memory_only = in_memory, .wipe_data = should_wipe, @@ -5757,7 +5761,7 @@ util::Result ChainstateManager::ActivateSnapshot( LOCK(::cs_main); snapshot_chainstate->InitCoinsDB( static_cast(current_coinsdb_cache_size * SNAPSHOT_CACHE_PERC), - in_memory, false, "chainstate"); + in_memory, /*should_wipe=*/false); snapshot_chainstate->InitCoinsCache( static_cast(current_coinstip_cache_size * SNAPSHOT_CACHE_PERC)); } @@ -6298,46 +6302,34 @@ bool IsBIP30Unspendable(const uint256& block_hash, int block_height) (block_height==91812 && block_hash == uint256{"00000000000af0aed4792b1acee3d966af36cf5def14935db8de83d6f9306f2f"}); } -static fs::path GetSnapshotCoinsDBPath(Chainstate& cs) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) -{ - AssertLockHeld(::cs_main); - // Should never be called on a non-snapshot chainstate. - assert(cs.m_from_snapshot_blockhash); - auto storage_path_maybe = cs.CoinsDB().StoragePath(); - // Should never be called with a non-existent storage path. - assert(storage_path_maybe); - return *storage_path_maybe; -} - util::Result Chainstate::InvalidateCoinsDBOnDisk() { - fs::path snapshot_datadir = GetSnapshotCoinsDBPath(*this); + // Should never be called on a non-snapshot chainstate. + assert(m_from_snapshot_blockhash); // Coins views no longer usable. m_coins_views.reset(); - auto invalid_path = snapshot_datadir + "_INVALID"; - std::string dbpath = fs::PathToString(snapshot_datadir); - std::string target = fs::PathToString(invalid_path); - LogInfo("[snapshot] renaming snapshot datadir %s to %s", dbpath, target); + const fs::path db_path{StoragePath()}; + const fs::path invalid_path{db_path + "_INVALID"}; + const std::string db_path_str{fs::PathToString(db_path)}; + const std::string invalid_path_str{fs::PathToString(invalid_path)}; + LogInfo("[snapshot] renaming snapshot datadir %s to %s", db_path_str, invalid_path_str); - // The invalid snapshot datadir is simply moved and not deleted because we may + // The invalid storage directory is simply moved and not deleted because we may // want to do forensics later during issue investigation. The user is instructed // accordingly in MaybeValidateSnapshot(). try { - fs::rename(snapshot_datadir, invalid_path); + fs::rename(db_path, invalid_path); } catch (const fs::filesystem_error& e) { - auto src_str = fs::PathToString(snapshot_datadir); - auto dest_str = fs::PathToString(invalid_path); - LogError("While invalidating the coins db: Error renaming file '%s' -> '%s': %s", - src_str, dest_str, e.what()); + db_path_str, invalid_path_str, e.what()); return util::Error{strprintf(_( "Rename of '%s' -> '%s' failed. " "You should resolve this by manually moving or deleting the invalid " "snapshot directory %s, otherwise you will encounter the same error again " "on the next startup."), - src_str, dest_str, src_str)}; + db_path_str, invalid_path_str, db_path_str)}; } return {}; } @@ -6381,33 +6373,17 @@ void ChainstateManager::RecalculateBestHeader() } } -bool ChainstateManager::ValidatedSnapshotCleanup() +bool ChainstateManager::ValidatedSnapshotCleanup(Chainstate& validated_cs, Chainstate& unvalidated_cs) { AssertLockHeld(::cs_main); - auto get_storage_path = [](auto& chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) -> std::optional { - if (!(chainstate && chainstate->HasCoinsViews())) { - return {}; - } - return chainstate->CoinsDB().StoragePath(); - }; - std::optional ibd_chainstate_path_maybe = get_storage_path(m_ibd_chainstate); - std::optional snapshot_chainstate_path_maybe = get_storage_path(m_snapshot_chainstate); - if (!this->IsSnapshotValidated()) { // No need to clean up. return false; } - // If either path doesn't exist, that means at least one of the chainstates - // is in-memory, in which case we can't do on-disk cleanup. You'd better be - // in a unittest! - if (!ibd_chainstate_path_maybe || !snapshot_chainstate_path_maybe) { - LogError("[snapshot] snapshot chainstate cleanup cannot happen with " - "in-memory chainstates. You are testing, right?"); - return false; - } - const auto& snapshot_chainstate_path = *snapshot_chainstate_path_maybe; - const auto& ibd_chainstate_path = *ibd_chainstate_path_maybe; + const fs::path validated_path{validated_cs.StoragePath()}; + const fs::path assumed_valid_path{unvalidated_cs.StoragePath()}; + const fs::path delete_path{validated_path + "_todelete"}; // Since we're going to be moving around the underlying leveldb filesystem content // for each chainstate, make sure that the chainstates (and their constituent @@ -6421,9 +6397,7 @@ bool ChainstateManager::ValidatedSnapshotCleanup() assert(this->GetAll().size() == 0); LogInfo("[snapshot] deleting background chainstate directory (now unnecessary) (%s)", - fs::PathToString(ibd_chainstate_path)); - - fs::path tmp_old{ibd_chainstate_path + "_todelete"}; + fs::PathToString(validated_path)); auto rename_failed_abort = [this]( fs::path p_old, @@ -6438,32 +6412,32 @@ bool ChainstateManager::ValidatedSnapshotCleanup() }; try { - fs::rename(ibd_chainstate_path, tmp_old); + fs::rename(validated_path, delete_path); } catch (const fs::filesystem_error& e) { - rename_failed_abort(ibd_chainstate_path, tmp_old, e); + rename_failed_abort(validated_path, delete_path, e); throw; } LogInfo("[snapshot] moving snapshot chainstate (%s) to " "default chainstate directory (%s)", - fs::PathToString(snapshot_chainstate_path), fs::PathToString(ibd_chainstate_path)); + fs::PathToString(assumed_valid_path), fs::PathToString(validated_path)); try { - fs::rename(snapshot_chainstate_path, ibd_chainstate_path); + fs::rename(assumed_valid_path, validated_path); } catch (const fs::filesystem_error& e) { - rename_failed_abort(snapshot_chainstate_path, ibd_chainstate_path, e); + rename_failed_abort(assumed_valid_path, validated_path, e); throw; } - if (!DeleteCoinsDBFromDisk(tmp_old, /*is_snapshot=*/false)) { + if (!DeleteCoinsDBFromDisk(delete_path, /*is_snapshot=*/false)) { // No need to FatalError because once the unneeded bg chainstate data is // moved, it will not interfere with subsequent initialization. LogWarning("Deletion of %s failed. Please remove it manually, as the " "directory is now unnecessary.", - fs::PathToString(tmp_old)); + fs::PathToString(delete_path)); } else { LogInfo("[snapshot] deleted background chainstate directory (%s)", - fs::PathToString(ibd_chainstate_path)); + fs::PathToString(validated_path)); } return true; } diff --git a/src/validation.h b/src/validation.h index 6fe31117034..37c6051e5b6 100644 --- a/src/validation.h +++ b/src/validation.h @@ -579,6 +579,9 @@ public: ChainstateManager& chainman, std::optional from_snapshot_blockhash = std::nullopt); + //! Return path to chainstate leveldb directory. + fs::path StoragePath() const; + //! Return the current role of the chainstate. See `ChainstateManager` //! documentation for a description of the different types of chainstates. //! @@ -594,8 +597,7 @@ public: void InitCoinsDB( size_t cache_size_bytes, bool in_memory, - bool should_wipe, - fs::path leveldb_name = "chainstate"); + bool should_wipe); //! Initialize the in-memory coins cache (to be done after the health of the on-disk database //! is verified). @@ -703,9 +705,6 @@ public: //! Destructs all objects related to accessing the UTXO set. void ResetCoinsViews() { m_coins_views.reset(); } - //! Does this chainstate have a UTXO set attached? - bool HasCoinsViews() const { return (bool)m_coins_views; } - //! The cache size of the on-disk coins view. size_t m_coinsdb_cache_size_bytes{0}; @@ -1335,7 +1334,7 @@ public: //! directories are moved or deleted. //! //! @sa node/chainstate:LoadChainstate() - bool ValidatedSnapshotCleanup() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + bool ValidatedSnapshotCleanup(Chainstate& validated_cs, Chainstate& unvalidated_cs) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); //! @returns the chainstate that indexes should consult when ensuring that an //! index is synced with a chain where we can expect block index entries to have From a229cb9477e6622087241be7a105551d1329503b Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 12:03:47 -0400 Subject: [PATCH 07/17] 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) { From 352ad27fc1b1b350c8dbeb26a9813b01025cad31 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 12:03:47 -0400 Subject: [PATCH 08/17] refactor: Add ChainstateManager::ValidatedChainstate() method ValidatedChainstate() accessor replaces GetChainstateForIndexing() with no change in behavior. --- src/index/base.cpp | 2 +- src/init.cpp | 2 +- src/validation.cpp | 8 -------- src/validation.h | 21 +++++++++++---------- 4 files changed, 13 insertions(+), 20 deletions(-) diff --git a/src/index/base.cpp b/src/index/base.cpp index 6d4248047c2..a94b4ca7225 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -109,7 +109,7 @@ bool BaseIndex::Init() // m_chainstate member gives indexing code access to node internals. It is // removed in followup https://github.com/bitcoin/bitcoin/pull/24230 m_chainstate = WITH_LOCK(::cs_main, - return &m_chain->context()->chainman->GetChainstateForIndexing()); + return &m_chain->context()->chainman->ValidatedChainstate()); // Register to validation interface before setting the 'm_synced' flag, so that // callbacks are not missed once m_synced is true. m_chain->context()->validation_signals->RegisterValidationInterface(this); diff --git a/src/init.cpp b/src/init.cpp index 593467e3439..9c710d69152 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -2229,7 +2229,7 @@ bool StartIndexBackgroundSync(NodeContext& node) std::optional indexes_start_block; std::string older_index_name; ChainstateManager& chainman = *Assert(node.chainman); - const Chainstate& chainstate = WITH_LOCK(::cs_main, return chainman.GetChainstateForIndexing()); + const Chainstate& chainstate = WITH_LOCK(::cs_main, return chainman.ValidatedChainstate()); const CChain& index_chain = chainstate.m_chain; for (auto index : node.indexes) { diff --git a/src/validation.cpp b/src/validation.cpp index fabff80c587..da60efbc3cf 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -6437,14 +6437,6 @@ bool ChainstateManager::ValidatedSnapshotCleanup(Chainstate& validated_cs, Chain return true; } -Chainstate& ChainstateManager::GetChainstateForIndexing() -{ - // We can't always return `m_ibd_chainstate` because after background validation - // has completed, `m_snapshot_chainstate == m_active_chainstate`, but it can be - // indexed. - return (this->GetAll().size() > 1) ? *m_ibd_chainstate : *m_active_chainstate; -} - std::pair ChainstateManager::GetPruneRange(const Chainstate& chainstate, int last_height_can_prune) { if (chainstate.m_chain.Height() <= 0) { diff --git a/src/validation.h b/src/validation.h index cafe24934be..464dcbd03a7 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1153,6 +1153,17 @@ public: return cs && cs->m_target_blockhash && !cs->m_target_utxohash ? cs : nullptr; } + //! Return fully validated chainstate that should be used for indexing, to + //! support indexes that need to index blocks in order and can't start from + //! the snapshot block. + Chainstate& ValidatedChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) + { + for (auto* cs : {&CurrentChainstate(), HistoricalChainstate()}) { + if (cs && cs->m_assumeutxo == Assumeutxo::VALIDATED) return *cs; + } + abort(); + } + //! 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 @@ -1344,16 +1355,6 @@ public: //! @sa node/chainstate:LoadChainstate() bool ValidatedSnapshotCleanup(Chainstate& validated_cs, Chainstate& unvalidated_cs) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! @returns the chainstate that indexes should consult when ensuring that an - //! index is synced with a chain where we can expect block index entries to have - //! BLOCK_HAVE_DATA beneath the tip. - //! - //! In other words, give us the chainstate for which we can reasonably expect - //! that all blocks beneath the tip have been indexed. In practice this means - //! when using an assumed-valid chainstate based upon a snapshot, return only the - //! fully validated chain. - Chainstate& GetChainstateForIndexing() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Return the [start, end] (inclusive) of block heights we can prune. //! //! start > end is possible, meaning no blocks can be pruned. From 4dfe383912761669a968f8535ed43437da160ec8 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 18 Jun 2025 09:51:34 -0400 Subject: [PATCH 09/17] refactor: Convert ChainstateRole enum to struct Change ChainstateRole parameter passed to wallets and indexes. Wallets and indexes need to know whether chainstate is historical and whether it is fully validated. They should not be aware of the assumeutxo snapshot validation process. --- src/bench/wallet_create_tx.cpp | 4 +++- src/bench/wallet_migration.cpp | 1 + src/index/base.cpp | 18 ++++++++-------- src/index/base.h | 4 ++-- src/interfaces/chain.h | 8 ++++--- src/kernel/bitcoinkernel.cpp | 3 ++- src/kernel/chain.cpp | 16 ++++++++------ src/kernel/chain.h | 20 ++--------------- src/kernel/types.h | 30 ++++++++++++++++++++++++++ src/net_processing.cpp | 11 +++++----- src/node/blockstorage.cpp | 2 ++ src/node/interfaces.cpp | 6 ++++-- src/test/chainstate_write_tests.cpp | 6 ++++-- src/test/coinstatsindex_tests.cpp | 5 ++++- src/test/util/validation.cpp | 11 ++++++---- src/test/util/validation.h | 2 +- src/test/validation_block_tests.cpp | 3 ++- src/test/validationinterface_tests.cpp | 1 - src/validation.cpp | 12 ++++++----- src/validation.h | 5 ++++- src/validationinterface.cpp | 10 ++++++--- src/validationinterface.h | 12 ++++++----- src/wallet/wallet.cpp | 7 +++--- src/wallet/wallet.h | 2 +- src/zmq/zmqnotificationinterface.cpp | 8 ++++--- src/zmq/zmqnotificationinterface.h | 2 +- 26 files changed, 130 insertions(+), 79 deletions(-) create mode 100644 src/kernel/types.h diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp index 7fb535f4c72..73ad0e7e568 100644 --- a/src/bench/wallet_create_tx.cpp +++ b/src/bench/wallet_create_tx.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -39,6 +40,7 @@ #include #include +using kernel::ChainstateRole; using wallet::CWallet; using wallet::CreateMockableWalletDatabase; using wallet::WALLET_FLAG_DESCRIPTORS; @@ -101,7 +103,7 @@ void generateFakeBlock(const CChainParams& params, // notify wallet const auto& pindex = WITH_LOCK(::cs_main, return context.chainman->ActiveChain().Tip()); - wallet.blockConnected(ChainstateRole::NORMAL, kernel::MakeBlockInfo(pindex, &block)); + wallet.blockConnected(ChainstateRole{}, kernel::MakeBlockInfo(pindex, &block)); } struct PreSelectInputs { diff --git a/src/bench/wallet_migration.cpp b/src/bench/wallet_migration.cpp index 0ce23a49b4e..9d61b984792 100644 --- a/src/bench/wallet_migration.cpp +++ b/src/bench/wallet_migration.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include diff --git a/src/index/base.cpp b/src/index/base.cpp index a94b4ca7225..425f9f7eb18 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -42,6 +43,8 @@ #include #include +using kernel::ChainstateRole; + constexpr uint8_t DB_BEST_BLOCK{'B'}; constexpr auto SYNC_LOG_INTERVAL{30s}; @@ -323,15 +326,13 @@ bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_ti return true; } -void BaseIndex::BlockConnected(ChainstateRole role, const std::shared_ptr& block, const CBlockIndex* pindex) +void BaseIndex::BlockConnected(const ChainstateRole& role, const std::shared_ptr& block, const CBlockIndex* pindex) { - // Ignore events from the assumed-valid chain; we will process its blocks - // (sequentially) after it is fully verified by the background chainstate. This - // is to avoid any out-of-order indexing. + // Ignore events from not fully validated chains to avoid out-of-order indexing. // // TODO at some point we could parameterize whether a particular index can be // built out of order, but for now just do the conservative simple thing. - if (role == ChainstateRole::ASSUMEDVALID) { + if (!role.validated) { return; } @@ -377,11 +378,10 @@ void BaseIndex::BlockConnected(ChainstateRole role, const std::shared_ptr& block, const CBlockIndex* pindex) override; + void BlockConnected(const kernel::ChainstateRole& role, const std::shared_ptr& block, const CBlockIndex* pindex) override; - void ChainStateFlushed(ChainstateRole role, const CBlockLocator& locator) override; + void ChainStateFlushed(const kernel::ChainstateRole& role, const CBlockLocator& locator) override; /// Return custom notification options for index. [[nodiscard]] virtual interfaces::Chain::NotifyOptions CustomOptions() { return {}; } diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 40291d20513..1741721828c 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -30,10 +30,12 @@ class Coin; class uint256; enum class MemPoolRemovalReason; enum class RBFTransactionState; -enum class ChainstateRole; struct bilingual_str; struct CBlockLocator; struct FeeCalculation; +namespace kernel { +struct ChainstateRole; +} // namespace kernel namespace node { struct NodeContext; } // namespace node @@ -321,10 +323,10 @@ public: virtual ~Notifications() = default; virtual void transactionAddedToMempool(const CTransactionRef& tx) {} virtual void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {} - virtual void blockConnected(ChainstateRole role, const BlockInfo& block) {} + virtual void blockConnected(const kernel::ChainstateRole& role, const BlockInfo& block) {} virtual void blockDisconnected(const BlockInfo& block) {} virtual void updatedBlockTip() {} - virtual void chainStateFlushed(ChainstateRole role, const CBlockLocator& locator) {} + virtual void chainStateFlushed(const kernel::ChainstateRole& role, const CBlockLocator& locator) {} }; //! Options specifying which chain notifications are required. diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp index be7c22de998..3a36e2c8e76 100644 --- a/src/kernel/bitcoinkernel.cpp +++ b/src/kernel/bitcoinkernel.cpp @@ -51,6 +51,7 @@ #include #include +using kernel::ChainstateRole; using util::ImmediateTaskRunner; // Define G_TRANSLATION_FUN symbol in libbitcoinkernel library so users of the @@ -359,7 +360,7 @@ protected: } } - void BlockConnected(ChainstateRole role, const std::shared_ptr& block, const CBlockIndex* pindex) override + void BlockConnected(const ChainstateRole& role, const std::shared_ptr& block, const CBlockIndex* pindex) override { if (m_cbs.block_connected) { m_cbs.block_connected(m_cbs.user_data, diff --git a/src/kernel/chain.cpp b/src/kernel/chain.cpp index 318c956b386..52da2e3c667 100644 --- a/src/kernel/chain.cpp +++ b/src/kernel/chain.cpp @@ -5,11 +5,14 @@ #include #include #include +#include #include #include class CBlock; +using kernel::ChainstateRole; + namespace kernel { interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* index, const CBlock* data) { @@ -25,14 +28,15 @@ interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* index, const CBlock* data info.data = data; return info; } -} // namespace kernel std::ostream& operator<<(std::ostream& os, const ChainstateRole& role) { - switch(role) { - case ChainstateRole::NORMAL: os << "normal"; break; - case ChainstateRole::ASSUMEDVALID: os << "assumedvalid"; break; - case ChainstateRole::BACKGROUND: os << "background"; break; - default: os.setstate(std::ios_base::failbit); + if (!role.validated) { + os << "assumedvalid"; + } else if (role.historical) { + os << "background"; + } else { + os << "normal"; } return os; } +} // namespace kernel diff --git a/src/kernel/chain.h b/src/kernel/chain.h index feba24a557e..7dbd0eb6f2a 100644 --- a/src/kernel/chain.h +++ b/src/kernel/chain.h @@ -14,26 +14,10 @@ struct BlockInfo; } // namespace interfaces namespace kernel { +struct ChainstateRole; //! Return data from block index. interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* block_index, const CBlock* data = nullptr); - +std::ostream& operator<<(std::ostream& os, const ChainstateRole& role); } // namespace kernel -//! This enum describes the various roles a specific Chainstate instance can take. -//! Other parts of the system sometimes need to vary in behavior depending on the -//! existence of a background validation chainstate, e.g. when building indexes. -enum class ChainstateRole { - // Single chainstate in use, "normal" IBD mode. - NORMAL, - - // Doing IBD-style validation in the background. Implies use of an assumed-valid - // chainstate. - BACKGROUND, - - // Active assumed-valid chainstate. Implies use of a background IBD chainstate. - ASSUMEDVALID, -}; - -std::ostream& operator<<(std::ostream& os, const ChainstateRole& role); - #endif // BITCOIN_KERNEL_CHAIN_H diff --git a/src/kernel/types.h b/src/kernel/types.h new file mode 100644 index 00000000000..164d4cfda81 --- /dev/null +++ b/src/kernel/types.h @@ -0,0 +1,30 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +//! @file kernel/types.h is a home for simple enum and struct type definitions +//! that can be used internally by functions in the libbitcoin_kernel library, +//! but also used externally by node, wallet, and GUI code. +//! +//! This file is intended to define only simple types that do not have external +//! dependencies. More complicated types should be defined in dedicated header +//! files. + +#ifndef BITCOIN_KERNEL_TYPES_H +#define BITCOIN_KERNEL_TYPES_H + +namespace kernel { +//! Information about chainstate that notifications are sent from. +struct ChainstateRole { + //! Whether this is a notification from a chainstate that's been fully + //! validated starting from the genesis block. False if it is from an + //! assumeutxo snapshot chainstate that has not been validated yet. + bool validated{true}; + + //! Whether this is a historical chainstate downloading old blocks to + //! validate an assumeutxo snapshot, not syncing to the network tip. + bool historical{false}; +}; +} // namespace kernel + +#endif // BITCOIN_KERNEL_TYPES_H diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 09b2f221c1e..d7fd2879880 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -22,7 +22,7 @@ #include #include #include -#include +#include #include #include #include @@ -85,6 +85,7 @@ #include #include +using kernel::ChainstateRole; using namespace util::hex_literals; TRACEPOINT_SEMAPHORE(net, inbound_message); @@ -507,7 +508,7 @@ public: /** Overridden from CValidationInterface. */ void ActiveTipChange(const CBlockIndex& new_tip, bool) override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); - void BlockConnected(ChainstateRole role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override + void BlockConnected(const ChainstateRole& role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); void BlockDisconnected(const std::shared_ptr &block, const CBlockIndex* pindex) override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); @@ -1946,7 +1947,7 @@ void PeerManagerImpl::ActiveTipChange(const CBlockIndex& new_tip, bool is_ibd) * possibly reduce dynamic block stalling timeout. */ void PeerManagerImpl::BlockConnected( - ChainstateRole role, + const ChainstateRole& role, const std::shared_ptr& pblock, const CBlockIndex* pindex) { @@ -1965,8 +1966,8 @@ void PeerManagerImpl::BlockConnected( } // The following task can be skipped since we don't maintain a mempool for - // the ibd/background chainstate. - if (role == ChainstateRole::BACKGROUND) { + // the historical chainstate. + if (role.historical) { return; } LOCK(m_tx_download_mutex); diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index a5984df6171..c5d26e5cbb0 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -12,9 +12,11 @@ #include #include #include +#include #include #include #include +#include #include #include #include diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index ac817e01b15..dfcf66290c8 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -81,6 +81,7 @@ using interfaces::MakeSignalHandler; using interfaces::Mining; using interfaces::Node; using interfaces::WalletLoader; +using kernel::ChainstateRole; using node::BlockAssembler; using node::BlockWaitOptions; using util::Join; @@ -461,7 +462,7 @@ public: { m_notifications->transactionRemovedFromMempool(tx, reason); } - void BlockConnected(ChainstateRole role, const std::shared_ptr& block, const CBlockIndex* index) override + void BlockConnected(const ChainstateRole& role, const std::shared_ptr& block, const CBlockIndex* index) override { m_notifications->blockConnected(role, kernel::MakeBlockInfo(index, block.get())); } @@ -473,7 +474,8 @@ public: { m_notifications->updatedBlockTip(); } - void ChainStateFlushed(ChainstateRole role, const CBlockLocator& locator) override { + void ChainStateFlushed(const ChainstateRole& role, const CBlockLocator& locator) override + { m_notifications->chainStateFlushed(role, locator); } std::shared_ptr m_notifications; diff --git a/src/test/chainstate_write_tests.cpp b/src/test/chainstate_write_tests.cpp index e1b82ebc121..5c459227763 100644 --- a/src/test/chainstate_write_tests.cpp +++ b/src/test/chainstate_write_tests.cpp @@ -8,6 +8,8 @@ #include +using kernel::ChainstateRole; + // Taken from validation.cpp static constexpr auto DATABASE_WRITE_INTERVAL_MIN{50min}; static constexpr auto DATABASE_WRITE_INTERVAL_MAX{70min}; @@ -18,7 +20,7 @@ BOOST_FIXTURE_TEST_CASE(chainstate_write_interval, TestingSetup) { struct TestSubscriber final : CValidationInterface { bool m_did_flush{false}; - void ChainStateFlushed(ChainstateRole, const CBlockLocator&) override + void ChainStateFlushed(const ChainstateRole&, const CBlockLocator&) override { m_did_flush = true; } @@ -55,7 +57,7 @@ BOOST_FIXTURE_TEST_CASE(write_during_multiblock_activation, TestChain100Setup) { const CBlockIndex* m_tip{nullptr}; const CBlockIndex* m_flushed_at_block{nullptr}; - void ChainStateFlushed(ChainstateRole, const CBlockLocator&) override + void ChainStateFlushed(const ChainstateRole&, const CBlockLocator&) override { m_flushed_at_block = m_tip; } diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp index 7d54f0fdf15..d55fb1b05fc 100644 --- a/src/test/coinstatsindex_tests.cpp +++ b/src/test/coinstatsindex_tests.cpp @@ -6,12 +6,15 @@ #include #include #include +#include #include #include #include #include +using kernel::ChainstateRole; + BOOST_AUTO_TEST_SUITE(coinstatsindex_tests) BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup) @@ -101,7 +104,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup) // Send block connected notification, then stop the index without // sending a chainstate flushed notification. Prior to #24138, this // would cause the index to be corrupted and fail to reload. - ValidationInterfaceTest::BlockConnected(ChainstateRole::NORMAL, index, new_block, new_block_index); + ValidationInterfaceTest::BlockConnected(ChainstateRole{}, index, new_block, new_block_index); index.Stop(); } diff --git a/src/test/util/validation.cpp b/src/test/util/validation.cpp index ce558078a6d..ae8642f9482 100644 --- a/src/test/util/validation.cpp +++ b/src/test/util/validation.cpp @@ -9,6 +9,8 @@ #include #include +using kernel::ChainstateRole; + void TestChainstateManager::DisableNextWrite() { struct TestChainstate : public Chainstate { @@ -18,6 +20,7 @@ void TestChainstateManager::DisableNextWrite() static_cast(cs)->ResetNextWrite(); } } + void TestChainstateManager::ResetIbd() { m_cached_finished_ibd = false; @@ -32,10 +35,10 @@ void TestChainstateManager::JumpOutOfIbd() } void ValidationInterfaceTest::BlockConnected( - ChainstateRole role, - CValidationInterface& obj, - const std::shared_ptr& block, - const CBlockIndex* pindex) + const ChainstateRole& role, + CValidationInterface& obj, + const std::shared_ptr& block, + const CBlockIndex* pindex) { obj.BlockConnected(role, block, pindex); } diff --git a/src/test/util/validation.h b/src/test/util/validation.h index f9c6a8ac02a..4a24c97ed24 100644 --- a/src/test/util/validation.h +++ b/src/test/util/validation.h @@ -22,7 +22,7 @@ class ValidationInterfaceTest { public: static void BlockConnected( - ChainstateRole role, + const kernel::ChainstateRole& role, CValidationInterface& obj, const std::shared_ptr& block, const CBlockIndex* pindex); diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index a0b23f5d3b7..9f574be86ef 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -19,6 +19,7 @@ #include +using kernel::ChainstateRole; using node::BlockAssembler; namespace validation_block_tests { @@ -43,7 +44,7 @@ struct TestSubscriber final : public CValidationInterface { BOOST_CHECK_EQUAL(m_expected_tip, pindexNew->GetBlockHash()); } - void BlockConnected(ChainstateRole role, const std::shared_ptr& block, const CBlockIndex* pindex) override + void BlockConnected(const ChainstateRole& role, const std::shared_ptr& block, const CBlockIndex* pindex) override { BOOST_CHECK_EQUAL(m_expected_tip, block->hashPrevBlock); BOOST_CHECK_EQUAL(m_expected_tip, pindex->pprev->GetBlockHash()); diff --git a/src/test/validationinterface_tests.cpp b/src/test/validationinterface_tests.cpp index 8a24b282458..0b0bae80721 100644 --- a/src/test/validationinterface_tests.cpp +++ b/src/test/validationinterface_tests.cpp @@ -8,7 +8,6 @@ #include #include #include -#include #include #include diff --git a/src/validation.cpp b/src/validation.cpp index da60efbc3cf..c229851e40d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -20,13 +20,13 @@ #include #include #include -#include #include #include #include #include #include #include +#include #include #include #include @@ -77,6 +77,7 @@ #include using kernel::CCoinsStats; +using kernel::ChainstateRole; using kernel::CoinStatsHashType; using kernel::ComputeUTXOStats; using kernel::Notifications; @@ -1986,7 +1987,7 @@ void Chainstate::CheckForkWarningConditions() { AssertLockHeld(cs_main); - if (this->GetRole() == ChainstateRole::BACKGROUND) { + if (this->GetRole().historical) { return; } @@ -2532,7 +2533,8 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, Ticks(m_chainman.time_forks) / m_chainman.num_blocks_total); const bool fScriptChecks{!!script_check_reason}; - if (script_check_reason != m_last_script_check_reason_logged && GetRole() == ChainstateRole::NORMAL) { + const kernel::ChainstateRole role{GetRole()}; + if (script_check_reason != m_last_script_check_reason_logged && role.validated && !role.historical) { if (fScriptChecks) { LogInfo("Enabling script verification at block #%d (%s): %s.", pindex->nHeight, block_hash.ToString(), script_check_reason); @@ -4656,7 +4658,7 @@ bool Chainstate::LoadChainTip() m_chainman.GuessVerificationProgress(tip)); // Ensure KernelNotifications m_tip_block is set even if no new block arrives. - if (this->GetRole() != ChainstateRole::BACKGROUND) { + if (!this->GetRole().historical) { // Ignoring return value for now. (void)m_chainman.GetNotifications().blockTip( /*state=*/GetSynchronizationState(/*init=*/true, m_chainman.m_blockman.m_blockfiles_indexed), @@ -6354,7 +6356,7 @@ bool ChainstateManager::DeleteSnapshotChainstate() ChainstateRole Chainstate::GetRole() const { - return m_target_blockhash ? ChainstateRole::BACKGROUND : m_assumeutxo == Assumeutxo::UNVALIDATED ? ChainstateRole::ASSUMEDVALID : ChainstateRole::NORMAL; + return ChainstateRole{.validated = m_assumeutxo == Assumeutxo::VALIDATED, .historical = bool{m_target_blockhash}}; } void ChainstateManager::RecalculateBestHeader() diff --git a/src/validation.h b/src/validation.h index 464dcbd03a7..0f7fa2cbb88 100644 --- a/src/validation.h +++ b/src/validation.h @@ -58,6 +58,9 @@ class DisconnectedBlockTransactions; struct PrecomputedTransactionData; struct LockPoints; struct AssumeutxoData; +namespace kernel { +struct ChainstateRole; +} // namespace kernel namespace node { class SnapshotMetadata; } // namespace node @@ -586,7 +589,7 @@ public: //! documentation for a description of the different types of chainstates. //! //! @sa ChainstateRole - ChainstateRole GetRole() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + kernel::ChainstateRole GetRole() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** * Initialize the CoinsViews UTXO set database management data structures. The in-memory diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 313f730ea09..c4cd10104f3 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -7,9 +7,9 @@ #include #include -#include #include #include +#include #include #include #include @@ -21,6 +21,8 @@ #include #include +using kernel::ChainstateRole; + /** * ValidationSignalsImpl manages a list of shared_ptr callbacks. * @@ -209,7 +211,8 @@ void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, RemovalReasonToString(reason)); } -void ValidationSignals::BlockConnected(ChainstateRole role, const std::shared_ptr &pblock, const CBlockIndex *pindex) { +void ValidationSignals::BlockConnected(const ChainstateRole& role, const std::shared_ptr& pblock, const CBlockIndex* pindex) +{ auto event = [role, pblock, pindex, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.BlockConnected(role, pblock, pindex); }); }; @@ -238,7 +241,8 @@ void ValidationSignals::BlockDisconnected(const std::shared_ptr& p pindex->nHeight); } -void ValidationSignals::ChainStateFlushed(ChainstateRole role, const CBlockLocator &locator) { +void ValidationSignals::ChainStateFlushed(const ChainstateRole& role, const CBlockLocator& locator) +{ auto event = [role, locator, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.ChainStateFlushed(role, locator); }); }; diff --git a/src/validationinterface.h b/src/validationinterface.h index e0a88ad0f21..4777e8dca8d 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -6,7 +6,6 @@ #ifndef BITCOIN_VALIDATIONINTERFACE_H #define BITCOIN_VALIDATIONINTERFACE_H -#include #include #include #include @@ -17,6 +16,9 @@ #include #include +namespace kernel { +struct ChainstateRole; +} // namespace kernel namespace util { class TaskRunnerInterface; } // namespace util @@ -118,7 +120,7 @@ protected: * * Called on a background thread. */ - virtual void BlockConnected(ChainstateRole role, const std::shared_ptr &block, const CBlockIndex *pindex) {} + virtual void BlockConnected(const kernel::ChainstateRole& role, const std::shared_ptr& block, const CBlockIndex* pindex) {} /** * Notifies listeners of a block being disconnected * Provides the block that was disconnected. @@ -143,7 +145,7 @@ protected: * * Called on a background thread. */ - virtual void ChainStateFlushed(ChainstateRole role, const CBlockLocator &locator) {} + virtual void ChainStateFlushed(const kernel::ChainstateRole& role, const CBlockLocator& locator) {} /** * Notifies listeners of a block validation result. * If the provided BlockValidationState IsValid, the provided block @@ -221,9 +223,9 @@ public: void TransactionAddedToMempool(const NewMempoolTransactionInfo&, uint64_t mempool_sequence); void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason, uint64_t mempool_sequence); void MempoolTransactionsRemovedForBlock(const std::vector&, unsigned int nBlockHeight); - void BlockConnected(ChainstateRole, const std::shared_ptr &, const CBlockIndex *pindex); + void BlockConnected(const kernel::ChainstateRole&, const std::shared_ptr&, const CBlockIndex* pindex); void BlockDisconnected(const std::shared_ptr &, const CBlockIndex* pindex); - void ChainStateFlushed(ChainstateRole, const CBlockLocator &); + void ChainStateFlushed(const kernel::ChainstateRole&, const CBlockLocator&); void BlockChecked(const std::shared_ptr&, const BlockValidationState&); void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr&); }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 86474a456d7..d7b749bcaed 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -22,8 +22,8 @@ #include #include #include -#include #include +#include #include #include #include @@ -87,6 +87,7 @@ using common::AmountErrMsg; using common::AmountHighWarn; using common::PSBTError; using interfaces::FoundBlock; +using kernel::ChainstateRole; using util::ReplaceAll; using util::ToString; @@ -1477,9 +1478,9 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRe } } -void CWallet::blockConnected(ChainstateRole role, const interfaces::BlockInfo& block) +void CWallet::blockConnected(const ChainstateRole& role, const interfaces::BlockInfo& block) { - if (role == ChainstateRole::BACKGROUND) { + if (role.historical) { return; } assert(block.data); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 275d4ead293..1d463564758 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -629,7 +629,7 @@ public: CWalletTx* AddToWallet(CTransactionRef tx, const TxState& state, const UpdateWalletTxFn& update_wtx=nullptr, bool rescanning_old_block = false); bool LoadToWallet(const Txid& hash, const UpdateWalletTxFn& fill_wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void transactionAddedToMempool(const CTransactionRef& tx) override; - void blockConnected(ChainstateRole role, const interfaces::BlockInfo& block) override; + void blockConnected(const kernel::ChainstateRole& role, const interfaces::BlockInfo& block) override; void blockDisconnected(const interfaces::BlockInfo& block) override; void updatedBlockTip() override; int64_t RescanFromTime(int64_t startTime, const WalletRescanReserver& reserver, bool update); diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index e5b5f9d8054..316c7a19b3c 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -5,8 +5,8 @@ #include #include -#include #include +#include #include #include #include @@ -24,6 +24,8 @@ #include #include +using kernel::ChainstateRole; + CZMQNotificationInterface::CZMQNotificationInterface() = default; CZMQNotificationInterface::~CZMQNotificationInterface() @@ -176,9 +178,9 @@ void CZMQNotificationInterface::TransactionRemovedFromMempool(const CTransaction }); } -void CZMQNotificationInterface::BlockConnected(ChainstateRole role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) +void CZMQNotificationInterface::BlockConnected(const ChainstateRole& role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) { - if (role == ChainstateRole::BACKGROUND) { + if (role.historical) { return; } for (const CTransactionRef& ptx : pblock->vtx) { diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 5dacf3b32e7..12d805c1094 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -35,7 +35,7 @@ protected: // CValidationInterface void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) override; void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override; - void BlockConnected(ChainstateRole role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override; + void BlockConnected(const kernel::ChainstateRole& role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override; void BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex* pindexDisconnected) override; void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override; From d9e82299fc4e45fbc0f5a34dcbb1d51397d0bd35 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 31 May 2024 07:00:40 -0400 Subject: [PATCH 10/17] refactor: Delete ChainstateManager::IsSnapshotActive() method IsSnapshotActive() method is only called one place outside of tests and asserts, and is confusing because it returns true even after the snapshot is fully validated. The documentation which said this "implies that a background validation chainstate is also in use" is also incorrect, because after the snapshot is validated, the background chainstate gets disabled and IsUsable() would return false. --- src/interfaces/chain.h | 4 +++- src/node/chainstate.cpp | 1 - src/node/interfaces.cpp | 3 ++- src/test/validation_chainstate_tests.cpp | 2 +- .../validation_chainstatemanager_tests.cpp | 19 +++++++++---------- src/validation.cpp | 6 ------ src/validation.h | 4 ---- 7 files changed, 15 insertions(+), 24 deletions(-) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 1741721828c..686da67bfd2 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -391,7 +391,9 @@ public: //! removed transactions and already added new transactions. virtual void requestMempoolTransactions(Notifications& notifications) = 0; - //! Return true if an assumed-valid chain is in use. + //! Return true if an assumed-valid snapshot is in use. Note that this + //! returns true even after the snapshot is validated, until the next node + //! restart. virtual bool hasAssumedValidChain() = 0; //! Get internal node context. Useful for testing, but not diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index bba518d1825..a301089811f 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -210,7 +210,6 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize // ChainstateManager::ResetChainstates(), reinitialize them here without // duplicating the blockindex work above. assert(chainman.GetAll().empty()); - assert(!chainman.IsSnapshotActive()); assert(!chainman.IsSnapshotValidated()); chainman.InitializeChainstate(options.mempool); diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index dfcf66290c8..5113f44ffc6 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -845,7 +845,8 @@ public: } bool hasAssumedValidChain() override { - return chainman().IsSnapshotActive(); + LOCK(::cs_main); + return bool{chainman().CurrentChainstate().m_from_snapshot_blockhash}; } NodeContext* context() override { return &m_node; } diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index b4aeb5b3370..e823193a17d 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -95,7 +95,7 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) this, NoMalleation, /*reset_chainstate=*/ true)); // Ensure our active chain is the snapshot chainstate. - BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.IsSnapshotActive())); + BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.CurrentChainstate().m_from_snapshot_blockhash)); curr_tip = get_notify_tip(); diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 036089c30f6..5d0bc907596 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -49,7 +49,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup) Chainstate& c1 = manager.ActiveChainstate(); chainstates.push_back(&c1); - BOOST_CHECK(!manager.IsSnapshotActive()); + BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.CurrentChainstate().m_from_snapshot_blockhash)); BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.IsSnapshotValidated())); auto all = manager.GetAll(); BOOST_CHECK_EQUAL_COLLECTIONS(all.begin(), all.end(), chainstates.begin(), chainstates.end()); @@ -84,7 +84,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup) BOOST_CHECK(c2.ActivateBestChain(_, nullptr)); BOOST_CHECK_EQUAL(manager.SnapshotBlockhash().value(), snapshot_blockhash); - BOOST_CHECK(manager.IsSnapshotActive()); + BOOST_CHECK(WITH_LOCK(::cs_main, return manager.CurrentChainstate().m_from_snapshot_blockhash)); BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.IsSnapshotValidated())); BOOST_CHECK_EQUAL(&c2, &manager.ActiveChainstate()); BOOST_CHECK(&c1 != &manager.ActiveChainstate()); @@ -182,10 +182,9 @@ struct SnapshotTestSetup : TestChain100Setup { { ChainstateManager& chainman = *Assert(m_node.chainman); - BOOST_CHECK(!chainman.IsSnapshotActive()); - { LOCK(::cs_main); + BOOST_CHECK(!chainman.CurrentChainstate().m_from_snapshot_blockhash); BOOST_CHECK(!chainman.IsSnapshotValidated()); BOOST_CHECK(!node::FindAssumeutxoChainstateDir(chainman.m_options.datadir)); } @@ -569,7 +568,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) BOOST_CHECK(fs::exists(snapshot_chainstate_dir)); BOOST_CHECK_EQUAL(snapshot_chainstate_dir, gArgs.GetDataDirNet() / "chainstate_snapshot"); - BOOST_CHECK(chainman.IsSnapshotActive()); + BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.CurrentChainstate().m_from_snapshot_blockhash)); const uint256 snapshot_tip_hash = WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip()->GetBlockHash()); @@ -611,7 +610,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) BOOST_CHECK_EQUAL(chainstates[0]->m_chain.Height(), 109); BOOST_CHECK_EQUAL(chainstates[1]->m_chain.Height(), 210); - BOOST_CHECK(chainman_restarted.IsSnapshotActive()); + BOOST_CHECK(chainman_restarted.CurrentChainstate().m_from_snapshot_blockhash); BOOST_CHECK(!chainman_restarted.IsSnapshotValidated()); BOOST_CHECK_EQUAL(chainman_restarted.ActiveTip()->GetBlockHash(), snapshot_tip_hash); @@ -650,7 +649,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup BOOST_CHECK(fs::exists(snapshot_chainstate_dir)); BOOST_CHECK_EQUAL(snapshot_chainstate_dir, gArgs.GetDataDirNet() / "chainstate_snapshot"); - BOOST_CHECK(chainman.IsSnapshotActive()); + BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.CurrentChainstate().m_from_snapshot_blockhash)); const uint256 snapshot_tip_hash = WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip()->GetBlockHash()); @@ -658,7 +657,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SUCCESS); WITH_LOCK(::cs_main, BOOST_CHECK(chainman.IsSnapshotValidated())); - BOOST_CHECK(chainman.IsSnapshotActive()); + BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.CurrentChainstate().m_from_snapshot_blockhash)); // Cache should have been rebalanced and reallocated to the "only" remaining // chainstate. @@ -699,7 +698,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup { LOCK(chainman_restarted.GetMutex()); BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 1); - BOOST_CHECK(!chainman_restarted.IsSnapshotActive()); + BOOST_CHECK(!chainman_restarted.CurrentChainstate().m_from_snapshot_blockhash); BOOST_CHECK(!chainman_restarted.IsSnapshotValidated()); BOOST_CHECK(active_cs2.m_coinstip_cache_size_bytes > tip_cache_before_complete); BOOST_CHECK(active_cs2.m_coinsdb_cache_size_bytes > db_cache_before_complete); @@ -771,7 +770,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna { LOCK(::cs_main); BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 1); - BOOST_CHECK(!chainman_restarted.IsSnapshotActive()); + BOOST_CHECK(!chainman_restarted.CurrentChainstate().m_from_snapshot_blockhash); BOOST_CHECK(!chainman_restarted.IsSnapshotValidated()); BOOST_CHECK_EQUAL(chainman_restarted.ActiveHeight(), 210); } diff --git a/src/validation.cpp b/src/validation.cpp index c229851e40d..ff868911da9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -6179,12 +6179,6 @@ Chainstate& ChainstateManager::ActiveChainstate() const return *m_active_chainstate; } -bool ChainstateManager::IsSnapshotActive() const -{ - LOCK(::cs_main); - return m_snapshot_chainstate && m_active_chainstate == m_snapshot_chainstate.get(); -} - void ChainstateManager::MaybeRebalanceCaches() { AssertLockHeld(::cs_main); diff --git a/src/validation.h b/src/validation.h index 0f7fa2cbb88..30d3c2aec37 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1189,10 +1189,6 @@ public: */ mutable VersionBitsCache m_versionbitscache; - //! @returns true if a snapshot-based chainstate is in use. Also implies - //! that a background validation chainstate is also in use. - bool IsSnapshotActive() const; - std::optional SnapshotBlockhash() const; //! Is there a snapshot in use and has it been fully validated? From ee35250683ab9a395b70a0e90ebc68b1858387c7 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 16:14:42 -0400 Subject: [PATCH 11/17] refactor: Delete ChainstateManager::IsSnapshotValidated() method IsSnapshotValidated() is only called one place outside of tests, and is use redundantly in some tests, asserting that a snapshot is not validated when a snapshot chainstate does not even exist. Simplify by dropping the method and checking Chainstate m_assumeutxo field directly. --- src/node/chainstate.cpp | 1 - src/test/validation_chainstatemanager_tests.cpp | 10 +++------- src/validation.cpp | 2 +- src/validation.h | 6 ------ 4 files changed, 4 insertions(+), 15 deletions(-) diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index a301089811f..2c27ccf464f 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -210,7 +210,6 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize // ChainstateManager::ResetChainstates(), reinitialize them here without // duplicating the blockindex work above. assert(chainman.GetAll().empty()); - assert(!chainman.IsSnapshotValidated()); chainman.InitializeChainstate(options.mempool); diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 5d0bc907596..0dfc34f760c 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -50,7 +50,6 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup) chainstates.push_back(&c1); BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.CurrentChainstate().m_from_snapshot_blockhash)); - BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.IsSnapshotValidated())); auto all = manager.GetAll(); BOOST_CHECK_EQUAL_COLLECTIONS(all.begin(), all.end(), chainstates.begin(), chainstates.end()); @@ -85,7 +84,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup) BOOST_CHECK_EQUAL(manager.SnapshotBlockhash().value(), snapshot_blockhash); BOOST_CHECK(WITH_LOCK(::cs_main, return manager.CurrentChainstate().m_from_snapshot_blockhash)); - BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.IsSnapshotValidated())); + BOOST_CHECK(WITH_LOCK(::cs_main, return manager.CurrentChainstate().m_assumeutxo == Assumeutxo::UNVALIDATED)); BOOST_CHECK_EQUAL(&c2, &manager.ActiveChainstate()); BOOST_CHECK(&c1 != &manager.ActiveChainstate()); auto all2 = manager.GetAll(); @@ -185,7 +184,6 @@ struct SnapshotTestSetup : TestChain100Setup { { LOCK(::cs_main); BOOST_CHECK(!chainman.CurrentChainstate().m_from_snapshot_blockhash); - BOOST_CHECK(!chainman.IsSnapshotValidated()); BOOST_CHECK(!node::FindAssumeutxoChainstateDir(chainman.m_options.datadir)); } @@ -611,7 +609,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) BOOST_CHECK_EQUAL(chainstates[1]->m_chain.Height(), 210); BOOST_CHECK(chainman_restarted.CurrentChainstate().m_from_snapshot_blockhash); - BOOST_CHECK(!chainman_restarted.IsSnapshotValidated()); + BOOST_CHECK(chainman_restarted.CurrentChainstate().m_assumeutxo == Assumeutxo::UNVALIDATED); BOOST_CHECK_EQUAL(chainman_restarted.ActiveTip()->GetBlockHash(), snapshot_tip_hash); BOOST_CHECK_EQUAL(chainman_restarted.ActiveHeight(), 210); @@ -656,7 +654,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup res = WITH_LOCK(::cs_main, return chainman.MaybeValidateSnapshot(validated_cs, active_cs)); BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SUCCESS); - WITH_LOCK(::cs_main, BOOST_CHECK(chainman.IsSnapshotValidated())); + BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.CurrentChainstate().m_assumeutxo == Assumeutxo::VALIDATED)); BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.CurrentChainstate().m_from_snapshot_blockhash)); // Cache should have been rebalanced and reallocated to the "only" remaining @@ -699,7 +697,6 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup LOCK(chainman_restarted.GetMutex()); BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 1); BOOST_CHECK(!chainman_restarted.CurrentChainstate().m_from_snapshot_blockhash); - BOOST_CHECK(!chainman_restarted.IsSnapshotValidated()); BOOST_CHECK(active_cs2.m_coinstip_cache_size_bytes > tip_cache_before_complete); BOOST_CHECK(active_cs2.m_coinsdb_cache_size_bytes > db_cache_before_complete); @@ -771,7 +768,6 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna LOCK(::cs_main); BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 1); BOOST_CHECK(!chainman_restarted.CurrentChainstate().m_from_snapshot_blockhash); - BOOST_CHECK(!chainman_restarted.IsSnapshotValidated()); BOOST_CHECK_EQUAL(chainman_restarted.ActiveHeight(), 210); } diff --git a/src/validation.cpp b/src/validation.cpp index ff868911da9..6ed044e2a41 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -6367,7 +6367,7 @@ void ChainstateManager::RecalculateBestHeader() bool ChainstateManager::ValidatedSnapshotCleanup(Chainstate& validated_cs, Chainstate& unvalidated_cs) { AssertLockHeld(::cs_main); - if (!this->IsSnapshotValidated()) { + if (unvalidated_cs.m_assumeutxo != Assumeutxo::VALIDATED) { // No need to clean up. return false; } diff --git a/src/validation.h b/src/validation.h index 30d3c2aec37..336c1af66ef 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1191,12 +1191,6 @@ public: std::optional SnapshotBlockhash() const; - //! Is there a snapshot in use and has it been fully validated? - bool IsSnapshotValidated() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) - { - return m_snapshot_chainstate && m_snapshot_chainstate->m_assumeutxo == Assumeutxo::VALIDATED; - } - /** Check whether we are doing an initial block download (synchronizing from disk or network) */ bool IsInitialBlockDownload() const; From e514fe61168109bd467d7cb2ac7561442b17b5f6 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 16:30:22 -0400 Subject: [PATCH 12/17] refactor: Delete ChainstateManager::SnapshotBlockhash() method SnapshotBlockhash() is only called two places outside of tests, and is used redundantly in some tests, checking the same field as other checks. Simplify by dropping the method and using the m_from_snapshot_blockhash field directly. --- src/test/fuzz/utxo_snapshot.cpp | 5 +---- .../validation_chainstatemanager_tests.cpp | 15 +++++---------- src/validation.cpp | 19 ++++--------------- src/validation.h | 2 -- 4 files changed, 10 insertions(+), 31 deletions(-) diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp index 5b088af4f30..e6ae29a873f 100644 --- a/src/test/fuzz/utxo_snapshot.cpp +++ b/src/test/fuzz/utxo_snapshot.cpp @@ -110,7 +110,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer) const auto snapshot_path = gArgs.GetDataDirNet() / "fuzzed_snapshot.dat"; - Assert(!chainman.SnapshotBlockhash()); + Assert(!chainman.ActiveChainstate().m_from_snapshot_blockhash); { AutoFile outfile{fsbridge::fopen(snapshot_path, "wb")}; @@ -183,8 +183,6 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer) if (ActivateFuzzedSnapshot()) { LOCK(::cs_main); Assert(!chainman.ActiveChainstate().m_from_snapshot_blockhash->IsNull()); - Assert(*chainman.ActiveChainstate().m_from_snapshot_blockhash == - *chainman.SnapshotBlockhash()); const auto& coinscache{chainman.ActiveChainstate().CoinsTip()}; for (const auto& block : *g_chain) { Assert(coinscache.HaveCoin(COutPoint{block->vtx.at(0)->GetHash(), 0})); @@ -202,7 +200,6 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer) Assert(g_chain->size() == coinscache.GetCacheSize()); dirty_chainman = true; } else { - Assert(!chainman.SnapshotBlockhash()); Assert(!chainman.ActiveChainstate().m_from_snapshot_blockhash); } // Snapshot should refuse to load a second time regardless of validity diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 0dfc34f760c..5a1ff8cd37d 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -42,7 +42,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup) ChainstateManager& manager = *m_node.chainman; std::vector chainstates; - BOOST_CHECK(!manager.SnapshotBlockhash().has_value()); + BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.CurrentChainstate().m_from_snapshot_blockhash)); // Create a legacy (IBD) chainstate. // @@ -63,7 +63,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup) auto exp_tip = c1.m_chain.Tip(); BOOST_CHECK_EQUAL(active_tip, exp_tip); - BOOST_CHECK(!manager.SnapshotBlockhash().has_value()); + BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.CurrentChainstate().m_from_snapshot_blockhash)); // Create a snapshot-based chainstate. // @@ -82,8 +82,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup) BlockValidationState _; BOOST_CHECK(c2.ActivateBestChain(_, nullptr)); - BOOST_CHECK_EQUAL(manager.SnapshotBlockhash().value(), snapshot_blockhash); - BOOST_CHECK(WITH_LOCK(::cs_main, return manager.CurrentChainstate().m_from_snapshot_blockhash)); + BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return *manager.CurrentChainstate().m_from_snapshot_blockhash), snapshot_blockhash); BOOST_CHECK(WITH_LOCK(::cs_main, return manager.CurrentChainstate().m_assumeutxo == Assumeutxo::UNVALIDATED)); BOOST_CHECK_EQUAL(&c2, &manager.ActiveChainstate()); BOOST_CHECK(&c1 != &manager.ActiveChainstate()); @@ -212,7 +211,6 @@ struct SnapshotTestSetup : TestChain100Setup { // Snapshot should refuse to load at this height. BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(this)); BOOST_CHECK(!chainman.ActiveChainstate().m_from_snapshot_blockhash); - BOOST_CHECK(!chainman.SnapshotBlockhash()); // Mine 10 more blocks, putting at us height 110 where a valid assumeutxo value can // be found. @@ -265,9 +263,6 @@ struct SnapshotTestSetup : TestChain100Setup { // Ensure our active chain is the snapshot chainstate. BOOST_CHECK(!chainman.ActiveChainstate().m_from_snapshot_blockhash->IsNull()); - BOOST_CHECK_EQUAL( - *chainman.ActiveChainstate().m_from_snapshot_blockhash, - *chainman.SnapshotBlockhash()); Chainstate& snapshot_chainstate = chainman.ActiveChainstate(); @@ -279,7 +274,7 @@ struct SnapshotTestSetup : TestChain100Setup { // Note: WriteSnapshotBaseBlockhash() is implicitly tested above. BOOST_CHECK_EQUAL( *node::ReadSnapshotBaseBlockhash(found), - *chainman.SnapshotBlockhash()); + *Assert(chainman.CurrentChainstate().m_from_snapshot_blockhash)); } const auto& au_data = ::Params().AssumeutxoForHeight(snapshot_height); @@ -288,7 +283,7 @@ struct SnapshotTestSetup : TestChain100Setup { BOOST_CHECK_EQUAL(tip->m_chain_tx_count, au_data->m_chain_tx_count); // To be checked against later when we try loading a subsequent snapshot. - uint256 loaded_snapshot_blockhash{*chainman.SnapshotBlockhash()}; + uint256 loaded_snapshot_blockhash{*Assert(WITH_LOCK(chainman.GetMutex(), return chainman.CurrentChainstate().m_from_snapshot_blockhash))}; // Make some assertions about the both chainstates. These checks ensure the // legacy chainstate hasn't changed and that the newly created chainstate diff --git a/src/validation.cpp b/src/validation.cpp index 6ed044e2a41..83a37d07079 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4960,7 +4960,7 @@ bool ChainstateManager::LoadBlockIndex() AssertLockHeld(cs_main); // Load block index from databases if (m_blockman.m_blockfiles_indexed) { - bool ret{m_blockman.LoadBlockIndexDB(SnapshotBlockhash())}; + bool ret{m_blockman.LoadBlockIndexDB(CurrentChainstate().m_from_snapshot_blockhash)}; if (!ret) return false; m_blockman.ScanAndUnlinkAlreadyPrunedFiles(); @@ -5605,16 +5605,6 @@ double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) c return std::min(pindex->m_chain_tx_count / fTxTotal, 1.0); } -std::optional ChainstateManager::SnapshotBlockhash() const -{ - LOCK(::cs_main); - if (m_active_chainstate && m_active_chainstate->m_from_snapshot_blockhash) { - // If a snapshot chainstate exists, it will always be our active. - return m_active_chainstate->m_from_snapshot_blockhash; - } - return std::nullopt; -} - std::vector ChainstateManager::GetAll() { LOCK(::cs_main); @@ -5685,15 +5675,14 @@ util::Result ChainstateManager::ActivateSnapshot( { uint256 base_blockhash = metadata.m_base_blockhash; - if (this->SnapshotBlockhash()) { - return util::Error{Untranslated("Can't activate a snapshot-based chainstate more than once")}; - } - CBlockIndex* snapshot_start_block{}; { LOCK(::cs_main); + if (this->CurrentChainstate().m_from_snapshot_blockhash) { + return util::Error{Untranslated("Can't activate a snapshot-based chainstate more than once")}; + } if (!GetParams().AssumeutxoForBlockhash(base_blockhash).has_value()) { auto available_heights = GetParams().GetAvailableSnapshotHeights(); std::string heights_formatted = util::Join(available_heights, ", ", [&](const auto& i) { return util::ToString(i); }); diff --git a/src/validation.h b/src/validation.h index 336c1af66ef..450c7ca887e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1189,8 +1189,6 @@ public: */ mutable VersionBitsCache m_versionbitscache; - std::optional SnapshotBlockhash() const; - /** Check whether we are doing an initial block download (synchronizing from disk or network) */ bool IsInitialBlockDownload() const; From 491d827d5284ed984ee2b11daaee50321217eac5 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 13:26:01 -0400 Subject: [PATCH 13/17] 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 */ From 6a572dbda92ceb8c5af379f51cf6f9b93fb5e486 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 14:52:28 -0400 Subject: [PATCH 14/17] refactor: Add ChainstateManager::ActivateBestChains() method Deduplicate code looping over chainstate objects and calling ActivateBestChain() and avoid need for code outside ChainstateManager to use the GetAll() method. --- src/kernel/bitcoinkernel.cpp | 10 +++------- src/node/blockstorage.cpp | 12 ++---------- src/validation.cpp | 28 ++++++++++++++++++---------- src/validation.h | 3 +++ 4 files changed, 26 insertions(+), 27 deletions(-) diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp index 3a36e2c8e76..5bb1a06b094 100644 --- a/src/kernel/bitcoinkernel.cpp +++ b/src/kernel/bitcoinkernel.cpp @@ -982,13 +982,9 @@ btck_ChainstateManager* btck_chainstate_manager_create( LogError("Failed to verify loaded chain state from your datadir: %s", chainstate_err.original); return nullptr; } - - for (Chainstate* chainstate : WITH_LOCK(chainman->GetMutex(), return chainman->GetAll())) { - BlockValidationState state; - if (!chainstate->ActivateBestChain(state, nullptr)) { - LogError("Failed to connect best block: %s", state.ToString()); - return nullptr; - } + if (auto result = chainman->ActivateBestChains(); !result) { + LogError("%s", util::ErrorString(result).original); + return nullptr; } } catch (const std::exception& e) { LogError("Failed to load chainstate: %s", e.what()); diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index c5d26e5cbb0..0bb22d901b7 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -1270,16 +1270,8 @@ void ImportBlocks(ChainstateManager& chainman, std::span import_ } // scan for better chains in the block chain database, that are not yet connected in the active best chain - - // We can't hold cs_main during ActivateBestChain even though we're accessing - // the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve - // the relevant pointers before the ABC call. - for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) { - BlockValidationState state; - if (!chainstate->ActivateBestChain(state, nullptr)) { - chainman.GetNotifications().fatalError(strprintf(_("Failed to connect best block (%s)."), state.ToString())); - return; - } + if (auto result = chainman.ActivateBestChains(); !result) { + chainman.GetNotifications().fatalError(util::ErrorString(result)); } // End scope of ImportingNow } diff --git a/src/validation.cpp b/src/validation.cpp index 0d16dc209a6..cc080d017d7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5137,16 +5137,8 @@ void ChainstateManager::LoadExternalBlockFile( // until after all of the block files are loaded. ActivateBestChain can be // called by concurrent network message processing. but, that is not // reliable for the purpose of pruning while importing. - bool activation_failure = false; - for (auto c : GetAll()) { - BlockValidationState state; - if (!c->ActivateBestChain(state, pblock)) { - LogDebug(BCLog::REINDEX, "failed to activate chain (%s)\n", state.ToString()); - activation_failure = true; - break; - } - } - if (activation_failure) { + if (auto result{ActivateBestChains()}; !result) { + LogDebug(BCLog::REINDEX, "%s\n", util::ErrorString(result).original); break; } } @@ -6446,3 +6438,19 @@ std::optional> ChainstateManag if (!chainstate) return {}; return std::make_pair(chainstate->m_chain.Tip(), chainstate->TargetBlock()); } + +util::Result ChainstateManager::ActivateBestChains() +{ + // We can't hold cs_main during ActivateBestChain even though we're accessing + // the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve + // the relevant pointers before the ABC call. + AssertLockNotHeld(cs_main); + for (Chainstate* chainstate : GetAll()) { + BlockValidationState state; + if (!chainstate->ActivateBestChain(state, nullptr)) { + LOCK(GetMutex()); + return util::Error{Untranslated(strprintf("%s Failed to connect best block (%s)", chainstate->ToString(), state.ToString()))}; + } + } + return {}; +} diff --git a/src/validation.h b/src/validation.h index f330d90af4c..82fbf04d009 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1337,6 +1337,9 @@ public: //! Get range of historical blocks to download. std::optional> GetHistoricalBlockRange() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! Call ActivateBestChain() on every chainstate. + util::Result ActivateBestChains() LOCKS_EXCLUDED(::cs_main); + //! If, due to invalidation / reconsideration of blocks, the previous //! best header is no longer valid / guaranteed to be the most-work //! header in our block-index not known to be invalid, recalculate it. From ae85c495f1b507ca5871ea98f5d884fccb15adba Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 19:45:14 -0400 Subject: [PATCH 15/17] refactor: Delete ChainstateManager::GetAll() method Just use m_chainstates array instead. --- src/init.cpp | 6 +- src/kernel/bitcoinkernel.cpp | 2 +- src/node/blockstorage.cpp | 11 ++- src/node/chainstate.cpp | 24 +++--- src/test/util/validation.cpp | 5 +- .../validation_chainstatemanager_tests.cpp | 83 ++++++++++--------- src/validation.cpp | 41 +++++---- src/validation.h | 3 - 8 files changed, 95 insertions(+), 80 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 9c710d69152..80055292c11 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -345,7 +345,7 @@ void Shutdown(NodeContext& node) // FlushStateToDisk generates a ChainStateFlushed callback, which we should avoid missing if (node.chainman) { LOCK(cs_main); - for (Chainstate* chainstate : node.chainman->GetAll()) { + for (const auto& chainstate : node.chainman->m_chainstates) { if (chainstate->CanFlushToDisk()) { chainstate->ForceFlushStateToDisk(); } @@ -371,7 +371,7 @@ void Shutdown(NodeContext& node) if (node.chainman) { LOCK(cs_main); - for (Chainstate* chainstate : node.chainman->GetAll()) { + for (const auto& chainstate : node.chainman->m_chainstates) { if (chainstate->CanFlushToDisk()) { chainstate->ForceFlushStateToDisk(); chainstate->ResetCoinsViews(); @@ -1873,7 +1873,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) if (chainman.m_blockman.IsPruneMode()) { if (chainman.m_blockman.m_blockfiles_indexed) { LOCK(cs_main); - for (Chainstate* chainstate : chainman.GetAll()) { + for (const auto& chainstate : chainman.m_chainstates) { uiInterface.InitMessage(_("Pruning blockstore…")); chainstate->PruneAndFlush(); } diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp index 5bb1a06b094..15812baf00c 100644 --- a/src/kernel/bitcoinkernel.cpp +++ b/src/kernel/bitcoinkernel.cpp @@ -1009,7 +1009,7 @@ void btck_chainstate_manager_destroy(btck_ChainstateManager* chainman) { { LOCK(btck_ChainstateManager::get(chainman).m_chainman->GetMutex()); - for (Chainstate* chainstate : btck_ChainstateManager::get(chainman).m_chainman->GetAll()) { + for (const auto& chainstate : btck_ChainstateManager::get(chainman).m_chainman->m_chainstates) { if (chainstate->CanFlushToDisk()) { chainstate->ForceFlushStateToDisk(); chainstate->ResetCoinsViews(); diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 0bb22d901b7..ec95f11b0b9 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -318,9 +318,16 @@ void BlockManager::FindFilesToPrune( ChainstateManager& chainman) { LOCK2(cs_main, cs_LastBlockFile); - // Distribute our -prune budget over all chainstates. + // Compute `target` value with maximum size (in bytes) of blocks below the + // `last_prune` height which should be preserved and not pruned. The + // `target` value will be derived from the -prune preference provided by the + // user. If there is a historical chainstate being used to populate indexes + // and validate the snapshot, the target is divided by two so half of the + // block storage will be reserved for the historical chainstate, and the + // other half will be reserved for the most-work chainstate. + const int num_chainstates{chainman.HistoricalChainstate() ? 2 : 1}; const auto target = std::max( - MIN_DISK_SPACE_FOR_BLOCK_FILES, GetPruneTarget() / chainman.GetAll().size()); + MIN_DISK_SPACE_FOR_BLOCK_FILES, GetPruneTarget() / num_chainstates); const uint64_t target_sync_height = chainman.m_best_header->nHeight; if (chain.m_chain.Height() < 0 || target == 0) { diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp index 59d6de63e5a..4a8f8240c81 100644 --- a/src/node/chainstate.cpp +++ b/src/node/chainstate.cpp @@ -66,8 +66,8 @@ static ChainstateLoadResult CompleteChainstateInitialization( return {ChainstateLoadStatus::FAILURE, _("Error initializing block database")}; } - auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { - return options.wipe_chainstate_db || chainstate->CoinsTip().GetBestBlock().IsNull(); + auto is_coinsview_empty = [&](Chainstate& chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + return options.wipe_chainstate_db || chainstate.CoinsTip().GetBestBlock().IsNull(); }; assert(chainman.m_total_coinstip_cache > 0); @@ -78,12 +78,12 @@ static ChainstateLoadResult CompleteChainstateInitialization( // recalculated by `chainman.MaybeRebalanceCaches()`. The discount factor // is conservatively chosen such that the sum of the caches does not exceed // the allowable amount during this temporary initialization state. - double init_cache_fraction = chainman.GetAll().size() > 1 ? 0.2 : 1.0; + double init_cache_fraction = chainman.HistoricalChainstate() ? 0.2 : 1.0; // At this point we're either in reindex or we've loaded a useful // block tree into BlockIndex()! - for (Chainstate* chainstate : chainman.GetAll()) { + for (const auto& chainstate : chainman.m_chainstates) { LogInfo("Initializing chainstate %s", chainstate->ToString()); try { @@ -117,7 +117,7 @@ static ChainstateLoadResult CompleteChainstateInitialization( chainstate->InitCoinsCache(chainman.m_total_coinstip_cache * init_cache_fraction); assert(chainstate->CanFlushToDisk()); - if (!is_coinsview_empty(chainstate)) { + if (!is_coinsview_empty(*chainstate)) { // LoadChainTip initializes the chain based on CoinsTip()'s best block if (!chainstate->LoadChainTip()) { return {ChainstateLoadStatus::FAILURE, _("Error initializing block database")}; @@ -126,9 +126,9 @@ static ChainstateLoadResult CompleteChainstateInitialization( } } - auto chainstates{chainman.GetAll()}; + const auto& chainstates{chainman.m_chainstates}; if (std::any_of(chainstates.begin(), chainstates.end(), - [](const Chainstate* cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) { + [](const auto& cs) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return cs->NeedsRedownload(); })) { return {ChainstateLoadStatus::FAILURE, strprintf(_("Witness data for blocks after height %d requires validation. Please restart with -reindex."), chainman.GetConsensus().SegwitHeight)}; }; @@ -209,7 +209,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize // Because ValidatedSnapshotCleanup() has torn down chainstates with // ChainstateManager::ResetChainstates(), reinitialize them here without // duplicating the blockindex work above. - assert(chainman.GetAll().empty()); + assert(chainman.m_chainstates.empty()); chainman.InitializeChainstate(options.mempool); @@ -232,14 +232,14 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const ChainstateLoadOptions& options) { - auto is_coinsview_empty = [&](Chainstate* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { - return options.wipe_chainstate_db || chainstate->CoinsTip().GetBestBlock().IsNull(); + auto is_coinsview_empty = [&](Chainstate& chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + return options.wipe_chainstate_db || chainstate.CoinsTip().GetBestBlock().IsNull(); }; LOCK(cs_main); - for (Chainstate* chainstate : chainman.GetAll()) { - if (!is_coinsview_empty(chainstate)) { + for (auto& chainstate : chainman.m_chainstates) { + if (!is_coinsview_empty(*chainstate)) { const CBlockIndex* tip = chainstate->m_chain.Tip(); if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) { return {ChainstateLoadStatus::FAILURE, _("The block database contains a block which appears to be from the future. " diff --git a/src/test/util/validation.cpp b/src/test/util/validation.cpp index ae8642f9482..c199d912a38 100644 --- a/src/test/util/validation.cpp +++ b/src/test/util/validation.cpp @@ -16,8 +16,9 @@ void TestChainstateManager::DisableNextWrite() struct TestChainstate : public Chainstate { void ResetNextWrite() { m_next_write = NodeClock::time_point::max() - 1s; } }; - for (auto* cs : GetAll()) { - static_cast(cs)->ResetNextWrite(); + LOCK(::cs_main); + for (const auto& cs : m_chainstates) { + static_cast(*cs).ResetNextWrite(); } } diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 5a1ff8cd37d..539ae4530a5 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -40,18 +40,19 @@ BOOST_FIXTURE_TEST_SUITE(validation_chainstatemanager_tests, TestingSetup) BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup) { ChainstateManager& manager = *m_node.chainman; - std::vector chainstates; BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.CurrentChainstate().m_from_snapshot_blockhash)); // Create a legacy (IBD) chainstate. // Chainstate& c1 = manager.ActiveChainstate(); - chainstates.push_back(&c1); BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.CurrentChainstate().m_from_snapshot_blockhash)); - auto all = manager.GetAll(); - BOOST_CHECK_EQUAL_COLLECTIONS(all.begin(), all.end(), chainstates.begin(), chainstates.end()); + { + LOCK(manager.GetMutex()); + BOOST_CHECK_EQUAL(manager.m_chainstates.size(), 1); + BOOST_CHECK_EQUAL(manager.m_chainstates[0].get(), &c1); + } auto& active_chain = WITH_LOCK(manager.GetMutex(), return manager.ActiveChain()); BOOST_CHECK_EQUAL(&active_chain, &c1.m_chain); @@ -69,7 +70,6 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup) // const uint256 snapshot_blockhash = active_tip->GetBlockHash(); Chainstate& c2{WITH_LOCK(::cs_main, return manager.AddChainstate(std::make_unique(nullptr, manager.m_blockman, manager, snapshot_blockhash)))}; - chainstates.push_back(&c2); c2.InitCoinsDB( /*cache_size_bytes=*/1 << 23, /*in_memory=*/true, /*should_wipe=*/false); { @@ -86,8 +86,12 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup) BOOST_CHECK(WITH_LOCK(::cs_main, return manager.CurrentChainstate().m_assumeutxo == Assumeutxo::UNVALIDATED)); BOOST_CHECK_EQUAL(&c2, &manager.ActiveChainstate()); BOOST_CHECK(&c1 != &manager.ActiveChainstate()); - auto all2 = manager.GetAll(); - BOOST_CHECK_EQUAL_COLLECTIONS(all2.begin(), all2.end(), chainstates.begin(), chainstates.end()); + { + LOCK(manager.GetMutex()); + BOOST_CHECK_EQUAL(manager.m_chainstates.size(), 2); + BOOST_CHECK_EQUAL(manager.m_chainstates[0].get(), &c1); + BOOST_CHECK_EQUAL(manager.m_chainstates[1].get(), &c2); + } auto& active_chain2 = WITH_LOCK(manager.GetMutex(), return manager.ActiveChain()); BOOST_CHECK_EQUAL(&active_chain2, &c2.m_chain); @@ -292,7 +296,7 @@ struct SnapshotTestSetup : TestChain100Setup { LOCK(::cs_main); int chains_tested{0}; - for (Chainstate* chainstate : chainman.GetAll()) { + for (const auto& chainstate : chainman.m_chainstates) { BOOST_TEST_MESSAGE("Checking coins in " << chainstate->ToString()); CCoinsViewCache& coinscache = chainstate->CoinsTip(); @@ -325,10 +329,10 @@ struct SnapshotTestSetup : TestChain100Setup { size_t coins_in_background{0}; size_t coins_missing_from_background{0}; - for (Chainstate* chainstate : chainman.GetAll()) { + for (const auto& chainstate : chainman.m_chainstates) { BOOST_TEST_MESSAGE("Checking coins in " << chainstate->ToString()); CCoinsViewCache& coinscache = chainstate->CoinsTip(); - bool is_background = chainstate != &chainman.ActiveChainstate(); + bool is_background = chainstate.get() != &chainman.ActiveChainstate(); for (CTransactionRef& txn : m_coinbase_txns) { COutPoint op{txn->GetHash(), 0}; @@ -365,15 +369,17 @@ struct SnapshotTestSetup : TestChain100Setup { BOOST_TEST_MESSAGE("Simulating node restart"); { - for (Chainstate* cs : chainman.GetAll()) { - LOCK(::cs_main); - cs->ForceFlushStateToDisk(); + LOCK(chainman.GetMutex()); + for (const auto& cs : chainman.m_chainstates) { + if (cs->CanFlushToDisk()) cs->ForceFlushStateToDisk(); } + } + { // Process all callbacks referring to the old manager before wiping it. m_node.validation_signals->SyncWithValidationInterfaceQueue(); LOCK(::cs_main); chainman.ResetChainstates(); - BOOST_CHECK_EQUAL(chainman.GetAll().size(), 0); + BOOST_CHECK_EQUAL(chainman.m_chainstates.size(), 0); m_node.notifications = std::make_unique(Assert(m_node.shutdown_request), m_node.exit_status, *Assert(m_node.warnings)); const ChainstateManager::Options chainman_opts{ .chainparams = ::Params(), @@ -438,17 +444,16 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) BOOST_CHECK_EQUAL(assumed_tip->nHeight, 120); auto reload_all_block_indexes = [&]() { + LOCK(chainman.GetMutex()); // For completeness, we also reset the block sequence counters to // ensure that no state which affects the ranking of tip-candidates is // retained (even though this isn't strictly necessary). - WITH_LOCK(::cs_main, return chainman.ResetBlockSequenceCounters()); - for (Chainstate* cs : chainman.GetAll()) { - LOCK(::cs_main); + chainman.ResetBlockSequenceCounters(); + for (const auto& cs : chainman.m_chainstates) { cs->ClearBlockIndexCandidates(); BOOST_CHECK(cs->setBlockIndexCandidates.empty()); } - - WITH_LOCK(::cs_main, chainman.LoadBlockIndex()); + chainman.LoadBlockIndex(); }; // Ensure that without any assumed-valid BlockIndex entries, only the current tip is @@ -565,8 +570,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) const uint256 snapshot_tip_hash = WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip()->GetBlockHash()); - auto all_chainstates = chainman.GetAll(); - BOOST_CHECK_EQUAL(all_chainstates.size(), 2); + BOOST_CHECK_EQUAL(WITH_LOCK(chainman.GetMutex(), return chainman.m_chainstates.size()), 2); // "Rewind" the background chainstate so that its tip is not at the // base block of the snapshot - this is so after simulating a node restart, @@ -591,23 +595,22 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) // This call reinitializes the chainstates. this->LoadVerifyActivateChainstate(); - std::vector chainstates; { LOCK(chainman_restarted.GetMutex()); - chainstates = chainman_restarted.GetAll(); - BOOST_CHECK_EQUAL(chainstates.size(), 2); + BOOST_CHECK_EQUAL(chainman_restarted.m_chainstates.size(), 2); // Background chainstate has height of 109 not 110 here due to a quirk // of the LoadVerifyActivate only calling ActivateBestChain on one // chainstate. The height would be 110 after a real restart, but it's // fine for this test which is focused on the snapshot chainstate. - BOOST_CHECK_EQUAL(chainstates[0]->m_chain.Height(), 109); - BOOST_CHECK_EQUAL(chainstates[1]->m_chain.Height(), 210); + BOOST_CHECK_EQUAL(chainman_restarted.m_chainstates[0]->m_chain.Height(), 109); + BOOST_CHECK_EQUAL(chainman_restarted.m_chainstates[1]->m_chain.Height(), 210); BOOST_CHECK(chainman_restarted.CurrentChainstate().m_from_snapshot_blockhash); BOOST_CHECK(chainman_restarted.CurrentChainstate().m_assumeutxo == Assumeutxo::UNVALIDATED); BOOST_CHECK_EQUAL(chainman_restarted.ActiveTip()->GetBlockHash(), snapshot_tip_hash); BOOST_CHECK_EQUAL(chainman_restarted.ActiveHeight(), 210); + BOOST_CHECK_EQUAL(chainman_restarted.HistoricalChainstate()->m_chain.Height(), 109); } BOOST_TEST_MESSAGE( @@ -619,9 +622,10 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) // Background chainstate should be unaware of new blocks on the snapshot // chainstate, but the block disconnected above is now reattached. - BOOST_CHECK_EQUAL(chainstates[0]->m_chain.Height(), 110); - BOOST_CHECK_EQUAL(chainstates[1]->m_chain.Height(), 220); - BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 1); + BOOST_CHECK_EQUAL(chainman_restarted.m_chainstates.size(), 2); + BOOST_CHECK_EQUAL(chainman_restarted.m_chainstates[0]->m_chain.Height(), 110); + BOOST_CHECK_EQUAL(chainman_restarted.m_chainstates[1]->m_chain.Height(), 220); + BOOST_CHECK_EQUAL(chainman_restarted.HistoricalChainstate(), nullptr); } } @@ -651,16 +655,13 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.CurrentChainstate().m_assumeutxo == Assumeutxo::VALIDATED)); BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.CurrentChainstate().m_from_snapshot_blockhash)); + BOOST_CHECK_EQUAL(WITH_LOCK(chainman.GetMutex(), return chainman.HistoricalChainstate()), nullptr); // Cache should have been rebalanced and reallocated to the "only" remaining // chainstate. BOOST_CHECK(active_cs.m_coinstip_cache_size_bytes > tip_cache_before_complete); BOOST_CHECK(active_cs.m_coinsdb_cache_size_bytes > db_cache_before_complete); - auto all_chainstates = chainman.GetAll(); - BOOST_CHECK_EQUAL(all_chainstates.size(), 1); - BOOST_CHECK_EQUAL(all_chainstates[0], &active_cs); - // Trying completion again should return false. res = WITH_LOCK(::cs_main, return chainman.MaybeValidateSnapshot(validated_cs, active_cs)); BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::SKIPPED); @@ -690,7 +691,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup { LOCK(chainman_restarted.GetMutex()); - BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 1); + BOOST_CHECK_EQUAL(chainman_restarted.m_chainstates.size(), 1); BOOST_CHECK(!chainman_restarted.CurrentChainstate().m_from_snapshot_blockhash); BOOST_CHECK(active_cs2.m_coinstip_cache_size_bytes > tip_cache_before_complete); BOOST_CHECK(active_cs2.m_coinsdb_cache_size_bytes > db_cache_before_complete); @@ -737,10 +738,14 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna BOOST_CHECK_EQUAL(res, SnapshotCompletionResult::HASH_MISMATCH); } - auto all_chainstates = chainman.GetAll(); - BOOST_CHECK_EQUAL(all_chainstates.size(), 1); - BOOST_CHECK_EQUAL(all_chainstates[0], &validation_chainstate); - BOOST_CHECK_EQUAL(&chainman.ActiveChainstate(), &validation_chainstate); + { + LOCK(chainman.GetMutex()); + BOOST_CHECK_EQUAL(chainman.m_chainstates.size(), 2); + BOOST_CHECK(chainman.m_chainstates[0]->m_assumeutxo == Assumeutxo::VALIDATED); + BOOST_CHECK(!chainman.m_chainstates[0]->SnapshotBase()); + BOOST_CHECK(chainman.m_chainstates[1]->m_assumeutxo == Assumeutxo::INVALID); + BOOST_CHECK(chainman.m_chainstates[1]->SnapshotBase()); + } fs::path snapshot_invalid_dir = gArgs.GetDataDirNet() / "chainstate_snapshot_INVALID"; BOOST_CHECK(fs::exists(snapshot_invalid_dir)); @@ -761,7 +766,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna { LOCK(::cs_main); - BOOST_CHECK_EQUAL(chainman_restarted.GetAll().size(), 1); + BOOST_CHECK_EQUAL(chainman_restarted.m_chainstates.size(), 1); BOOST_CHECK(!chainman_restarted.CurrentChainstate().m_from_snapshot_blockhash); BOOST_CHECK_EQUAL(chainman_restarted.ActiveHeight(), 210); } diff --git a/src/validation.cpp b/src/validation.cpp index cc080d017d7..72872c5ce72 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3811,6 +3811,15 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) { void Chainstate::TryAddBlockIndexCandidate(CBlockIndex* pindex) { AssertLockHeld(cs_main); + + // Do not continue building a chainstate that is based on an invalid + // snapshot. This is a belt-and-suspenders type of check because if an + // invalid snapshot is loaded, the node will shut down to force a manual + // intervention. But it is good to handle this case correctly regardless. + if (m_assumeutxo == Assumeutxo::INVALID) { + return; + } + // The block only is a candidate for the most-work-chain if it has the same // or more work than our current tip. if (m_chain.Tip() != nullptr && setBlockIndexCandidates.value_comp()(pindex, m_chain.Tip())) { @@ -3877,7 +3886,7 @@ void ChainstateManager::ReceivedBlockTransactions(const CBlock& block, CBlockInd } pindex->m_chain_tx_count = prev_tx_sum(*pindex); pindex->nSequenceId = nBlockSequenceId++; - for (Chainstate *c : GetAll()) { + for (const auto& c : m_chainstates) { c->TryAddBlockIndexCandidate(pindex); } std::pair::iterator, std::multimap::iterator> range = m_blockman.m_blocks_unlinked.equal_range(pindex); @@ -4980,7 +4989,7 @@ bool ChainstateManager::LoadBlockIndex() (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && (pindex->HaveNumChainTxs() || pindex->pprev == nullptr))) { - for (Chainstate* chainstate : GetAll()) { + for (const auto& chainstate : m_chainstates) { chainstate->TryAddBlockIndexCandidate(pindex); } } @@ -5595,18 +5604,6 @@ double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) c return std::min(pindex->m_chain_tx_count / fTxTotal, 1.0); } -std::vector ChainstateManager::GetAll() -{ - LOCK(::cs_main); - std::vector out; - - for (const auto& cs : m_chainstates) { - if (cs && cs->m_assumeutxo != Assumeutxo::INVALID && !cs->m_target_utxohash) out.push_back(cs.get()); - } - - return out; -} - Chainstate& ChainstateManager::InitializeChainstate(CTxMemPool* mempool) { AssertLockHeld(::cs_main); @@ -6354,9 +6351,7 @@ bool ChainstateManager::ValidatedSnapshotCleanup(Chainstate& validated_cs, Chain // The caller of this method will be responsible for reinitializing chainstates // if they want to continue operation. this->ResetChainstates(); - - // No chainstates should be considered usable. - assert(this->GetAll().size() == 0); + assert(this->m_chainstates.size() == 0); LogInfo("[snapshot] deleting background chainstate directory (now unnecessary) (%s)", fs::PathToString(validated_path)); @@ -6445,7 +6440,17 @@ util::Result ChainstateManager::ActivateBestChains() // the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve // the relevant pointers before the ABC call. AssertLockNotHeld(cs_main); - for (Chainstate* chainstate : GetAll()) { + std::vector chainstates; + { + LOCK(GetMutex()); + chainstates.reserve(m_chainstates.size()); + for (const auto& chainstate : m_chainstates) { + if (chainstate && chainstate->m_assumeutxo != Assumeutxo::INVALID && !chainstate->m_target_utxohash) { + chainstates.push_back(chainstate.get()); + } + } + } + for (Chainstate* chainstate : chainstates) { BlockValidationState state; if (!chainstate->ActivateBestChain(state, nullptr)) { LOCK(GetMutex()); diff --git a/src/validation.h b/src/validation.h index 82fbf04d009..e14dac74bbd 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1081,9 +1081,6 @@ public: // constructor Chainstate& InitializeChainstate(CTxMemPool* mempool) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Get all chainstates currently being used. - std::vector GetAll(); - //! Construct and activate a Chainstate on the basis of UTXO snapshot data. //! //! Steps: From af455dcb39dbd53700105e29c87de5db65ecf43c Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 14 Mar 2025 22:19:17 +0100 Subject: [PATCH 16/17] refactor: Simplify pruning functions Move GetPruneRange from ChainstateManager to Chainstate. --- src/node/blockstorage.cpp | 7 +++---- src/node/blockstorage.h | 3 +-- src/validation.cpp | 12 ++++++------ src/validation.h | 11 +++++------ 4 files changed, 15 insertions(+), 18 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index ec95f11b0b9..41931a81090 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -284,8 +284,7 @@ void BlockManager::PruneOneBlockFile(const int fileNumber) void BlockManager::FindFilesToPruneManual( std::set& setFilesToPrune, int nManualPruneHeight, - const Chainstate& chain, - ChainstateManager& chainman) + const Chainstate& chain) { assert(IsPruneMode() && nManualPruneHeight > 0); @@ -294,7 +293,7 @@ void BlockManager::FindFilesToPruneManual( return; } - const auto [min_block_to_prune, last_block_can_prune] = chainman.GetPruneRange(chain, nManualPruneHeight); + const auto [min_block_to_prune, last_block_can_prune] = chain.GetPruneRange(nManualPruneHeight); int count = 0; for (int fileNumber = 0; fileNumber < this->MaxBlockfileNum(); fileNumber++) { @@ -337,7 +336,7 @@ void BlockManager::FindFilesToPrune( return; } - const auto [min_block_to_prune, last_block_can_prune] = chainman.GetPruneRange(chain, last_prune); + const auto [min_block_to_prune, last_block_can_prune] = chain.GetPruneRange(last_prune); uint64_t nCurrentUsage = CalculateCurrentUsage(); // We don't check to prune until after we've allocated new space for files diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 9ba7873cdca..70baa273eb8 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -218,8 +218,7 @@ private: void FindFilesToPruneManual( std::set& setFilesToPrune, int nManualPruneHeight, - const Chainstate& chain, - ChainstateManager& chainman); + const Chainstate& chain); /** * Prune block and undo files (blk???.dat and rev???.dat) so that the disk space used is less than a user-defined target. diff --git a/src/validation.cpp b/src/validation.cpp index 72872c5ce72..9ecfdfa1b1a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2785,7 +2785,7 @@ bool Chainstate::FlushStateToDisk( m_blockman.FindFilesToPruneManual( setFilesToPrune, std::min(last_prune, nManualPruneHeight), - *this, m_chainman); + *this); } else { LOG_TIME_MILLIS_WITH_CATEGORY("find files to prune", BCLog::BENCH); @@ -6399,22 +6399,22 @@ bool ChainstateManager::ValidatedSnapshotCleanup(Chainstate& validated_cs, Chain return true; } -std::pair ChainstateManager::GetPruneRange(const Chainstate& chainstate, int last_height_can_prune) +std::pair Chainstate::GetPruneRange(int last_height_can_prune) const { - if (chainstate.m_chain.Height() <= 0) { + if (m_chain.Height() <= 0) { return {0, 0}; } int prune_start{0}; - if (chainstate.m_from_snapshot_blockhash && chainstate.m_assumeutxo != Assumeutxo::VALIDATED) { + if (m_from_snapshot_blockhash && m_assumeutxo != Assumeutxo::VALIDATED) { // Only prune blocks _after_ the snapshot if this is a snapshot chain // that has not been fully validated yet. The earlier blocks need to be // kept to validate the snapshot - prune_start = Assert(chainstate.SnapshotBase())->nHeight + 1; + prune_start = Assert(SnapshotBase())->nHeight + 1; } int max_prune = std::max( - 0, chainstate.m_chain.Height() - static_cast(MIN_BLOCKS_TO_KEEP)); + 0, m_chain.Height() - static_cast(MIN_BLOCKS_TO_KEEP)); // last block to prune is the lesser of (caller-specified height, MIN_BLOCKS_TO_KEEP from the tip) // diff --git a/src/validation.h b/src/validation.h index e14dac74bbd..e810e872e87 100644 --- a/src/validation.h +++ b/src/validation.h @@ -835,6 +835,11 @@ public: return m_mempool ? &m_mempool->cs : nullptr; } + //! Return the [start, end] (inclusive) of block heights we can prune. + //! + //! start > end is possible, meaning no blocks can be pruned. + std::pair GetPruneRange(int last_height_can_prune) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + protected: bool ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs); bool ConnectTip( @@ -1325,12 +1330,6 @@ public: //! @sa node/chainstate:LoadChainstate() bool ValidatedSnapshotCleanup(Chainstate& validated_cs, Chainstate& unvalidated_cs) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Return the [start, end] (inclusive) of block heights we can prune. - //! - //! start > end is possible, meaning no blocks can be pruned. - std::pair GetPruneRange( - const Chainstate& chainstate, int last_height_can_prune) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Get range of historical blocks to download. std::optional> GetHistoricalBlockRange() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); From 82be652e40ec7e1bea4b260ee804a92a3e05f496 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 19:50:47 -0400 Subject: [PATCH 17/17] doc: Improve ChainstateManager documentation, use consistent terms --- src/validation.cpp | 2 +- src/validation.h | 53 ++++++++++++++++++++-------------------------- 2 files changed, 24 insertions(+), 31 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 9ecfdfa1b1a..2052748343a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -6120,7 +6120,7 @@ SnapshotCompletionResult ChainstateManager::MaybeValidateSnapshot(Chainstate& va return SnapshotCompletionResult::STATS_FAILED; } - // Compare the background validation chainstate's UTXO set hash against the hard-coded + // Compare the validated chainstate's UTXO set hash against the hard-coded // assumeutxo hash we expect. // // TODO: For belt-and-suspenders, we could cache the UTXO set diff --git a/src/validation.h b/src/validation.h index e810e872e87..77958b53dd1 100644 --- a/src/validation.h +++ b/src/validation.h @@ -756,9 +756,9 @@ public: * validationinterface callback. * * Note that if this is called while a snapshot chainstate is active, and if - * it is called on a background chainstate whose tip has reached the base block - * of the snapshot, its execution will take *MINUTES* while it hashes the - * background UTXO set to verify the assumeutxo value the snapshot was activated + * it is called on a validated chainstate whose tip has reached the base + * block of the snapshot, its execution will take *MINUTES* while it hashes + * the UTXO set to verify the assumeutxo value the snapshot was activated * with. `cs_main` will be held during this time. * * @returns true unless a system error occurred @@ -897,41 +897,34 @@ enum class SnapshotCompletionResult { // base block. MISSING_CHAINPARAMS, - // Failed to generate UTXO statistics (to check UTXO set hash) for the background - // chainstate. + // Failed to generate UTXO statistics (to check UTXO set hash) for the + // validated chainstate. STATS_FAILED, - // The UTXO set hash of the background validation chainstate does not match - // the one expected by assumeutxo chainparams. + // The UTXO set hash of the validated chainstate does not match the one + // expected by assumeutxo chainparams. HASH_MISMATCH, }; /** - * Provides an interface for creating and interacting with one or two - * chainstates: an IBD chainstate generated by downloading blocks, and - * an optional snapshot chainstate loaded from a UTXO snapshot. Managed - * chainstates can be maintained at different heights simultaneously. + * Interface for managing multiple \ref Chainstate objects, where each + * chainstate is associated with chainstate* subdirectory in the data directory + * and contains a database of UTXOs existing at a different point in history. + * (See \ref Chainstate class for more information.) * - * This class provides abstractions that allow the retrieval of the current - * most-work chainstate ("Active") as well as chainstates which may be in - * background use to validate UTXO snapshots. + * Normally there is exactly one Chainstate, which contains the UTXO set of + * chain tip if syncing is completed, or the UTXO set the most recent validated + * block if the initial sync is still in progress. * - * Definitions: - * - * *IBD chainstate*: a chainstate whose current state has been "fully" - * validated by the initial block download process. - * - * *Snapshot chainstate*: a chainstate populated by loading in an - * assumeutxo UTXO snapshot. - * - * *Active chainstate*: the chainstate containing the current most-work - * chain. Consulted by most parts of the system (net_processing, - * wallet) as a reflection of the current chain and UTXO set. - * This may either be an IBD chainstate or a snapshot chainstate. - * - * *Background IBD chainstate*: an IBD chainstate for which the - * IBD process is happening in the background while use of the - * active (snapshot) chainstate allows the rest of the system to function. + * However, if an assumeutxo snapshot is loaded before syncing is completed, + * there will be two chainstates. The original fully validated chainstate will + * continue to exist and download new blocks in the background. But the new + * snapshot which is loaded will become a second chainstate. The second + * chainstate will be used as the chain tip for the wallet and RPCs even though + * it is only assumed to be valid. When the initial chainstate catches up to the + * snapshot height and confirms that the assumeutxo snapshot is actually valid, + * the second chainstate will be marked validated and become the only chainstate + * again. */ class ChainstateManager {