From c61d3f02f5122b38ea8bf0029aa9dfbbf38e10d0 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 25 May 2023 14:06:21 -0400 Subject: [PATCH 1/3] tests, bench: Consolidate {Test,Bench}Un/LoadWallet helper The wallet tests and benchmarks both had helper functions for loading and unloading the wallet for the test that were almost identical. These functions are consolidated and reused. --- src/bench/wallet_balance.cpp | 8 ++----- src/bench/wallet_loading.cpp | 36 ++++++-------------------------- src/wallet/test/util.cpp | 31 +++++++++++++++++++++++++++ src/wallet/test/util.h | 5 +++++ src/wallet/test/wallet_tests.cpp | 31 +++++++-------------------- 5 files changed, 51 insertions(+), 60 deletions(-) diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp index 2c77b9b22b2..099ef1635af 100644 --- a/src/bench/wallet_balance.cpp +++ b/src/bench/wallet_balance.cpp @@ -14,14 +14,9 @@ #include -using wallet::CWallet; -using wallet::CreateMockableWalletDatabase; -using wallet::DBErrors; -using wallet::GetBalance; -using wallet::WALLET_FLAG_DESCRIPTORS; - const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj"; +namespace wallet { static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const bool add_mine) { const auto test_setup = MakeNoLogFileContext(); @@ -63,3 +58,4 @@ BENCHMARK(WalletBalanceDirty, benchmark::PriorityLevel::HIGH); BENCHMARK(WalletBalanceClean, benchmark::PriorityLevel::HIGH); BENCHMARK(WalletBalanceMine, benchmark::PriorityLevel::HIGH); BENCHMARK(WalletBalanceWatch, benchmark::PriorityLevel::HIGH); +} // namespace wallet diff --git a/src/bench/wallet_loading.cpp b/src/bench/wallet_loading.cpp index cf32fde51ea..5453238728b 100644 --- a/src/bench/wallet_loading.cpp +++ b/src/bench/wallet_loading.cpp @@ -16,32 +16,7 @@ #include -using wallet::CWallet; -using wallet::CreateMockableWalletDatabase; -using wallet::TxStateInactive; -using wallet::WALLET_FLAG_DESCRIPTORS; -using wallet::WalletContext; -using wallet::WalletDatabase; - -static std::shared_ptr BenchLoadWallet(std::unique_ptr database, WalletContext& context, uint64_t create_flags) -{ - bilingual_str error; - std::vector warnings; - auto wallet = CWallet::Create(context, "", std::move(database), create_flags, error, warnings); - NotifyWalletLoaded(context, wallet); - if (context.chain) { - wallet->postInitProcess(); - } - return wallet; -} - -static void BenchUnloadWallet(std::shared_ptr&& wallet) -{ - SyncWithValidationInterfaceQueue(); - wallet->m_chain_notifications_handler.reset(); - UnloadWallet(std::move(wallet)); -} - +namespace wallet{ static void AddTx(CWallet& wallet) { CMutableTransaction mtx; @@ -66,7 +41,7 @@ static void WalletLoading(benchmark::Bench& bench, bool legacy_wallet) create_flags = WALLET_FLAG_DESCRIPTORS; } auto database = CreateMockableWalletDatabase(); - auto wallet = BenchLoadWallet(std::move(database), context, create_flags); + auto wallet = TestLoadWallet(std::move(database), context, create_flags); // Generate a bunch of transactions and addresses to put into the wallet for (int i = 0; i < 1000; ++i) { @@ -76,14 +51,14 @@ static void WalletLoading(benchmark::Bench& bench, bool legacy_wallet) database = DuplicateMockDatabase(wallet->GetDatabase()); // reload the wallet for the actual benchmark - BenchUnloadWallet(std::move(wallet)); + TestUnloadWallet(std::move(wallet)); bench.epochs(5).run([&] { - wallet = BenchLoadWallet(std::move(database), context, create_flags); + wallet = TestLoadWallet(std::move(database), context, create_flags); // Cleanup database = DuplicateMockDatabase(wallet->GetDatabase()); - BenchUnloadWallet(std::move(wallet)); + TestUnloadWallet(std::move(wallet)); }); } @@ -96,3 +71,4 @@ BENCHMARK(WalletLoadingLegacy, benchmark::PriorityLevel::HIGH); static void WalletLoadingDescriptors(benchmark::Bench& bench) { WalletLoading(bench, /*legacy_wallet=*/false); } BENCHMARK(WalletLoadingDescriptors, benchmark::PriorityLevel::HIGH); #endif +} // namespace wallet diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index 09e7979c02b..eacb70cd691 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -45,6 +46,36 @@ std::unique_ptr CreateSyncedWallet(interfaces::Chain& chain, CChain& cc return wallet; } +std::shared_ptr TestLoadWallet(std::unique_ptr database, WalletContext& context, uint64_t create_flags) +{ + bilingual_str error; + std::vector warnings; + auto wallet = CWallet::Create(context, "", std::move(database), create_flags, error, warnings); + NotifyWalletLoaded(context, wallet); + if (context.chain) { + wallet->postInitProcess(); + } + return wallet; +} + +std::shared_ptr TestLoadWallet(WalletContext& context) +{ + DatabaseOptions options; + options.create_flags = WALLET_FLAG_DESCRIPTORS; + DatabaseStatus status; + bilingual_str error; + std::vector warnings; + auto database = MakeWalletDatabase("", options, status, error); + return TestLoadWallet(std::move(database), context, options.create_flags); +} + +void TestUnloadWallet(std::shared_ptr&& wallet) +{ + SyncWithValidationInterfaceQueue(); + wallet->m_chain_notifications_handler.reset(); + UnloadWallet(std::move(wallet)); +} + std::unique_ptr DuplicateMockDatabase(WalletDatabase& database) { return std::make_unique(dynamic_cast(database).m_records); diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index eb1cfd9e213..fc9448632d0 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -21,6 +21,7 @@ class Chain; namespace wallet { class CWallet; class WalletDatabase; +struct WalletContext; static const DatabaseFormat DATABASE_FORMATS[] = { #ifdef USE_SQLITE @@ -33,6 +34,10 @@ static const DatabaseFormat DATABASE_FORMATS[] = { std::unique_ptr CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key); +std::shared_ptr TestLoadWallet(WalletContext& context); +std::shared_ptr TestLoadWallet(std::unique_ptr database, WalletContext& context, uint64_t create_flags); +void TestUnloadWallet(std::shared_ptr&& wallet); + // Creates a copy of the provided database std::unique_ptr DuplicateMockDatabase(WalletDatabase& database); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 194c8663db6..cb1be9ea5be 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -42,26 +42,6 @@ static_assert(WALLET_INCREMENTAL_RELAY_FEE >= DEFAULT_INCREMENTAL_RELAY_FEE, "wa BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup) -static std::shared_ptr TestLoadWallet(WalletContext& context) -{ - DatabaseOptions options; - options.create_flags = WALLET_FLAG_DESCRIPTORS; - DatabaseStatus status; - bilingual_str error; - std::vector warnings; - auto database = MakeWalletDatabase("", options, status, error); - auto wallet = CWallet::Create(context, "", std::move(database), options.create_flags, error, warnings); - NotifyWalletLoaded(context, wallet); - return wallet; -} - -static void TestUnloadWallet(std::shared_ptr&& wallet) -{ - SyncWithValidationInterfaceQueue(); - wallet->m_chain_notifications_handler.reset(); - UnloadWallet(std::move(wallet)); -} - static CMutableTransaction TestSimpleSpend(const CTransaction& from, uint32_t index, const CKey& key, const CScript& pubkey) { CMutableTransaction mtx; @@ -845,10 +825,11 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) // Reload wallet and make sure new transactions are detected despite events // being blocked + // Loading will also ask for current mempool transactions wallet = TestLoadWallet(context); BOOST_CHECK(rescan_completed); - // AddToWallet events for block_tx and mempool_tx - BOOST_CHECK_EQUAL(addtx_count, 2); + // AddToWallet events for block_tx and mempool_tx (x2) + BOOST_CHECK_EQUAL(addtx_count, 3); { LOCK(wallet->cs_wallet); BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_tx.GetHash()), 1U); @@ -862,7 +843,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) SyncWithValidationInterfaceQueue(); // AddToWallet events for block_tx and mempool_tx events are counted a // second time as the notification queue is processed - BOOST_CHECK_EQUAL(addtx_count, 4); + BOOST_CHECK_EQUAL(addtx_count, 5); TestUnloadWallet(std::move(wallet)); @@ -885,7 +866,9 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) SyncWithValidationInterfaceQueue(); }); wallet = TestLoadWallet(context); - BOOST_CHECK_EQUAL(addtx_count, 2); + // Since mempool transactions are requested at the end of loading, there will + // be 2 additional AddToWallet calls, one from the previous test, and a duplicate for mempool_tx + BOOST_CHECK_EQUAL(addtx_count, 2 + 2); { LOCK(wallet->cs_wallet); BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_tx.GetHash()), 1U); From 846b2fe67ed76a678770d343153acedadfdacd0b Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 25 May 2023 14:08:13 -0400 Subject: [PATCH 2/3] tests: Move ADDRESS_BCRT1_UNSPENDABLE to wallet/test/util.h This static address is usable for other wallet tests and benchmarks, so make it available to them. --- src/bench/wallet_balance.cpp | 2 -- src/wallet/test/util.h | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp index 099ef1635af..cafe7a0c606 100644 --- a/src/bench/wallet_balance.cpp +++ b/src/bench/wallet_balance.cpp @@ -14,8 +14,6 @@ #include -const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj"; - namespace wallet { static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const bool add_mine) { diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index fc9448632d0..b1ad1c959bd 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -32,6 +32,8 @@ static const DatabaseFormat DATABASE_FORMATS[] = { #endif }; +const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj"; + std::unique_ptr CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key); std::shared_ptr TestLoadWallet(WalletContext& context); From 7379a54ec416c8c0a029cc41835a23d42cb6d800 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 17 Feb 2023 19:54:47 -0500 Subject: [PATCH 3/3] bench: Remove incorrect LoadWallet call in WalletBalance The WalletBalance benchmarks would incorrectly call LoadWallet after the wallet has been setup. LoadWallet expects to be the first thing that is called and for the CWallet to be in a fresh state. When it is not, it results in bogus pointers which can cause segfaults during this benchmark. --- src/bench/wallet_balance.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp index cafe7a0c606..509716c716c 100644 --- a/src/bench/wallet_balance.cpp +++ b/src/bench/wallet_balance.cpp @@ -26,7 +26,6 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b LOCK(wallet.cs_wallet); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); wallet.SetupDescriptorScriptPubKeyMans(); - if (wallet.LoadWallet() != DBErrors::LOAD_OK) assert(false); } auto handler = test_setup->m_node.chain->handleNotifications({&wallet, [](CWallet*) {}});