tests: remove ResetIbd and JumpOutOfIbd helpers, update test logic to use IsInitialBlockDownload

src/chain.h

@@ -420,6 +420,15 @@ class CChain
         return int(vChain.size()) - 1;
     }
 
+    /** Check whether this chain's tip exists, has enough work, and is recent. */
+    bool IsTipRecent(const arith_uint256& min_chain_work, std::chrono::seconds max_tip_age) const
+    {
+        const auto tip{Tip()};
+        return tip &&
+               tip->nChainWork >= min_chain_work &&
+               tip->Time() >= Now<NodeSeconds>() - max_tip_age;
+    }
+
     /** Set/initialize a chain with a given tip. */
     void SetTip(CBlockIndex& block);
 

src/test/fuzz/p2p_handshake.cpp

@@ -18,6 +18,7 @@
 #include <test/util/net.h>
 #include <test/util/setup_common.h>
 #include <test/util/validation.h>
+#include <util/check.h>
 #include <util/time.h>
 #include <validationinterface.h>
 
@@ -43,9 +44,9 @@ FUZZ_TARGET(p2p_handshake, .init = ::initialize)
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
 
     auto& connman = static_cast<ConnmanTestMsg&>(*g_setup->m_node.connman);
-    auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
+    auto& chainman = *g_setup->m_node.chainman;
     SetMockTime(1610000000); // any time to successfully reset ibd
-    chainman.ResetIbd();
+    Assert(chainman.IsInitialBlockDownload());
 
     node::Warnings warnings{};
     NetGroupManager netgroupman{{}};

src/test/fuzz/process_message.cpp

@@ -35,7 +35,7 @@ std::string_view LIMIT_TO_MESSAGE_TYPE{};
 
 void ResetChainman(TestingSetup& setup)
 {
-    SetMockTime(setup.m_node.chainman->GetParams().GenesisBlock().Time());
+    SetMockTime(setup.m_node.chainman->GetParams().GenesisBlock().Time() + std::chrono::seconds{48 * 60 * 60});
     setup.m_node.chainman.reset();
     setup.m_make_chainman();
     setup.LoadVerifyActivateChainstate();
@@ -71,9 +71,8 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
     connman.ResetAddrCache();
     connman.ResetMaxOutboundCycle();
     auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
+    Assert(chainman.IsInitialBlockDownload());
     const auto block_index_size{WITH_LOCK(chainman.GetMutex(), return chainman.BlockIndex().size())};
-    SetMockTime(1610000000); // any time to successfully reset ibd
-    chainman.ResetIbd();
     chainman.DisableNextWrite();
 
     node::Warnings warnings{};

src/test/fuzz/process_messages.cpp

@@ -17,6 +17,7 @@
 #include <test/util/net.h>
 #include <test/util/setup_common.h>
 #include <test/util/validation.h>
+#include <util/check.h>
 #include <util/time.h>
 #include <validationinterface.h>
 
@@ -30,7 +31,7 @@ TestingSetup* g_setup;
 
 void ResetChainman(TestingSetup& setup)
 {
-    SetMockTime(setup.m_node.chainman->GetParams().GenesisBlock().Time());
+    SetMockTime(setup.m_node.chainman->GetParams().GenesisBlock().Time() + std::chrono::seconds{48 * 60 * 60});
     setup.m_node.chainman.reset();
     setup.m_make_chainman();
     setup.LoadVerifyActivateChainstate();
@@ -61,9 +62,8 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
     connman.ResetAddrCache();
     connman.ResetMaxOutboundCycle();
     auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
+    Assert(chainman.IsInitialBlockDownload());
     const auto block_index_size{WITH_LOCK(chainman.GetMutex(), return chainman.BlockIndex().size())};
-    SetMockTime(1610000000); // any time to successfully reset ibd
-    chainman.ResetIbd();
     chainman.DisableNextWrite();
 
     node::Warnings warnings{};

src/test/net_tests.cpp

@@ -16,6 +16,7 @@
 #include <serialize.h>
 #include <span.h>
 #include <streams.h>
+#include <test/util/mining.h>
 #include <test/util/random.h>
 #include <test/util/setup_common.h>
 #include <test/util/validation.h>
@@ -801,8 +802,6 @@ BOOST_AUTO_TEST_CASE(LocalAddress_BasicLifecycle)
 
 BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
 {
-    LOCK(NetEventsInterface::g_msgproc_mutex);
-
     // Tests the following scenario:
     // * -bind=3.4.5.6:20001 is specified
     // * we make an outbound connection to a peer
@@ -838,10 +837,15 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
     const uint64_t services{NODE_NETWORK | NODE_WITNESS};
     const int64_t time{0};
 
-    // Force ChainstateManager::IsInitialBlockDownload() to return false.
-    // Otherwise PushAddress() isn't called by PeerManager::ProcessMessage().
-    auto& chainman = static_cast<TestChainstateManager&>(*m_node.chainman);
-    chainman.JumpOutOfIbd();
+    // Ensure ChainstateManager::IsInitialBlockDownload() is false, otherwise
+    // PushAddress() isn't called by PeerManager::ProcessMessage().
+    BOOST_CHECK(m_node.chainman->IsInitialBlockDownload());
+    SetMockTime(WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Tip()->GetMedianTimePast() + 1));
+    MineBlock(m_node, {});
+    BOOST_CHECK(!m_node.chainman->IsInitialBlockDownload());
+    BOOST_CHECK(m_node.chainman->m_cached_finished_ibd.load(std::memory_order_relaxed));
+
+    LOCK(NetEventsInterface::g_msgproc_mutex);
 
     m_node.peerman->InitializeNode(peer, NODE_NETWORK);
 
@@ -891,7 +895,6 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
     BOOST_CHECK(sent);
 
     CaptureMessage = CaptureMessageOrig;
-    chainman.ResetIbd();
     m_node.connman->SetCaptureMessages(false);
     m_node.args->ForceSetArg("-bind", "");
 }

src/test/util/validation.cpp

@@ -30,19 +30,6 @@ void TestChainstateManager::DisableNextWrite()
     }
 }
 
-void TestChainstateManager::ResetIbd()
-{
-    m_cached_finished_ibd = false;
-    assert(IsInitialBlockDownload());
-}
-
-void TestChainstateManager::JumpOutOfIbd()
-{
-    Assert(IsInitialBlockDownload());
-    m_cached_finished_ibd = true;
-    Assert(!IsInitialBlockDownload());
-}
-
 void ValidationInterfaceTest::BlockConnected(
     const ChainstateRole& role,
     CValidationInterface& obj,

src/test/util/validation.h

@@ -20,10 +20,6 @@ struct TestBlockManager : public node::BlockManager {
 struct TestChainstateManager : public ChainstateManager {
     /** Disable the next write of all chainstates */
     void DisableNextWrite();
-    /** Reset the ibd cache to its initial state */
-    void ResetIbd();
-    /** Toggle IsInitialBlockDownload from true to false */
-    void JumpOutOfIbd();
     /** Wrappers that avoid making chainstatemanager internals public for tests */
     void InvalidBlockFound(CBlockIndex* pindex, const BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
     void InvalidChainFound(CBlockIndex* pindexNew) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

src/test/validation_chainstatemanager_tests.cpp

@@ -111,7 +111,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager, TestChain100Setup)
 }
 
 //! Test rebalancing the caches associated with each chainstate.
-BOOST_FIXTURE_TEST_CASE(chainstatemanager_rebalance_caches, TestChain100Setup)
+BOOST_FIXTURE_TEST_CASE(chainstatemanager_rebalance_caches, RegTestingSetup)
 {
     ChainstateManager& manager = *m_node.chainman;
 
@@ -142,15 +142,7 @@ 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();
-
+    BOOST_CHECK(manager.IsInitialBlockDownload());
     {
         LOCK(::cs_main);
         c2.InitCoinsCache(1 << 23);

src/validation.cpp

@@ -1946,28 +1946,18 @@ void Chainstate::InitCoinsCache(size_t cache_size_bytes)
 //
 bool ChainstateManager::IsInitialBlockDownload() const
 {
-    // Optimization: pre-test latch before taking the lock.
-    if (m_cached_finished_ibd.load(std::memory_order_relaxed))
+    if (m_cached_finished_ibd.load(std::memory_order_relaxed)) {
         return false;
-
-    LOCK(cs_main);
-    if (m_cached_finished_ibd.load(std::memory_order_relaxed))
-        return false;
-    if (m_blockman.LoadingBlocks()) {
-        return true;
     }
-    CChain& chain{ActiveChain()};
-    if (chain.Tip() == nullptr) {
+    if (m_blockman.LoadingBlocks()) {
         return true;
     }
-    if (chain.Tip()->nChainWork < MinimumChainWork()) {
+    if (!m_cached_tip_recent.load(std::memory_order_relaxed)) {
         return true;
     }
-    if (chain.Tip()->Time() < Now<NodeSeconds>() - m_options.max_tip_age) {
-        return true;
+    if (!m_cached_finished_ibd.exchange(true, std::memory_order_relaxed)) {
+        LogInfo("Leaving InitialBlockDownload (latching to false)");
     }
-    LogInfo("Leaving InitialBlockDownload (latching to false)");
-    m_cached_finished_ibd.store(true, std::memory_order_relaxed);
     return false;
 }
 
@@ -3011,7 +3001,7 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
         }
     }
 
-    m_chain.SetTip(*pindexDelete->pprev);
+    m_chainman.SetTip(m_chain, *pindexDelete->pprev);
 
     UpdateTip(pindexDelete->pprev);
     // Let wallets know transactions went from 1-confirmed to
@@ -3139,8 +3129,8 @@ bool Chainstate::ConnectTip(
         m_mempool->removeForBlock(block_to_connect->vtx, pindexNew->nHeight);
         disconnectpool.removeForBlock(block_to_connect->vtx);
     }
-    // Update m_chain & related variables.
-    m_chain.SetTip(*pindexNew);
+    // Update m_chain and (for the active chainstate) m_cached_tip_recent and related variables.
+    m_chainman.SetTip(m_chain, *pindexNew);
     UpdateTip(pindexNew);
 
     const auto time_6{SteadyClock::now()};
@@ -3344,6 +3334,17 @@ static SynchronizationState GetSynchronizationState(bool init, bool blockfiles_i
     return SynchronizationState::INIT_DOWNLOAD;
 }
 
+void ChainstateManager::SetTip(CChain& chain, CBlockIndex& block)
+{
+    AssertLockHeld(cs_main);
+    chain.SetTip(block);
+    if (m_cached_tip_recent.load(std::memory_order_relaxed)) return;
+    if (&chain != &CurrentChainstate().m_chain) return;
+    if (chain.IsTipRecent(MinimumChainWork(), m_options.max_tip_age)) {
+        m_cached_tip_recent.store(true, std::memory_order_relaxed);
+    }
+}
+
 bool ChainstateManager::NotifyHeaderTip()
 {
     bool fNotify = false;
@@ -4626,7 +4627,7 @@ bool Chainstate::LoadChainTip()
     if (!pindex) {
         return false;
     }
-    m_chain.SetTip(*pindex);
+    m_chainman.SetTip(m_chain, *pindex);
     tip = m_chain.Tip();
 
     // Make sure our chain tip before shutting down scores better than any other candidate

src/validation.h

@@ -1036,7 +1036,10 @@ class ChainstateManager
      * Mutable because we need to be able to mark IsInitialBlockDownload()
      * const, which latches this for caching purposes.
      */
-    mutable std::atomic<bool> m_cached_finished_ibd{false};
+    mutable std::atomic_bool m_cached_finished_ibd{false};
+
+    /** Latch that becomes true once the active tip has met IsTipRecent(). */
+    mutable std::atomic_bool m_cached_tip_recent{false};
 
     /**
      * Every received block is assigned a unique and increasing identifier, so we
@@ -1157,6 +1160,9 @@ class ChainstateManager
     CBlockIndex* ActiveTip() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Tip(); }
     //! @}
 
+    /** Set a chain tip and latch m_cached_tip_recent if this is the active chain and the new tip is recent. */
+    void SetTip(CChain& chain, CBlockIndex& block) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+
     node::BlockMap& BlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
     {
         AssertLockHeld(::cs_main);