Skip to content

Commit 06ea870

Browse files
laanwjPastaPastaPasta
laanwj
authored and
PastaPastaPasta
committed
Merge bitcoin#20773: refactor: split CWallet::Create
489ebb7 wallet: make chain optional for CWallet::Create (Ivan Metlushko) d73ae93 CWallet::Create move chain init message up into calling code (Ivan Metlushko) 44c430f refactor: Add CWallet:::AttachChain method (Russell Yanofsky) e2a47ce refactor: move first run detection to client code (Ivan Metlushko) Pull request description: This is a followup for bitcoin#20365 (comment) First part of a refactoring with overall goal to simplify `CWallet` and de-duplicate code with `wallettool` **Rationale**: split `CWallet::Create` and create `CWallet::AttachChain`. `CWallet::AttachChain` takes chain as first parameter on purpose. In future I suggest we can remove `chain` from `CWallet` constructor. The second commit is based on be164f9 from bitcoin#15719 (thanks ryanofsky) cc ryanofsky achow101 ACKs for top commit: ryanofsky: Code review ACK 489ebb7. Only changes since last review were adding a const variable declaration, and implementing suggestion not to move feerate option checks to AttachChain. Thanks for updates and fast responses! Tree-SHA512: 00235abfe1b00874c56c449adcab8a36582424abb9ba27440bf750af8f3f217b68c11ca74eb30f78a2109ad1d9009315480effc78345e16a3074a1b5d8128721
1 parent d89847f commit 06ea870

12 files changed

+106
-87
lines changed

‎src/bench/wallet_balance.cpp

+1-2
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,7 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b
2121
CWallet wallet{test_setup->m_node.chain.get(), test_setup->m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase()};
2222
{
2323
wallet.SetupLegacyScriptPubKeyMan();
24-
bool first_run;
25-
if (wallet.LoadWallet(first_run) != DBErrors::LOAD_OK) assert(false);
24+
if (wallet.LoadWallet() != DBErrors::LOAD_OK) assert(false);
2625
}
2726
auto handler = test_setup->m_node.chain->handleNotifications({&wallet, [](CWallet*) {}});
2827

‎src/qt/test/addressbooktests.cpp

+1-2
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
6262
node.setContext(&test.m_node);
6363
std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", CreateMockWalletDatabase());
6464
wallet->SetupLegacyScriptPubKeyMan();
65-
bool firstRun;
66-
wallet->LoadWallet(firstRun);
65+
wallet->LoadWallet();
6766

6867
auto build_address = [wallet]() {
6968
CKey key;

‎src/qt/test/wallettests.cpp

+1-2
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,7 @@ void TestGUI(interfaces::Node& node)
110110
node.setContext(&test.m_node);
111111
std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", CreateMockWalletDatabase());
112112
AddWallet(wallet);
113-
bool firstRun;
114-
wallet->LoadWallet(firstRun);
113+
wallet->LoadWallet();
115114
{
116115
auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan();
117116
LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore);

‎src/wallet/dump.cpp

+1-2
Original file line numberDiff line numberDiff line change
@@ -202,8 +202,7 @@ bool CreateFromDump(const std::string& name, const fs::path& wallet_path, biling
202202
std::shared_ptr<CWallet> wallet(new CWallet(nullptr /* chain */, /*coinjoin_loader=*/ nullptr, name, std::move(database)), WalletToolReleaseWallet);
203203
{
204204
LOCK(wallet->cs_wallet);
205-
bool first_run = true;
206-
DBErrors load_wallet_ret = wallet->LoadWallet(first_run);
205+
DBErrors load_wallet_ret = wallet->LoadWallet();
207206
if (load_wallet_ret != DBErrors::LOAD_OK) {
208207
error = strprintf(_("Error creating %s"), name);
209208
return false;

‎src/wallet/load.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,8 @@ bool LoadWallets(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoi
110110
if (!database && status == DatabaseStatus::FAILED_NOT_FOUND) {
111111
continue;
112112
}
113-
std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(chain, coinjoin_loader, name, std::move(database), options.create_flags, error_string, warnings) : nullptr;
113+
chain.initMessage(_("Loading wallet...").translated);
114+
std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(&chain, coinjoin_loader, name, std::move(database), options.create_flags, error_string, warnings) : nullptr;
114115
if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
115116
if (!pwallet) {
116117
chain.initError(error_string);

‎src/wallet/test/coinjoin_tests.cpp

+1-2
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,7 @@ class CTransactionBuilderTestSetup : public TestChain100Setup
132132
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
133133
wallet = std::make_unique<CWallet>(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase());
134134
wallet->SetupLegacyScriptPubKeyMan();
135-
bool firstRun;
136-
wallet->LoadWallet(firstRun);
135+
wallet->LoadWallet();
137136
AddWallet(wallet);
138137
{
139138
LOCK2(wallet->cs_wallet, cs_main);

‎src/wallet/test/coinselector_tests.cpp

+5-10
Original file line numberDiff line numberDiff line change
@@ -259,8 +259,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
259259
{
260260
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase());
261261
wallet->SetupLegacyScriptPubKeyMan();
262-
bool firstRun;
263-
wallet->LoadWallet(firstRun);
262+
wallet->LoadWallet();
264263
LOCK(wallet->cs_wallet);
265264

266265
bool bnb_used;
@@ -283,8 +282,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
283282

284283
{
285284
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase());
286-
bool firstRun;
287-
wallet->LoadWallet(firstRun);
285+
wallet->LoadWallet();
288286
wallet->SetupLegacyScriptPubKeyMan();
289287
LOCK(wallet->cs_wallet);
290288

@@ -309,8 +307,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
309307
BOOST_AUTO_TEST_CASE(knapsack_solver_test)
310308
{
311309
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase());
312-
bool firstRun;
313-
wallet->LoadWallet(firstRun);
310+
wallet->LoadWallet();
314311
wallet->SetupLegacyScriptPubKeyMan();
315312
LOCK(wallet->cs_wallet);
316313

@@ -591,8 +588,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
591588
BOOST_AUTO_TEST_CASE(ApproximateBestSubset)
592589
{
593590
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase());
594-
bool firstRun;
595-
wallet->LoadWallet(firstRun);
591+
wallet->LoadWallet();
596592
wallet->SetupLegacyScriptPubKeyMan();
597593
LOCK(wallet->cs_wallet);
598594

@@ -615,8 +611,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset)
615611
BOOST_AUTO_TEST_CASE(SelectCoins_test)
616612
{
617613
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase());
618-
bool firstRun;
619-
wallet->LoadWallet(firstRun);
614+
wallet->LoadWallet();
620615
wallet->SetupLegacyScriptPubKeyMan();
621616
LOCK(wallet->cs_wallet);
622617

‎src/wallet/test/wallet_test_fixture.cpp

+1-2
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,7 @@ WalletTestingSetup::WalletTestingSetup(const std::string& chainName)
1010
m_wallet_loader{interfaces::MakeWalletLoader(*m_node.chain, m_node.coinjoin_loader, *Assert(m_node.args))},
1111
m_wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase())
1212
{
13-
bool fFirstRun;
14-
m_wallet.LoadWallet(fFirstRun);
13+
m_wallet.LoadWallet();
1514
m_chain_notifications_handler = m_node.chain->handleNotifications({ &m_wallet, [](CWallet*) {} });
1615
m_wallet_loader->registerRpcs();
1716
}

‎src/wallet/test/wallet_tests.cpp

+18-11
Original file line numberDiff line numberDiff line change
@@ -51,15 +51,17 @@ namespace {
5151
constexpr CAmount fallbackFee = 1000;
5252
} // anonymous namespace
5353

54-
static std::shared_ptr<CWallet> TestLoadWallet(NodeContext& node)
54+
static std::shared_ptr<CWallet> TestLoadWallet(interfaces::Chain* chain, interfaces::CoinJoin::Loader& coinjoin_loader)
5555
{
5656
DatabaseOptions options;
5757
DatabaseStatus status;
5858
bilingual_str error;
5959
std::vector<bilingual_str> warnings;
6060
auto database = MakeWalletDatabase("", options, status, error);
61-
auto wallet = CWallet::Create(*node.chain, *node.coinjoin_loader, "", std::move(database), options.create_flags, error, warnings);
62-
wallet->postInitProcess();
61+
auto wallet = CWallet::Create(chain, coinjoin_loader, "", std::move(database), options.create_flags, error, warnings);
62+
if (chain) {
63+
wallet->postInitProcess();
64+
}
6365
return wallet;
6466
}
6567

@@ -581,8 +583,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
581583
LOCK(wallet->cs_wallet);
582584
wallet->SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
583585
}
584-
bool firstRun;
585-
wallet->LoadWallet(firstRun);
586+
wallet->LoadWallet();
586587
AddKey(*wallet, coinbaseKey);
587588
WalletRescanReserver reserver(*wallet);
588589
reserver.reserve();
@@ -715,8 +716,7 @@ class CreateTransactionTestSetup : public TestChain100Setup
715716
{
716717
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
717718
wallet = std::make_unique<CWallet>(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase());
718-
bool firstRun;
719-
wallet->LoadWallet(firstRun);
719+
wallet->LoadWallet();
720720
AddWallet(wallet);
721721
AddKey(*wallet, coinbaseKey);
722722
WalletRescanReserver reserver(*wallet);
@@ -1232,7 +1232,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
12321232
{
12331233
gArgs.ForceSetArg("-unsafesqlitesync", "1");
12341234
// Create new wallet with known key and unload it.
1235-
auto wallet = TestLoadWallet(m_node);
1235+
auto wallet = TestLoadWallet(m_node.chain.get(), *m_node.coinjoin_loader);
12361236
CKey key;
12371237
key.MakeNewKey(true);
12381238
AddKey(*wallet, key);
@@ -1272,7 +1272,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
12721272

12731273
// Reload wallet and make sure new transactions are detected despite events
12741274
// being blocked
1275-
wallet = TestLoadWallet(m_node);
1275+
wallet = TestLoadWallet(m_node.chain.get(), *m_node.coinjoin_loader);
12761276
BOOST_CHECK(rescan_completed);
12771277
BOOST_CHECK_EQUAL(addtx_count, 2);
12781278
{
@@ -1311,7 +1311,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
13111311
ENTER_CRITICAL_SECTION(wallet->wallet()->cs_wallet);
13121312
ENTER_CRITICAL_SECTION(cs_wallets);
13131313
});
1314-
wallet = TestLoadWallet(m_node);
1314+
wallet = TestLoadWallet(m_node.chain.get(), *m_node.coinjoin_loader);
13151315
BOOST_CHECK_EQUAL(addtx_count, 4);
13161316
{
13171317
LOCK(wallet->cs_wallet);
@@ -1390,11 +1390,18 @@ BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup)
13901390
BOOST_CHECK_EXCEPTION(vr >> w_desc, std::ios_base::failure, malformed_descriptor);
13911391
}
13921392

1393+
BOOST_FIXTURE_TEST_CASE(CreateWalletWithoutChain, BasicTestingSetup)
1394+
{
1395+
auto wallet = TestLoadWallet(nullptr, *m_node.coinjoin_loader);
1396+
BOOST_CHECK(wallet);
1397+
UnloadWallet(std::move(wallet));
1398+
}
1399+
13931400
BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup)
13941401
{
13951402
gArgs.ForceSetArg("-unsafesqlitesync", "1");
13961403
auto chain = interfaces::MakeChain(m_node);
1397-
auto wallet = TestLoadWallet(m_node);
1404+
auto wallet = TestLoadWallet(m_node.chain.get(), *m_node.coinjoin_loader);
13981405
CKey key;
13991406
key.MakeNewKey(true);
14001407
AddKey(*wallet, key);

0 commit comments

Comments
 (0)