diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp --- a/src/bench/wallet_balance.cpp +++ b/src/bench/wallet_balance.cpp @@ -26,9 +26,10 @@ if (wallet.LoadWallet(first_run) != DBErrors::LOAD_OK) { assert(false); } - wallet.handleNotifications(); } + auto handler = chain->handleNotifications({&wallet, [](CWallet *) {}}); + const Optional address_mine{ add_mine ? Optional{getnewaddress(config, wallet)} : nullopt}; diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -238,7 +238,7 @@ //! Register handler for notifications. virtual std::unique_ptr - handleNotifications(Notifications ¬ifications) = 0; + handleNotifications(std::shared_ptr notifications) = 0; //! Wait for pending notifications to be processed unless block hash points //! to the current chain tip, or to a possible descendant of the current diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -151,20 +151,12 @@ using UniqueLock::UniqueLock; }; // namespace interfaces - class NotificationsHandlerImpl : public Handler, CValidationInterface { + class NotificationsProxy : public CValidationInterface { public: - explicit NotificationsHandlerImpl(Chain &chain, - Chain::Notifications ¬ifications) - : m_chain(chain), m_notifications(¬ifications) { - RegisterValidationInterface(this); - } - ~NotificationsHandlerImpl() override { disconnect(); } - void disconnect() override { - if (m_notifications) { - m_notifications = nullptr; - UnregisterValidationInterface(this); - } - } + explicit NotificationsProxy( + std::shared_ptr notifications) + : m_notifications(std::move(notifications)) {} + virtual ~NotificationsProxy() = default; void TransactionAddedToMempool(const CTransactionRef &tx) override { m_notifications->TransactionAddedToMempool(tx); } @@ -189,8 +181,25 @@ void ChainStateFlushed(const CBlockLocator &locator) override { m_notifications->ChainStateFlushed(locator); } - Chain &m_chain; - Chain::Notifications *m_notifications; + std::shared_ptr m_notifications; + }; + + class NotificationsHandlerImpl : public Handler { + public: + explicit NotificationsHandlerImpl( + std::shared_ptr notifications) + : m_proxy(std::make_shared( + std::move(notifications))) { + RegisterSharedValidationInterface(m_proxy); + } + ~NotificationsHandlerImpl() override { disconnect(); } + void disconnect() override { + if (m_proxy) { + UnregisterSharedValidationInterface(m_proxy); + m_proxy.reset(); + } + } + std::shared_ptr m_proxy; }; class RpcHandlerImpl : public Handler { @@ -358,10 +367,10 @@ bool resume_possible) override { ::uiInterface.ShowProgress(title, progress, resume_possible); } - std::unique_ptr - handleNotifications(Notifications ¬ifications) override { - return std::make_unique(*this, - notifications); + std::unique_ptr handleNotifications( + std::shared_ptr notifications) override { + return std::make_unique( + std::move(notifications)); } void waitForNotificationsIfNewBlocksConnected( const BlockHash &old_tip) override { diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -109,22 +109,25 @@ wallet_model->setParent(this); m_wallets.push_back(wallet_model); - connect(wallet_model, &WalletModel::unload, [this, wallet_model] { - // Defer removeAndDeleteWallet when no modal widget is active. - // TODO: remove this workaround by removing usage of QDiallog::exec. - if (QApplication::activeModalWidget()) { - connect( - qApp, &QApplication::focusWindowChanged, wallet_model, - [this, wallet_model]() { - if (!QApplication::activeModalWidget()) { - removeAndDeleteWallet(wallet_model); - } - }, - Qt::QueuedConnection); - } else { - removeAndDeleteWallet(wallet_model); - } - }); + connect( + wallet_model, &WalletModel::unload, this, + [this, wallet_model] { + // Defer removeAndDeleteWallet when no modal widget is active. + // TODO: remove this workaround by removing usage of QDiallog::exec. + if (QApplication::activeModalWidget()) { + connect( + qApp, &QApplication::focusWindowChanged, wallet_model, + [this, wallet_model]() { + if (!QApplication::activeModalWidget()) { + removeAndDeleteWallet(wallet_model); + } + }, + Qt::QueuedConnection); + } else { + removeAndDeleteWallet(wallet_model); + } + }, + Qt::QueuedConnection); // Re-emit coinsSent signal from wallet model. connect(wallet_model, &WalletModel::coinsSent, this, diff --git a/src/validationinterface.h b/src/validationinterface.h --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -32,6 +32,16 @@ void UnregisterValidationInterface(CValidationInterface *pwalletIn); /** Unregister all wallets from core */ void UnregisterAllValidationInterfaces(); + +// Alternate registration functions that release a shared_ptr after the last +// notification is sent. These are useful for race-free cleanup, since +// unregistration is nonblocking and can return before the last notification is +// processed. +void RegisterSharedValidationInterface( + std::shared_ptr callbacks); +void UnregisterSharedValidationInterface( + std::shared_ptr callbacks); + /** * Pushes a function to callback onto the notification queue, guaranteeing any * callbacks generated prior to now are finished when the function is called. @@ -153,7 +163,8 @@ */ virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr &block){}; - friend void ::RegisterValidationInterface(CValidationInterface *); + friend void ::RegisterSharedValidationInterface( + std::shared_ptr); friend void ::UnregisterValidationInterface(CValidationInterface *); friend void ::UnregisterAllValidationInterfaces(); }; @@ -163,7 +174,8 @@ private: std::unique_ptr m_internals; - friend void ::RegisterValidationInterface(CValidationInterface *); + friend void ::RegisterSharedValidationInterface( + std::shared_ptr); friend void ::UnregisterValidationInterface(CValidationInterface *); friend void ::UnregisterAllValidationInterfaces(); friend void ::CallFunctionInValidationInterfaceQueue( diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -86,9 +86,12 @@ return g_signals; } -void RegisterValidationInterface(CValidationInterface *pwalletIn) { +void RegisterSharedValidationInterface( + std::shared_ptr pwalletIn) { + // Each connection captures pwalletIn to ensure that each callback is + // executed before pwalletIn is destroyed. For more details see #18338. ValidationInterfaceConnections &conns = - g_signals.m_internals->m_connMainSignals[pwalletIn]; + g_signals.m_internals->m_connMainSignals[pwalletIn.get()]; conns.UpdatedBlockTip = g_signals.m_internals->UpdatedBlockTip.connect( std::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, std::placeholders::_1, std::placeholders::_2, @@ -119,6 +122,18 @@ std::placeholders::_1, std::placeholders::_2)); } +void RegisterValidationInterface(CValidationInterface *callbacks) { + // Create a shared_ptr with a no-op deleter - CValidationInterface lifecycle + // is managed by the caller. + RegisterSharedValidationInterface( + {callbacks, [](CValidationInterface *) {}}); +} + +void UnregisterSharedValidationInterface( + std::shared_ptr callbacks) { + UnregisterValidationInterface(callbacks.get()); +} + void UnregisterValidationInterface(CValidationInterface *pwalletIn) { if (g_signals.m_internals) { g_signals.m_internals->m_connMainSignals.erase(pwalletIn); diff --git a/src/wallet/test/wallet_test_fixture.h b/src/wallet/test/wallet_test_fixture.h --- a/src/wallet/test/wallet_test_fixture.h +++ b/src/wallet/test/wallet_test_fixture.h @@ -26,6 +26,7 @@ std::unique_ptr m_chain_client = interfaces::MakeWalletClient(*m_chain, {}); CWallet m_wallet; + std::unique_ptr m_chain_notifications_handler; }; #endif // BITCOIN_WALLET_TEST_WALLET_TEST_FIXTURE_H diff --git a/src/wallet/test/wallet_test_fixture.cpp b/src/wallet/test/wallet_test_fixture.cpp --- a/src/wallet/test/wallet_test_fixture.cpp +++ b/src/wallet/test/wallet_test_fixture.cpp @@ -15,7 +15,7 @@ WalletDatabase::CreateMock()) { bool fFirstRun; m_wallet.LoadWallet(fFirstRun); - m_wallet.handleNotifications(); - + m_chain_notifications_handler = + m_chain->handleNotifications({&m_wallet, [](CWallet *) {}}); m_chain_client->registerRpcs(); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -758,7 +758,7 @@ * transactions. */ class CWallet final : public FillableSigningProvider, - private interfaces::Chain::Notifications { + public interfaces::Chain::Notifications { private: CKeyingMaterial vMasterKey GUARDED_BY(cs_KeyStore); @@ -1029,9 +1029,6 @@ /** Registered interfaces::Chain::Notifications handler. */ std::unique_ptr m_chain_notifications_handler; - /** Register the wallet for chain notifications */ - void handleNotifications(); - /** Interface for accessing chain state. */ interfaces::Chain &chain() const { assert(m_chain); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -67,8 +67,10 @@ } bool RemoveWallet(const std::shared_ptr &wallet) { - LOCK(cs_wallets); assert(wallet); + // Unregister with the validation interface which also drops shared ponters. + wallet->m_chain_notifications_handler.reset(); + LOCK(cs_wallets); std::vector>::iterator i = std::find(vpwallets.begin(), vpwallets.end(), wallet); if (i == vpwallets.end()) { @@ -104,14 +106,9 @@ // Custom deleter for shared_ptr. static void ReleaseWallet(CWallet *wallet) { - // Unregister and delete the wallet right after - // BlockUntilSyncedToCurrentChain so that it's in sync with the current - // chainstate. const std::string name = wallet->GetName(); wallet->WalletLogPrintf("Releasing wallet\n"); - wallet->BlockUntilSyncedToCurrentChain(); wallet->Flush(); - wallet->m_chain_notifications_handler.reset(); delete wallet; // Wallet is now released, notify UnloadWallet, if any. { @@ -136,6 +133,7 @@ // Notify the unload intent so that all remaining shared pointers are // released. wallet->NotifyUnload(); + // Time to ditch our shared_ptr and wait for ReleaseWallet call. wallet.reset(); { @@ -5107,7 +5105,8 @@ // Register with the validation interface. It's ok to do this after rescan // since we're still holding locked_chain. - walletInstance->handleNotifications(); + walletInstance->m_chain_notifications_handler = + walletInstance->chain().handleNotifications(walletInstance); walletInstance->SetBroadcastTransactions( gArgs.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST)); @@ -5122,10 +5121,6 @@ return walletInstance; } -void CWallet::handleNotifications() { - m_chain_notifications_handler = m_chain->handleNotifications(*this); -} - void CWallet::postInitProcess() { auto locked_chain = chain().lock(); LOCK(cs_wallet);