Merge bitcoin/bitcoin#28218: refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate

fanquake · fanquake · commit 723f1c669f9b · 2023-08-21T10:55:35.000+01:00
94a98fb assumeutxo cleanup: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager (Ryan Ofsky) Pull request description: This change makes `IsInitialBlockDownload` and `NotifyHeaderTip` functions no longer tied to individual `Chainstate` objects. It makes them work with the `ChainstateManager` object instead so code is simpler and it is no longer possible to call them incorrectly with an inactive `Chainstate`. This change also makes `m_cached_finished_ibd` caching easier to reason about, because now there is only one cached value instead of two (for background and snapshot chainstates) so the cached IBD state now no longer gets reset when a snapshot is loaded. There should be no change in behavior because these functions were always called on the active `ChainState` objects. These changes were discussed previously bitcoin/bitcoin#27746 (comment) and bitcoin/bitcoin#27746 (comment) as possible followups for that PR. ACKs for top commit: MarcoFalke: re-ACK 94a98fb 🐺 naumenkogs: ACK 94a98fb dergoegge: reACK 94a98fb Tree-SHA512: 374d6e5c9bbc7564c143f634bd709a4e8f4a42c8d77e7a8554c832acdcf60fa2a134f3ea10827db1a1e0191006496329c0ebf5c64f3ab868398c3722bb7ff56f
diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
@@ -163,7 +163,7 @@ int main(int argc, char* argv[])
         << "\t" << "Reindexing: " << std::boolalpha << node::fReindex.load() << std::noboolalpha << std::endl
         << "\t" << "Snapshot Active: " << std::boolalpha << chainman.IsSnapshotActive() << std::noboolalpha << std::endl
         << "\t" << "Active Height: " << chainman.ActiveHeight() << std::endl
-        << "\t" << "Active IBD: " << std::boolalpha << chainman.ActiveChainstate().IsInitialBlockDownload() << std::noboolalpha << std::endl;
+        << "\t" << "Active IBD: " << std::boolalpha << chainman.IsInitialBlockDownload() << std::noboolalpha << std::endl;
         CBlockIndex* tip = chainman.ActiveTip();
         if (tip) {
             std::cout << "\t" << tip->ToString() << std::endl;
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -2010,7 +2010,7 @@ void PeerManagerImpl::BlockChecked(const CBlock& block, const BlockValidationSta
     //    the tip yet so we have no way to check this directly here. Instead we
     //    just check that there are currently no other blocks in flight.
     else if (state.IsValid() &&
-             !m_chainman.ActiveChainstate().IsInitialBlockDownload() &&
+             !m_chainman.IsInitialBlockDownload() &&
              mapBlocksInFlight.count(hash) == mapBlocksInFlight.size()) {
         if (it != mapBlockSource.end()) {
             MaybeSetPeerAsAnnouncingHeaderAndIDs(it->second.first);
@@ -2729,7 +2729,7 @@ void PeerManagerImpl::UpdatePeerStateForReceivedHeaders(CNode& pfrom, Peer& peer
 
     // If we're in IBD, we want outbound peers that will serve us a useful
     // chain. Disconnect peers that are on chains with insufficient work.
-    if (m_chainman.ActiveChainstate().IsInitialBlockDownload() && !may_have_more_headers) {
+    if (m_chainman.IsInitialBlockDownload() && !may_have_more_headers) {
         // If the peer has no more headers to give us, then we know we have
         // their tip.
         if (nodestate->pindexBestKnownBlock && nodestate->pindexBestKnownBlock->nChainWork < m_chainman.MinimumChainWork()) {
@@ -3808,7 +3808,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                 LogPrint(BCLog::NET, "got inv: %s  %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId());
 
                 AddKnownTx(*peer, inv.hash);
-                if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
+                if (!fAlreadyHave && !m_chainman.IsInitialBlockDownload()) {
                     AddTxAnnouncement(pfrom, gtxid, current_time);
                 }
             } else {
@@ -4080,7 +4080,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         // Stop processing the transaction early if we are still in IBD since we don't
         // have enough information to validate it yet. Sending unsolicited transactions
         // is not considered a protocol violation, so don't punish the peer.
-        if (m_chainman.ActiveChainstate().IsInitialBlockDownload()) return;
+        if (m_chainman.IsInitialBlockDownload()) return;
 
         CTransactionRef ptx;
         vRecv >> ptx;
@@ -4284,7 +4284,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         const CBlockIndex* prev_block = m_chainman.m_blockman.LookupBlockIndex(cmpctblock.header.hashPrevBlock);
         if (!prev_block) {
             // Doesn't connect (or is genesis), instead of DoSing in AcceptBlockHeader, request deeper headers
-            if (!m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
+            if (!m_chainman.IsInitialBlockDownload()) {
                 MaybeSendGetHeaders(pfrom, GetLocator(m_chainman.m_best_header), *peer);
             }
             return;
@@ -5228,7 +5228,7 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
 
     LOCK(peer.m_addr_send_times_mutex);
     // Periodically advertise our local address to the peer.
-    if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload() &&
+    if (fListen && !m_chainman.IsInitialBlockDownload() &&
         peer.m_next_local_addr_send < current_time) {
         // If we've sent before, clear the bloom filter for the peer, so that our
         // self-announcement will actually go out.
@@ -5323,7 +5323,7 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi
     CAmount currentFilter = m_mempool.GetMinFee().GetFeePerK();
     static FeeFilterRounder g_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}};
 
-    if (m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
+    if (m_chainman.IsInitialBlockDownload()) {
         // Received tx-inv messages are discarded when the active
         // chainstate is in IBD, so tell the peer to not send them.
         currentFilter = MAX_MONEY;
@@ -5827,7 +5827,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
         // Message: getdata (blocks)
         //
         std::vector<CInv> vGetData;
-        if (CanServeBlocks(*peer) && ((sync_blocks_and_headers_from_peer && !IsLimitedPeer(*peer)) || !m_chainman.ActiveChainstate().IsInitialBlockDownload()) && state.vBlocksInFlight.size() < MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
+        if (CanServeBlocks(*peer) && ((sync_blocks_and_headers_from_peer && !IsLimitedPeer(*peer)) || !m_chainman.IsInitialBlockDownload()) && state.vBlocksInFlight.size() < MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
             std::vector<const CBlockIndex*> vToDownload;
             NodeId staller = -1;
             FindNextBlocksToDownload(*peer, MAX_BLOCKS_IN_TRANSIT_PER_PEER - state.vBlocksInFlight.size(), vToDownload, staller);
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
@@ -298,8 +298,9 @@ class NodeImpl : public Node
     {
         return GuessVerificationProgress(chainman().GetParams().TxData(), WITH_LOCK(::cs_main, return chainman().ActiveChain().Tip()));
     }
-    bool isInitialBlockDownload() override {
-        return chainman().ActiveChainstate().IsInitialBlockDownload();
+    bool isInitialBlockDownload() override
+    {
+        return chainman().IsInitialBlockDownload();
     }
     bool isLoadingBlocks() override { return chainman().m_blockman.LoadingBlocks(); }
     void setNetworkActive(bool active) override
@@ -720,7 +721,7 @@ class ChainImpl : public Chain
     bool isReadyToBroadcast() override { return !chainman().m_blockman.LoadingBlocks() && !isInitialBlockDownload(); }
     bool isInitialBlockDownload() override
     {
-        return chainman().ActiveChainstate().IsInitialBlockDownload();
+        return chainman().IsInitialBlockDownload();
     }
     bool shutdownRequested() override { return ShutdownRequested(); }
     void initMessage(const std::string& message) override { ::uiInterface.InitMessage(message); }
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
@@ -1260,7 +1260,7 @@ RPCHelpMan getblockchaininfo()
     obj.pushKV("time", tip.GetBlockTime());
     obj.pushKV("mediantime", tip.GetMedianTimePast());
     obj.pushKV("verificationprogress", GuessVerificationProgress(chainman.GetParams().TxData(), &tip));
-    obj.pushKV("initialblockdownload", active_chainstate.IsInitialBlockDownload());
+    obj.pushKV("initialblockdownload", chainman.IsInitialBlockDownload());
     obj.pushKV("chainwork", tip.nChainWork.GetHex());
     obj.pushKV("size_on_disk", chainman.m_blockman.CalculateCurrentUsage());
     obj.pushKV("pruned", chainman.m_blockman.IsPruneMode());
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
@@ -752,9 +752,10 @@ static RPCHelpMan importmempool()
             const NodeContext& node{EnsureAnyNodeContext(request.context)};
 
             CTxMemPool& mempool{EnsureMemPool(node)};
-            Chainstate& chainstate = EnsureChainman(node).ActiveChainstate();
+            ChainstateManager& chainman = EnsureChainman(node);
+            Chainstate& chainstate = chainman.ActiveChainstate();
 
-            if (chainstate.IsInitialBlockDownload()) {
+            if (chainman.IsInitialBlockDownload()) {
                 throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, "Can only import the mempool after the block download and sync is done.");
             }
 
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
@@ -706,7 +706,7 @@ static RPCHelpMan getblocktemplate()
             throw JSONRPCError(RPC_CLIENT_NOT_CONNECTED, PACKAGE_NAME " is not connected!");
         }
 
-        if (active_chainstate.IsInitialBlockDownload()) {
+        if (chainman.IsInitialBlockDownload()) {
             throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, PACKAGE_NAME " is in initial sync and waiting for blocks...");
         }
     }
diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
@@ -63,9 +63,9 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
 
     ConnmanTestMsg& connman = *static_cast<ConnmanTestMsg*>(g_setup->m_node.connman.get());
-    TestChainState& chainstate = *static_cast<TestChainState*>(&g_setup->m_node.chainman->ActiveChainstate());
+    auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
     SetMockTime(1610000000); // any time to successfully reset ibd
-    chainstate.ResetIbd();
+    chainman.ResetIbd();
 
     LOCK(NetEventsInterface::g_msgproc_mutex);
 
diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp
@@ -38,9 +38,9 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
 
     ConnmanTestMsg& connman = *static_cast<ConnmanTestMsg*>(g_setup->m_node.connman.get());
-    TestChainState& chainstate = *static_cast<TestChainState*>(&g_setup->m_node.chainman->ActiveChainstate());
+    auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
     SetMockTime(1610000000); // any time to successfully reset ibd
-    chainstate.ResetIbd();
+    chainman.ResetIbd();
 
     LOCK(NetEventsInterface::g_msgproc_mutex);
 
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
@@ -841,11 +841,10 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
     const int64_t time{0};
     const CNetMsgMaker msg_maker{PROTOCOL_VERSION};
 
-    // Force Chainstate::IsInitialBlockDownload() to return false.
+    // Force ChainstateManager::IsInitialBlockDownload() to return false.
     // Otherwise PushAddress() isn't called by PeerManager::ProcessMessage().
-    TestChainState& chainstate =
-        *static_cast<TestChainState*>(&m_node.chainman->ActiveChainstate());
-    chainstate.JumpOutOfIbd();
+    auto& chainman = static_cast<TestChainstateManager&>(*m_node.chainman);
+    chainman.JumpOutOfIbd();
 
     m_node.peerman->InitializeNode(peer, NODE_NETWORK);
 
@@ -895,7 +894,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
     BOOST_CHECK(sent);
 
     CaptureMessage = CaptureMessageOrig;
-    chainstate.ResetIbd();
+    chainman.ResetIbd();
     m_node.args->ForceSetArg("-capturemessages", "0");
     m_node.args->ForceSetArg("-bind", "");
     // PeerManager::ProcessMessage() calls AddTimeData() which changes the internal state
diff --git a/src/test/util/validation.cpp b/src/test/util/validation.cpp
@@ -9,13 +9,13 @@
 #include <validation.h>
 #include <validationinterface.h>
 
-void TestChainState::ResetIbd()
+void TestChainstateManager::ResetIbd()
 {
     m_cached_finished_ibd = false;
     assert(IsInitialBlockDownload());
 }
 
-void TestChainState::JumpOutOfIbd()
+void TestChainstateManager::JumpOutOfIbd()
 {
     Assert(IsInitialBlockDownload());
     m_cached_finished_ibd = true;
diff --git a/src/test/util/validation.h b/src/test/util/validation.h
@@ -9,7 +9,7 @@
 
 class CValidationInterface;
 
-struct TestChainState : public Chainstate {
+struct TestChainstateManager : public ChainstateManager {
     /** Reset the ibd cache to its initial state */
     void ResetIbd();
     /** Toggle IsInitialBlockDownload from true to false */
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
@@ -13,6 +13,7 @@
 #include <test/util/logging.h>
 #include <test/util/random.h>
 #include <test/util/setup_common.h>
+#include <test/util/validation.h>
 #include <timedata.h>
 #include <uint256.h>
 #include <validation.h>
@@ -143,14 +144,21 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_rebalance_caches, TestChain100Setup)
     c2.InitCoinsDB(
         /*cache_size_bytes=*/1 << 23, /*in_memory=*/true, /*should_wipe=*/false);
 
+    // Reset IBD state so IsInitialBlockDownload() returns true and causes
+    // MaybeRebalancesCaches() to prioritize the snapshot chainstate, giving it
+    // more cache space than the snapshot chainstate. Calling ResetIbd() is
+    // necessary because m_cached_finished_ibd is already latched to true before
+    // the test starts due to the test setup. After ResetIbd() is called.
+    // IsInitialBlockDownload will return true because at this point the active
+    // chainstate has a null chain tip.
+    static_cast<TestChainstateManager&>(manager).ResetIbd();
+
     {
         LOCK(::cs_main);
         c2.InitCoinsCache(1 << 23);
         manager.MaybeRebalanceCaches();
     }
 
-    // Since both chainstates are considered to be in initial block download,
-    // the snapshot chainstate should take priority.
     BOOST_CHECK_CLOSE(c1.m_coinstip_cache_size_bytes, max_cache * 0.05, 1);
     BOOST_CHECK_CLOSE(c1.m_coinsdb_cache_size_bytes, max_cache * 0.05, 1);
     BOOST_CHECK_CLOSE(c2.m_coinstip_cache_size_bytes, max_cache * 0.95, 1);
diff --git a/src/validation.cpp b/src/validation.cpp
diff --git a/src/validation.h b/src/validation.h

Original file line number	Diff line number	Diff line change
`@@ -298,8 +298,9 @@ class NodeImpl : public Node`
`298`	`298`	`{`
`299`	`299`	`return GuessVerificationProgress(chainman().GetParams().TxData(), WITH_LOCK(::cs_main, return chainman().ActiveChain().Tip()));`
`300`	`300`	`}`
`301`		`- bool isInitialBlockDownload() override {`
`302`		`- return chainman().ActiveChainstate().IsInitialBlockDownload();`
	`301`	`+ bool isInitialBlockDownload() override`
	`302`	`+ {`
	`303`	`+ return chainman().IsInitialBlockDownload();`
`303`	`304`	`}`
`304`	`305`	`bool isLoadingBlocks() override { return chainman().m_blockman.LoadingBlocks(); }`
`305`	`306`	`void setNetworkActive(bool active) override`
`@@ -720,7 +721,7 @@ class ChainImpl : public Chain`
`720`	`721`	`bool isReadyToBroadcast() override { return !chainman().m_blockman.LoadingBlocks() && !isInitialBlockDownload(); }`
`721`	`722`	`bool isInitialBlockDownload() override`
`722`	`723`	`{`
`723`		`- return chainman().ActiveChainstate().IsInitialBlockDownload();`
	`724`	`+ return chainman().IsInitialBlockDownload();`
`724`	`725`	`}`
`725`	`726`	`bool shutdownRequested() override { return ShutdownRequested(); }`
`726`	`727`	`void initMessage(const std::string& message) override { ::uiInterface.InitMessage(message); }`
Original file line number	Diff line number	Diff line change
`@@ -706,7 +706,7 @@ static RPCHelpMan getblocktemplate()`
`706`	`706`	`throw JSONRPCError(RPC_CLIENT_NOT_CONNECTED, PACKAGE_NAME " is not connected!");`
`707`	`707`	`}`
`708`	`708`
`709`		`- if (active_chainstate.IsInitialBlockDownload()) {`
	`709`	`+ if (chainman.IsInitialBlockDownload()) {`
`710`	`710`	`throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, PACKAGE_NAME " is in initial sync and waiting for blocks...");`
`711`	`711`	`}`
`712`	`712`	`}`
Original file line number	Diff line number	Diff line change
`@@ -9,13 +9,13 @@`
`9`	`9`	`#include <validation.h>`
`10`	`10`	`#include <validationinterface.h>`
`11`	`11`
`12`		`-void TestChainState::ResetIbd()`
	`12`	`+void TestChainstateManager::ResetIbd()`
`13`	`13`	`{`
`14`	`14`	`m_cached_finished_ibd = false;`
`15`	`15`	`assert(IsInitialBlockDownload());`
`16`	`16`	`}`
`17`	`17`
`18`		`-void TestChainState::JumpOutOfIbd()`
	`18`	`+void TestChainstateManager::JumpOutOfIbd()`
`19`	`19`	`{`
`20`	`20`	`Assert(IsInitialBlockDownload());`
`21`	`21`	`m_cached_finished_ibd = true;`