From d9e82299fc4e45fbc0f5a34dcbb1d51397d0bd35 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 31 May 2024 07:00:40 -0400 Subject: [PATCH] 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?