From 0c74da690480bb4f7fba7cca7cc55dc338808d10 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Wed, 27 Nov 2024 15:03:04 +0100 Subject: [PATCH 1/6] shutdown: Tear down mempool after chainman Next to being more consistent, since tearing down the mempool first leaves a dangling pointer in the chainman, this also enables the changes in the next commits, since flushing calls GetCoinsCacheSizeState, which would access this dangling pointer otherwise. --- src/init.cpp | 2 +- src/test/util/setup_common.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 7fdbf75dc66..fec5f88410b 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -388,9 +388,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/util/setup_common.cpp b/src/test/util/setup_common.cpp index bf26997c076..0724782b393 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -272,9 +272,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(); } From dac88306e9f4b65e7d073f6b8575bb759ccf98c7 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Wed, 27 Nov 2024 17:48:17 +0100 Subject: [PATCH 2/6] tests: Add replace mempool method This is done in preparation for the next commit to also change the mempool pointer within the chainstate once the old mempool is reset. Without this the old, dangling pointer will be reused when destructing the chainstate manager. --- src/test/fuzz/package_eval.cpp | 4 ++++ src/test/fuzz/tx_pool.cpp | 3 ++- src/test/fuzz/util/mempool.h | 11 ----------- src/test/fuzz/validation_load_mempool.cpp | 24 ++++++++++------------- src/test/miner_tests.cpp | 9 +-------- src/test/util/setup_common.cpp | 14 +++++++++++++ src/test/util/setup_common.h | 2 ++ src/test/util/txmempool.h | 10 ++++++++++ 8 files changed, 43 insertions(+), 34 deletions(-) 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 5bfcde77183..6c31fad9e09 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/setup_common.cpp b/src/test/util/setup_common.cpp index 0724782b393..eca44e93ffb 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -302,6 +302,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 33ad2584573..6e23478be9d 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}; From 9c5fa1f7470e5572337122592859f1686569fde7 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Wed, 27 Nov 2024 10:46:30 +0100 Subject: [PATCH 3/6] kernel: Flush in ChainstateManager dtor Moving the second flush out of the Shutdown function into the ChainstateManager destructor, should be safe since there should be no new events created after the first flush. This simplifies the shutdown code a bit and allows for getting rid of the `goto` in bitcoin-chainstate. --- src/bitcoin-chainstate.cpp | 9 --------- src/init.cpp | 24 +++++++----------------- src/validation.cpp | 6 ++++++ 3 files changed, 13 insertions(+), 26 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index e657f018715..abbce1286a3 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -261,13 +261,4 @@ 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 fec5f88410b..e085171981f 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(); } diff --git a/src/validation.cpp b/src/validation.cpp index 93da4f326d4..77e3164348b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -6318,6 +6318,12 @@ ChainstateManager::ChainstateManager(const util::SignalInterrupt& interrupt, Opt ChainstateManager::~ChainstateManager() { LOCK(::cs_main); + for (Chainstate* chainstate : GetAll()) { + if (chainstate->CanFlushToDisk()) { + chainstate->ForceFlushStateToDisk(); + chainstate->ResetCoinsViews(); + } + } m_versionbitscache.Clear(); } From 0db551377f69fc130bc7eb79251129b3d1cc7d8f Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Wed, 27 Nov 2024 13:00:07 +0100 Subject: [PATCH 4/6] tests: Use chainman dtor logic to simulate restart Also remove the reference to the destructed chainman, which could cause UB once the chainman is reset. --- src/test/util/chainstate.h | 1 + .../validation_chainstatemanager_tests.cpp | 19 ++++++------------- 2 files changed, 7 insertions(+), 13 deletions(-) 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/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); } From f7a0533e3ba00b38e4e91ab2fa8f61c17d63deeb Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Wed, 27 Nov 2024 11:02:54 +0100 Subject: [PATCH 5/6] validation: Remove ResetCoinsViews Now that its only callsite has been moved to the destructor, let the smart pointer do its thing. --- src/validation.cpp | 1 - src/validation.h | 3 --- 2 files changed, 4 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 77e3164348b..dfe4325ce2f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -6321,7 +6321,6 @@ ChainstateManager::~ChainstateManager() for (Chainstate* chainstate : GetAll()) { if (chainstate->CanFlushToDisk()) { chainstate->ForceFlushStateToDisk(); - chainstate->ResetCoinsViews(); } } diff --git a/src/validation.h b/src/validation.h index 9e4fdbe6809..19c5dd8a330 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; } From fe7d09b5017b748fc71dab2a8bb186ff2c1624bb Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Wed, 27 Nov 2024 14:01:38 +0100 Subject: [PATCH 6/6] kernel: Remove epilogue in bitcoin-chainstate There is no need to flush the background callbacks, since the immediate task runner is used for the validation signals, which does not have any pending callbacks. This allows for removal of the epilogue goto. --- src/bitcoin-chainstate.cpp | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index abbce1286a3..47e7aae689c 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; } } @@ -256,9 +256,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(); }