Merge fe7d09b5017b748fc71dab2a8bb186ff2c1624bb into db2c57ae9eebdb75c58cd165ac929919969c19a9

src/bitcoin-chainstate.cpp

@@ -64,10 +64,10 @@ int main(int argc, char* argv[])
 
     // SETUP: Context
     kernel::Context kernel_context{};
-    // We can't use a goto here, but we can use an assert since none of the
-    // things instantiated so far requires running the epilogue to be torn down
-    // properly
-    assert(kernel::SanityChecks(kernel_context));
+    if (!kernel::SanityChecks(kernel_context)) {
+        std::cerr << "Failed sanity check.";
+        return 1;
+    }
 
     ValidationSignals validation_signals{std::make_unique<util::ImmediateTaskRunner>()};
 
@@ -132,12 +132,12 @@ int main(int argc, char* argv[])
     auto [status, error] = node::LoadChainstate(chainman, cache_sizes, options);
     if (status != node::ChainstateLoadStatus::SUCCESS) {
         std::cerr << "Failed to load Chain state from your datadir." << std::endl;
-        goto epilogue;
+        return 1;
     } else {
         std::tie(status, error) = node::VerifyLoadedChainstate(chainman, options);
         if (status != node::ChainstateLoadStatus::SUCCESS) {
             std::cerr << "Failed to verify loaded Chain state from your datadir." << std::endl;
-            goto epilogue;
+            return 1;
         }
     }
 
@@ -145,7 +145,7 @@ int main(int argc, char* argv[])
         BlockValidationState state;
         if (!chainstate->ActivateBestChain(state, nullptr)) {
             std::cerr << "Failed to connect best block (" << state.ToString() << ")" << std::endl;
-            goto epilogue;
+            return 1;
         }
     }
 
@@ -253,18 +253,4 @@ int main(int argc, char* argv[])
             break;
         }
     }
-
-epilogue:
-    // Without this precise shutdown sequence, there will be a lot of nullptr
-    // dereferencing and UB.
-    validation_signals.FlushBackgroundCallbacks();
-    {
-        LOCK(cs_main);
-        for (Chainstate* chainstate : chainman.GetAll()) {
-            if (chainstate->CanFlushToDisk()) {
-                chainstate->ForceFlushStateToDisk();
-                chainstate->ResetCoinsViews();
-            }
-        }
-    }
 }

src/init.cpp

@@ -347,8 +347,13 @@ void Shutdown(NodeContext& node)
         }
     }
 
-    // After there are no more peers/RPC left to give us new data which may generate
-    // CValidationInterface callbacks, flush them...
+    // After there are no more peers/RPC left to give us new data which may
+    // generate CValidationInterface callbacks, flush them. Any future
+    // callbacks will be dropped. This should absolutely be safe - if missing a
+    // callback results in an unrecoverable situation, unclean shutdown would
+    // too. The only reason to do the above flushes is to let the wallet and
+    // indexes catch up with our current chain to avoid any strange pruning
+    // edge cases and make next startup faster by avoiding rescan.
     if (node.validation_signals) node.validation_signals->FlushBackgroundCallbacks();
 
     // Stop and delete all indexes only after flushing background callbacks.
@@ -358,21 +363,6 @@ void Shutdown(NodeContext& node)
     DestroyAllBlockFilterIndexes();
     node.indexes.clear(); // all instances are nullptr now
 
-    // Any future callbacks will be dropped. This should absolutely be safe - if
-    // missing a callback results in an unrecoverable situation, unclean shutdown
-    // would too. The only reason to do the above flushes is to let the wallet catch
-    // up with our current chain to avoid any strange pruning edge cases and make
-    // next startup faster by avoiding rescan.
-
-    if (node.chainman) {
-        LOCK(cs_main);
-        for (Chainstate* chainstate : node.chainman->GetAll()) {
-            if (chainstate->CanFlushToDisk()) {
-                chainstate->ForceFlushStateToDisk();
-                chainstate->ResetCoinsViews();
-            }
-        }
-    }
     for (const auto& client : node.chain_clients) {
         client->stop();
     }
@@ -388,9 +378,9 @@ void Shutdown(NodeContext& node)
     if (node.validation_signals) {
         node.validation_signals->UnregisterAllValidationInterfaces();
     }
+    node.chainman.reset();
     node.mempool.reset();
     node.fee_estimator.reset();
-    node.chainman.reset();
     node.validation_signals.reset();
     node.scheduler.reset();
     node.ecc_context.reset();

src/test/fuzz/package_eval.cpp

@@ -341,6 +341,8 @@ FUZZ_TARGET(ephemeral_package_eval, .init = initialize_tx_pool)
 
     node.validation_signals->UnregisterSharedValidationInterface(outpoints_updater);
 
+    chainstate.SetMempool(g_setup->m_node.mempool.get());
+
     WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
 }
 
@@ -536,6 +538,8 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
 
     node.validation_signals->UnregisterSharedValidationInterface(outpoints_updater);
 
+    chainstate.SetMempool(g_setup->m_node.mempool.get());
+
     WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
 }
 } // namespace

src/test/fuzz/tx_pool.cpp

@@ -93,7 +93,7 @@ void SetMempoolConstraints(ArgsManager& args, FuzzedDataProvider& fuzzed_data_pr
                      ToString(fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 999)));
 }
 
-void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Chainstate& chainstate)
+void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, DummyChainState& chainstate)
 {
     WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
     {
@@ -112,6 +112,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha
         WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
     }
     g_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue();
+    chainstate.SetMempool(g_setup->m_node.mempool.get());
 }
 
 void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chainstate)

src/test/fuzz/util/mempool.h

@@ -6,21 +6,10 @@
 #define BITCOIN_TEST_FUZZ_UTIL_MEMPOOL_H
 
 #include <kernel/mempool_entry.h>
-#include <validation.h>
 
 class CTransaction;
-class CTxMemPool;
 class FuzzedDataProvider;
 
-class DummyChainState final : public Chainstate
-{
-public:
-    void SetMempool(CTxMemPool* mempool)
-    {
-        m_mempool = mempool;
-    }
-};
-
 [[nodiscard]] CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider, const CTransaction& tx) noexcept;
 
 #endif // BITCOIN_TEST_FUZZ_UTIL_MEMPOOL_H

src/test/fuzz/validation_load_mempool.cpp

@@ -4,22 +4,21 @@
 
 #include <node/mempool_persist.h>
 
-#include <node/mempool_args.h>
 #include <node/mempool_persist_args.h>
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
-#include <test/fuzz/util/mempool.h>
 #include <test/util/setup_common.h>
-#include <test/util/txmempool.h>
 #include <txmempool.h>
-#include <util/check.h>
 #include <util/time.h>
-#include <util/translation.h>
 #include <validation.h>
 
-#include <cstdint>
-#include <vector>
+#include <functional>
+#include <memory>
+
+namespace fs {
+class path;
+}
 
 using node::DumpMempool;
 using node::LoadMempool;
@@ -27,12 +26,12 @@ using node::LoadMempool;
 using node::MempoolPath;
 
 namespace {
-const TestingSetup* g_setup;
+TestingSetup* g_setup;
 } // namespace
 
 void initialize_validation_load_mempool()
 {
-    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
+    static auto testing_setup = MakeNoLogFileContext<TestingSetup>();
     g_setup = testing_setup.get();
 }
 
@@ -43,12 +42,9 @@ FUZZ_TARGET(validation_load_mempool, .init = initialize_validation_load_mempool)
     SetMockTime(ConsumeTime(fuzzed_data_provider));
     FuzzedFileProvider fuzzed_file_provider{fuzzed_data_provider};
 
-    bilingual_str error;
-    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error};
-    Assert(error.empty());
+    auto& pool{g_setup->ReplaceMempool()};
 
-    auto& chainstate{static_cast<DummyChainState&>(g_setup->m_node.chainman->ActiveChainstate())};
-    chainstate.SetMempool(&pool);
+    auto& chainstate{g_setup->m_node.chainman->ActiveChainstate()};
 
     auto fuzzed_fopen = [&](const fs::path&, const char*) {
         return fuzzed_file_provider.open();

src/test/miner_tests.cpp

@@ -49,14 +49,7 @@ struct MinerTestingSetup : public TestingSetup {
     }
     CTxMemPool& MakeMempool()
     {
-        // Delete the previous mempool to ensure with valgrind that the old
-        // pointer is not accessed, when the new one should be accessed
-        // instead.
-        m_node.mempool.reset();
-        bilingual_str error;
-        m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node), error);
-        Assert(error.empty());
-        return *m_node.mempool;
+        return ReplaceMempool();
     }
     std::unique_ptr<Mining> MakeMining()
     {

src/test/util/chainstate.h

@@ -74,6 +74,7 @@ CreateAndActivateUTXOSnapshot(
             CBlockIndex *orig_tip = node.chainman->ActiveChainstate().m_chain.Tip();
             uint256 gen_hash = node.chainman->ActiveChainstate().m_chain[0]->GetBlockHash();
             node.chainman->ResetChainstates();
+            Assert(node.chainman->GetAll().size() == 0);
             node.chainman->InitializeChainstate(node.mempool.get());
             Chainstate& chain = node.chainman->ActiveChainstate();
             Assert(chain.LoadGenesisBlock());

src/test/util/setup_common.cpp

@@ -274,9 +274,9 @@ ChainTestingSetup::~ChainTestingSetup()
     m_node.addrman.reset();
     m_node.netgroupman.reset();
     m_node.args = nullptr;
+    m_node.chainman.reset();
     m_node.mempool.reset();
     Assert(!m_node.fee_estimator); // Each test must create a local object, if they wish to use the fee_estimator
-    m_node.chainman.reset();
     m_node.validation_signals.reset();
     m_node.scheduler.reset();
 }
@@ -304,6 +304,20 @@ void ChainTestingSetup::LoadVerifyActivateChainstate()
     }
 }
 
+CTxMemPool& ChainTestingSetup::ReplaceMempool()
+{
+    // Delete the previous mempool to ensure with valgrind that the old
+    // pointer is not accessed, when the new one should be accessed
+    // instead.
+    m_node.mempool.reset();
+    bilingual_str error;
+    m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node), error);
+    Assert(error.empty());
+    auto& dummy_chainstate{static_cast<DummyChainState&>(m_node.chainman->ActiveChainstate())};
+    dummy_chainstate.SetMempool(m_node.mempool.get());
+    return *m_node.mempool;
+}
+
 TestingSetup::TestingSetup(
     const ChainType chainType,
     TestOpts opts)

src/test/util/setup_common.h

@@ -114,6 +114,8 @@ struct ChainTestingSetup : public BasicTestingSetup {
 
     // Supplies a chainstate, if one is needed
     void LoadVerifyActivateChainstate();
+
+    CTxMemPool& ReplaceMempool();
 };
 
 /** Testing setup that configures a complete environment.

src/test/util/txmempool.h

@@ -8,6 +8,7 @@
 #include <policy/packages.h>
 #include <txmempool.h>
 #include <util/time.h>
+#include <validation.h>
 
 namespace node {
 struct NodeContext;
@@ -16,6 +17,15 @@ struct PackageMempoolAcceptResult;
 
 CTxMemPool::Options MemPoolOptionsForTest(const node::NodeContext& node);
 
+class DummyChainState final : public Chainstate
+{
+public:
+    void SetMempool(CTxMemPool* mempool)
+    {
+        m_mempool = mempool;
+    }
+};
+
 struct TestMemPoolEntryHelper {
     // Default values
     CAmount nFee{0};

src/test/validation_chainstatemanager_tests.cpp

@@ -369,23 +369,12 @@ struct SnapshotTestSetup : TestChain100Setup {
     // @returns a reference to the "restarted" ChainstateManager
     ChainstateManager& SimulateNodeRestart()
     {
-        ChainstateManager& chainman = *Assert(m_node.chainman);
-
         BOOST_TEST_MESSAGE("Simulating node restart");
         {
-            for (Chainstate* cs : chainman.GetAll()) {
-                LOCK(::cs_main);
-                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);
             m_node.notifications = std::make_unique<KernelNotifications>(Assert(m_node.shutdown_request), m_node.exit_status, *Assert(m_node.warnings));
             const ChainstateManager::Options chainman_opts{
                 .chainparams = ::Params(),
-                .datadir = chainman.m_options.datadir,
+                .datadir = Assert(m_node.chainman)->m_options.datadir,
                 .notifications = *m_node.notifications,
                 .signals = m_node.validation_signals.get(),
             };
@@ -394,7 +383,7 @@ struct SnapshotTestSetup : TestChain100Setup {
                 .blocks_dir = m_args.GetBlocksDirPath(),
                 .notifications = chainman_opts.notifications,
                 .block_tree_db_params = DBParams{
-                    .path = chainman.m_options.datadir / "blocks" / "index",
+                    .path = Assert(m_node.chainman)->m_options.datadir / "blocks" / "index",
                     .cache_bytes = m_kernel_cache_sizes.block_tree_db,
                     .memory_only = m_block_tree_db_in_memory,
                 },
@@ -402,7 +391,11 @@ struct SnapshotTestSetup : TestChain100Setup {
             // For robustness, ensure the old manager is destroyed before creating a
             // new one.
             m_node.chainman.reset();
+            // Process all callbacks referring to the old manager before creating a
+            // new one.
+            m_node.validation_signals->SyncWithValidationInterfaceQueue();
             m_node.chainman = std::make_unique<ChainstateManager>(*Assert(m_node.shutdown_signal), chainman_opts, blockman_opts);
+            BOOST_CHECK_EQUAL(m_node.chainman->GetAll().size(), 0);
         }
         return *Assert(m_node.chainman);
     }

src/validation.cpp

@@ -6306,6 +6306,11 @@ ChainstateManager::ChainstateManager(const util::SignalInterrupt& interrupt, Opt
 ChainstateManager::~ChainstateManager()
 {
     LOCK(::cs_main);
+    for (Chainstate* chainstate : GetAll()) {
+        if (chainstate->CanFlushToDisk()) {
+            chainstate->ForceFlushStateToDisk();
+        }
+    }
 
     m_versionbitscache.Clear();
 }

src/validation.h

@@ -636,9 +636,6 @@ public:
         return Assert(m_coins_views)->m_catcherview;
     }
 
-    //! 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; }