diff --git a/src/bench/wallet_create.cpp b/src/bench/wallet_create.cpp index 5c0557bf6f073..1adced1e9980a 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 fe35f6b223e4f..e26347d437af3 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 7a0b0103c0b99..39582b3f6a122 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 b21a9a601d1c2..ec6c9e6f3f081 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 53f3bcc421750..44ffddb168701 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 8e31950bebc8c..057fa729d7114 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); @@ -223,38 +227,35 @@ 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 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(); { 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. } - // 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. + // Time to ditch our shared_ptr and wait for FlushAndDeleteWallet call. wallet.reset(); { WAIT_LOCK(g_wallet_release_mutex, lock); @@ -2971,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", ""); @@ -4387,7 +4388,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 +4532,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 984a2d9c482e1..3ea1cf48b22bb 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);