Merge bitcoin/bitcoin#27666: wallet, bench: Move commonly used functions to their own file and fix a bug

7379a54ec416c8c0a029cc41835a23d42cb6d800 bench: Remove incorrect LoadWallet call in WalletBalance (Andrew Chow)
846b2fe67ed76a678770d343153acedadfdacd0b tests: Move ADDRESS_BCRT1_UNSPENDABLE to wallet/test/util.h (Andrew Chow)
c61d3f02f5122b38ea8bf0029aa9dfbbf38e10d0 tests, bench: Consolidate {Test,Bench}Un/LoadWallet helper (Andrew Chow)

Pull request description:

  I have a few PRs and branches that use these two commits, probably makes sense to split them into a separate PR to be merged sooner.

  The first commit contains some things that end up being commonly used in new wallet benchmarks. These are moved into `wallet_common.{h/cpp}`.

  The second commit contains a bugfix for the wallet_balance benchmark where it calls `LoadWallet` in the wrong place. It's unnecessary to call that function in this benchmark. Although this does not cause any issues currently, it ends up causing issues in some PRs and branches that I'm working on.

ACKs for top commit:
  Sjors:
    utACK 7379a54ec416c8c0a029cc41835a23d42cb6d800
  furszy:
    ACK 7379a54

Tree-SHA512: 47773887a16c69ac7121c699d3446a8c399bd792a6a31714998b7b7a19fea179c6d3b29cb898b04397b2962c1b4120d57009352b8460b8283e188d4cb480c9ba

5 files changed, 53 insertions, 63 deletions

diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp
index 6cf71f4b83..bf2195293e 100644
--- a/src/bench/wallet_balance.cpp
+++ b/src/bench/wallet_balance.cpp
@@ -15,14 +15,7 @@
 
 #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>();
@@ -37,7 +30,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*) {}});
 
@@ -67,3 +59,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 cf32fde51e..5453238728 100644
--- a/src/bench/wallet_loading.cpp
+++ b/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
diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp
index 09e7979c02..eacb70cd69 100644
--- a/src/wallet/test/util.cpp
+++ b/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);
diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h
index eb1cfd9e21..b1ad1c959b 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
@@ -31,8 +32,14 @@ static const DatabaseFormat DATABASE_FORMATS[] = {
 #endif
 };
 
+const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj";
+
 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);
 
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 194c8663db..cb1be9ea5b 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<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);