Merge bitcoin#23288: tests: remove usage of LegacyScriptPubKeyMan from some wallet tests

2d2edc1 tests: Use Descriptor wallets for generic wallet tests (Andrew Chow)
9951628 tests: Use legacy change type in subtract fee from outputs test (Andrew Chow)
dcd6eeb tests: Use descriptors in psbt_wallet_tests (Andrew Chow)
4b1588c tests: Use DescriptorScriptPubKeyMan in coinselector_tests (Andrew Chow)
811319f tests, gui: Use DescriptorScriptPubKeyMan in GUI tests (Andrew Chow)
9bf0243 bench: Use DescriptorScriptPubKeyMan for wallet things (Andrew Chow)
5e54aa9 bench: remove global testWallet from CoinSelection benchmark (Andrew Chow)
a5595b1 tests: Remove global vCoins and testWallet from coinselector_tests (Andrew Chow)

Pull request description:

  Currently, various tests use `LegacyScriptPubKeyMan` because it was convenient for the refactor that introduced the `ScriptPubKeyMan` interface. However, with the legacy wallet slated to be removed, these tests should not continue to use `LegacyScriptPubKeyMan` as they are not testing any specific legacy wallet behavior. These tests are changed to use `DescriptorScriptPubKeyMan`s.

  Some of the coin selection tests and benchmarks had a global `testWallet`, but this seemed to cause some issues with ensuring that descriptors were set up in that wallet for each test. Those have been restructured to not have any global variables that may be modified between tests.

  The tests which test specific legacy wallet behavior remain unchanged.

ACKs for top commit:
  laanwj:
    Code review ACK 2d2edc1
  brunoerg:
    tACK 2d2edc1

Tree-SHA512: 6d60e5978e822d48e46cfc0dae4635fcb1939f21ea9d84eb72e36112e925554b7ee8f932c7ed0c4881b6566c6c19260bec346abdff1956ca9f300b30fb4e2dd1

Author: laanwj

src/bench/coin_selection.cpp

@@ -33,7 +33,6 @@ static void CoinSelection(benchmark::Bench& bench)
     NodeContext node;
     auto chain = interfaces::MakeChain(node);
     CWallet wallet(chain.get(), "", CreateDummyWalletDatabase());
-    wallet.SetupLegacyScriptPubKeyMan();
     std::vector<std::unique_ptr<CWalletTx>> wtxs;
     LOCK(wallet.cs_wallet);
 
@@ -65,21 +64,16 @@ static void CoinSelection(benchmark::Bench& bench)
 }
 
 typedef std::set<CInputCoin> CoinSet;
-static NodeContext testNode;
-static auto testChain = interfaces::MakeChain(testNode);
-static CWallet testWallet(testChain.get(), "", CreateDummyWalletDatabase());
-std::vector<std::unique_ptr<CWalletTx>> wtxn;
 
 // Copied from src/wallet/test/coinselector_tests.cpp
 static void add_coin(const CAmount& nValue, int nInput, std::vector<OutputGroup>& set)
 {
     CMutableTransaction tx;
     tx.vout.resize(nInput + 1);
     tx.vout[nInput].nValue = nValue;
-    std::unique_ptr<CWalletTx> wtx = std::make_unique<CWalletTx>(MakeTransactionRef(std::move(tx)));
+    CInputCoin coin(MakeTransactionRef(tx), nInput);
     set.emplace_back();
-    set.back().Insert(COutput(testWallet, *wtx, nInput, 0, true, true, true).GetInputCoin(), 0, true, 0, 0, false);
-    wtxn.emplace_back(std::move(wtx));
+    set.back().Insert(coin, 0, true, 0, 0, false);
 }
 // Copied from src/wallet/test/coinselector_tests.cpp
 static CAmount make_hard_case(int utxos, std::vector<OutputGroup>& utxo_pool)
@@ -97,7 +91,6 @@ static CAmount make_hard_case(int utxos, std::vector<OutputGroup>& utxo_pool)
 static void BnBExhaustion(benchmark::Bench& bench)
 {
     // Setup
-    testWallet.SetupLegacyScriptPubKeyMan();
     std::vector<OutputGroup> utxo_pool;
     CoinSet selection;
     CAmount value_ret = 0;

src/bench/wallet_balance.cpp

@@ -14,21 +14,22 @@
 
 #include <optional>
 
-static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const bool add_watchonly, const bool add_mine)
+static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const bool add_mine)
 {
     const auto test_setup = MakeNoLogFileContext<const TestingSetup>();
 
     const auto& ADDRESS_WATCHONLY = ADDRESS_BCRT1_UNSPENDABLE;
 
     CWallet wallet{test_setup->m_node.chain.get(), "", CreateMockWalletDatabase()};
     {
-        wallet.SetupLegacyScriptPubKeyMan();
+        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*) {}});
 
     const std::optional<std::string> address_mine{add_mine ? std::optional<std::string>{getnewaddress(wallet)} : std::nullopt};
-    if (add_watchonly) importaddress(wallet, ADDRESS_WATCHONLY);
 
     for (int i = 0; i < 100; ++i) {
         generatetoaddress(test_setup->m_node, address_mine.value_or(ADDRESS_WATCHONLY));
@@ -42,14 +43,13 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b
         if (set_dirty) wallet.MarkDirty();
         bal = GetBalance(wallet);
         if (add_mine) assert(bal.m_mine_trusted > 0);
-        if (add_watchonly) assert(bal.m_watchonly_trusted > 0);
     });
 }
 
-static void WalletBalanceDirty(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ true, /* add_watchonly */ true, /* add_mine */ true); }
-static void WalletBalanceClean(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ false, /* add_watchonly */ true, /* add_mine */ true); }
-static void WalletBalanceMine(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ false, /* add_watchonly */ false, /* add_mine */ true); }
-static void WalletBalanceWatch(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ false, /* add_watchonly */ true, /* add_mine */ false); }
+static void WalletBalanceDirty(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ true, /* add_mine */ true); }
+static void WalletBalanceClean(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ false, /* add_mine */ true); }
+static void WalletBalanceMine(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ false, /* add_mine */ true); }
+static void WalletBalanceWatch(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ false, /* add_mine */ false); }
 
 BENCHMARK(WalletBalanceDirty);
 BENCHMARK(WalletBalanceClean);

src/qt/test/addressbooktests.cpp

@@ -64,8 +64,12 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
     test.m_node.wallet_client = wallet_client.get();
     node.setContext(&test.m_node);
     std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), "", CreateMockWalletDatabase());
-    wallet->SetupLegacyScriptPubKeyMan();
     wallet->LoadWallet();
+    wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
+    {
+        LOCK(wallet->cs_wallet);
+        wallet->SetupDescriptorScriptPubKeyMans();
+    }
 
     auto build_address = [&wallet]() {
         CKey key;

src/qt/test/wallettests.cpp

@@ -143,11 +143,20 @@ void TestGUI(interfaces::Node& node)
     node.setContext(&test.m_node);
     std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), "", CreateMockWalletDatabase());
     wallet->LoadWallet();
+    wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
     {
-        auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan();
-        LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore);
-        wallet->SetAddressBook(GetDestinationForKey(test.coinbaseKey.GetPubKey(), wallet->m_default_address_type), "", "receive");
-        spk_man->AddKeyPubKey(test.coinbaseKey, test.coinbaseKey.GetPubKey());
+        LOCK(wallet->cs_wallet);
+        wallet->SetupDescriptorScriptPubKeyMans();
+
+        // Add the coinbase key
+        FlatSigningProvider provider;
+        std::string error;
+        std::unique_ptr<Descriptor> desc = Parse("combo(" + EncodeSecret(test.coinbaseKey) + ")", provider, error, /* require_checksum=*/ false);
+        assert(desc);
+        WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);
+        if (!wallet->AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
+        CTxDestination dest = GetDestinationForKey(test.coinbaseKey.GetPubKey(), wallet->m_default_address_type);
+        wallet->SetAddressBook(dest, "", "receive");
         wallet->SetLastBlockProcessed(105, node.context()->chainman->ActiveChain().Tip()->GetBlockHash());
     }
     {

src/test/util/wallet.cpp

@@ -25,16 +25,4 @@ std::string getnewaddress(CWallet& w)
     return EncodeDestination(dest);
 }
 
-void importaddress(CWallet& wallet, const std::string& address)
-{
-    auto spk_man = wallet.GetLegacyScriptPubKeyMan();
-    LOCK2(wallet.cs_wallet, spk_man->cs_KeyStore);
-    const auto dest = DecodeDestination(address);
-    assert(IsValidDestination(dest));
-    const auto script = GetScriptForDestination(dest);
-    wallet.MarkDirty();
-    assert(!spk_man->HaveWatchOnly(script));
-    if (!spk_man->AddWatchOnly(script, 0 /* nCreateTime */)) assert(false);
-    wallet.SetAddressBook(dest, /* label */ "", "receive");
-}
 #endif // ENABLE_WALLET