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.

Author: achow101

src/bench/wallet_balance.cpp

@@ -14,14 +14,9 @@
 
 #include <optional>
 
-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<const TestingSetup>();
@@ -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

src/bench/wallet_loading.cpp

@@ -16,32 +16,7 @@
 
 #include <optional>
 
-using wallet::CWallet;
-using wallet::CreateMockableWalletDatabase;
-using wallet::TxStateInactive;
-using wallet::WALLET_FLAG_DESCRIPTORS;
-using wallet::WalletContext;
-using wallet::WalletDatabase;
-
-static std::shared_ptr<CWallet> BenchLoadWallet(std::unique_ptr<WalletDatabase> database, WalletContext& context, uint64_t create_flags)
-{
-    bilingual_str error;
-    std::vector<bilingual_str> 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<CWallet>&& 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

src/wallet/test/util.cpp

@@ -9,6 +9,7 @@
 #include <key_io.h>
 #include <streams.h>
 #include <test/util/setup_common.h>
+#include <wallet/context.h>
 #include <wallet/wallet.h>
 #include <wallet/walletdb.h>
 
@@ -45,6 +46,36 @@ std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cc
     return wallet;
 }
 
+std::shared_ptr<CWallet> TestLoadWallet(std::unique_ptr<WalletDatabase> database, WalletContext& context, uint64_t create_flags)
+{
+    bilingual_str error;
+    std::vector<bilingual_str> 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<CWallet> TestLoadWallet(WalletContext& context)
+{
+    DatabaseOptions options;
+    options.create_flags = WALLET_FLAG_DESCRIPTORS;
+    DatabaseStatus status;
+    bilingual_str error;
+    std::vector<bilingual_str> warnings;
+    auto database = MakeWalletDatabase("", options, status, error);
+    return TestLoadWallet(std::move(database), context, options.create_flags);
+}
+
+void TestUnloadWallet(std::shared_ptr<CWallet>&& wallet)
+{
+    SyncWithValidationInterfaceQueue();
+    wallet->m_chain_notifications_handler.reset();
+    UnloadWallet(std::move(wallet));
+}
+
 std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& database)
 {
     return std::make_unique<MockableDatabase>(dynamic_cast<MockableDatabase&>(database).m_records);

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<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key);
 
+std::shared_ptr<CWallet> TestLoadWallet(WalletContext& context);
+std::shared_ptr<CWallet> TestLoadWallet(std::unique_ptr<WalletDatabase> database, WalletContext& context, uint64_t create_flags);
+void TestUnloadWallet(std::shared_ptr<CWallet>&& wallet);
+
 // Creates a copy of the provided database
 std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& database);
 

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<CWallet> TestLoadWallet(WalletContext& context)
-{
-    DatabaseOptions options;
-    options.create_flags = WALLET_FLAG_DESCRIPTORS;
-    DatabaseStatus status;
-    bilingual_str error;
-    std::vector<bilingual_str> 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<CWallet>&& 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);