Skip to content

Commit a00fc53

Browse files
ryanofskyryanofsky
committed
refactor: remove ::vpwallets and related global variables
Move global wallet variables to WalletContext struct
1 parent 61e8104 commit a00fc53

File tree

14 files changed

+173
-126
lines changed

14 files changed

+173
-126
lines changed

‎src/interfaces/wallet.cpp‎

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ WalletTxOut MakeWalletTxOut(CWallet& wallet,
9696
class WalletImpl : public Wallet
9797
{
9898
public:
99-
explicit WalletImpl(const std::shared_ptr<CWallet>& wallet) : m_wallet(wallet) {}
99+
explicit WalletImpl(WalletContext& context, const std::shared_ptr<CWallet>& wallet) : m_context(context), m_wallet(wallet) {}
100100

101101
bool encryptWallet(const SecureString& wallet_passphrase) override
102102
{
@@ -441,7 +441,7 @@ class WalletImpl : public Wallet
441441
CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; }
442442
void remove() override
443443
{
444-
RemoveWallet(m_wallet);
444+
RemoveWallet(m_context, m_wallet);
445445
}
446446
bool isLegacy() override { return m_wallet->IsLegacy(); }
447447
std::unique_ptr<Handler> handleUnload(UnloadFn fn) override
@@ -477,6 +477,7 @@ class WalletImpl : public Wallet
477477
}
478478
CWallet* wallet() override { return m_wallet.get(); }
479479

480+
WalletContext& m_context;
480481
std::shared_ptr<CWallet> m_wallet;
481482
};
482483

@@ -489,7 +490,7 @@ class WalletClientImpl : public WalletClient
489490
m_context.chain = &chain;
490491
m_context.args = &args;
491492
}
492-
~WalletClientImpl() override { UnloadWallets(); }
493+
~WalletClientImpl() override { UnloadWallets(m_context); }
493494

494495
//! ChainClient methods
495496
void registerRpcs() override
@@ -501,23 +502,23 @@ class WalletClientImpl : public WalletClient
501502
m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back()));
502503
}
503504
}
504-
bool verify() override { return VerifyWallets(*m_context.chain, m_wallet_filenames); }
505-
bool load() override { return LoadWallets(*m_context.chain, m_wallet_filenames); }
506-
void start(CScheduler& scheduler) override { return StartWallets(scheduler, *Assert(m_context.args)); }
507-
void flush() override { return FlushWallets(); }
508-
void stop() override { return StopWallets(); }
505+
bool verify() override { return VerifyWallets(m_context, m_wallet_filenames); }
506+
bool load() override { return LoadWallets(m_context, m_wallet_filenames); }
507+
void start(CScheduler& scheduler) override { return StartWallets(m_context, scheduler); }
508+
void flush() override { return FlushWallets(m_context); }
509+
void stop() override { return StopWallets(m_context); }
509510
void setMockTime(int64_t time) override { return SetMockTime(time); }
510511

511512
//! WalletClient methods
512513
std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, WalletCreationStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings) override
513514
{
514515
std::shared_ptr<CWallet> wallet;
515-
status = CreateWallet(*m_context.chain, passphrase, wallet_creation_flags, name, error, warnings, wallet);
516-
return MakeWallet(std::move(wallet));
516+
status = CreateWallet(m_context, passphrase, wallet_creation_flags, name, error, warnings, wallet);
517+
return MakeWallet(m_context, std::move(wallet));
517518
}
518519
std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
519520
{
520-
return MakeWallet(LoadWallet(*m_context.chain, WalletLocation(name), error, warnings));
521+
return MakeWallet(m_context, LoadWallet(m_context, WalletLocation(name), error, warnings));
521522
}
522523
std::string getWalletDir() override
523524
{
@@ -534,15 +535,16 @@ class WalletClientImpl : public WalletClient
534535
std::vector<std::unique_ptr<Wallet>> getWallets() override
535536
{
536537
std::vector<std::unique_ptr<Wallet>> wallets;
537-
for (const auto& wallet : GetWallets()) {
538-
wallets.emplace_back(MakeWallet(wallet));
538+
for (const auto& wallet : GetWallets(m_context)) {
539+
wallets.emplace_back(MakeWallet(m_context, wallet));
539540
}
540541
return wallets;
541542
}
542543
std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) override
543544
{
544-
return HandleLoadWallet(std::move(fn));
545+
return HandleLoadWallet(m_context, std::move(fn));
545546
}
547+
WalletContext* context() override { return &m_context; }
546548

547549
WalletContext m_context;
548550
const std::vector<std::string> m_wallet_filenames;
@@ -552,7 +554,7 @@ class WalletClientImpl : public WalletClient
552554

553555
} // namespace
554556

555-
std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet) { return wallet ? MakeUnique<WalletImpl>(wallet) : nullptr; }
557+
std::unique_ptr<Wallet> MakeWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet) { return wallet ? MakeUnique<WalletImpl>(context, wallet) : nullptr; }
556558

557559
std::unique_ptr<WalletClient> MakeWalletClient(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames)
558560
{

‎src/interfaces/wallet.h‎

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

335338
//! Information about one wallet address.
@@ -408,7 +411,7 @@ struct WalletTxOut
408411

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

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

‎src/qt/bitcoin.cpp‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,15 @@ void BitcoinApplication::requestShutdown()
336336
window->setClientModel(nullptr);
337337
pollShutdownTimer->stop();
338338

339+
#ifdef ENABLE_WALLET
340+
// Delete wallet controller here to unregister wallet callbacks. Without
341+
// this, the callbacks may be unregistered too late after the wallet
342+
// context is deleted, or the wallet may try to call back into the GUI when
343+
// the GUI is deleted, resulting in segfaults either way.
344+
delete m_wallet_controller;
345+
m_wallet_controller = nullptr;
346+
#endif // ENABLE_WALLET
347+
339348
delete clientModel;
340349
clientModel = nullptr;
341350

‎src/qt/test/addressbooktests.cpp‎

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

‎src/qt/test/wallettests.cpp‎

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

‎src/wallet/context.h‎

Lines changed: 14 additions & 0 deletions
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.
@@ -23,6 +34,9 @@ class Chain;
2334
struct WalletContext {
2435
interfaces::Chain* chain{nullptr};
2536
ArgsManager* args{nullptr};
37+
RecursiveMutex 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/load.cpp‎

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,13 @@
1010
#include <util/string.h>
1111
#include <util/system.h>
1212
#include <util/translation.h>
13+
#include <wallet/context.h>
1314
#include <wallet/wallet.h>
1415
#include <wallet/walletdb.h>
1516

16-
bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files)
17+
bool VerifyWallets(WalletContext& context, const std::vector<std::string>& wallet_files)
1718
{
19+
interfaces::Chain& chain = *context.chain;
1820
if (gArgs.IsArgSet("-walletdir")) {
1921
fs::path wallet_dir = gArgs.GetArg("-walletdir", "");
2022
boost::system::error_code error;
@@ -51,7 +53,7 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
5153

5254
bilingual_str error_string;
5355
std::vector<bilingual_str> warnings;
54-
bool verify_success = CWallet::Verify(chain, location, error_string, warnings);
56+
bool verify_success = CWallet::Verify(context, location, error_string, warnings);
5557
if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
5658
if (!verify_success) {
5759
chain.initError(error_string);
@@ -62,19 +64,20 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
6264
return true;
6365
}
6466

65-
bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files)
67+
bool LoadWallets(WalletContext& context, const std::vector<std::string>& wallet_files)
6668
{
69+
interfaces::Chain& chain = *context.chain;
6770
try {
6871
for (const std::string& walletFile : wallet_files) {
6972
bilingual_str error;
7073
std::vector<bilingual_str> warnings;
71-
std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(chain, WalletLocation(walletFile), error, warnings);
74+
std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(context, WalletLocation(walletFile), error, warnings);
7275
if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
7376
if (!pwallet) {
7477
chain.initError(error);
7578
return false;
7679
}
77-
AddWallet(pwallet);
80+
AddWallet(context, pwallet);
7881
}
7982
return true;
8083
} catch (const std::runtime_error& e) {
@@ -83,40 +86,40 @@ bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& walle
8386
}
8487
}
8588

86-
void StartWallets(CScheduler& scheduler, const ArgsManager& args)
89+
void StartWallets(WalletContext& context, CScheduler& scheduler)
8790
{
88-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
91+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
8992
pwallet->postInitProcess();
9093
}
9194

9295
// Schedule periodic wallet flushes and tx rebroadcasts
93-
if (args.GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
94-
scheduler.scheduleEvery(MaybeCompactWalletDB, std::chrono::milliseconds{500});
96+
if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
97+
scheduler.scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});
9598
}
96-
scheduler.scheduleEvery(MaybeResendWalletTxs, std::chrono::milliseconds{1000});
99+
scheduler.scheduleEvery([&context] { MaybeResendWalletTxs(context); }, std::chrono::milliseconds{1000});
97100
}
98101

99-
void FlushWallets()
102+
void FlushWallets(WalletContext& context)
100103
{
101-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
104+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
102105
pwallet->Flush();
103106
}
104107
}
105108

106-
void StopWallets()
109+
void StopWallets(WalletContext& context)
107110
{
108-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
111+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
109112
pwallet->Close();
110113
}
111114
}
112115

113-
void UnloadWallets()
116+
void UnloadWallets(WalletContext& context)
114117
{
115-
auto wallets = GetWallets();
118+
auto wallets = GetWallets(context);
116119
while (!wallets.empty()) {
117120
auto wallet = wallets.back();
118121
wallets.pop_back();
119-
RemoveWallet(wallet);
122+
RemoveWallet(context, wallet);
120123
UnloadWallet(std::move(wallet));
121124
}
122125
}

‎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, const std::vector<std::string>& wallet_files);
21+
bool VerifyWallets(WalletContext& context, const std::vector<std::string>& wallet_files);
2122

2223
//! Load wallet databases.
23-
bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files);
24+
bool LoadWallets(WalletContext& context, const std::vector<std::string>& wallet_files);
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.fHelp);
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
}
@@ -2467,7 +2469,8 @@ static UniValue listwallets(const JSONRPCRequest& request)
24672469

24682470
UniValue obj(UniValue::VARR);
24692471

2470-
for (const std::shared_ptr<CWallet>& wallet : GetWallets()) {
2472+
WalletContext& context = EnsureWalletContext(request.context);
2473+
for (const std::shared_ptr<CWallet>& wallet : GetWallets(context)) {
24712474
LOCK(wallet->cs_wallet);
24722475
obj.push_back(wallet->GetName());
24732476
}
@@ -2512,7 +2515,7 @@ static UniValue loadwallet(const JSONRPCRequest& request)
25122515

25132516
bilingual_str error;
25142517
std::vector<bilingual_str> warnings;
2515-
std::shared_ptr<CWallet> const wallet = LoadWallet(*context.chain, location, error, warnings);
2518+
std::shared_ptr<CWallet> const wallet = LoadWallet(context, location, error, warnings);
25162519
if (!wallet) throw JSONRPCError(RPC_WALLET_ERROR, error.original);
25172520

25182521
UniValue obj(UniValue::VOBJ);
@@ -2643,7 +2646,7 @@ static UniValue createwallet(const JSONRPCRequest& request)
26432646

26442647
bilingual_str error;
26452648
std::shared_ptr<CWallet> wallet;
2646-
WalletCreationStatus status = CreateWallet(*context.chain, passphrase, flags, request.params[0].get_str(), error, warnings, wallet);
2649+
WalletCreationStatus status = CreateWallet(context, passphrase, flags, request.params[0].get_str(), error, warnings, wallet);
26472650
switch (status) {
26482651
case WalletCreationStatus::CREATION_FAILED:
26492652
throw JSONRPCError(RPC_WALLET_ERROR, error.original);
@@ -2685,15 +2688,16 @@ static UniValue unloadwallet(const JSONRPCRequest& request)
26852688
wallet_name = request.params[0].get_str();
26862689
}
26872690

2688-
std::shared_ptr<CWallet> wallet = GetWallet(wallet_name);
2691+
WalletContext& context = EnsureWalletContext(request.context);
2692+
std::shared_ptr<CWallet> wallet = GetWallet(context, wallet_name);
26892693
if (!wallet) {
26902694
throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
26912695
}
26922696

26932697
// Release the "main" shared pointer and prevent further notifications.
26942698
// Note that any attempt to load the same wallet would fail until the wallet
26952699
// is destroyed (see CheckUniqueFileid).
2696-
if (!RemoveWallet(wallet)) {
2700+
if (!RemoveWallet(context, wallet)) {
26972701
throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded");
26982702
}
26992703

0 commit comments

Comments
 (0)