diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index eec3362d4e2..9b117e29b76 100644 --- a/src/bitcoin-chainstate.cpp +++ b/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()}; @@ -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(); - } - } - } } diff --git a/src/init.cpp b/src/init.cpp index fae45eb90a5..1c76392ae54 100644 --- a/src/init.cpp +++ b/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(); diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 8e3d84a9e63..97d67ba2a86 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/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 diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index a697ee9d838..d7b2cb5780b 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -93,7 +93,7 @@ void SetMempoolConstraints(ArgsManager& args, FuzzedDataProvider& fuzzed_data_pr ToString(fuzzed_data_provider.ConsumeIntegralInRange(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) diff --git a/src/test/fuzz/util/mempool.h b/src/test/fuzz/util/mempool.h index 31b578dc4b8..b69b920c922 100644 --- a/src/test/fuzz/util/mempool.h +++ b/src/test/fuzz/util/mempool.h @@ -6,21 +6,10 @@ #define BITCOIN_TEST_FUZZ_UTIL_MEMPOOL_H #include -#include 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 diff --git a/src/test/fuzz/validation_load_mempool.cpp b/src/test/fuzz/validation_load_mempool.cpp index 4f90138892d..4ffa0ce9de7 100644 --- a/src/test/fuzz/validation_load_mempool.cpp +++ b/src/test/fuzz/validation_load_mempool.cpp @@ -4,22 +4,21 @@ #include -#include #include #include #include #include -#include #include -#include #include -#include #include -#include #include -#include -#include +#include +#include + +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(); + static auto testing_setup = MakeNoLogFileContext(); 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(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(); diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 5b87d4443d9..8c8da134123 100644 --- a/src/test/miner_tests.cpp +++ b/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(MemPoolOptionsForTest(m_node), error); - Assert(error.empty()); - return *m_node.mempool; + return ReplaceMempool(); } std::unique_ptr MakeMining() { diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h index e604b44c16a..993b059aaac 100644 --- a/src/test/util/chainstate.h +++ b/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()); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 083ba3bd6d8..d925880f409 100644 --- a/src/test/util/setup_common.cpp +++ b/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(MemPoolOptionsForTest(m_node), error); + Assert(error.empty()); + auto& dummy_chainstate{static_cast(m_node.chainman->ActiveChainstate())}; + dummy_chainstate.SetMempool(m_node.mempool.get()); + return *m_node.mempool; +} + TestingSetup::TestingSetup( const ChainType chainType, TestOpts opts) diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 57bea9086b9..afc5fecb390 100644 --- a/src/test/util/setup_common.h +++ b/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. diff --git a/src/test/util/txmempool.h b/src/test/util/txmempool.h index 36caad2ae1a..70121dc74ea 100644 --- a/src/test/util/txmempool.h +++ b/src/test/util/txmempool.h @@ -8,6 +8,7 @@ #include #include #include +#include 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}; diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index bf440ca6397..95fa260f5ad 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/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(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(*Assert(m_node.shutdown_signal), chainman_opts, blockman_opts); + BOOST_CHECK_EQUAL(m_node.chainman->GetAll().size(), 0); } return *Assert(m_node.chainman); } diff --git a/src/validation.cpp b/src/validation.cpp index bf370d171a5..43ce8579904 100644 --- a/src/validation.cpp +++ b/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(); } diff --git a/src/validation.h b/src/validation.h index f6cbee28fc5..b44c0c0b515 100644 --- a/src/validation.h +++ b/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; }