Merge bitcoin/bitcoin#25789: test: clean and extend availablecoins_tests coverage

9622fe6 test: move coins result test to wallet_tests.cpp (furszy)
f69347d test: extend and simplify availablecoins_tests (furszy)
212ccdf wallet: AvailableCoins, add arg to include/skip locked coins (furszy)

Pull request description:

  Negative PR with extended test coverage :).

  1) Cleaned duplicated code and added coverage for the 'AvailableCoins' incremental result.

  2) The class `AvailableCoinsTestingSetup` inside `availablecoins_tests.cpp` is a plain copy
  of `ListCoinsTestingSetup` that is inside `wallet_tests.cpp`.

      So, deleted the file and moved the `BasicOutputTypesTest` test case to `wallet_tests.cpp`.

  3) Added arg to include/skip locked coins from the `AvailableCoins` result. This is needed for point (1) as otherwise the wallet will spend the coins that we recently created due its closeness to the recipient amount.
  Note: this last point comes from #25659 where I'm using the same functionality to clean/speedup another flow as well.

ACKs for top commit:
  achow101:
    ACK 9622fe6
  theStack:
    ACK 9622fe6
  aureleoules:
    reACK 9622fe6, nice cleanup!

Tree-SHA512: 1ed9133120bfe8815455d1ad317bb0ff96e11a0cc34ee8098716ab9b001749168fa649212b2fa14b330c1686cb1f29039ff1f88ae306db68881b0428c038f388

Author: achow101

src/Makefile.test.include

@@ -174,7 +174,6 @@ BITCOIN_TESTS += \
   wallet/test/wallet_crypto_tests.cpp \
   wallet/test/wallet_transaction_tests.cpp \
   wallet/test/coinselector_tests.cpp \
-  wallet/test/availablecoins_tests.cpp \
   wallet/test/init_tests.cpp \
   wallet/test/ismine_tests.cpp \
   wallet/test/rpc_util_tests.cpp \

src/wallet/spend.cpp

@@ -287,7 +287,7 @@ CoinsResult AvailableCoins(const CWallet& wallet,
             if (coinControl && coinControl->HasSelected() && coinControl->IsSelected(outpoint))
                 continue;
 
-            if (wallet.IsLockedCoin(outpoint))
+            if (wallet.IsLockedCoin(outpoint) && params.skip_locked)
                 continue;
 
             if (wallet.IsSpent(outpoint))

src/wallet/spend.h

@@ -76,6 +76,8 @@ struct CoinFilterParams {
     bool only_spendable{true};
     // By default, do not include immature coinbase outputs
     bool include_immature_coinbase{false};
+    // By default, skip locked UTXOs
+    bool skip_locked{true};
 };
 
 /**

src/wallet/test/availablecoins_tests.cpp

@@ -622,6 +622,46 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup)
     BOOST_CHECK_EQUAL(list.begin()->second.size(), 2U);
 }
 
+void TestCoinsResult(ListCoinsTest& context, OutputType out_type, CAmount amount,
+                     std::map<OutputType, size_t>& expected_coins_sizes)
+{
+    LOCK(context.wallet->cs_wallet);
+    util::Result<CTxDestination> dest = Assert(context.wallet->GetNewDestination(out_type, ""));
+    CWalletTx& wtx = context.AddTx(CRecipient{{GetScriptForDestination(*dest)}, amount, /*fSubtractFeeFromAmount=*/true});
+    CoinFilterParams filter;
+    filter.skip_locked = false;
+    CoinsResult available_coins = AvailableCoins(*context.wallet, nullptr, std::nullopt, filter);
+    // Lock outputs so they are not spent in follow-up transactions
+    for (uint32_t i = 0; i < wtx.tx->vout.size(); i++) context.wallet->LockCoin({wtx.GetHash(), i});
+    for (const auto& [type, size] : expected_coins_sizes) BOOST_CHECK_EQUAL(size, available_coins.coins[type].size());
+}
+
+BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, ListCoinsTest)
+{
+    std::map<OutputType, size_t> expected_coins_sizes;
+    for (const auto& out_type : OUTPUT_TYPES) { expected_coins_sizes[out_type] = 0U; }
+
+    // Verify our wallet has one usable coinbase UTXO before starting
+    // This UTXO is a P2PK, so it should show up in the Other bucket
+    expected_coins_sizes[OutputType::UNKNOWN] = 1U;
+    CoinsResult available_coins = WITH_LOCK(wallet->cs_wallet, return AvailableCoins(*wallet));
+    BOOST_CHECK_EQUAL(available_coins.Size(), expected_coins_sizes[OutputType::UNKNOWN]);
+    BOOST_CHECK_EQUAL(available_coins.coins[OutputType::UNKNOWN].size(), expected_coins_sizes[OutputType::UNKNOWN]);
+
+    // We will create a self transfer for each of the OutputTypes and
+    // verify it is put in the correct bucket after running GetAvailablecoins
+    //
+    // For each OutputType, We expect 2 UTXOs in our wallet following the self transfer:
+    //   1. One UTXO as the recipient
+    //   2. One UTXO from the change, due to payment address matching logic
+
+    for (const auto& out_type : OUTPUT_TYPES) {
+        if (out_type == OutputType::UNKNOWN) continue;
+        expected_coins_sizes[out_type] = 2U;
+        TestCoinsResult(*this, out_type, 1 * COIN, expected_coins_sizes);
+    }
+}
+
 BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup)
 {
     {