Skip to content

Commit

Permalink
Merge bitcoin#20773: refactor: split CWallet::Create
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@laanwj @PastaPastaPasta
laanwj authored and PastaPastaPasta committed Aug 12, 2024
1 parent d89847f commit 06ea870
Show file tree
Hide file tree
Showing 12 changed files with 106 additions and 87 deletions.
3 changes: 1 addition & 2 deletions src/bench/wallet_balance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b
CWallet wallet{test_setup->m_node.chain.get(), test_setup->m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase()};
{
wallet.SetupLegacyScriptPubKeyMan();
bool first_run;
if (wallet.LoadWallet(first_run) != DBErrors::LOAD_OK) assert(false);
if (wallet.LoadWallet() != DBErrors::LOAD_OK) assert(false);
}
auto handler = test_setup->m_node.chain->handleNotifications({&wallet, [](CWallet*) {}});

Expand Down
3 changes: 1 addition & 2 deletions src/qt/test/addressbooktests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
node.setContext(&test.m_node);
std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", CreateMockWalletDatabase());
wallet->SetupLegacyScriptPubKeyMan();
bool firstRun;
wallet->LoadWallet(firstRun);
wallet->LoadWallet();

auto build_address = [wallet]() {
CKey key;
Expand Down
3 changes: 1 addition & 2 deletions src/qt/test/wallettests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,7 @@ void TestGUI(interfaces::Node& node)
node.setContext(&test.m_node);
std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", CreateMockWalletDatabase());
AddWallet(wallet);
bool firstRun;
wallet->LoadWallet(firstRun);
wallet->LoadWallet();
{
auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan();
LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore);
Expand Down
3 changes: 1 addition & 2 deletions src/wallet/dump.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,7 @@ bool CreateFromDump(const std::string& name, const fs::path& wallet_path, biling
std::shared_ptr<CWallet> wallet(new CWallet(nullptr /* chain */, /*coinjoin_loader=*/ nullptr, name, std::move(database)), WalletToolReleaseWallet);
{
LOCK(wallet->cs_wallet);
bool first_run = true;
DBErrors load_wallet_ret = wallet->LoadWallet(first_run);
DBErrors load_wallet_ret = wallet->LoadWallet();
if (load_wallet_ret != DBErrors::LOAD_OK) {
error = strprintf(_("Error creating %s"), name);
return false;
Expand Down
3 changes: 2 additions & 1 deletion src/wallet/load.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ bool LoadWallets(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoi
if (!database && status == DatabaseStatus::FAILED_NOT_FOUND) {
continue;
}
std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(chain, coinjoin_loader, name, std::move(database), options.create_flags, error_string, warnings) : nullptr;
chain.initMessage(_("Loading wallet...").translated);
std::shared_ptr<CWallet> pwallet = database ? CWallet::Create(&chain, coinjoin_loader, name, std::move(database), options.create_flags, error_string, warnings) : nullptr;
if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
if (!pwallet) {
chain.initError(error_string);
Expand Down
3 changes: 1 addition & 2 deletions src/wallet/test/coinjoin_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,7 @@ class CTransactionBuilderTestSetup : public TestChain100Setup
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
wallet = std::make_unique<CWallet>(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase());
wallet->SetupLegacyScriptPubKeyMan();
bool firstRun;
wallet->LoadWallet(firstRun);
wallet->LoadWallet();
AddWallet(wallet);
{
LOCK2(wallet->cs_wallet, cs_main);
Expand Down
15 changes: 5 additions & 10 deletions src/wallet/test/coinselector_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
{
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase());
wallet->SetupLegacyScriptPubKeyMan();
bool firstRun;
wallet->LoadWallet(firstRun);
wallet->LoadWallet();
LOCK(wallet->cs_wallet);

bool bnb_used;
Expand All @@ -283,8 +282,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)

{
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase());
bool firstRun;
wallet->LoadWallet(firstRun);
wallet->LoadWallet();
wallet->SetupLegacyScriptPubKeyMan();
LOCK(wallet->cs_wallet);

Expand All @@ -309,8 +307,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
BOOST_AUTO_TEST_CASE(knapsack_solver_test)
{
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase());
bool firstRun;
wallet->LoadWallet(firstRun);
wallet->LoadWallet();
wallet->SetupLegacyScriptPubKeyMan();
LOCK(wallet->cs_wallet);

Expand Down Expand Up @@ -591,8 +588,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
BOOST_AUTO_TEST_CASE(ApproximateBestSubset)
{
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase());
bool firstRun;
wallet->LoadWallet(firstRun);
wallet->LoadWallet();
wallet->SetupLegacyScriptPubKeyMan();
LOCK(wallet->cs_wallet);

Expand All @@ -615,8 +611,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset)
BOOST_AUTO_TEST_CASE(SelectCoins_test)
{
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase());
bool firstRun;
wallet->LoadWallet(firstRun);
wallet->LoadWallet();
wallet->SetupLegacyScriptPubKeyMan();
LOCK(wallet->cs_wallet);

Expand Down
3 changes: 1 addition & 2 deletions src/wallet/test/wallet_test_fixture.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ WalletTestingSetup::WalletTestingSetup(const std::string& chainName)
m_wallet_loader{interfaces::MakeWalletLoader(*m_node.chain, m_node.coinjoin_loader, *Assert(m_node.args))},
m_wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase())
{
bool fFirstRun;
m_wallet.LoadWallet(fFirstRun);
m_wallet.LoadWallet();
m_chain_notifications_handler = m_node.chain->handleNotifications({ &m_wallet, [](CWallet*) {} });
m_wallet_loader->registerRpcs();
}
29 changes: 18 additions & 11 deletions src/wallet/test/wallet_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,17 @@ namespace {
constexpr CAmount fallbackFee = 1000;
} // anonymous namespace

static std::shared_ptr<CWallet> TestLoadWallet(NodeContext& node)
static std::shared_ptr<CWallet> TestLoadWallet(interfaces::Chain* chain, interfaces::CoinJoin::Loader& coinjoin_loader)
{
DatabaseOptions options;
DatabaseStatus status;
bilingual_str error;
std::vector<bilingual_str> warnings;
auto database = MakeWalletDatabase("", options, status, error);
auto wallet = CWallet::Create(*node.chain, *node.coinjoin_loader, "", std::move(database), options.create_flags, error, warnings);
wallet->postInitProcess();
auto wallet = CWallet::Create(chain, coinjoin_loader, "", std::move(database), options.create_flags, error, warnings);
if (chain) {
wallet->postInitProcess();
}
return wallet;
}

Expand Down Expand Up @@ -581,8 +583,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
LOCK(wallet->cs_wallet);
wallet->SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
}
bool firstRun;
wallet->LoadWallet(firstRun);
wallet->LoadWallet();
AddKey(*wallet, coinbaseKey);
WalletRescanReserver reserver(*wallet);
reserver.reserve();
Expand Down Expand Up @@ -715,8 +716,7 @@ class CreateTransactionTestSetup : public TestChain100Setup
{
CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
wallet = std::make_unique<CWallet>(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase());
bool firstRun;
wallet->LoadWallet(firstRun);
wallet->LoadWallet();
AddWallet(wallet);
AddKey(*wallet, coinbaseKey);
WalletRescanReserver reserver(*wallet);
Expand Down Expand Up @@ -1232,7 +1232,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
{
gArgs.ForceSetArg("-unsafesqlitesync", "1");
// Create new wallet with known key and unload it.
auto wallet = TestLoadWallet(m_node);
auto wallet = TestLoadWallet(m_node.chain.get(), *m_node.coinjoin_loader);
CKey key;
key.MakeNewKey(true);
AddKey(*wallet, key);
Expand Down Expand Up @@ -1272,7 +1272,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)

// Reload wallet and make sure new transactions are detected despite events
// being blocked
wallet = TestLoadWallet(m_node);
wallet = TestLoadWallet(m_node.chain.get(), *m_node.coinjoin_loader);
BOOST_CHECK(rescan_completed);
BOOST_CHECK_EQUAL(addtx_count, 2);
{
Expand Down Expand Up @@ -1311,7 +1311,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
ENTER_CRITICAL_SECTION(wallet->wallet()->cs_wallet);
ENTER_CRITICAL_SECTION(cs_wallets);
});
wallet = TestLoadWallet(m_node);
wallet = TestLoadWallet(m_node.chain.get(), *m_node.coinjoin_loader);
BOOST_CHECK_EQUAL(addtx_count, 4);
{
LOCK(wallet->cs_wallet);
Expand Down Expand Up @@ -1390,11 +1390,18 @@ BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup)
BOOST_CHECK_EXCEPTION(vr >> w_desc, std::ios_base::failure, malformed_descriptor);
}

BOOST_FIXTURE_TEST_CASE(CreateWalletWithoutChain, BasicTestingSetup)
{
auto wallet = TestLoadWallet(nullptr, *m_node.coinjoin_loader);
BOOST_CHECK(wallet);
UnloadWallet(std::move(wallet));
}

BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup)
{
gArgs.ForceSetArg("-unsafesqlitesync", "1");
auto chain = interfaces::MakeChain(m_node);
auto wallet = TestLoadWallet(m_node);
auto wallet = TestLoadWallet(m_node.chain.get(), *m_node.coinjoin_loader);
CKey key;
key.MakeNewKey(true);
AddKey(*wallet, key);
Expand Down
Loading

0 comments on commit 06ea870

Please sign in to comment.