From e514fe61168109bd467d7cb2ac7561442b17b5f6 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 30 May 2024 16:30:22 -0400 Subject: [PATCH] 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;