Skip to content

Commit 1548058

Browse files
kwvgkwvg
committed
partial bitcoin#19101: remove ::vpwallets and related global variables
The PR is logically split in two for ease of review, this commit deals with folding ArgsManager, interfaces::Chain and interfaces::CoinJoin ::Loader into WalletContext.
1 parent 1fcdc96 commit 1548058

File tree

9 files changed

+65
-45
lines changed

9 files changed

+65
-45
lines changed

‎src/interfaces/wallet.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,9 @@ class WalletLoader : public ChainClient
358358
//! loaded at startup or by RPC.
359359
using LoadWalletFn = std::function<void(std::unique_ptr<Wallet> wallet)>;
360360
virtual std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) = 0;
361+
362+
//! Return pointer to internal context, useful for testing.
363+
virtual WalletContext* context() { return nullptr; }
361364
};
362365

363366
//! Information about one wallet address.

‎src/wallet/context.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ class Loader;
2727
//! behavior.
2828
struct WalletContext {
2929
interfaces::Chain* chain{nullptr};
30-
ArgsManager* args{nullptr};
30+
ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
3131
interfaces::CoinJoin::Loader* coinjoin_loader{nullptr};
3232

3333
//! Declare default constructor and destructor that are not inline, so code

‎src/wallet/interfaces.cpp‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -574,9 +574,9 @@ class WalletLoaderImpl : public WalletLoader
574574
m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back()));
575575
}
576576
}
577-
bool verify() override { return VerifyWallets(*m_context.chain); }
578-
bool load() override { return LoadWallets(*m_context.chain, *m_context.coinjoin_loader); }
579-
void start(CScheduler& scheduler) override { return StartWallets(scheduler, *Assert(m_context.args)); }
577+
bool verify() override { return VerifyWallets(m_context); }
578+
bool load() override { return LoadWallets(m_context); }
579+
void start(CScheduler& scheduler) override { return StartWallets(m_context, scheduler); }
580580
void flush() override { return FlushWallets(); }
581581
void stop() override { return StopWallets(); }
582582
void setMockTime(int64_t time) override { return SetMockTime(time); }
@@ -590,19 +590,19 @@ class WalletLoaderImpl : public WalletLoader
590590
options.require_create = true;
591591
options.create_flags = wallet_creation_flags;
592592
options.create_passphrase = passphrase;
593-
return MakeWallet(CreateWallet(*m_context.chain, *m_context.coinjoin_loader, name, true /* load_on_start */, options, status, error, warnings));
593+
return MakeWallet(CreateWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
594594
}
595595
std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
596596
{
597597
DatabaseOptions options;
598598
DatabaseStatus status;
599599
options.require_existing = true;
600-
return MakeWallet(LoadWallet(*m_context.chain, *m_context.coinjoin_loader, name, true /* load_on_start */, options, status, error, warnings));
600+
return MakeWallet(LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
601601
}
602602
std::unique_ptr<Wallet> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
603603
{
604604
DatabaseStatus status;
605-
return MakeWallet(RestoreWallet(*m_context.chain, *m_context.coinjoin_loader, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings));
605+
return MakeWallet(RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings));
606606
}
607607
std::string getWalletDir() override
608608
{
@@ -628,6 +628,7 @@ class WalletLoaderImpl : public WalletLoader
628628
{
629629
return HandleLoadWallet(std::move(fn));
630630
}
631+
WalletContext* context() override { return &m_context; }
631632

632633
WalletContext m_context;
633634
const std::vector<std::string> m_wallet_filenames;

‎src/wallet/load.cpp‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,17 @@
1414
#include <util/string.h>
1515
#include <util/system.h>
1616
#include <util/translation.h>
17+
#include <wallet/context.h>
1718
#include <wallet/wallet.h>
1819
#include <wallet/walletdb.h>
1920

2021
#include <univalue.h>
2122

2223
#include <system_error>
2324

24-
bool VerifyWallets(interfaces::Chain& chain)
25+
bool VerifyWallets(WalletContext& context)
2526
{
27+
interfaces::Chain& chain = *context.chain;
2628
if (gArgs.IsArgSet("-walletdir")) {
2729
const fs::path wallet_dir{gArgs.GetPathArg("-walletdir")};
2830
std::error_code error;
@@ -96,8 +98,9 @@ bool VerifyWallets(interfaces::Chain& chain)
9698
return true;
9799
}
98100

99-
bool LoadWallets(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoin_loader)
101+
bool LoadWallets(WalletContext& context)
100102
{
103+
interfaces::Chain& chain = *context.chain;
101104
try {
102105
std::set<fs::path> wallet_paths;
103106
for (const auto& wallet : chain.getSettingsList("wallet")) {
@@ -116,7 +119,7 @@ bool LoadWallets(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoi
116119
continue;
117120
}
118121
chain.initMessage(_("Loading wallet…").translated);
119-
const std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(&chain, &coinjoin_loader, name, std::move(database), options.create_flags, error_string, warnings) : nullptr;
122+
const std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(context, name, std::move(database), options.create_flags, error_string, warnings) : nullptr;
120123
if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
121124
if (!pwallet) {
122125
chain.initError(error_string);
@@ -131,14 +134,14 @@ bool LoadWallets(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoi
131134
}
132135
}
133136

134-
void StartWallets(CScheduler& scheduler, const ArgsManager& args)
137+
void StartWallets(WalletContext& context, CScheduler& scheduler)
135138
{
136139
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
137140
pwallet->postInitProcess();
138141
}
139142

140143
// Schedule periodic wallet flushes and tx rebroadcasts
141-
if (args.GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
144+
if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) {
142145
scheduler.scheduleEvery(MaybeCompactWalletDB, std::chrono::milliseconds{500});
143146
}
144147
scheduler.scheduleEvery(MaybeResendWalletTxs, std::chrono::milliseconds{1000});

‎src/wallet/load.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
class ArgsManager;
1313
class CConnman;
1414
class CScheduler;
15+
struct WalletContext;
1516

1617
namespace interfaces {
1718
class Chain;
@@ -21,13 +22,13 @@ class Loader;
2122
} // namespace interfaces
2223

2324
//! Responsible for reading and validating the -wallet arguments and verifying the wallet database.
24-
bool VerifyWallets(interfaces::Chain& chain);
25+
bool VerifyWallets(WalletContext& context);
2526

2627
//! Load wallet databases.
27-
bool LoadWallets(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoin_loader);
28+
bool LoadWallets(WalletContext& context);
2829

2930
//! Complete startup of wallets.
30-
void StartWallets(CScheduler& scheduler, const ArgsManager& args);
31+
void StartWallets(WalletContext& context, CScheduler& scheduler);
3132

3233
//! Flush all wallets in preparation for shutdown.
3334
void FlushWallets();

‎src/wallet/rpcwallet.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2747,7 +2747,7 @@ static RPCHelpMan loadwallet()
27472747
bilingual_str error;
27482748
std::vector<bilingual_str> warnings;
27492749
std::optional<bool> load_on_start = request.params[1].isNull() ? std::nullopt : std::optional<bool>(request.params[1].get_bool());
2750-
std::shared_ptr<CWallet> const wallet = LoadWallet(*context.chain, *context.coinjoin_loader, name, load_on_start, options, status, error, warnings);
2750+
std::shared_ptr<CWallet> const wallet = LoadWallet(context, name, load_on_start, options, status, error, warnings);
27512751

27522752
HandleWalletError(wallet, status, error);
27532753

@@ -2903,7 +2903,7 @@ static RPCHelpMan createwallet()
29032903
options.create_passphrase = passphrase;
29042904
bilingual_str error;
29052905
std::optional<bool> load_on_start = request.params[6].isNull() ? std::nullopt : std::optional<bool>(request.params[6].get_bool());
2906-
const std::shared_ptr<CWallet> wallet = CreateWallet(*context.chain, *context.coinjoin_loader, request.params[0].get_str(), load_on_start, options, status, error, warnings);
2906+
const std::shared_ptr<CWallet> wallet = CreateWallet(context, request.params[0].get_str(), load_on_start, options, status, error, warnings);
29072907
if (!wallet) {
29082908
RPCErrorCode code = status == DatabaseStatus::FAILED_ENCRYPT ? RPC_WALLET_ENCRYPTION_FAILED : RPC_WALLET_ERROR;
29092909
throw JSONRPCError(code, error.original);
@@ -2957,7 +2957,7 @@ static RPCHelpMan restorewallet()
29572957
bilingual_str error;
29582958
std::vector<bilingual_str> warnings;
29592959

2960-
const std::shared_ptr<CWallet> wallet = RestoreWallet(*context.chain, *context.coinjoin_loader, backup_file, wallet_name, load_on_start, status, error, warnings);
2960+
const std::shared_ptr<CWallet> wallet = RestoreWallet(context, backup_file, wallet_name, load_on_start, status, error, warnings);
29612961

29622962
HandleWalletError(wallet, status, error);
29632963

‎src/wallet/test/wallet_tests.cpp‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include <policy/settings.h>
2727
#include <validation.h>
2828
#include <wallet/coincontrol.h>
29+
#include <wallet/context.h>
2930
#include <wallet/test/util.h>
3031
#include <wallet/test/wallet_test_fixture.h>
3132

@@ -47,19 +48,19 @@ static_assert(WALLET_INCREMENTAL_RELAY_FEE >= DEFAULT_INCREMENTAL_RELAY_FEE, "wa
4748

4849
BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup)
4950

50-
static const std::shared_ptr<CWallet> TestLoadWallet(interfaces::Chain* chain, interfaces::CoinJoin::Loader* coinjoin_loader)
51+
static const std::shared_ptr<CWallet> TestLoadWallet(WalletContext& context)
5152
{
5253
DatabaseOptions options;
5354
DatabaseStatus status;
5455
bilingual_str error;
5556
std::vector<bilingual_str> warnings;
5657
auto database = MakeWalletDatabase("", options, status, error);
57-
auto wallet = CWallet::Create(chain, coinjoin_loader, "", std::move(database), options.create_flags, error, warnings);
58-
if (coinjoin_loader) {
58+
auto wallet = CWallet::Create(context, "", std::move(database), options.create_flags, error, warnings);
59+
if (context.coinjoin_loader) {
5960
// TODO: see CreateWalletWithoutChain
6061
AddWallet(wallet);
6162
}
62-
if (chain) {
63+
if (context.chain) {
6364
wallet->postInitProcess();
6465
}
6566
return wallet;
@@ -697,7 +698,10 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
697698
{
698699
gArgs.ForceSetArg("-unsafesqlitesync", "1");
699700
// Create new wallet with known key and unload it.
700-
auto wallet = TestLoadWallet(m_node.chain.get(), m_node.coinjoin_loader.get());
701+
WalletContext context;
702+
context.chain = m_node.chain.get();
703+
context.coinjoin_loader = m_node.coinjoin_loader.get();
704+
auto wallet = TestLoadWallet(context);
701705
CKey key;
702706
key.MakeNewKey(true);
703707
AddKey(*wallet, key);
@@ -737,7 +741,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
737741

738742
// Reload wallet and make sure new transactions are detected despite events
739743
// being blocked
740-
wallet = TestLoadWallet(m_node.chain.get(), m_node.coinjoin_loader.get());
744+
wallet = TestLoadWallet(context);
741745
BOOST_CHECK(rescan_completed);
742746
BOOST_CHECK_EQUAL(addtx_count, 2);
743747
{
@@ -772,7 +776,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
772776
BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
773777
SyncWithValidationInterfaceQueue();
774778
});
775-
wallet = TestLoadWallet(m_node.chain.get(), m_node.coinjoin_loader.get());
779+
wallet = TestLoadWallet(context);
776780
BOOST_CHECK_EQUAL(addtx_count, 4);
777781
{
778782
LOCK(wallet->cs_wallet);
@@ -785,16 +789,20 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
785789

786790
BOOST_FIXTURE_TEST_CASE(CreateWalletWithoutChain, BasicTestingSetup)
787791
{
788-
// TODO: FIX FIX FIX - coinjoin_loader is null heere!
789-
auto wallet = TestLoadWallet(nullptr, nullptr);
792+
WalletContext context;
793+
context.coinjoin_loader = nullptr; // TODO: FIX FIX FIX
794+
auto wallet = TestLoadWallet(context);
790795
BOOST_CHECK(wallet);
791796
UnloadWallet(std::move(wallet));
792797
}
793798

794799
BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup)
795800
{
796801
gArgs.ForceSetArg("-unsafesqlitesync", "1");
797-
auto wallet = TestLoadWallet(m_node.chain.get(), m_node.coinjoin_loader.get());
802+
WalletContext context;
803+
context.chain = m_node.chain.get();
804+
context.coinjoin_loader = m_node.coinjoin_loader.get();
805+
auto wallet = TestLoadWallet(context);
798806
CKey key;
799807
key.MakeNewKey(true);
800808
AddKey(*wallet, key);

‎src/wallet/wallet.cpp‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
#endif
4242
#include <wallet/coincontrol.h>
4343
#include <wallet/coinselection.h>
44+
#include <wallet/context.h>
4445
#include <wallet/fees.h>
4546
#include <warnings.h>
4647

@@ -230,7 +231,7 @@ void UnloadWallet(std::shared_ptr<CWallet>&& wallet)
230231
}
231232

232233
namespace {
233-
std::shared_ptr<CWallet> LoadWalletInternal(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoin_loader, const std::string& name, std::optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
234+
std::shared_ptr<CWallet> LoadWalletInternal(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
234235
{
235236
try {
236237
std::unique_ptr<WalletDatabase> database = MakeWalletDatabase(name, options, status, error);
@@ -239,8 +240,8 @@ std::shared_ptr<CWallet> LoadWalletInternal(interfaces::Chain& chain, interfaces
239240
return nullptr;
240241
}
241242

242-
chain.initMessage(_("Loading wallet…").translated);
243-
const std::shared_ptr<CWallet> wallet = CWallet::Create(&chain, &coinjoin_loader, name, std::move(database), options.create_flags, error, warnings);
243+
context.chain->initMessage(_("Loading wallet…").translated);
244+
const std::shared_ptr<CWallet> wallet = CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings);
244245
if (!wallet) {
245246
error = Untranslated("Wallet loading failed.") + Untranslated(" ") + error;
246247
status = DatabaseStatus::FAILED_LOAD;
@@ -250,7 +251,7 @@ std::shared_ptr<CWallet> LoadWalletInternal(interfaces::Chain& chain, interfaces
250251
wallet->postInitProcess();
251252

252253
// Write the wallet setting
253-
UpdateWalletSetting(chain, name, load_on_start, warnings);
254+
UpdateWalletSetting(*context.chain, name, load_on_start, warnings);
254255

255256
return wallet;
256257
} catch (const std::runtime_error& e) {
@@ -261,20 +262,20 @@ std::shared_ptr<CWallet> LoadWalletInternal(interfaces::Chain& chain, interfaces
261262
}
262263
} // namespace
263264

264-
std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoin_loader, const std::string& name, std::optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
265+
std::shared_ptr<CWallet> LoadWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
265266
{
266267
auto result = WITH_LOCK(g_loading_wallet_mutex, return g_loading_wallet_set.insert(name));
267268
if (!result.second) {
268269
error = Untranslated("Wallet already loading.");
269270
status = DatabaseStatus::FAILED_LOAD;
270271
return nullptr;
271272
}
272-
auto wallet = LoadWalletInternal(chain, coinjoin_loader, name, load_on_start, options, status, error, warnings);
273+
auto wallet = LoadWalletInternal(context, name, load_on_start, options, status, error, warnings);
273274
WITH_LOCK(g_loading_wallet_mutex, g_loading_wallet_set.erase(result.first));
274275
return wallet;
275276
}
276277

277-
std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoin_loader, const std::string& name, std::optional<bool> load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
278+
std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
278279
{
279280
uint64_t wallet_creation_flags = options.create_flags;
280281
const SecureString& passphrase = options.create_passphrase;
@@ -305,8 +306,8 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, interfaces::Coin
305306
}
306307

307308
// Make the wallet
308-
chain.initMessage(_("Loading wallet…").translated);
309-
const std::shared_ptr<CWallet> wallet = CWallet::Create(&chain, &coinjoin_loader, name, std::move(database), wallet_creation_flags, error, warnings);
309+
context.chain->initMessage(_("Loading wallet…").translated);
310+
const std::shared_ptr<CWallet> wallet = CWallet::Create(context, name, std::move(database), wallet_creation_flags, error, warnings);
310311
if (!wallet) {
311312
error = Untranslated("Wallet creation failed.") + Untranslated(" ") + error;
312313
status = DatabaseStatus::FAILED_CREATE;
@@ -362,13 +363,13 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, interfaces::Coin
362363
wallet->postInitProcess();
363364

364365
// Write the wallet settings
365-
UpdateWalletSetting(chain, name, load_on_start, warnings);
366+
UpdateWalletSetting(*context.chain, name, load_on_start, warnings);
366367

367368
status = DatabaseStatus::SUCCESS;
368369
return wallet;
369370
}
370371

371-
std::shared_ptr<CWallet> RestoreWallet(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoin_loader, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
372+
std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
372373
{
373374
DatabaseOptions options;
374375
options.require_existing = true;
@@ -390,7 +391,7 @@ std::shared_ptr<CWallet> RestoreWallet(interfaces::Chain& chain, interfaces::Coi
390391
auto wallet_file = wallet_path / "wallet.dat";
391392
fs::copy_file(backup_file, wallet_file, fs::copy_options::none);
392393

393-
auto wallet = LoadWallet(chain, coinjoin_loader, wallet_name, load_on_start, options, status, error, warnings);
394+
auto wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
394395

395396
if (!wallet) {
396397
fs::remove(wallet_file);
@@ -3253,8 +3254,10 @@ std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, cons
32533254
return MakeDatabase(wallet_path, options, status, error_string);
32543255
}
32553256

3256-
std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain* chain, interfaces::CoinJoin::Loader* coinjoin_loader, const std::string& name, std::unique_ptr<WalletDatabase> database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings)
3257+
std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::string& name, std::unique_ptr<WalletDatabase> database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings)
32573258
{
3259+
interfaces::Chain* chain = context.chain;
3260+
interfaces::CoinJoin::Loader* coinjoin_loader = context.coinjoin_loader;
32583261
const std::string& walletFile = database->Filename();
32593262

32603263
const auto start{SteadyClock::now()};

0 commit comments

Comments
 (0)