merge bitcoin#19101: remove ::vpwallets and related global variables

This is the second half of the backport, which deals with deglobalizing
wallet-specific variables like vpwallets, cs_wallets and friends.

src/interfaces/wallet.h

@@ -457,7 +457,7 @@ struct WalletTxOut
 
 //! Return implementation of Wallet interface. This function is defined in
 //! dummywallet.cpp and throws if the wallet component is not compiled.
-std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet);
+std::unique_ptr<Wallet> MakeWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet);
 
 //! Return implementation of ChainClient interface for a wallet loader. This
 //! function will be undefined in builds where ENABLE_WALLET is false.

src/qt/test/addressbooktests.cpp

@@ -108,9 +108,10 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
     // Initialize relevant QT models.
     OptionsModel optionsModel;
     ClientModel clientModel(node, &optionsModel);
-    AddWallet(wallet);
-    WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel);
-    RemoveWallet(wallet, std::nullopt);
+    WalletContext& context = *node.walletLoader().context();
+    AddWallet(context, wallet);
+    WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel);
+    RemoveWallet(context, wallet, /*load_on_startup=*/std::nullopt);
     EditAddressDialog editAddressDialog(EditAddressDialog::NewSendingAddress);
     editAddressDialog.setModel(walletModel.getAddressTableModel());
 

src/qt/test/wallettests.cpp

@@ -109,8 +109,9 @@ void TestGUI(interfaces::Node& node)
         test.CreateAndProcessBlock({}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey()));
     }
     node.setContext(&test.m_node);
+    WalletContext& context = *node.walletLoader().context();
     std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", CreateMockWalletDatabase());
-    AddWallet(wallet);
+    AddWallet(context, wallet);
     wallet->LoadWallet();
     {
         auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan();
@@ -134,7 +135,7 @@ void TestGUI(interfaces::Node& node)
     TransactionView transactionView;
     OptionsModel optionsModel;
     ClientModel clientModel(node, &optionsModel);
-    WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel);
+    WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel);
     sendCoinsDialog.setModel(&walletModel);
     transactionView.setModel(&walletModel);
 
@@ -246,7 +247,7 @@ void TestGUI(interfaces::Node& node)
     QPushButton* removeRequestButton = receiveCoinsDialog.findChild<QPushButton*>("removeRequestButton");
     removeRequestButton->click();
     QCOMPARE(requestTableModel->rowCount({}), currentRowCount-1);
-    RemoveWallet(wallet, std::nullopt);
+    RemoveWallet(context, wallet, /*load_on_startup=*/std::nullopt);
 
     // Check removal from wallet
     QCOMPARE(walletModel.wallet().getAddressReceiveRequests().size(), size_t{0});

src/wallet/context.h

@@ -5,17 +5,26 @@
 #ifndef BITCOIN_WALLET_CONTEXT_H
 #define BITCOIN_WALLET_CONTEXT_H
 
+#include <sync.h>
+
+#include <functional>
+#include <list>
 #include <memory>
+#include <vector>
 
 class ArgsManager;
+class CWallet;
 struct NodeContext;
 namespace interfaces {
 class Chain;
 namespace CoinJoin {
 class Loader;
 } // namspace CoinJoin
+class Wallet;
 } // namespace interfaces
 
+using LoadWalletFn = std::function<void(std::unique_ptr<interfaces::Wallet> wallet)>;
+
 //! WalletContext struct containing references to state shared between CWallet
 //! instances, like the reference to the chain interface, and the list of opened
 //! wallets.
@@ -29,6 +38,9 @@ class Loader;
 struct WalletContext {
     interfaces::Chain* chain{nullptr};
     ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
+    Mutex wallets_mutex;
+    std::vector<std::shared_ptr<CWallet>> wallets GUARDED_BY(wallets_mutex);
+    std::list<LoadWalletFn> wallet_load_fns GUARDED_BY(wallets_mutex);
     // TODO: replace this unique_ptr to a pointer
     // probably possible to do after bitcoin/bitcoin#22219
     const std::unique_ptr<interfaces::CoinJoin::Loader>& coinjoin_loader;

src/wallet/interfaces.cpp

@@ -127,7 +127,7 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet,
 class WalletImpl : public Wallet
 {
 public:
-    explicit WalletImpl(const std::shared_ptr<CWallet>& wallet) : m_wallet(wallet) {}
+    explicit WalletImpl(WalletContext& context, const std::shared_ptr<CWallet>& wallet) : m_context(context), m_wallet(wallet) {}
 
     void markDirty() override
     {
@@ -510,7 +510,7 @@ class WalletImpl : public Wallet
     CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; }
     void remove() override
     {
-        RemoveWallet(m_wallet, false /* load_on_start */);
+        RemoveWallet(m_context, m_wallet, false /* load_on_start */);
     }
     bool isLegacy() override { return m_wallet->IsLegacy(); }
     std::unique_ptr<Handler> handleUnload(UnloadFn fn) override
@@ -556,6 +556,7 @@ class WalletImpl : public Wallet
     }
     CWallet* wallet() override { return m_wallet.get(); }
 
+    WalletContext& m_context;
     std::shared_ptr<CWallet> m_wallet;
     std::unique_ptr<interfaces::CoinJoin::Client> m_coinjoin_client;
 };
@@ -584,7 +585,7 @@ class WalletLoaderImpl : public WalletLoader
         m_context.chain = &chain;
         m_context.node_context = &node_context;
     }
-    ~WalletLoaderImpl() override { UnloadWallets(); }
+    ~WalletLoaderImpl() override { UnloadWallets(m_context); }
 
     //! ChainClient methods
     void registerRpcs() override
@@ -594,8 +595,8 @@ class WalletLoaderImpl : public WalletLoader
     bool verify() override { return VerifyWallets(m_context); }
     bool load() override { assert(m_context.coinjoin_loader); return LoadWallets(m_context); }
     void start(CScheduler& scheduler) override { return StartWallets(m_context, scheduler); }
-    void flush() override { return FlushWallets(); }
-    void stop() override { return StopWallets(); }
+    void flush() override { return FlushWallets(m_context); }
+    void stop() override { return StopWallets(m_context); }
     void setMockTime(int64_t time) override { return SetMockTime(time); }
 
     //! WalletLoader methods
@@ -612,21 +613,21 @@ class WalletLoaderImpl : public WalletLoader
         options.create_flags = wallet_creation_flags;
         options.create_passphrase = passphrase;
         assert(m_context.coinjoin_loader);
-        return MakeWallet(CreateWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
+        return MakeWallet(m_context, CreateWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
     }
     std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
     {
         DatabaseOptions options;
         DatabaseStatus status;
         options.require_existing = true;
         assert(m_context.coinjoin_loader);
-        return MakeWallet(LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
+        return MakeWallet(m_context, LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
     }
     std::unique_ptr<Wallet> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
     {
         DatabaseStatus status;
         assert(m_context.coinjoin_loader);
-        return MakeWallet(RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings));
+        return MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings));
     }
     std::string getWalletDir() override
     {
@@ -643,14 +644,14 @@ class WalletLoaderImpl : public WalletLoader
     std::vector<std::unique_ptr<Wallet>> getWallets() override
     {
         std::vector<std::unique_ptr<Wallet>> wallets;
-        for (const auto& wallet : GetWallets()) {
-            wallets.emplace_back(MakeWallet(wallet));
+        for (const auto& wallet : GetWallets(m_context)) {
+            wallets.emplace_back(MakeWallet(m_context, wallet));
         }
         return wallets;
     }
     std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) override
     {
-        return HandleLoadWallet(std::move(fn));
+        return HandleLoadWallet(m_context, std::move(fn));
     }
     WalletContext* context() override  { return &m_context; }
 
@@ -663,7 +664,7 @@ class WalletLoaderImpl : public WalletLoader
 } // namespace wallet
 
 namespace interfaces {
-std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet) { return wallet ? std::make_unique<wallet::WalletImpl>(wallet) : nullptr; }
+std::unique_ptr<Wallet> MakeWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet) { return wallet ? std::make_unique<wallet::WalletImpl>(context, wallet) : nullptr; }
 std::unique_ptr<WalletLoader> MakeWalletLoader(Chain& chain, ArgsManager& args, NodeContext& node_context,
                                                const std::unique_ptr<interfaces::CoinJoin::Loader>& coinjoin_loader) {
     return std::make_unique<wallet::WalletLoaderImpl>(chain, args, node_context, coinjoin_loader);

src/wallet/load.cpp

@@ -125,7 +125,7 @@ bool LoadWallets(WalletContext& context)
                 chain.initError(error_string);
                 return false;
             }
-            AddWallet(pwallet);
+            AddWallet(context, pwallet);
         }
         return true;
     } catch (const std::runtime_error& e) {
@@ -136,20 +136,20 @@ bool LoadWallets(WalletContext& context)
 
 void StartWallets(WalletContext& context, CScheduler& scheduler)
 {
-    for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
+    for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
         pwallet->postInitProcess();
     }
 
     // Schedule periodic wallet flushes and tx rebroadcasts
     if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
-        scheduler.scheduleEvery(MaybeCompactWalletDB, std::chrono::milliseconds{500});
+        scheduler.scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});
     }
-    scheduler.scheduleEvery(MaybeResendWalletTxs, std::chrono::milliseconds{1000});
+    scheduler.scheduleEvery([&context] { MaybeResendWalletTxs(context); }, std::chrono::milliseconds{1000});
 }
 
-void FlushWallets()
+void FlushWallets(WalletContext& context)
 {
-    for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
+    for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
         if (CCoinJoinClientOptions::IsEnabled()) {
             // Stop CoinJoin, release keys
             pwallet->coinjoin_loader().FlushWallet(pwallet->GetName());
@@ -158,21 +158,21 @@ void FlushWallets()
     }
 }
 
-void StopWallets()
+void StopWallets(WalletContext& context)
 {
-    for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
+    for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
         pwallet->Close();
     }
 }
 
-void UnloadWallets()
+void UnloadWallets(WalletContext& context)
 {
-    auto wallets = GetWallets();
+    auto wallets = GetWallets(context);
     while (!wallets.empty()) {
         auto wallet = wallets.back();
         wallets.pop_back();
         std::vector<bilingual_str> warnings;
-        RemoveWallet(wallet, std::nullopt, warnings);
+        RemoveWallet(context, wallet, /*load_on_startup=*/std::nullopt, warnings);
         UnloadWallet(std::move(wallet));
     }
 }

src/wallet/load.h

@@ -31,12 +31,12 @@ bool LoadWallets(WalletContext& context);
 void StartWallets(WalletContext& context, CScheduler& scheduler);
 
 //! Flush all wallets in preparation for shutdown.
-void FlushWallets();
+void FlushWallets(WalletContext& context);
 
 //! Stop all wallets. Wallets will be flushed first.
-void StopWallets();
+void StopWallets(WalletContext& context);
 
 //! Close all wallets.
-void UnloadWallets();
+void UnloadWallets(WalletContext& context);
 
 #endif // BITCOIN_WALLET_LOAD_H

src/wallet/rpcwallet.cpp

@@ -100,15 +100,16 @@ bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string&
 std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
 {
     CHECK_NONFATAL(request.mode == JSONRPCRequest::EXECUTE);
+    WalletContext& context = EnsureWalletContext(request.context);
 
     std::string wallet_name;
     if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) {
-        std::shared_ptr<CWallet> pwallet = GetWallet(wallet_name);
+        std::shared_ptr<CWallet> pwallet = GetWallet(context, wallet_name);
         if (!pwallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
         return pwallet;
     }
 
-    std::vector<std::shared_ptr<CWallet>> wallets = GetWallets();
+    std::vector<std::shared_ptr<CWallet>> wallets = GetWallets(context);
     if (wallets.size() == 1) {
         return wallets[0];
     }
@@ -2726,7 +2727,8 @@ static RPCHelpMan listwallets()
 {
     UniValue obj(UniValue::VARR);
 
-    for (const std::shared_ptr<CWallet>& wallet : GetWallets()) {
+    WalletContext& context = EnsureWalletContext(request.context);
+    for (const std::shared_ptr<CWallet>& wallet : GetWallets(context)) {
         LOCK(wallet->cs_wallet);
         obj.push_back(wallet->GetName());
     }
@@ -3145,7 +3147,8 @@ static RPCHelpMan unloadwallet()
         wallet_name = request.params[0].get_str();
     }
 
-    std::shared_ptr<CWallet> wallet = GetWallet(wallet_name);
+    WalletContext& context = EnsureWalletContext(request.context);
+    std::shared_ptr<CWallet> wallet = GetWallet(context, wallet_name);
     if (!wallet) {
         throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
     }
@@ -3155,7 +3158,7 @@ static RPCHelpMan unloadwallet()
     // is destroyed (see CheckUniqueFileid).
     std::vector<bilingual_str> warnings;
     std::optional<bool> load_on_start = request.params[1].isNull() ? std::nullopt : std::optional<bool>(request.params[1].get_bool());
-    if (!RemoveWallet(wallet, load_on_start, warnings)) {
+    if (!RemoveWallet(context, wallet, load_on_start, warnings)) {
         throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded");
     }
 

src/wallet/test/coinjoin_tests.cpp

@@ -14,6 +14,7 @@
 #include <util/translation.h>
 #include <policy/settings.h>
 #include <validation.h>
+#include <wallet/context.h>
 #include <wallet/wallet.h>
 
 #include <boost/test/unit_test.hpp>
@@ -129,12 +130,14 @@ class CTransactionBuilderTestSetup : public TestChain100Setup
 {
 public:
     CTransactionBuilderTestSetup()
+        : context{m_node.coinjoin_loader}
     {
+        context.chain = m_node.chain.get();
         CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
         wallet = std::make_unique<CWallet>(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase());
         wallet->SetupLegacyScriptPubKeyMan();
         wallet->LoadWallet();
-        AddWallet(wallet);
+        AddWallet(context, wallet);
         {
             LOCK2(wallet->cs_wallet, cs_main);
             wallet->GetLegacyScriptPubKeyMan()->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
@@ -148,9 +151,10 @@ class CTransactionBuilderTestSetup : public TestChain100Setup
 
     ~CTransactionBuilderTestSetup()
     {
-        RemoveWallet(wallet, std::nullopt);
+        RemoveWallet(context, wallet, /*load_on_startup=*/std::nullopt);
     }
 
+    WalletContext context;
     std::shared_ptr<CWallet> wallet;
 
     CWalletTx& AddTxToChain(uint256 nTxHash)