From 5d15485aafefdc759ba97e039bb1b9ccac267358 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 13 Aug 2024 22:33:31 -0300 Subject: [PATCH 1/4] wallet: unload, notify GUI as soon as possible Releases wallet shared pointers prior to doing the final settings update and prevent GUI races trying to access a wallet that is no longer loaded. --- src/wallet/wallet.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8e31950bebc..8abdd336a20 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -162,10 +162,14 @@ bool RemoveWallet(WalletContext& context, const std::shared_ptr& wallet // Unregister with the validation interface which also drops shared pointers. wallet->m_chain_notifications_handler.reset(); - LOCK(context.wallets_mutex); - std::vector>::iterator i = std::find(context.wallets.begin(), context.wallets.end(), wallet); - if (i == context.wallets.end()) return false; - context.wallets.erase(i); + { + LOCK(context.wallets_mutex); + std::vector>::iterator i = std::find(context.wallets.begin(), context.wallets.end(), wallet); + if (i == context.wallets.end()) return false; + context.wallets.erase(i); + } + // Notify unload so that upper layers release the shared pointer. + wallet->NotifyUnload(); // Write the wallet setting UpdateWalletSetting(chain, name, load_on_start, warnings); @@ -249,10 +253,6 @@ void UnloadWallet(std::shared_ptr&& wallet) auto it = g_unloading_wallet_set.insert(name); assert(it.second); } - // The wallet can be in use so it's not possible to explicitly unload here. - // 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(); From 8872b4a6ca91a83bf8d5a118fb808c043b9e879d Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 13 Aug 2024 23:00:45 -0300 Subject: [PATCH 2/4] wallet: rename UnloadWallet to WaitForDeleteWallet And update function's documentation. --- src/bench/wallet_create.cpp | 2 +- src/wallet/load.cpp | 2 +- src/wallet/rpc/wallet.cpp | 2 +- src/wallet/test/util.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 10 +++++----- src/wallet/wallet.h | 9 +++------ 7 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/bench/wallet_create.cpp b/src/bench/wallet_create.cpp index 5c0557bf6f0..1adced1e998 100644 --- a/src/bench/wallet_create.cpp +++ b/src/bench/wallet_create.cpp @@ -42,7 +42,7 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted) // Release wallet RemoveWallet(context, wallet, /*load_on_start=*/ std::nullopt); - UnloadWallet(std::move(wallet)); + WaitForDeleteWallet(std::move(wallet)); fs::remove_all(wallet_path); }); } diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index fe35f6b223e..e26347d437a 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -178,7 +178,7 @@ void UnloadWallets(WalletContext& context) wallets.pop_back(); std::vector warnings; RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt, warnings); - UnloadWallet(std::move(wallet)); + WaitForDeleteWallet(std::move(wallet)); } } } // namespace wallet diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index ae1c67ef2a2..83e386b3d9c 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -495,7 +495,7 @@ static RPCHelpMan unloadwallet() } } - UnloadWallet(std::move(wallet)); + WaitForDeleteWallet(std::move(wallet)); UniValue result(UniValue::VOBJ); PushWarnings(warnings, result); diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index b21a9a601d1..ec6c9e6f3f0 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -74,7 +74,7 @@ void TestUnloadWallet(std::shared_ptr&& wallet) // Calls SyncWithValidationInterfaceQueue wallet->chain().waitForNotificationsIfTipChanged({}); wallet->m_chain_notifications_handler.reset(); - UnloadWallet(std::move(wallet)); + WaitForDeleteWallet(std::move(wallet)); } std::unique_ptr DuplicateMockDatabase(WalletDatabase& database) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 53f3bcc4217..44ffddb1687 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -889,7 +889,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWalletWithoutChain, BasicTestingSetup) context.args = &m_args; auto wallet = TestLoadWallet(context); BOOST_CHECK(wallet); - UnloadWallet(std::move(wallet)); + WaitForDeleteWallet(std::move(wallet)); } BOOST_FIXTURE_TEST_CASE(RemoveTxs, TestChain100Setup) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8abdd336a20..4734fc7b1d3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -233,18 +233,18 @@ static void ReleaseWallet(CWallet* wallet) wallet->WalletLogPrintf("Releasing wallet\n"); wallet->Flush(); delete wallet; - // Wallet is now released, notify UnloadWallet, if any. + // Wallet is now released, notify WaitForDeleteWallet, if any. { LOCK(g_wallet_release_mutex); if (g_unloading_wallet_set.erase(name) == 0) { - // UnloadWallet was not called for this wallet, all done. + // WaitForDeleteWallet was not called for this wallet, all done. return; } } g_wallet_release_cv.notify_all(); } -void UnloadWallet(std::shared_ptr&& wallet) +void WaitForDeleteWallet(std::shared_ptr&& wallet) { // Mark wallet for unloading. const std::string name = wallet->GetName(); @@ -4387,7 +4387,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) { return util::Error{_("Unable to unload the wallet before migrating")}; } - UnloadWallet(std::move(wallet)); + WaitForDeleteWallet(std::move(wallet)); was_loaded = true; } else { // Check if the wallet is BDB @@ -4531,7 +4531,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle error += _("\nUnable to cleanup failed migration"); return util::Error{error}; } - UnloadWallet(std::move(w)); + WaitForDeleteWallet(std::move(w)); } else { // Unloading for wallets in local context assert(w.use_count() == 1); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 984a2d9c482..3ea1cf48b22 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -83,12 +83,9 @@ struct bilingual_str; namespace wallet { struct WalletContext; -//! Explicitly unload and delete the wallet. -//! Blocks the current thread after signaling the unload intent so that all -//! wallet pointer owners release the wallet. -//! Note that, when blocking is not required, the wallet is implicitly unloaded -//! by the shared pointer deleter. -void UnloadWallet(std::shared_ptr&& wallet); +//! Explicitly delete the wallet. +//! Blocks the current thread until the wallet is destructed. +void WaitForDeleteWallet(std::shared_ptr&& wallet); bool AddWallet(WalletContext& context, const std::shared_ptr& wallet); bool RemoveWallet(WalletContext& context, const std::shared_ptr& wallet, std::optional load_on_start, std::vector& warnings); From 64e736d79efc7201768244fc297084f70c0bebc1 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 13 Aug 2024 22:52:01 -0300 Subject: [PATCH 3/4] wallet: WaitForDeleteWallet, do not expect thread safety Multiple threads could try to delete the wallet at the same time. --- src/wallet/wallet.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4734fc7b1d3..09e12d52b91 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -250,8 +250,9 @@ void WaitForDeleteWallet(std::shared_ptr&& wallet) const std::string name = wallet->GetName(); { LOCK(g_wallet_release_mutex); - auto it = g_unloading_wallet_set.insert(name); - assert(it.second); + g_unloading_wallet_set.insert(name); + // Do not expect to be the only one removing this wallet. + // Multiple threads could simultaneously be waiting for deletion. } // Time to ditch our shared_ptr and wait for ReleaseWallet call. From f550a8e035b4603787273ea250f403f6f0be453f Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 15 Aug 2024 11:08:17 -0300 Subject: [PATCH 4/4] Rename ReleaseWallet to FlushAndDeleteWallet To better describe the function's behavior. And add wallet name to logprint. --- src/wallet/wallet.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 09e12d52b91..057fa729d71 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -227,10 +227,10 @@ static std::set g_loading_wallet_set GUARDED_BY(g_loading_wallet_mu static std::set g_unloading_wallet_set GUARDED_BY(g_wallet_release_mutex); // Custom deleter for shared_ptr. -static void ReleaseWallet(CWallet* wallet) +static void FlushAndDeleteWallet(CWallet* wallet) { const std::string name = wallet->GetName(); - wallet->WalletLogPrintf("Releasing wallet\n"); + wallet->WalletLogPrintf("Releasing wallet %s..\n", name); wallet->Flush(); delete wallet; // Wallet is now released, notify WaitForDeleteWallet, if any. @@ -255,7 +255,7 @@ void WaitForDeleteWallet(std::shared_ptr&& wallet) // Multiple threads could simultaneously be waiting for deletion. } - // Time to ditch our shared_ptr and wait for ReleaseWallet call. + // Time to ditch our shared_ptr and wait for FlushAndDeleteWallet call. wallet.reset(); { WAIT_LOCK(g_wallet_release_mutex, lock); @@ -2972,7 +2972,7 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri const auto start{SteadyClock::now()}; // TODO: Can't use std::make_shared because we need a custom deleter but // should be possible to use std::allocate_shared. - std::shared_ptr walletInstance(new CWallet(chain, name, std::move(database)), ReleaseWallet); + std::shared_ptr walletInstance(new CWallet(chain, name, std::move(database)), FlushAndDeleteWallet); walletInstance->m_keypool_size = std::max(args.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), int64_t{1}); walletInstance->m_notify_tx_changed_script = args.GetArg("-walletnotify", "");