From ab31b9d6fe7b39713682e3f52d11238dbe042c16 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 10 Mar 2020 15:46:20 -0400 Subject: [PATCH 1/2] Fix wallet unload race condition Currently it's possible for ReleaseWallet to delete the CWallet pointer while it is processing BlockConnected, etc chain notifications. To fix this, unregister from notifications earlier in UnloadWallet instead of ReleaseWallet, and use a new RegisterSharedValidationInterface function to prevent the CValidationInterface shared_ptr from being deleted until the last notification is actually finished. --- src/bench/wallet_balance.cpp | 3 +- src/interfaces/chain.cpp | 44 +++++++++++++++---------- src/interfaces/chain.h | 2 +- src/validationinterface.cpp | 18 ++++++++-- src/validationinterface.h | 12 +++++-- src/wallet/test/wallet_test_fixture.cpp | 3 +- src/wallet/test/wallet_test_fixture.h | 1 + src/wallet/wallet.cpp | 16 +++------ src/wallet/wallet.h | 5 +-- 9 files changed, 62 insertions(+), 42 deletions(-) diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp index 62568a9da5a..0381369218b 100644 --- a/src/bench/wallet_balance.cpp +++ b/src/bench/wallet_balance.cpp @@ -23,9 +23,8 @@ static void WalletBalance(benchmark::State& state, const bool set_dirty, const b wallet.SetupLegacyScriptPubKeyMan(); bool first_run; 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(wallet)} : nullopt}; if (add_watchonly) importaddress(wallet, ADDRESS_WATCHONLY); diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 9dc0d37cd92..880edcf132d 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -148,22 +148,12 @@ class LockImpl : public Chain::Lock, public UniqueLock using UniqueLock::UniqueLock; }; -class NotificationsHandlerImpl : public Handler, CValidationInterface +class NotificationsProxy : public CValidationInterface { public: - explicit NotificationsHandlerImpl(Chain& chain, Chain::Notifications& notifications) - : 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); @@ -185,8 +175,26 @@ public: m_notifications->updatedBlockTip(); } 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 @@ -343,9 +351,9 @@ public: { ::uiInterface.ShowProgress(title, progress, resume_possible); } - std::unique_ptr handleNotifications(Notifications& notifications) override + std::unique_ptr handleNotifications(std::shared_ptr notifications) override { - return MakeUnique(*this, notifications); + return MakeUnique(std::move(notifications)); } void waitForNotificationsIfTipChanged(const uint256& old_tip) override { diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index caefa87e117..e1bc9bbbf3a 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -229,7 +229,7 @@ public: }; //! Register handler for notifications. - virtual std::unique_ptr handleNotifications(Notifications& notifications) = 0; + virtual std::unique_ptr handleNotifications(std::shared_ptr notifications) = 0; //! Wait for pending notifications to be processed unless block hash points to the current //! chain tip. diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index c895904b191..f9f61e8a092 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -75,8 +75,10 @@ CMainSignals& GetMainSignals() return g_signals; } -void RegisterValidationInterface(CValidationInterface* pwalletIn) { - ValidationInterfaceConnections& conns = g_signals.m_internals->m_connMainSignals[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.get()]; conns.UpdatedBlockTip = g_signals.m_internals->UpdatedBlockTip.connect(std::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3)); conns.TransactionAddedToMempool = g_signals.m_internals->TransactionAddedToMempool.connect(std::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, std::placeholders::_1)); conns.BlockConnected = g_signals.m_internals->BlockConnected.connect(std::bind(&CValidationInterface::BlockConnected, pwalletIn, std::placeholders::_1, std::placeholders::_2)); @@ -87,6 +89,18 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) { conns.NewPoWValidBlock = g_signals.m_internals->NewPoWValidBlock.connect(std::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, 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/validationinterface.h b/src/validationinterface.h index 57074226354..f9a359b7ad8 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -30,6 +30,14 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn); 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. @@ -163,7 +171,7 @@ protected: * Notifies listeners that a block which builds directly on our current tip * has been received and connected to the headers tree, though not validated yet */ 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(); }; @@ -173,7 +181,7 @@ class CMainSignals { 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(std::function func); diff --git a/src/wallet/test/wallet_test_fixture.cpp b/src/wallet/test/wallet_test_fixture.cpp index ba0843f352a..b9e714946de 100644 --- a/src/wallet/test/wallet_test_fixture.cpp +++ b/src/wallet/test/wallet_test_fixture.cpp @@ -10,7 +10,6 @@ WalletTestingSetup::WalletTestingSetup(const std::string& chainName) { 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/test/wallet_test_fixture.h b/src/wallet/test/wallet_test_fixture.h index 4e4129fb2c5..81d8a60b8ae 100644 --- a/src/wallet/test/wallet_test_fixture.h +++ b/src/wallet/test/wallet_test_fixture.h @@ -23,6 +23,7 @@ struct WalletTestingSetup: public TestingSetup { std::unique_ptr m_chain = interfaces::MakeChain(m_node); 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/wallet.cpp b/src/wallet/wallet.cpp index 9a972febab1..98f308f927f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -62,8 +62,10 @@ bool AddWallet(const std::shared_ptr& wallet) 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()) return false; vpwallets.erase(i); @@ -105,13 +107,9 @@ static std::set g_unloading_wallet_set; // 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. { @@ -137,6 +135,7 @@ void UnloadWallet(std::shared_ptr&& wallet) // 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(); { @@ -4092,7 +4091,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, } // 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)); @@ -4105,11 +4104,6 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, return walletInstance; } -void CWallet::handleNotifications() -{ - m_chain_notifications_handler = m_chain->handleNotifications(*this); -} - void CWallet::postInitProcess() { auto locked_chain = chain().lock(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 75fd14a80e1..e3903bfcf4f 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -605,7 +605,7 @@ class WalletRescanReserver; //forward declarations for ScanForWalletTransactions /** * A CWallet maintains a set of transactions and balances, and provides the ability to create new transactions. */ -class CWallet final : public WalletStorage, private interfaces::Chain::Notifications +class CWallet final : public WalletStorage, public interfaces::Chain::Notifications { private: CKeyingMaterial vMasterKey GUARDED_BY(cs_wallet); @@ -781,9 +781,6 @@ public: /** 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); return *m_chain; } From 41b0baf43c243b64b394e774e336475a489cca2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Thu, 19 Mar 2020 22:53:33 +0000 Subject: [PATCH 2/2] gui: Handle WalletModel::unload asynchronous This change prevents deleting a WalletModel instance while it's being used. --- src/qt/walletcontroller.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index 233c0ab6be5..88c694567ee 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -116,7 +116,7 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr