Merge bitcoin#19101: refactor: remove ::vpwallets and related global …

…variables

62a09a3 refactor: remove ::vpwallets and related global variables (Russell Yanofsky)

Pull request description:

  Get rid of global wallet list variables by moving them to WalletContext struct

  - [`cs_wallets`](https://github.com/bitcoin/bitcoin/blob/e638acf6970394f8eb1957366ad2d39512f33b31/src/wallet/wallet.cpp#L56) is now [`WalletContext::wallet_mutex`](https://github.com/ryanofsky/bitcoin/blob/4be544c7ec08a81952fd3f4349151cbb8bdb60e8/src/wallet/context.h#L37)
  - [`vpwallets`](https://github.com/bitcoin/bitcoin/blob/e638acf6970394f8eb1957366ad2d39512f33b31/src/wallet/wallet.cpp#L57) is now [`WalletContext::wallets`](https://github.com/ryanofsky/bitcoin/blob/4be544c7ec08a81952fd3f4349151cbb8bdb60e8/src/wallet/context.h#L38)
  - [`g_load_wallet_fns`](https://github.com/bitcoin/bitcoin/blob/e638acf6970394f8eb1957366ad2d39512f33b31/src/wallet/wallet.cpp#L58) is now [`WalletContext::wallet_load_fns`](https://github.com/ryanofsky/bitcoin/blob/4be544c7ec08a81952fd3f4349151cbb8bdb60e8/src/wallet/context.h#L39)

ACKs for top commit:
  achow101:
    ACK 62a09a3
  meshcollider:
    re-utACK 62a09a3

Tree-SHA512: 74428180d57b4214c3d96963e6ff43e8778f6f23b6880262d1272f2de67d02714fdc3ebb558f62e48655b221a642c36f80ef37c8f89d362e2d66fd93cbf03b8f

src/interfaces/wallet.h

@@ -360,6 +360,9 @@ class WalletLoader : public ChainClient
    //! loaded at startup or by RPC.
    using LoadWalletFn = std::function<void(std::unique_ptr<Wallet> wallet)>;
    virtual std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) = 0;
+
+   //! Return pointer to internal context, useful for testing.
+   virtual WalletContext* context() { return nullptr; }
 };
 
 //! Information about one wallet address.
@@ -443,7 +446,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

@@ -106,9 +106,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.walletClient().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

@@ -133,7 +133,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);
 

src/wallet/context.h

@@ -6,15 +6,25 @@
 #define BITCOIN_WALLET_CONTEXT_H
 
 #include <memory>
+#include <sync.h>
+
+#include <functional>
+#include <list>
+#include <memory>
+#include <vector>
 
 class ArgsManager;
+class CWallet;
 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.
@@ -27,7 +37,10 @@ class Loader;
 //! behavior.
 struct WalletContext {
     interfaces::Chain* chain{nullptr};
-    ArgsManager* args{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>& m_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
     {
@@ -509,7 +509,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
@@ -555,6 +555,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;
 };
@@ -568,7 +569,7 @@ class WalletLoaderImpl : public WalletLoader
         m_context.chain = &chain;
         m_context.args = &args;
     }
-    ~WalletLoaderImpl() override { UnloadWallets(); }
+    ~WalletClientImpl() override { UnloadWallets(m_context); }
 
     //! ChainClient methods
     void registerRpcs() override
@@ -582,11 +583,11 @@ class WalletLoaderImpl : public WalletLoader
             m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back()));
         }
     }
-    bool verify() override { return VerifyWallets(*m_context.chain); }
-    bool load() override { assert(m_context.m_coinjoin_loader); return LoadWallets(*m_context.chain, *m_context.m_coinjoin_loader); }
-    void start(CScheduler& scheduler) override { return StartWallets(scheduler, *Assert(m_context.args)); }
-    void flush() override { return FlushWallets(); }
-    void stop() override { return StopWallets(); }
+    bool verify() override { return VerifyWallets(m_context); }
+    bool load() override { assert(m_context.m_coinjoin_loader); return LoadWallets(m_context, *m_context.m_coinjoin_loader); }
+    void start(CScheduler& scheduler) override { return StartWallets(m_context, scheduler); }
+    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
@@ -599,15 +600,15 @@ class WalletLoaderImpl : public WalletLoader
         options.create_flags = wallet_creation_flags;
         options.create_passphrase = passphrase;
         assert(m_context.m_coinjoin_loader);
-        return MakeWallet(CreateWallet(*m_context.chain, *m_context.m_coinjoin_loader, name, true /* load_on_start */, options, status, error, warnings));
+        return MakeWallet(m_context, CreateWallet(*m_context.chain, *m_context.m_coinjoin_loader, 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.m_coinjoin_loader);
-        return MakeWallet(LoadWallet(*m_context.chain, *m_context.m_coinjoin_loader, name, true /* load_on_start */, options, status, error, warnings));
+        return MakeWallet(m_context, LoadWallet(m_context, *m_context.m_coinjoin_loader, 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
     {
@@ -630,15 +631,16 @@ 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; }
 
     WalletContext m_context;
     const std::vector<std::string> m_wallet_filenames;
@@ -649,7 +651,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>(wallet) : nullptr; }
 std::unique_ptr<WalletLoader> MakeWalletLoader(Chain& chain, const std::unique_ptr<interfaces::CoinJoin::Loader>& coinjoin_loader, ArgsManager& args) {
     return std::make_unique<wallet::WalletLoaderImpl>(chain, coinjoin_loader, args);
 }

src/wallet/load.cpp

@@ -14,15 +14,17 @@
 #include <util/string.h>
 #include <util/system.h>
 #include <util/translation.h>
+#include <wallet/context.h>
 #include <wallet/wallet.h>
 #include <wallet/walletdb.h>
 
 #include <univalue.h>
 
 #include <system_error>
 
-bool VerifyWallets(interfaces::Chain& chain)
+bool VerifyWallets(WalletContext& context)
 {
+    interfaces::Chain& chain = *context.chain;
     if (gArgs.IsArgSet("-walletdir")) {
         fs::path wallet_dir = fs::PathFromString(gArgs.GetArg("-walletdir", ""));
         std::error_code error;
@@ -94,8 +96,9 @@ bool VerifyWallets(interfaces::Chain& chain)
     return true;
 }
 
-bool LoadWallets(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoin_loader)
+bool LoadWallets(WalletContext& context, interfaces::CoinJoin::Loader& coinjoin_loader)
 {
+    interfaces::Chain& chain = *context.chain;
     try {
         std::set<fs::path> wallet_paths;
         for (const std::string& name : gArgs.GetArgs("-wallet")) {
@@ -113,13 +116,13 @@ bool LoadWallets(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoi
                 continue;
             }
             chain.initMessage(_("Loading wallet...").translated);
-            std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(&chain, &coinjoin_loader, name, std::move(database), options.create_flags, error_string, warnings) : nullptr;
+            std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(context, &coinjoin_loader, name, std::move(database), options.create_flags, error_string, warnings) : nullptr;
             if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
             if (!pwallet) {
                 chain.initError(error_string);
                 return false;
             }
-            AddWallet(pwallet);
+            AddWallet(context, pwallet);
         }
         return true;
     } catch (const std::runtime_error& e) {
@@ -128,22 +131,22 @@ bool LoadWallets(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoi
     }
 }
 
-void StartWallets(CScheduler& scheduler, const ArgsManager& args)
+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 (args.GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
-        scheduler.scheduleEvery(MaybeCompactWalletDB, std::chrono::milliseconds{500});
+    if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
+        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());
@@ -152,21 +155,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

@@ -12,6 +12,7 @@
 class ArgsManager;
 class CConnman;
 class CScheduler;
+struct WalletContext;
 
 namespace interfaces {
 class Chain;
@@ -21,21 +22,21 @@ class Loader;
 } // namespace interfaces
 
 //! Responsible for reading and validating the -wallet arguments and verifying the wallet database.
-bool VerifyWallets(interfaces::Chain& chain);
+bool VerifyWallets(WalletContext& context);
 
 //! Load wallet databases.
-bool LoadWallets(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoin_loader);
+bool LoadWallets(WalletContext& context, interfaces::CoinJoin::Loader& coinjoin_loader);
 
 //! Complete startup of wallets.
-void StartWallets(CScheduler& scheduler, const ArgsManager& args);
+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

@@ -103,15 +103,19 @@ bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string&
 std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
 {
     CHECK_NONFATAL(request.mode == JSONRPCRequest::EXECUTE);
+<<<<<<< HEAD
+=======
+    WalletContext& context = EnsureWalletContext(request.context);
+>>>>>>> 638855af63... Merge bitcoin/bitcoin#19101: refactor: remove ::vpwallets and related global variables
 
     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];
     }
@@ -2715,7 +2719,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());
     }
@@ -3039,7 +3044,7 @@ static RPCHelpMan createwallet()
     options.create_passphrase = passphrase;
     bilingual_str error;
     std::optional<bool> load_on_start = request.params[6].isNull() ? std::nullopt : std::optional<bool>(request.params[6].get_bool());
-    std::shared_ptr<CWallet> wallet = CreateWallet(*context.chain, *context.m_coinjoin_loader, request.params[0].get_str(), load_on_start, options, status, error, warnings);
+    std::shared_ptr<CWallet> wallet = CreateWallet(context, *context.m_coinjoin_loader, request.params[0].get_str(), load_on_start, options, status, error, warnings);
     if (!wallet) {
         RPCErrorCode code = status == DatabaseStatus::FAILED_ENCRYPT ? RPC_WALLET_ENCRYPTION_FAILED : RPC_WALLET_ERROR;
         throw JSONRPCError(code, error.original);
@@ -3134,7 +3139,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");
     }
@@ -3144,7 +3150,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");
     }