Merge bitcoin/bitcoin#24584: wallet: avoid mixing different OutputTypes during coin selection

71d1d13 test: add unit test for AvailableCoins (josibake)
da03cb4 test: functional test for new coin selection logic (josibake)
438e048 wallet: run coin selection by `OutputType` (josibake)
77b0707 refactor: use CoinsResult struct in SelectCoins (josibake)
2e67291 refactor: store by OutputType in CoinsResult (josibake)

Pull request description:

  # Concept

  Following bitcoin/bitcoin#23789, Bitcoin Core wallet will now generate a change address that matches the payment address type. This improves privacy by not revealing which of the outputs is the change at the time of the transaction in scenarios where the input address types differ from the payment address type. However, information about the change can be leaked in a later transaction. This proposal attempts to address that concern.

  ## Leaking information in a later transaction

  Consider the following scenario:

  ![mix input types(1)](https://user-images.githubusercontent.com/7444140/158597086-788339b0-c698-4b60-bd45-9ede4cd3a483.png)

  1. Alice has a wallet with bech32 type UTXOs and pays Bob, who gives her a P2SH address
  2. Alice's wallet generates a P2SH change output, preserving her privacy in `txid: a`
  3. Alice then pays Carol, who gives her a bech32 address
  4. Alice's wallet combines the P2SH UTXO with a bech32 UTXO and `txid: b` has two bech32 outputs

  From a chain analysis perspective, it is reasonable to infer that the P2SH input in `txid: b` was the change from `txid: a`. To avoid leaking information in this scenario, Alice's wallet should avoid picking the P2SH output and instead fund the transaction with only bech32 Outputs. If the payment to Carol can be funded with just the P2SH output, it should be preferred over the bech32 outputs as this will convert the P2SH UTXO to bech32 UTXOs via the payment and change outputs of the new transaction.

  **TLDR;** Avoid mixing output types, spend non-default `OutputTypes` when it is economical to do so.

  # Approach

  `AvailableCoins` now populates a struct, which makes it easier to access coins by `OutputType`. Coin selection tries to find a funding solution by each output type and chooses the most economical by waste metric. If a solution can't be found without mixing, coin selection runs over the entire wallet, allowing mixing, which is the same as the current behavior.

  I've also added a functional test (`test/functional/wallet_avoid_mixing_output_types.py`) and unit test (`src/wallet/test/availablecoins_tests.cpp`.

ACKs for top commit:
  achow101:
    re-ACK 71d1d13
  aureleoules:
    ACK 71d1d13.
  Xekyo:
    reACK 71d1d13 via `git range-diff master 6530d19 71d1d13`
  LarryRuane:
    ACK 71d1d13

Tree-SHA512: 2e0716efdae5adf5479446fabc731ae81d595131d3b8bade98b64ba323d0e0c6d964a67f8c14c89c428998bda47993fa924f3cfca1529e2bd49eaa4e31b7e426

Author: achow101

src/Makefile.test.include

@@ -168,6 +168,7 @@ 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/scriptpubkeyman_tests.cpp

src/bench/coin_selection.cpp

@@ -56,10 +56,10 @@ static void CoinSelection(benchmark::Bench& bench)
     addCoin(3 * COIN, wallet, wtxs);
 
     // Create coins
-    std::vector<COutput> coins;
+    wallet::CoinsResult available_coins;
     for (const auto& wtx : wtxs) {
         const auto txout = wtx->tx->vout.at(0);
-        coins.emplace_back(COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0);
+        available_coins.bech32.emplace_back(COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0);
     }
 
     const CoinEligibilityFilter filter_standard(1, 6, 0);
@@ -76,7 +76,7 @@ static void CoinSelection(benchmark::Bench& bench)
         /*avoid_partial=*/ false,
     };
     bench.run([&] {
-        auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, coins, coin_selection_params);
+        auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, available_coins, coin_selection_params, /*allow_mixed_output_types=*/true);
         assert(result);
         assert(result->GetSelectedValue() == 1003 * COIN);
         assert(result->GetInputSet().size() == 2);

src/wallet/rpc/coins.cpp

@@ -638,7 +638,7 @@ RPCHelpMan listunspent()
         cctl.m_max_depth = nMaxDepth;
         cctl.m_include_unsafe_inputs = include_unsafe;
         LOCK(pwallet->cs_wallet);
-        vecOutputs = AvailableCoinsListUnspent(*pwallet, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount).coins;
+        vecOutputs = AvailableCoinsListUnspent(*pwallet, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount).all();
     }
 
     LOCK(pwallet->cs_wallet);

src/wallet/rpc/spend.cpp

@@ -1382,7 +1382,7 @@ RPCHelpMan sendall()
                     total_input_value += tx->tx->vout[input.prevout.n].nValue;
                 }
             } else {
-                for (const COutput& output : AvailableCoins(*pwallet, &coin_control, fee_rate, /*nMinimumAmount=*/0).coins) {
+                for (const COutput& output : AvailableCoins(*pwallet, &coin_control, fee_rate, /*nMinimumAmount=*/0).all()) {
                     CHECK_NONFATAL(output.input_bytes > 0);
                     if (send_max && fee_rate.GetFee(output.input_bytes) > output.txout.nValue) {
                         continue;

src/wallet/spend.cpp

@@ -29,14 +29,38 @@ struct TxSize {
 TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* wallet, const std::vector<CTxOut>& txouts, const CCoinControl* coin_control = nullptr);
 TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* wallet, const CCoinControl* coin_control = nullptr) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet);
 
+/**
+ * COutputs available for spending, stored by OutputType.
+ * This struct is really just a wrapper around OutputType vectors with a convenient
+ * method for concatenating and returning all COutputs as one vector.
+ *
+ * clear(), size() methods are implemented so that one can interact with
+ * the CoinsResult struct as if it was a vector
+ */
 struct CoinsResult {
-    std::vector<COutput> coins;
-    // Sum of all the coins amounts
+    /** Vectors for each OutputType */
+    std::vector<COutput> legacy;
+    std::vector<COutput> P2SH_segwit;
+    std::vector<COutput> bech32;
+    std::vector<COutput> bech32m;
+
+    /** Other is a catch-all for anything that doesn't match the known OutputTypes */
+    std::vector<COutput> other;
+
+    /** Concatenate and return all COutputs as one vector */
+    std::vector<COutput> all() const;
+
+    /** The following methods are provided so that CoinsResult can mimic a vector,
+     * i.e., methods can work with individual OutputType vectors or on the entire object */
+    uint64_t size() const;
+    void clear();
+
+    /** Sum of all available coins */
     CAmount total_amount{0};
 };
+
 /**
- * Return vector of available COutputs.
- * By default, returns only the spendable coins.
+ * Populate the CoinsResult struct with vectors of available COutputs, organized by OutputType.
  */
 CoinsResult AvailableCoins(const CWallet& wallet,
                            const CCoinControl* coinControl = nullptr,
@@ -67,35 +91,52 @@ const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint&
 std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 
 std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<COutput>& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only);
+/**
+ * Attempt to find a valid input set that preserves privacy by not mixing OutputTypes.
+ * `ChooseSelectionResult()` will be called on each OutputType individually and the best
+ * the solution (according to the waste metric) will be chosen. If a valid input cannot be found from any
+ * single OutputType, fallback to running `ChooseSelectionResult()` over all available coins.
+ *
+ * param@[in]  wallet                    The wallet which provides solving data for the coins
+ * param@[in]  nTargetValue              The target value
+ * param@[in]  eligilibity_filter        A filter containing rules for which coins are allowed to be included in this selection
+ * param@[in]  available_coins           The struct of coins, organized by OutputType, available for selection prior to filtering
+ * param@[in]  coin_selection_params     Parameters for the coin selection
+ * param@[in]  allow_mixed_output_types  Relax restriction that SelectionResults must be of the same OutputType
+ * returns                               If successful, a SelectionResult containing the input set
+ *                                       If failed, a nullopt
+ */
+std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins,
+                        const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types);
 
 /**
  * Attempt to find a valid input set that meets the provided eligibility filter and target.
  * Multiple coin selection algorithms will be run and the input set that produces the least waste
  * (according to the waste metric) will be chosen.
  *
- * param@[in]  wallet                 The wallet which provides solving data for the coins
- * param@[in]  nTargetValue           The target value
- * param@[in]  eligilibity_filter     A filter containing rules for which coins are allowed to be included in this selection
- * param@[in]  coins                  The vector of coins available for selection prior to filtering
- * param@[in]  coin_selection_params  Parameters for the coin selection
- * returns                            If successful, a SelectionResult containing the input set
- *                                    If failed, a nullopt
+ * param@[in]  wallet                    The wallet which provides solving data for the coins
+ * param@[in]  nTargetValue              The target value
+ * param@[in]  eligilibity_filter        A filter containing rules for which coins are allowed to be included in this selection
+ * param@[in]  available_coins           The struct of coins, organized by OutputType, available for selection prior to filtering
+ * param@[in]  coin_selection_params     Parameters for the coin selection
+ * returns                               If successful, a SelectionResult containing the input set
+ *                                       If failed, a nullopt
  */
-std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins,
+std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins,
                         const CoinSelectionParams& coin_selection_params);
 
 /**
  * Select a set of coins such that nTargetValue is met and at least
  * all coins from coin_control are selected; never select unconfirmed coins if they are not ours
  * param@[in]   wallet                 The wallet which provides data necessary to spend the selected coins
- * param@[in]   vAvailableCoins        The vector of coins available to be spent
+ * param@[in]   available_coins        The struct of coins, organized by OutputType, available for selection prior to filtering
  * param@[in]   nTargetValue           The target value
  * param@[in]   coin_selection_params  Parameters for this coin selection such as feerates, whether to avoid partial spends,
  *                                     and whether to subtract the fee from the outputs.
  * returns                             If successful, a SelectionResult containing the selected coins
  *                                     If failed, a nullopt.
  */
-std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, const CCoinControl& coin_control,
+std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CCoinControl& coin_control,
                  const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 
 struct CreatedTransactionResult

src/wallet/spend.h

@@ -0,0 +1,105 @@
+// Copyright (c) 2022 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
+
+#include <validation.h>
+#include <wallet/coincontrol.h>
+#include <wallet/spend.h>
+#include <wallet/test/util.h>
+#include <wallet/test/wallet_test_fixture.h>
+
+#include <boost/test/unit_test.hpp>
+
+namespace wallet {
+BOOST_FIXTURE_TEST_SUITE(availablecoins_tests, WalletTestingSetup)
+class AvailableCoinsTestingSetup : public TestChain100Setup
+{
+public:
+    AvailableCoinsTestingSetup()
+    {
+        CreateAndProcessBlock({}, {});
+        wallet = CreateSyncedWallet(*m_node.chain, m_node.chainman->ActiveChain(), m_args, coinbaseKey);
+    }
+
+    ~AvailableCoinsTestingSetup()
+    {
+        wallet.reset();
+    }
+    CWalletTx& AddTx(CRecipient recipient)
+    {
+        CTransactionRef tx;
+        CCoinControl dummy;
+        {
+            constexpr int RANDOM_CHANGE_POSITION = -1;
+            auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, dummy);
+            BOOST_CHECK(res);
+            tx = res.GetObj().tx;
+        }
+        wallet->CommitTransaction(tx, {}, {});
+        CMutableTransaction blocktx;
+        {
+            LOCK(wallet->cs_wallet);
+            blocktx = CMutableTransaction(*wallet->mapWallet.at(tx->GetHash()).tx);
+        }
+        CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
+
+        LOCK(wallet->cs_wallet);
+        wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, m_node.chainman->ActiveChain().Tip()->GetBlockHash());
+        auto it = wallet->mapWallet.find(tx->GetHash());
+        BOOST_CHECK(it != wallet->mapWallet.end());
+        it->second.m_state = TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*index=*/1};
+        return it->second;
+    }
+
+    std::unique_ptr<CWallet> wallet;
+};
+
+BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup)
+{
+    CoinsResult available_coins;
+    BResult<CTxDestination> dest;
+    LOCK(wallet->cs_wallet);
+
+    // Verify our wallet has one usable coinbase UTXO before starting
+    // This UTXO is a P2PK, so it should show up in the Other bucket
+    available_coins = AvailableCoins(*wallet);
+    BOOST_CHECK_EQUAL(available_coins.size(), 1U);
+    BOOST_CHECK_EQUAL(available_coins.other.size(), 1U);
+
+    // 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
+
+    // Bech32m
+    dest = wallet->GetNewDestination(OutputType::BECH32M, "");
+    BOOST_ASSERT(dest.HasRes());
+    AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 1 * COIN, /*fSubtractFeeFromAmount=*/true});
+    available_coins = AvailableCoins(*wallet);
+    BOOST_CHECK_EQUAL(available_coins.bech32m.size(), 2U);
+
+    // Bech32
+    dest = wallet->GetNewDestination(OutputType::BECH32, "");
+    BOOST_ASSERT(dest.HasRes());
+    AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 2 * COIN, /*fSubtractFeeFromAmount=*/true});
+    available_coins = AvailableCoins(*wallet);
+    BOOST_CHECK_EQUAL(available_coins.bech32.size(), 2U);
+
+    // P2SH-SEGWIT
+    dest = wallet->GetNewDestination(OutputType::P2SH_SEGWIT, "");
+    AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 3 * COIN, /*fSubtractFeeFromAmount=*/true});
+    available_coins = AvailableCoins(*wallet);
+    BOOST_CHECK_EQUAL(available_coins.P2SH_segwit.size(), 2U);
+
+    // Legacy (P2PKH)
+    dest = wallet->GetNewDestination(OutputType::LEGACY, "");
+    BOOST_ASSERT(dest.HasRes());
+    AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 4 * COIN, /*fSubtractFeeFromAmount=*/true});
+    available_coins = AvailableCoins(*wallet);
+    BOOST_CHECK_EQUAL(available_coins.legacy.size(), 2U);
+}
+
+BOOST_AUTO_TEST_SUITE_END()
+} // namespace wallet