Skip to content

Commit 3df8f44

Browse files
ryanofskyryanofsky
committed
refactor: remove ::vpwallets and related global variables
Move global wallet variables to WalletContext struct
1 parent a0a422c commit 3df8f44

File tree

14 files changed

+178
-131
lines changed

14 files changed

+178
-131
lines changed

‎src/interfaces/wallet.cpp‎

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

102102
bool encryptWallet(const SecureString& wallet_passphrase) override
103103
{
@@ -446,7 +446,7 @@ class WalletImpl : public Wallet
446446
CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; }
447447
void remove() override
448448
{
449-
RemoveWallet(m_wallet, false /* load_on_start */);
449+
RemoveWallet(m_context, m_wallet, false /* load_on_start */);
450450
}
451451
bool isLegacy() override { return m_wallet->IsLegacy(); }
452452
std::unique_ptr<Handler> handleUnload(UnloadFn fn) override
@@ -482,6 +482,7 @@ class WalletImpl : public Wallet
482482
}
483483
CWallet* wallet() override { return m_wallet.get(); }
484484

485+
WalletContext& m_context;
485486
std::shared_ptr<CWallet> m_wallet;
486487
};
487488

@@ -494,7 +495,7 @@ class WalletClientImpl : public WalletClient
494495
m_context.chain = &chain;
495496
m_context.args = &args;
496497
}
497-
~WalletClientImpl() override { UnloadWallets(); }
498+
~WalletClientImpl() override { UnloadWallets(m_context); }
498499

499500
//! ChainClient methods
500501
void registerRpcs() override
@@ -506,23 +507,23 @@ class WalletClientImpl : public WalletClient
506507
m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back()));
507508
}
508509
}
509-
bool verify() override { return VerifyWallets(*m_context.chain, m_wallet_filenames); }
510-
bool load() override { return LoadWallets(*m_context.chain, m_wallet_filenames); }
511-
void start(CScheduler& scheduler) override { return StartWallets(scheduler, *Assert(m_context.args)); }
512-
void flush() override { return FlushWallets(); }
513-
void stop() override { return StopWallets(); }
510+
bool verify() override { return VerifyWallets(m_context, m_wallet_filenames); }
511+
bool load() override { return LoadWallets(m_context, m_wallet_filenames); }
512+
void start(CScheduler& scheduler) override { return StartWallets(m_context, scheduler); }
513+
void flush() override { return FlushWallets(m_context); }
514+
void stop() override { return StopWallets(m_context); }
514515
void setMockTime(int64_t time) override { return SetMockTime(time); }
515516

516517
//! WalletClient methods
517518
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
518519
{
519520
std::shared_ptr<CWallet> wallet;
520-
status = CreateWallet(*m_context.chain, passphrase, wallet_creation_flags, name, true /* load_on_start */, error, warnings, wallet);
521-
return MakeWallet(std::move(wallet));
521+
status = CreateWallet(m_context, passphrase, wallet_creation_flags, name, true /* load_on_start */, error, warnings, wallet);
522+
return MakeWallet(m_context, std::move(wallet));
522523
}
523524
std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
524525
{
525-
return MakeWallet(LoadWallet(*m_context.chain, WalletLocation(name), true /* load_on_start */, error, warnings));
526+
return MakeWallet(m_context, LoadWallet(m_context, WalletLocation(name), true /* load_on_start */, error, warnings));
526527
}
527528
std::string getWalletDir() override
528529
{
@@ -539,15 +540,16 @@ class WalletClientImpl : public WalletClient
539540
std::vector<std::unique_ptr<Wallet>> getWallets() override
540541
{
541542
std::vector<std::unique_ptr<Wallet>> wallets;
542-
for (const auto& wallet : GetWallets()) {
543-
wallets.emplace_back(MakeWallet(wallet));
543+
for (const auto& wallet : GetWallets(m_context)) {
544+
wallets.emplace_back(MakeWallet(m_context, wallet));
544545
}
545546
return wallets;
546547
}
547548
std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) override
548549
{
549-
return HandleLoadWallet(std::move(fn));
550+
return HandleLoadWallet(m_context, std::move(fn));
550551
}
552+
WalletContext* context() override { return &m_context; }
551553

552554
WalletContext m_context;
553555
const std::vector<std::string> m_wallet_filenames;
@@ -557,7 +559,7 @@ class WalletClientImpl : public WalletClient
557559

558560
} // namespace
559561

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

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

‎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
@@ -350,6 +350,15 @@ void BitcoinApplication::requestShutdown()
350350
window->setClientModel(nullptr);
351351
pollShutdownTimer->stop();
352352

353+
#ifdef ENABLE_WALLET
354+
// Delete wallet controller here to unregister wallet callbacks. Without
355+
// this, the callbacks may be unregistered too late after the wallet
356+
// context is deleted, or the wallet may try to call back into the GUI when
357+
// the GUI is deleted, resulting in segfaults either way.
358+
delete m_wallet_controller;
359+
m_wallet_controller = nullptr;
360+
#endif // ENABLE_WALLET
361+
353362
delete clientModel;
354363
clientModel = nullptr;
355364

‎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;
112112
ClientModel clientModel(node, &optionsModel);
113-
AddWallet(wallet);
114-
WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel, platformStyle.get());
115-
RemoveWallet(wallet, nullopt);
113+
WalletContext& context = *node.walletClient().context();
114+
AddWallet(context, wallet);
115+
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
116+
RemoveWallet(context, wallet, nullopt);
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;
167167
ClientModel clientModel(node, &optionsModel);
168-
AddWallet(wallet);
169-
WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel, platformStyle.get());
170-
RemoveWallet(wallet, nullopt);
168+
WalletContext& context = *node.walletClient().context();
169+
AddWallet(context, wallet);
170+
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
171+
RemoveWallet(context, wallet, nullopt);
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,13 +10,15 @@
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

1617
#include <univalue.h>
1718

18-
bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files)
19+
bool VerifyWallets(WalletContext& context, const std::vector<std::string>& wallet_files)
1920
{
21+
interfaces::Chain& chain = *context.chain;
2022
if (gArgs.IsArgSet("-walletdir")) {
2123
fs::path wallet_dir = gArgs.GetArg("-walletdir", "");
2224
boost::system::error_code error;
@@ -53,7 +55,7 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
5355

5456
bilingual_str error_string;
5557
std::vector<bilingual_str> warnings;
56-
bool verify_success = CWallet::Verify(chain, location, error_string, warnings);
58+
bool verify_success = CWallet::Verify(context, location, error_string, warnings);
5759
if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
5860
if (!verify_success) {
5961
chain.initError(error_string);
@@ -64,19 +66,20 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
6466
return true;
6567
}
6668

67-
bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files)
69+
bool LoadWallets(WalletContext& context, const std::vector<std::string>& wallet_files)
6870
{
71+
interfaces::Chain& chain = *context.chain;
6972
try {
7073
for (const std::string& walletFile : wallet_files) {
7174
bilingual_str error;
7275
std::vector<bilingual_str> warnings;
73-
std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(chain, WalletLocation(walletFile), error, warnings);
76+
std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(context, WalletLocation(walletFile), error, warnings);
7477
if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
7578
if (!pwallet) {
7679
chain.initError(error);
7780
return false;
7881
}
79-
AddWallet(pwallet);
82+
AddWallet(context, pwallet);
8083
}
8184
return true;
8285
} catch (const std::runtime_error& e) {
@@ -85,41 +88,41 @@ bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& walle
8588
}
8689
}
8790

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

9497
// Schedule periodic wallet flushes and tx rebroadcasts
95-
if (args.GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
96-
scheduler.scheduleEvery(MaybeCompactWalletDB, std::chrono::milliseconds{500});
98+
if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
99+
scheduler.scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});
97100
}
98-
scheduler.scheduleEvery(MaybeResendWalletTxs, std::chrono::milliseconds{1000});
101+
scheduler.scheduleEvery([&context] { MaybeResendWalletTxs(context); }, std::chrono::milliseconds{1000});
99102
}
100103

101-
void FlushWallets()
104+
void FlushWallets(WalletContext& context)
102105
{
103-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
106+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
104107
pwallet->Flush();
105108
}
106109
}
107110

108-
void StopWallets()
111+
void StopWallets(WalletContext& context)
109112
{
110-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
113+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
111114
pwallet->Close();
112115
}
113116
}
114117

115-
void UnloadWallets()
118+
void UnloadWallets(WalletContext& context)
116119
{
117-
auto wallets = GetWallets();
120+
auto wallets = GetWallets(context);
118121
while (!wallets.empty()) {
119122
auto wallet = wallets.back();
120123
wallets.pop_back();
121124
std::vector<bilingual_str> warnings;
122-
RemoveWallet(wallet, nullopt, warnings);
125+
RemoveWallet(context, wallet, nullopt, warnings);
123126
UnloadWallet(std::move(wallet));
124127
}
125128
}

‎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
@@ -97,14 +97,16 @@ bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string&
9797
std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
9898
{
9999
CHECK_NONFATAL(!request.fHelp);
100+
WalletContext& context = EnsureWalletContext(request.context);
101+
100102
std::string wallet_name;
101103
if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) {
102-
std::shared_ptr<CWallet> pwallet = GetWallet(wallet_name);
104+
std::shared_ptr<CWallet> pwallet = GetWallet(context, wallet_name);
103105
if (!pwallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
104106
return pwallet;
105107
}
106108

107-
std::vector<std::shared_ptr<CWallet>> wallets = GetWallets();
109+
std::vector<std::shared_ptr<CWallet>> wallets = GetWallets(context);
108110
if (wallets.size() == 1) {
109111
return wallets[0];
110112
}
@@ -2470,7 +2472,8 @@ static UniValue listwallets(const JSONRPCRequest& request)
24702472

24712473
UniValue obj(UniValue::VARR);
24722474

2473-
for (const std::shared_ptr<CWallet>& wallet : GetWallets()) {
2475+
WalletContext& context = EnsureWalletContext(request.context);
2476+
for (const std::shared_ptr<CWallet>& wallet : GetWallets(context)) {
24742477
LOCK(wallet->cs_wallet);
24752478
obj.push_back(wallet->GetName());
24762479
}
@@ -2517,7 +2520,7 @@ static UniValue loadwallet(const JSONRPCRequest& request)
25172520
bilingual_str error;
25182521
std::vector<bilingual_str> warnings;
25192522
Optional<bool> load_on_start = request.params[1].isNull() ? nullopt : Optional<bool>(request.params[1].get_bool());
2520-
std::shared_ptr<CWallet> const wallet = LoadWallet(*context.chain, location, load_on_start, error, warnings);
2523+
std::shared_ptr<CWallet> const wallet = LoadWallet(context, location, load_on_start, error, warnings);
25212524
if (!wallet) throw JSONRPCError(RPC_WALLET_ERROR, error.original);
25222525

25232526
UniValue obj(UniValue::VOBJ);
@@ -2650,7 +2653,7 @@ static UniValue createwallet(const JSONRPCRequest& request)
26502653
bilingual_str error;
26512654
std::shared_ptr<CWallet> wallet;
26522655
Optional<bool> load_on_start = request.params[6].isNull() ? nullopt : Optional<bool>(request.params[6].get_bool());
2653-
WalletCreationStatus status = CreateWallet(*context.chain, passphrase, flags, request.params[0].get_str(), load_on_start, error, warnings, wallet);
2656+
WalletCreationStatus status = CreateWallet(context, passphrase, flags, request.params[0].get_str(), load_on_start, error, warnings, wallet);
26542657
switch (status) {
26552658
case WalletCreationStatus::CREATION_FAILED:
26562659
throw JSONRPCError(RPC_WALLET_ERROR, error.original);
@@ -2695,7 +2698,8 @@ static UniValue unloadwallet(const JSONRPCRequest& request)
26952698
wallet_name = request.params[0].get_str();
26962699
}
26972700

2698-
std::shared_ptr<CWallet> wallet = GetWallet(wallet_name);
2701+
WalletContext& context = EnsureWalletContext(request.context);
2702+
std::shared_ptr<CWallet> wallet = GetWallet(context, wallet_name);
26992703
if (!wallet) {
27002704
throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
27012705
}
@@ -2705,7 +2709,7 @@ static UniValue unloadwallet(const JSONRPCRequest& request)
27052709
// is destroyed (see CheckUniqueFileid).
27062710
std::vector<bilingual_str> warnings;
27072711
Optional<bool> load_on_start = request.params[1].isNull() ? nullopt : Optional<bool>(request.params[1].get_bool());
2708-
if (!RemoveWallet(wallet, load_on_start, warnings)) {
2712+
if (!RemoveWallet(context, wallet, load_on_start, warnings)) {
27092713
throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded");
27102714
}
27112715

0 commit comments

Comments
 (0)