Skip to content

Commit 14aa05d

Browse files
kwvgkwvg
committed
merge bitcoin#19101: remove ::vpwallets and related global variables
This is the second half of the backport, which deals with deglobalizing wallet-specific variables like vpwallets, cs_wallets and friends.
1 parent 0d64290 commit 14aa05d

File tree

15 files changed

+135
-106
lines changed

15 files changed

+135
-106
lines changed

‎src/interfaces/wallet.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ struct WalletTxOut
453453

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

458458
//! Return implementation of ChainClient interface for a wallet loader. This
459459
//! 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
@@ -110,9 +110,10 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
110110
// Initialize relevant QT models.
111111
OptionsModel optionsModel;
112112
ClientModel clientModel(node, &optionsModel);
113-
AddWallet(wallet);
114-
WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel);
115-
RemoveWallet(wallet, std::nullopt);
113+
WalletContext& context = *node.walletLoader().context();
114+
AddWallet(context, wallet);
115+
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel);
116+
RemoveWallet(context, wallet, /*load_on_start=*/std::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
@@ -109,8 +109,9 @@ void TestGUI(interfaces::Node& node)
109109
test.CreateAndProcessBlock({}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey()));
110110
}
111111
node.setContext(&test.m_node);
112+
WalletContext& context = *node.walletLoader().context();
112113
const std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", CreateMockWalletDatabase());
113-
AddWallet(wallet);
114+
AddWallet(context, wallet);
114115
wallet->LoadWallet();
115116
{
116117
auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan();
@@ -134,7 +135,7 @@ void TestGUI(interfaces::Node& node)
134135
TransactionView transactionView;
135136
OptionsModel optionsModel;
136137
ClientModel clientModel(node, &optionsModel);
137-
WalletModel walletModel(interfaces::MakeWallet(wallet), clientModel);
138+
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel);
138139
sendCoinsDialog.setModel(&walletModel);
139140
transactionView.setModel(&walletModel);
140141

@@ -246,7 +247,7 @@ void TestGUI(interfaces::Node& node)
246247
QPushButton* removeRequestButton = receiveCoinsDialog.findChild<QPushButton*>("removeRequestButton");
247248
removeRequestButton->click();
248249
QCOMPARE(requestTableModel->rowCount({}), currentRowCount-1);
249-
RemoveWallet(wallet, std::nullopt);
250+
RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt);
250251

251252
// Check removal from wallet
252253
QCOMPARE(walletModel.wallet().getAddressReceiveRequests().size(), size_t{0});

‎src/wallet/context.h‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,26 @@
55
#ifndef BITCOIN_WALLET_CONTEXT_H
66
#define BITCOIN_WALLET_CONTEXT_H
77

8+
#include <sync.h>
9+
10+
#include <functional>
11+
#include <list>
812
#include <memory>
13+
#include <vector>
914

1015
class ArgsManager;
16+
class CWallet;
1117
struct NodeContext;
1218
namespace interfaces {
1319
class Chain;
1420
namespace CoinJoin {
1521
class Loader;
1622
} // namspace CoinJoin
23+
class Wallet;
1724
} // namespace interfaces
1825

26+
using LoadWalletFn = std::function<void(std::unique_ptr<interfaces::Wallet> wallet)>;
27+
1928
//! WalletContext struct containing references to state shared between CWallet
2029
//! instances, like the reference to the chain interface, and the list of opened
2130
//! wallets.
@@ -29,6 +38,9 @@ class Loader;
2938
struct WalletContext {
3039
interfaces::Chain* chain{nullptr};
3140
ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
41+
Mutex wallets_mutex;
42+
std::vector<std::shared_ptr<CWallet>> wallets GUARDED_BY(wallets_mutex);
43+
std::list<LoadWalletFn> wallet_load_fns GUARDED_BY(wallets_mutex);
3244
interfaces::CoinJoin::Loader* coinjoin_loader{nullptr};
3345
// Some Dash RPCs rely on WalletContext yet access NodeContext members
3446
// even though wallet RPCs should refrain from accessing non-wallet

‎src/wallet/interfaces.cpp‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet,
127127
class WalletImpl : public Wallet
128128
{
129129
public:
130-
explicit WalletImpl(const std::shared_ptr<CWallet>& wallet) : m_wallet(wallet) {}
130+
explicit WalletImpl(WalletContext& context, const std::shared_ptr<CWallet>& wallet) : m_context(context), m_wallet(wallet) {}
131131

132132
void markDirty() override
133133
{
@@ -501,7 +501,7 @@ class WalletImpl : public Wallet
501501
CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; }
502502
void remove() override
503503
{
504-
RemoveWallet(m_wallet, false /* load_on_start */);
504+
RemoveWallet(m_context, m_wallet, false /* load_on_start */);
505505
}
506506
bool isLegacy() override { return m_wallet->IsLegacy(); }
507507
std::unique_ptr<Handler> handleUnload(UnloadFn fn) override
@@ -547,6 +547,7 @@ class WalletImpl : public Wallet
547547
}
548548
CWallet* wallet() override { return m_wallet.get(); }
549549

550+
WalletContext& m_context;
550551
std::shared_ptr<CWallet> m_wallet;
551552
std::unique_ptr<interfaces::CoinJoin::Client> m_coinjoin_client;
552553
};
@@ -575,7 +576,7 @@ class WalletLoaderImpl : public WalletLoader
575576
m_context.node_context = &node_context;
576577
m_context.coinjoin_loader = &coinjoin_loader;
577578
}
578-
~WalletLoaderImpl() override { UnloadWallets(); }
579+
~WalletLoaderImpl() override { UnloadWallets(m_context); }
579580

580581
//! ChainClient methods
581582
void registerRpcs() override
@@ -585,8 +586,8 @@ class WalletLoaderImpl : public WalletLoader
585586
bool verify() override { return VerifyWallets(m_context); }
586587
bool load() override { return LoadWallets(m_context); }
587588
void start(CScheduler& scheduler) override { return StartWallets(m_context, scheduler); }
588-
void flush() override { return FlushWallets(); }
589-
void stop() override { return StopWallets(); }
589+
void flush() override { return FlushWallets(m_context); }
590+
void stop() override { return StopWallets(m_context); }
590591
void setMockTime(int64_t time) override { return SetMockTime(time); }
591592

592593
//! WalletLoader methods
@@ -602,19 +603,19 @@ class WalletLoaderImpl : public WalletLoader
602603
options.require_create = true;
603604
options.create_flags = wallet_creation_flags;
604605
options.create_passphrase = passphrase;
605-
return MakeWallet(CreateWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
606+
return MakeWallet(m_context, CreateWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
606607
}
607608
std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
608609
{
609610
DatabaseOptions options;
610611
DatabaseStatus status;
611612
options.require_existing = true;
612-
return MakeWallet(LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
613+
return MakeWallet(m_context, LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
613614
}
614615
std::unique_ptr<Wallet> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
615616
{
616617
DatabaseStatus status;
617-
return MakeWallet(RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings));
618+
return MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings));
618619
}
619620
std::string getWalletDir() override
620621
{
@@ -631,14 +632,14 @@ class WalletLoaderImpl : public WalletLoader
631632
std::vector<std::unique_ptr<Wallet>> getWallets() override
632633
{
633634
std::vector<std::unique_ptr<Wallet>> wallets;
634-
for (const auto& wallet : GetWallets()) {
635-
wallets.emplace_back(MakeWallet(wallet));
635+
for (const auto& wallet : GetWallets(m_context)) {
636+
wallets.emplace_back(MakeWallet(m_context, wallet));
636637
}
637638
return wallets;
638639
}
639640
std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) override
640641
{
641-
return HandleLoadWallet(std::move(fn));
642+
return HandleLoadWallet(m_context, std::move(fn));
642643
}
643644
WalletContext* context() override { return &m_context; }
644645

@@ -651,7 +652,7 @@ class WalletLoaderImpl : public WalletLoader
651652
} // namespace wallet
652653

653654
namespace interfaces {
654-
std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet) { return wallet ? std::make_unique<wallet::WalletImpl>(wallet) : nullptr; }
655+
std::unique_ptr<Wallet> MakeWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet) { return wallet ? std::make_unique<wallet::WalletImpl>(context, wallet) : nullptr; }
655656
std::unique_ptr<WalletLoader> MakeWalletLoader(Chain& chain, ArgsManager& args, NodeContext& node_context,
656657
interfaces::CoinJoin::Loader& coinjoin_loader)
657658
{

‎src/wallet/load.cpp‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ bool LoadWallets(WalletContext& context)
125125
chain.initError(error_string);
126126
return false;
127127
}
128-
AddWallet(pwallet);
128+
AddWallet(context, pwallet);
129129
}
130130
return true;
131131
} catch (const std::runtime_error& e) {
@@ -136,20 +136,20 @@ bool LoadWallets(WalletContext& context)
136136

137137
void StartWallets(WalletContext& context, CScheduler& scheduler)
138138
{
139-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
139+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
140140
pwallet->postInitProcess();
141141
}
142142

143143
// Schedule periodic wallet flushes and tx rebroadcasts
144144
if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
145-
scheduler.scheduleEvery(MaybeCompactWalletDB, std::chrono::milliseconds{500});
145+
scheduler.scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500});
146146
}
147-
scheduler.scheduleEvery(MaybeResendWalletTxs, std::chrono::milliseconds{1000});
147+
scheduler.scheduleEvery([&context] { MaybeResendWalletTxs(context); }, std::chrono::milliseconds{1000});
148148
}
149149

150-
void FlushWallets()
150+
void FlushWallets(WalletContext& context)
151151
{
152-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
152+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
153153
if (CCoinJoinClientOptions::IsEnabled()) {
154154
// Stop CoinJoin, release keys
155155
pwallet->coinjoin_loader().FlushWallet(pwallet->GetName());
@@ -158,21 +158,21 @@ void FlushWallets()
158158
}
159159
}
160160

161-
void StopWallets()
161+
void StopWallets(WalletContext& context)
162162
{
163-
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
163+
for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
164164
pwallet->Close();
165165
}
166166
}
167167

168-
void UnloadWallets()
168+
void UnloadWallets(WalletContext& context)
169169
{
170-
auto wallets = GetWallets();
170+
auto wallets = GetWallets(context);
171171
while (!wallets.empty()) {
172172
auto wallet = wallets.back();
173173
wallets.pop_back();
174174
std::vector<bilingual_str> warnings;
175-
RemoveWallet(wallet, std::nullopt, warnings);
175+
RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings);
176176
UnloadWallet(std::move(wallet));
177177
}
178178
}

‎src/wallet/load.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,12 @@ bool LoadWallets(WalletContext& context);
3131
void StartWallets(WalletContext& context, CScheduler& scheduler);
3232

3333
//! Flush all wallets in preparation for shutdown.
34-
void FlushWallets();
34+
void FlushWallets(WalletContext& context);
3535

3636
//! Stop all wallets. Wallets will be flushed first.
37-
void StopWallets();
37+
void StopWallets(WalletContext& context);
3838

3939
//! Close all wallets.
40-
void UnloadWallets();
40+
void UnloadWallets(WalletContext& context);
4141

4242
#endif // BITCOIN_WALLET_LOAD_H

‎src/wallet/rpc/util.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,15 +53,16 @@ bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string&
5353
std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
5454
{
5555
CHECK_NONFATAL(request.mode == JSONRPCRequest::EXECUTE);
56+
WalletContext& context = EnsureWalletContext(request.context);
5657

5758
std::string wallet_name;
5859
if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) {
59-
const std::shared_ptr<CWallet> pwallet = GetWallet(wallet_name);
60+
const std::shared_ptr<CWallet> pwallet = GetWallet(context, wallet_name);
6061
if (!pwallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
6162
return pwallet;
6263
}
6364

64-
std::vector<std::shared_ptr<CWallet>> wallets = GetWallets();
65+
std::vector<std::shared_ptr<CWallet>> wallets = GetWallets(context);
6566
if (wallets.size() == 1) {
6667
return wallets[0];
6768
}

‎src/wallet/rpcwallet.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2579,7 +2579,8 @@ static RPCHelpMan listwallets()
25792579
{
25802580
UniValue obj(UniValue::VARR);
25812581

2582-
for (const std::shared_ptr<CWallet>& wallet : GetWallets()) {
2582+
WalletContext& context = EnsureWalletContext(request.context);
2583+
for (const std::shared_ptr<CWallet>& wallet : GetWallets(context)) {
25832584
LOCK(wallet->cs_wallet);
25842585
obj.push_back(wallet->GetName());
25852586
}
@@ -2998,7 +2999,8 @@ static RPCHelpMan unloadwallet()
29982999
wallet_name = request.params[0].get_str();
29993000
}
30003001

3001-
std::shared_ptr<CWallet> wallet = GetWallet(wallet_name);
3002+
WalletContext& context = EnsureWalletContext(request.context);
3003+
std::shared_ptr<CWallet> wallet = GetWallet(context, wallet_name);
30023004
if (!wallet) {
30033005
throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
30043006
}
@@ -3008,7 +3010,7 @@ static RPCHelpMan unloadwallet()
30083010
// is destroyed (see CheckUniqueFileid).
30093011
std::vector<bilingual_str> warnings;
30103012
std::optional<bool> load_on_start = request.params[1].isNull() ? std::nullopt : std::optional<bool>(request.params[1].get_bool());
3011-
if (!RemoveWallet(wallet, load_on_start, warnings)) {
3013+
if (!RemoveWallet(context, wallet, load_on_start, warnings)) {
30123014
throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded");
30133015
}
30143016

‎src/wallet/test/coinjoin_tests.cpp‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include <util/translation.h>
1515
#include <policy/settings.h>
1616
#include <validation.h>
17+
#include <wallet/context.h>
1718
#include <wallet/wallet.h>
1819

1920
#include <boost/test/unit_test.hpp>
@@ -128,13 +129,15 @@ BOOST_AUTO_TEST_CASE(coinjoin_accept_tests)
128129
class CTransactionBuilderTestSetup : public TestChain100Setup
129130
{
130131
public:
131-
CTransactionBuilderTestSetup()
132-
: wallet{std::make_unique<CWallet>(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase())}
132+
CTransactionBuilderTestSetup() :
133+
wallet{std::make_unique<CWallet>(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase())}
133134
{
135+
context.chain = m_node.chain.get();
136+
context.coinjoin_loader = m_node.coinjoin_loader.get();
134137
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
135138
wallet->SetupLegacyScriptPubKeyMan();
136139
wallet->LoadWallet();
137-
AddWallet(wallet);
140+
AddWallet(context, wallet);
138141
{
139142
LOCK2(wallet->cs_wallet, cs_main);
140143
wallet->GetLegacyScriptPubKeyMan()->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
@@ -150,9 +153,10 @@ class CTransactionBuilderTestSetup : public TestChain100Setup
150153

151154
~CTransactionBuilderTestSetup()
152155
{
153-
RemoveWallet(wallet, std::nullopt);
156+
RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt);
154157
}
155158

159+
WalletContext context;
156160
const std::shared_ptr<CWallet> wallet;
157161

158162
CWalletTx& AddTxToChain(uint256 nTxHash)

0 commit comments

Comments
 (0)