Skip to content

Commit f2d9af6

Browse files
ryanofskyryanofsky
committed
refactor: remove ::vpwallets and related global variables
Move global wallet variables to WalletContext struct
1 parent 0cfee3b commit f2d9af6

File tree

14 files changed

+172
-127
lines changed

14 files changed

+172
-127
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

@@ -488,7 +489,7 @@ class WalletClientImpl : public WalletClient
488489
{
489490
m_context.chain = &chain;
490491
}
491-
~WalletClientImpl() override { UnloadWallets(); }
492+
~WalletClientImpl() override { UnloadWallets(m_context); }
492493

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

510511
//! WalletClient methods
511512
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
512513
{
513514
std::shared_ptr<CWallet> wallet;
514-
status = CreateWallet(m_chain, passphrase, wallet_creation_flags, name, error, warnings, wallet);
515-
return MakeWallet(std::move(wallet));
515+
status = CreateWallet(m_context, passphrase, wallet_creation_flags, name, error, warnings, wallet);
516+
return MakeWallet(m_context, std::move(wallet));
516517
}
517518
std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
518519
{
519-
return MakeWallet(LoadWallet(m_chain, WalletLocation(name), error, warnings));
520+
return MakeWallet(m_context, LoadWallet(m_context, WalletLocation(name), error, warnings));
520521
}
521522
std::string getWalletDir() override
522523
{
@@ -533,15 +534,16 @@ class WalletClientImpl : public WalletClient
533534
std::vector<std::unique_ptr<Wallet>> getWallets() override
534535
{
535536
std::vector<std::unique_ptr<Wallet>> wallets;
536-
for (const auto& wallet : GetWallets()) {
537-
wallets.emplace_back(MakeWallet(wallet));
537+
for (const auto& wallet : GetWallets(m_context)) {
538+
wallets.emplace_back(MakeWallet(m_context, wallet));
538539
}
539540
return wallets;
540541
}
541542
std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) override
542543
{
543-
return HandleLoadWallet(std::move(fn));
544+
return HandleLoadWallet(m_context, std::move(fn));
544545
}
546+
WalletContext* context() override { return &m_context; }
545547

546548
WalletContext m_context;
547549
std::vector<std::string> m_wallet_filenames;
@@ -551,7 +553,7 @@ class WalletClientImpl : public WalletClient
551553

552554
} // namespace
553555

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

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

‎src/interfaces/wallet.h‎

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

334337
//! Information about one wallet address.
@@ -407,7 +410,7 @@ struct WalletTxOut
407410

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

412415
//! Return implementation of ChainClient interface for a wallet client. This
413416
//! 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
@@ -112,9 +112,10 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
112112
std::unique_ptr<const PlatformStyle> platformStyle(PlatformStyle::instantiate("other"));
113113
OptionsModel optionsModel(node);
114114
ClientModel clientModel(node, &optionsModel);
115-
AddWallet(wallet);
116-
WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel, platformStyle.get());
117-
RemoveWallet(wallet);
115+
WalletContext& context = *node.walletClient().context();
116+
AddWallet(context, wallet);
117+
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
118+
RemoveWallet(context, wallet);
118119
EditAddressDialog editAddressDialog(EditAddressDialog::NewSendingAddress);
119120
editAddressDialog.setModel(walletModel.getAddressTableModel());
120121

‎src/qt/test/wallettests.cpp‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -168,9 +168,10 @@ void TestGUI(interfaces::Node& node)
168168
TransactionView transactionView(platformStyle.get());
169169
OptionsModel optionsModel(node);
170170
ClientModel clientModel(node, &optionsModel);
171-
AddWallet(wallet);
172-
WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel, platformStyle.get());
173-
RemoveWallet(wallet);
171+
WalletContext& context = *node.walletClient().context();
172+
AddWallet(context, wallet);
173+
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
174+
RemoveWallet(context, wallet);
174175
sendCoinsDialog.setModel(&walletModel);
175176
transactionView.setModel(&walletModel);
176177

‎src/wallet/context.h‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,21 @@
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+
15+
class CWallet;
816
namespace interfaces {
917
class Chain;
18+
class Wallet;
1019
} // namespace interfaces
1120

21+
using LoadWalletFn = std::function<void(std::unique_ptr<interfaces::Wallet> wallet)>;
22+
1223
//! WalletContext struct containing references to state shared between CWallet
1324
//! instances, like the reference to the chain interface, and the list of opened
1425
//! wallets.
@@ -21,6 +32,9 @@ class Chain;
2132
//! behavior.
2233
struct WalletContext {
2334
interfaces::Chain* chain{nullptr};
35+
RecursiveMutex wallets_mutex;
36+
std::vector<std::shared_ptr<CWallet>> wallets GUARDED_BY(wallets_mutex);
37+
std::list<LoadWalletFn> wallet_load_fns GUARDED_BY(wallets_mutex);
2438

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

‎src/wallet/load.cpp‎

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,12 @@
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

15-
bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files)
16+
bool VerifyWallets(WalletContext& context, const std::vector<std::string>& wallet_files)
1617
{
18+
interfaces::Chain& chain = *context.chain;
1719
if (gArgs.IsArgSet("-walletdir")) {
1820
fs::path wallet_dir = gArgs.GetArg("-walletdir", "");
1921
boost::system::error_code error;
@@ -50,7 +52,7 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
5052

5153
bilingual_str error_string;
5254
std::vector<bilingual_str> warnings;
53-
bool verify_success = CWallet::Verify(chain, location, error_string, warnings);
55+
bool verify_success = CWallet::Verify(context, location, error_string, warnings);
5456
if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
5557
if (!verify_success) {
5658
chain.initError(error_string);
@@ -61,19 +63,20 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
6163
return true;
6264
}
6365

64-
bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files)
66+
bool LoadWallets(WalletContext& context, const std::vector<std::string>& wallet_files)
6567
{
68+
interfaces::Chain& chain = *context.chain;
6669
try {
6770
for (const std::string& walletFile : wallet_files) {
6871
bilingual_str error;
6972
std::vector<bilingual_str> warnings;
70-
std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(chain, WalletLocation(walletFile), error, warnings);
73+
std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(context, WalletLocation(walletFile), error, warnings);
7174
if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
7275
if (!pwallet) {
7376
chain.initError(error);
7477
return false;
7578
}
76-
AddWallet(pwallet);
79+
AddWallet(context, pwallet);
7780
}
7881
return true;
7982
} catch (const std::runtime_error& e) {
@@ -82,38 +85,38 @@ bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& walle
8285
}
8386
}
8487

85-
void StartWallets(CScheduler& scheduler)
88+
void StartWallets(WalletContext& context, CScheduler& scheduler)
8689
{
87-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
90+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
8891
pwallet->postInitProcess();
8992
}
9093

9194
// Schedule periodic wallet flushes and tx rebroadcasts
92-
scheduler.scheduleEvery(MaybeCompactWalletDB, std::chrono::milliseconds{500});
93-
scheduler.scheduleEvery(MaybeResendWalletTxs, std::chrono::milliseconds{1000});
95+
scheduler.scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});
96+
scheduler.scheduleEvery([&context] { MaybeResendWalletTxs(context); }, std::chrono::milliseconds{1000});
9497
}
9598

96-
void FlushWallets()
99+
void FlushWallets(WalletContext& context)
97100
{
98-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
101+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
99102
pwallet->Flush(false);
100103
}
101104
}
102105

103-
void StopWallets()
106+
void StopWallets(WalletContext& context)
104107
{
105-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
108+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
106109
pwallet->Flush(true);
107110
}
108111
}
109112

110-
void UnloadWallets()
113+
void UnloadWallets(WalletContext& context)
111114
{
112-
auto wallets = GetWallets();
115+
auto wallets = GetWallets(context);
113116
while (!wallets.empty()) {
114117
auto wallet = wallets.back();
115118
wallets.pop_back();
116-
RemoveWallet(wallet);
119+
RemoveWallet(context, wallet);
117120
UnloadWallet(std::move(wallet));
118121
}
119122
}

‎src/wallet/load.h‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,27 +10,28 @@
1010
#include <vector>
1111

1212
class CScheduler;
13+
struct WalletContext;
1314

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

1819
//! Responsible for reading and validating the -wallet arguments and verifying the wallet database.
19-
bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files);
20+
bool VerifyWallets(WalletContext& context, const std::vector<std::string>& wallet_files);
2021

2122
//! Load wallet databases.
22-
bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files);
23+
bool LoadWallets(WalletContext& context, const std::vector<std::string>& wallet_files);
2324

2425
//! Complete startup of wallets.
25-
void StartWallets(CScheduler& scheduler);
26+
void StartWallets(WalletContext& context, CScheduler& scheduler);
2627

2728
//! Flush all wallets in preparation for shutdown.
28-
void FlushWallets();
29+
void FlushWallets(WalletContext& context);
2930

3031
//! Stop all wallets. Wallets will be flushed first.
31-
void StopWallets();
32+
void StopWallets(WalletContext& context);
3233

3334
//! Close all wallets.
34-
void UnloadWallets();
35+
void UnloadWallets(WalletContext& context);
3536

3637
#endif // BITCOIN_WALLET_LOAD_H

‎src/wallet/rpcwallet.cpp‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -93,21 +93,23 @@ bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string&
9393

9494
std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
9595
{
96+
WalletContext& context = EnsureWalletContext(request.context);
97+
9698
std::string wallet_name;
9799
if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) {
98-
std::shared_ptr<CWallet> pwallet = GetWallet(wallet_name);
100+
std::shared_ptr<CWallet> pwallet = GetWallet(context, wallet_name);
99101
if (!pwallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
100102
return pwallet;
101103
}
102104

103-
std::vector<std::shared_ptr<CWallet>> wallets = GetWallets();
105+
std::vector<std::shared_ptr<CWallet>> wallets = GetWallets(context);
104106
if (wallets.size() == 1 || (request.fHelp && wallets.size() > 0)) {
105107
return wallets[0];
106108
}
107109

108110
if (request.fHelp) return nullptr;
109111

110-
if (!HasWallets()) {
112+
if (!HasWallets(context)) {
111113
throw JSONRPCError(
112114
RPC_METHOD_NOT_FOUND, "Method not found (wallet method is disabled because no wallet is loaded)");
113115
}
@@ -2472,7 +2474,8 @@ static UniValue listwallets(const JSONRPCRequest& request)
24722474

24732475
UniValue obj(UniValue::VARR);
24742476

2475-
for (const std::shared_ptr<CWallet>& wallet : GetWallets()) {
2477+
WalletContext& context = EnsureWalletContext(request.context);
2478+
for (const std::shared_ptr<CWallet>& wallet : GetWallets(context)) {
24762479
LOCK(wallet->cs_wallet);
24772480
obj.push_back(wallet->GetName());
24782481
}
@@ -2517,7 +2520,7 @@ static UniValue loadwallet(const JSONRPCRequest& request)
25172520

25182521
bilingual_str error;
25192522
std::vector<bilingual_str> warnings;
2520-
std::shared_ptr<CWallet> const wallet = LoadWallet(*context.chain, location, error, warnings);
2523+
std::shared_ptr<CWallet> const wallet = LoadWallet(context, location, error, warnings);
25212524
if (!wallet) throw JSONRPCError(RPC_WALLET_ERROR, error.original);
25222525

25232526
UniValue obj(UniValue::VOBJ);
@@ -2648,7 +2651,7 @@ static UniValue createwallet(const JSONRPCRequest& request)
26482651

26492652
bilingual_str error;
26502653
std::shared_ptr<CWallet> wallet;
2651-
WalletCreationStatus status = CreateWallet(*context.chain, passphrase, flags, request.params[0].get_str(), error, warnings, wallet);
2654+
WalletCreationStatus status = CreateWallet(context, passphrase, flags, request.params[0].get_str(), error, warnings, wallet);
26522655
switch (status) {
26532656
case WalletCreationStatus::CREATION_FAILED:
26542657
throw JSONRPCError(RPC_WALLET_ERROR, error.original);
@@ -2690,15 +2693,16 @@ static UniValue unloadwallet(const JSONRPCRequest& request)
26902693
wallet_name = request.params[0].get_str();
26912694
}
26922695

2693-
std::shared_ptr<CWallet> wallet = GetWallet(wallet_name);
2696+
WalletContext& context = EnsureWalletContext(request.context);
2697+
std::shared_ptr<CWallet> wallet = GetWallet(context, wallet_name);
26942698
if (!wallet) {
26952699
throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
26962700
}
26972701

26982702
// Release the "main" shared pointer and prevent further notifications.
26992703
// Note that any attempt to load the same wallet would fail until the wallet
27002704
// is destroyed (see CheckUniqueFileid).
2701-
if (!RemoveWallet(wallet)) {
2705+
if (!RemoveWallet(context, wallet)) {
27022706
throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded");
27032707
}
27042708

0 commit comments

Comments
 (0)