refactor: store by OutputType in CoinsResult

Store COutputs by OutputType in CoinsResult.

The struct stores vectors of `COutput`s by `OutputType`
for more convenient access

Author: josibake

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

@@ -79,6 +79,32 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *walle
     return CalculateMaximumSignedTxSize(tx, wallet, txouts, coin_control);
 }
 
+uint64_t CoinsResult::size() const
+{
+    return bech32m.size() + bech32.size() + P2SH_segwit.size() + legacy.size() + other.size();
+}
+
+std::vector<COutput> CoinsResult::all() const
+{
+    std::vector<COutput> all;
+    all.reserve(this->size());
+    all.insert(all.end(), bech32m.begin(), bech32m.end());
+    all.insert(all.end(), bech32.begin(), bech32.end());
+    all.insert(all.end(), P2SH_segwit.begin(), P2SH_segwit.end());
+    all.insert(all.end(), legacy.begin(), legacy.end());
+    all.insert(all.end(), other.begin(), other.end());
+    return all;
+}
+
+void CoinsResult::clear()
+{
+    bech32m.clear();
+    bech32.clear();
+    P2SH_segwit.clear();
+    legacy.clear();
+    other.clear();
+}
+
 CoinsResult AvailableCoins(const CWallet& wallet,
                            const CCoinControl* coinControl,
                            std::optional<CFeeRate> feerate,
@@ -193,10 +219,55 @@ CoinsResult AvailableCoins(const CWallet& wallet,
             // Filter by spendable outputs only
             if (!spendable && only_spendable) continue;
 
+            // When parsing a scriptPubKey, Solver returns the parsed pubkeys or hashes (depending on the script)
+            // We don't need those here, so we are leaving them in return_values_unused
+            std::vector<std::vector<uint8_t>> return_values_unused;
+            TxoutType type;
+            bool is_from_p2sh{false};
+
+            // If the Output is P2SH and spendable, we want to know if it is
+            // a P2SH (legacy) or one of P2SH-P2WPKH, P2SH-P2WSH (P2SH-Segwit). We can determine
+            // this from the redeemScript. If the Output is not spendable, it will be classified
+            // as a P2SH (legacy), since we have no way of knowing otherwise without the redeemScript
+            if (output.scriptPubKey.IsPayToScriptHash() && solvable) {
+                CScript redeemScript;
+                CTxDestination destination;
+                if (!ExtractDestination(output.scriptPubKey, destination))
+                    continue;
+                const CScriptID& hash = CScriptID(std::get<ScriptHash>(destination));
+                if (!provider->GetCScript(hash, redeemScript))
+                    continue;
+                type = Solver(redeemScript, return_values_unused);
+                is_from_p2sh = true;
+            } else {
+                type = Solver(output.scriptPubKey, return_values_unused);
+            }
+
             int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), coinControl);
-            result.coins.emplace_back(outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate);
+            COutput coin(outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate);
+            switch (type) {
+            case TxoutType::WITNESS_UNKNOWN:
+            case TxoutType::WITNESS_V1_TAPROOT:
+                result.bech32m.push_back(coin);
+                break;
+            case TxoutType::WITNESS_V0_KEYHASH:
+            case TxoutType::WITNESS_V0_SCRIPTHASH:
+                if (is_from_p2sh) {
+                    result.P2SH_segwit.push_back(coin);
+                    break;
+                }
+                result.bech32.push_back(coin);
+                break;
+            case TxoutType::SCRIPTHASH:
+            case TxoutType::PUBKEYHASH:
+                result.legacy.push_back(coin);
+                break;
+            default:
+                result.other.push_back(coin);
+            };
+
+            // Cache total amount as we go
             result.total_amount += output.nValue;
-
             // Checks the sum amount of all UTXO's.
             if (nMinimumSumAmount != MAX_MONEY) {
                 if (result.total_amount >= nMinimumSumAmount) {
@@ -205,7 +276,7 @@ CoinsResult AvailableCoins(const CWallet& wallet,
             }
 
             // Checks the maximum number of UTXO's.
-            if (nMaximumCount > 0 && result.coins.size() >= nMaximumCount) {
+            if (nMaximumCount > 0 && result.size() >= nMaximumCount) {
                 return result;
             }
         }
@@ -261,7 +332,7 @@ std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet)
 
     std::map<CTxDestination, std::vector<COutput>> result;
 
-    for (const COutput& coin : AvailableCoinsListUnspent(wallet).coins) {
+    for (const COutput& coin : AvailableCoinsListUnspent(wallet).all()) {
         CTxDestination address;
         if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable)) &&
             ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) {
@@ -787,7 +858,7 @@ static BResult<CreatedTransactionResult> CreateTransactionInternal(
                                               0);           /*nMaximumCount*/
 
     // Choose coins to use
-    std::optional<SelectionResult> result = SelectCoins(wallet, res_available_coins.coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params);
+    std::optional<SelectionResult> result = SelectCoins(wallet, res_available_coins.all(), /*nTargetValue=*/selection_target, coin_control, coin_selection_params);
     if (!result) {
         return _("Insufficient funds");
     }

src/wallet/spend.h

@@ -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 were 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,

src/wallet/test/wallet_tests.cpp

@@ -591,7 +591,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup)
     // Lock both coins. Confirm number of available coins drops to 0.
     {
         LOCK(wallet->cs_wallet);
-        BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).coins.size(), 2U);
+        BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).size(), 2U);
     }
     for (const auto& group : list) {
         for (const auto& coin : group.second) {
@@ -601,7 +601,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup)
     }
     {
         LOCK(wallet->cs_wallet);
-        BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).coins.size(), 0U);
+        BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).size(), 0U);
     }
     // Confirm ListCoins still returns same result as before, despite coins
     // being locked.