Skip to content

Commit 62a09a3

Browse files
ryanofskyryanofsky
committed
refactor: remove ::vpwallets and related global variables
Move global wallet variables to WalletContext struct
1 parent fdd80b0 commit 62a09a3

File tree

13 files changed

+167
-127
lines changed

13 files changed

+167
-127
lines changed

‎src/interfaces/wallet.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,9 @@ class WalletClient : public ChainClient
332332
//! loaded at startup or by RPC.
333333
using LoadWalletFn = std::function<void(std::unique_ptr<Wallet> wallet)>;
334334
virtual std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) = 0;
335+
336+
//! Return pointer to internal context, useful for testing.
337+
virtual WalletContext* context() { return nullptr; }
335338
};
336339

337340
//! Information about one wallet address.
@@ -410,7 +413,7 @@ struct WalletTxOut
410413

411414
//! Return implementation of Wallet interface. This function is defined in
412415
//! dummywallet.cpp and throws if the wallet component is not compiled.
413-
std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet);
416+
std::unique_ptr<Wallet> MakeWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet);
414417

415418
//! Return implementation of ChainClient interface for a wallet client. This
416419
//! function will be undefined in builds where ENABLE_WALLET is false.

‎src/qt/test/addressbooktests.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,10 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
109109
std::unique_ptr<const PlatformStyle> platformStyle(PlatformStyle::instantiate("other"));
110110
OptionsModel optionsModel;
111111
ClientModel clientModel(node, &optionsModel);
112-
AddWallet(wallet);
113-
WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel, platformStyle.get());
114-
RemoveWallet(wallet, std::nullopt);
112+
WalletContext& context = *node.walletClient().context();
113+
AddWallet(context, wallet);
114+
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
115+
RemoveWallet(context, wallet, /* load_on_startup= */ std::nullopt);
115116
EditAddressDialog editAddressDialog(EditAddressDialog::NewSendingAddress);
116117
editAddressDialog.setModel(walletModel.getAddressTableModel());
117118

‎src/qt/test/wallettests.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -164,9 +164,10 @@ void TestGUI(interfaces::Node& node)
164164
TransactionView transactionView(platformStyle.get());
165165
OptionsModel optionsModel;
166166
ClientModel clientModel(node, &optionsModel);
167-
AddWallet(wallet);
168-
WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel, platformStyle.get());
169-
RemoveWallet(wallet, std::nullopt);
167+
WalletContext& context = *node.walletClient().context();
168+
AddWallet(context, wallet);
169+
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
170+
RemoveWallet(context, wallet, /* load_on_startup= */ std::nullopt);
170171
sendCoinsDialog.setModel(&walletModel);
171172
transactionView.setModel(&walletModel);
172173

‎src/wallet/context.h

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,22 @@
55
#ifndef BITCOIN_WALLET_CONTEXT_H
66
#define BITCOIN_WALLET_CONTEXT_H
77

8+
#include <sync.h>
9+
10+
#include <functional>
11+
#include <list>
12+
#include <memory>
13+
#include <vector>
14+
815
class ArgsManager;
16+
class CWallet;
917
namespace interfaces {
1018
class Chain;
19+
class Wallet;
1120
} // namespace interfaces
1221

22+
using LoadWalletFn = std::function<void(std::unique_ptr<interfaces::Wallet> wallet)>;
23+
1324
//! WalletContext struct containing references to state shared between CWallet
1425
//! instances, like the reference to the chain interface, and the list of opened
1526
//! wallets.
@@ -22,7 +33,10 @@ class Chain;
2233
//! behavior.
2334
struct WalletContext {
2435
interfaces::Chain* chain{nullptr};
25-
ArgsManager* args{nullptr};
36+
ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
37+
Mutex wallets_mutex;
38+
std::vector<std::shared_ptr<CWallet>> wallets GUARDED_BY(wallets_mutex);
39+
std::list<LoadWalletFn> wallet_load_fns GUARDED_BY(wallets_mutex);
2640

2741
//! Declare default constructor and destructor that are not inline, so code
2842
//! instantiating the WalletContext struct doesn't need to #include class

‎src/wallet/interfaces.cpp

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet,
110110
class WalletImpl : public Wallet
111111
{
112112
public:
113-
explicit WalletImpl(const std::shared_ptr<CWallet>& wallet) : m_wallet(wallet) {}
113+
explicit WalletImpl(WalletContext& context, const std::shared_ptr<CWallet>& wallet) : m_context(context), m_wallet(wallet) {}
114114

115115
bool encryptWallet(const SecureString& wallet_passphrase) override
116116
{
@@ -458,7 +458,7 @@ class WalletImpl : public Wallet
458458
CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; }
459459
void remove() override
460460
{
461-
RemoveWallet(m_wallet, false /* load_on_start */);
461+
RemoveWallet(m_context, m_wallet, false /* load_on_start */);
462462
}
463463
bool isLegacy() override { return m_wallet->IsLegacy(); }
464464
std::unique_ptr<Handler> handleUnload(UnloadFn fn) override
@@ -494,6 +494,7 @@ class WalletImpl : public Wallet
494494
}
495495
CWallet* wallet() override { return m_wallet.get(); }
496496

497+
WalletContext& m_context;
497498
std::shared_ptr<CWallet> m_wallet;
498499
};
499500

@@ -505,7 +506,7 @@ class WalletClientImpl : public WalletClient
505506
m_context.chain = &chain;
506507
m_context.args = &args;
507508
}
508-
~WalletClientImpl() override { UnloadWallets(); }
509+
~WalletClientImpl() override { UnloadWallets(m_context); }
509510

510511
//! ChainClient methods
511512
void registerRpcs() override
@@ -519,11 +520,11 @@ class WalletClientImpl : public WalletClient
519520
m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back()));
520521
}
521522
}
522-
bool verify() override { return VerifyWallets(*m_context.chain); }
523-
bool load() override { return LoadWallets(*m_context.chain); }
524-
void start(CScheduler& scheduler) override { return StartWallets(scheduler, *Assert(m_context.args)); }
525-
void flush() override { return FlushWallets(); }
526-
void stop() override { return StopWallets(); }
523+
bool verify() override { return VerifyWallets(m_context); }
524+
bool load() override { return LoadWallets(m_context); }
525+
void start(CScheduler& scheduler) override { return StartWallets(m_context, scheduler); }
526+
void flush() override { return FlushWallets(m_context); }
527+
void stop() override { return StopWallets(m_context); }
527528
void setMockTime(int64_t time) override { return SetMockTime(time); }
528529

529530
//! WalletClient methods
@@ -535,14 +536,14 @@ class WalletClientImpl : public WalletClient
535536
options.require_create = true;
536537
options.create_flags = wallet_creation_flags;
537538
options.create_passphrase = passphrase;
538-
return MakeWallet(CreateWallet(*m_context.chain, name, true /* load_on_start */, options, status, error, warnings));
539+
return MakeWallet(m_context, CreateWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
539540
}
540541
std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
541542
{
542543
DatabaseOptions options;
543544
DatabaseStatus status;
544545
options.require_existing = true;
545-
return MakeWallet(LoadWallet(*m_context.chain, name, true /* load_on_start */, options, status, error, warnings));
546+
return MakeWallet(m_context, LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
546547
}
547548
std::string getWalletDir() override
548549
{
@@ -559,15 +560,16 @@ class WalletClientImpl : public WalletClient
559560
std::vector<std::unique_ptr<Wallet>> getWallets() override
560561
{
561562
std::vector<std::unique_ptr<Wallet>> wallets;
562-
for (const auto& wallet : GetWallets()) {
563-
wallets.emplace_back(MakeWallet(wallet));
563+
for (const auto& wallet : GetWallets(m_context)) {
564+
wallets.emplace_back(MakeWallet(m_context, wallet));
564565
}
565566
return wallets;
566567
}
567568
std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) override
568569
{
569-
return HandleLoadWallet(std::move(fn));
570+
return HandleLoadWallet(m_context, std::move(fn));
570571
}
572+
WalletContext* context() override { return &m_context; }
571573

572574
WalletContext m_context;
573575
const std::vector<std::string> m_wallet_filenames;
@@ -578,7 +580,7 @@ class WalletClientImpl : public WalletClient
578580
} // namespace wallet
579581

580582
namespace interfaces {
581-
std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet) { return wallet ? std::make_unique<wallet::WalletImpl>(wallet) : nullptr; }
583+
std::unique_ptr<Wallet> MakeWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet) { return wallet ? std::make_unique<wallet::WalletImpl>(context, wallet) : nullptr; }
582584

583585
std::unique_ptr<WalletClient> MakeWalletClient(Chain& chain, ArgsManager& args)
584586
{

‎src/wallet/load.cpp

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,15 @@
1111
#include <util/string.h>
1212
#include <util/system.h>
1313
#include <util/translation.h>
14+
#include <wallet/context.h>
1415
#include <wallet/wallet.h>
1516
#include <wallet/walletdb.h>
1617

1718
#include <univalue.h>
1819

19-
bool VerifyWallets(interfaces::Chain& chain)
20+
bool VerifyWallets(WalletContext& context)
2021
{
22+
interfaces::Chain& chain = *context.chain;
2123
if (gArgs.IsArgSet("-walletdir")) {
2224
fs::path wallet_dir = gArgs.GetArg("-walletdir", "");
2325
boost::system::error_code error;
@@ -87,8 +89,9 @@ bool VerifyWallets(interfaces::Chain& chain)
8789
return true;
8890
}
8991

90-
bool LoadWallets(interfaces::Chain& chain)
92+
bool LoadWallets(WalletContext& context)
9193
{
94+
interfaces::Chain& chain = *context.chain;
9295
try {
9396
std::set<fs::path> wallet_paths;
9497
for (const std::string& name : gArgs.GetArgs("-wallet")) {
@@ -106,13 +109,13 @@ bool LoadWallets(interfaces::Chain& chain)
106109
continue;
107110
}
108111
chain.initMessage(_("Loading wallet…").translated);
109-
std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(&chain, name, std::move(database), options.create_flags, error, warnings) : nullptr;
112+
std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings) : nullptr;
110113
if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
111114
if (!pwallet) {
112115
chain.initError(error);
113116
return false;
114117
}
115-
AddWallet(pwallet);
118+
AddWallet(context, pwallet);
116119
}
117120
return true;
118121
} catch (const std::runtime_error& e) {
@@ -121,41 +124,41 @@ bool LoadWallets(interfaces::Chain& chain)
121124
}
122125
}
123126

124-
void StartWallets(CScheduler& scheduler, const ArgsManager& args)
127+
void StartWallets(WalletContext& context, CScheduler& scheduler)
125128
{
126-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
129+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
127130
pwallet->postInitProcess();
128131
}
129132

130133
// Schedule periodic wallet flushes and tx rebroadcasts
131-
if (args.GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
132-
scheduler.scheduleEvery(MaybeCompactWalletDB, std::chrono::milliseconds{500});
134+
if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
135+
scheduler.scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});
133136
}
134-
scheduler.scheduleEvery(MaybeResendWalletTxs, std::chrono::milliseconds{1000});
137+
scheduler.scheduleEvery([&context] { MaybeResendWalletTxs(context); }, std::chrono::milliseconds{1000});
135138
}
136139

137-
void FlushWallets()
140+
void FlushWallets(WalletContext& context)
138141
{
139-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
142+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
140143
pwallet->Flush();
141144
}
142145
}
143146

144-
void StopWallets()
147+
void StopWallets(WalletContext& context)
145148
{
146-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
149+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
147150
pwallet->Close();
148151
}
149152
}
150153

151-
void UnloadWallets()
154+
void UnloadWallets(WalletContext& context)
152155
{
153-
auto wallets = GetWallets();
156+
auto wallets = GetWallets(context);
154157
while (!wallets.empty()) {
155158
auto wallet = wallets.back();
156159
wallets.pop_back();
157160
std::vector<bilingual_str> warnings;
158-
RemoveWallet(wallet, std::nullopt, warnings);
161+
RemoveWallet(context, wallet, /* load_on_startup= */ std::nullopt, warnings);
159162
UnloadWallet(std::move(wallet));
160163
}
161164
}

‎src/wallet/load.h

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,27 +11,28 @@
1111

1212
class ArgsManager;
1313
class CScheduler;
14+
struct WalletContext;
1415

1516
namespace interfaces {
1617
class Chain;
1718
} // namespace interfaces
1819

1920
//! Responsible for reading and validating the -wallet arguments and verifying the wallet database.
20-
bool VerifyWallets(interfaces::Chain& chain);
21+
bool VerifyWallets(WalletContext& context);
2122

2223
//! Load wallet databases.
23-
bool LoadWallets(interfaces::Chain& chain);
24+
bool LoadWallets(WalletContext& context);
2425

2526
//! Complete startup of wallets.
26-
void StartWallets(CScheduler& scheduler, const ArgsManager& args);
27+
void StartWallets(WalletContext& context, CScheduler& scheduler);
2728

2829
//! Flush all wallets in preparation for shutdown.
29-
void FlushWallets();
30+
void FlushWallets(WalletContext& context);
3031

3132
//! Stop all wallets. Wallets will be flushed first.
32-
void StopWallets();
33+
void StopWallets(WalletContext& context);
3334

3435
//! Close all wallets.
35-
void UnloadWallets();
36+
void UnloadWallets(WalletContext& context);
3637

3738
#endif // BITCOIN_WALLET_LOAD_H

‎src/wallet/rpcwallet.cpp

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -96,14 +96,16 @@ bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string&
9696
std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
9797
{
9898
CHECK_NONFATAL(request.mode == JSONRPCRequest::EXECUTE);
99+
WalletContext& context = EnsureWalletContext(request.context);
100+
99101
std::string wallet_name;
100102
if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) {
101-
std::shared_ptr<CWallet> pwallet = GetWallet(wallet_name);
103+
std::shared_ptr<CWallet> pwallet = GetWallet(context, wallet_name);
102104
if (!pwallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
103105
return pwallet;
104106
}
105107

106-
std::vector<std::shared_ptr<CWallet>> wallets = GetWallets();
108+
std::vector<std::shared_ptr<CWallet>> wallets = GetWallets(context);
107109
if (wallets.size() == 1) {
108110
return wallets[0];
109111
}
@@ -2562,7 +2564,8 @@ static RPCHelpMan listwallets()
25622564
{
25632565
UniValue obj(UniValue::VARR);
25642566

2565-
for (const std::shared_ptr<CWallet>& wallet : GetWallets()) {
2567+
WalletContext& context = EnsureWalletContext(request.context);
2568+
for (const std::shared_ptr<CWallet>& wallet : GetWallets(context)) {
25662569
LOCK(wallet->cs_wallet);
25672570
obj.push_back(wallet->GetName());
25682571
}
@@ -2580,7 +2583,7 @@ static std::tuple<std::shared_ptr<CWallet>, std::vector<bilingual_str>> LoadWall
25802583
bilingual_str error;
25812584
std::vector<bilingual_str> warnings;
25822585
std::optional<bool> load_on_start = load_on_start_param.isNull() ? std::nullopt : std::optional<bool>(load_on_start_param.get_bool());
2583-
std::shared_ptr<CWallet> const wallet = LoadWallet(*context.chain, wallet_name, load_on_start, options, status, error, warnings);
2586+
std::shared_ptr<CWallet> const wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
25842587

25852588
if (!wallet) {
25862589
// Map bad format to not found, since bad format is returned when the
@@ -2788,7 +2791,7 @@ static RPCHelpMan createwallet()
27882791
options.create_passphrase = passphrase;
27892792
bilingual_str error;
27902793
std::optional<bool> load_on_start = request.params[6].isNull() ? std::nullopt : std::optional<bool>(request.params[6].get_bool());
2791-
std::shared_ptr<CWallet> wallet = CreateWallet(*context.chain, request.params[0].get_str(), load_on_start, options, status, error, warnings);
2794+
std::shared_ptr<CWallet> wallet = CreateWallet(context, request.params[0].get_str(), load_on_start, options, status, error, warnings);
27922795
if (!wallet) {
27932796
RPCErrorCode code = status == DatabaseStatus::FAILED_ENCRYPT ? RPC_WALLET_ENCRYPTION_FAILED : RPC_WALLET_ERROR;
27942797
throw JSONRPCError(code, error.original);
@@ -2892,7 +2895,8 @@ static RPCHelpMan unloadwallet()
28922895
wallet_name = request.params[0].get_str();
28932896
}
28942897

2895-
std::shared_ptr<CWallet> wallet = GetWallet(wallet_name);
2898+
WalletContext& context = EnsureWalletContext(request.context);
2899+
std::shared_ptr<CWallet> wallet = GetWallet(context, wallet_name);
28962900
if (!wallet) {
28972901
throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
28982902
}
@@ -2902,7 +2906,7 @@ static RPCHelpMan unloadwallet()
29022906
// is destroyed (see CheckUniqueFileid).
29032907
std::vector<bilingual_str> warnings;
29042908
std::optional<bool> load_on_start = request.params[1].isNull() ? std::nullopt : std::optional<bool>(request.params[1].get_bool());
2905-
if (!RemoveWallet(wallet, load_on_start, warnings)) {
2909+
if (!RemoveWallet(context, wallet, load_on_start, warnings)) {
29062910
throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded");
29072911
}
29082912

0 commit comments

Comments
 (0)