partial bitcoin#25734: wallet, refactor: bitcoin#24584 follow-ups

DashCoreAutoGuix · achow101 · PastaPastaPasta · commit e12871f64aa5 · 2025-12-16T08:28:37.000-06:00
Co-authored-by: Andrew Chow &lt;achow101-github@achow101.com&gt;
diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp
@@ -57,7 +57,7 @@ static void CoinSelection(benchmark::Bench& bench)
     wallet::CoinsResult available_coins;
     for (const auto& wtx : wtxs) {
         const auto txout = wtx->tx->vout.at(0);
-        available_coins.legacy.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.coins[OutputType::LEGACY].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);
     FastRandomContext rand{};
diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp
@@ -1555,7 +1555,7 @@ bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& tx
     AssertLockHeld(m_wallet->cs_wallet);
 
     CCoinControl coin_control(CoinType::ONLY_COINJOIN_COLLATERAL);
-    std::vector<COutput> vCoins{AvailableCoinsListUnspent(*m_wallet, &coin_control).all()};
+    std::vector<COutput> vCoins{AvailableCoinsListUnspent(*m_wallet, &coin_control).All()};
     if (vCoins.empty()) {
         strReason = strprintf("%s requires a collateral transaction and could not locate an acceptable input!", gCoinJoinName);
         return false;
diff --git a/src/outputtype.cpp b/src/outputtype.cpp
@@ -18,13 +18,14 @@
 static const std::string OUTPUT_TYPE_STRING_LEGACY = "legacy";
 static const std::string OUTPUT_TYPE_STRING_UNKNOWN = "unknown";
 
-const std::array<OutputType, 1> OUTPUT_TYPES = {OutputType::LEGACY};
-
 bool ParseOutputType(const std::string& type, OutputType& output_type)
 {
     if (type == OUTPUT_TYPE_STRING_LEGACY) {
         output_type = OutputType::LEGACY;
         return true;
+    } else if (type == OUTPUT_TYPE_STRING_UNKNOWN) {
+        output_type = OutputType::UNKNOWN;
+        return true;
     }
     return false;
 }
diff --git a/src/outputtype.h b/src/outputtype.h
@@ -18,7 +18,10 @@ enum class OutputType {
     UNKNOWN,
 };
 
-extern const std::array<OutputType, 1> OUTPUT_TYPES;
+static constexpr auto OUTPUT_TYPES = std::array{
+    OutputType::LEGACY,
+    OutputType::UNKNOWN,
+};
 
 [[nodiscard]] bool ParseOutputType(const std::string& str, OutputType& output_type);
 const std::string& FormatOutputType(OutputType type);
diff --git a/src/rpc/evo.cpp b/src/rpc/evo.cpp
@@ -292,7 +292,7 @@ static void FundSpecialTx(CWallet& wallet, CMutableTransaction& tx, const Specia
     coinControl.destChange = fundDest;
     coinControl.fRequireAllInputs = false;
 
-    for (const auto& out : AvailableCoinsListUnspent(wallet).all()) {
+    for (const auto& out : AvailableCoinsListUnspent(wallet).All()) {
         CTxDestination txDest;
         if (ExtractDestination(out.txout.scriptPubKey, txDest) && txDest == fundDest) {
             coinControl.Select(out.outpoint);
diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp
@@ -163,7 +163,7 @@ static RPCHelpMan masternode_outputs()
     CCoinControl coin_control(CoinType::ONLY_MASTERNODE_COLLATERAL);
 
     UniValue outputsArr(UniValue::VARR);
-    for (const auto& out : WITH_LOCK(wallet->cs_wallet, return AvailableCoinsListUnspent(*wallet, &coin_control).all())) {
+    for (const auto& out : WITH_LOCK(wallet->cs_wallet, return AvailableCoinsListUnspent(*wallet, &coin_control).All())) {
         outputsArr.push_back(out.outpoint.ToStringShort());
     }
 
diff --git a/src/wallet/coinjoin.cpp b/src/wallet/coinjoin.cpp
@@ -58,7 +58,7 @@ bool CWallet::SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::ve
     CAmount nValueTotal{0};
     CCoinControl coin_control(CoinType::ONLY_READY_TO_MIX);
     std::set<uint256> setRecentTxIds;
-    std::vector<COutput> vCoins{AvailableCoinsListUnspent(*this, &coin_control).all()};
+    std::vector<COutput> vCoins{AvailableCoinsListUnspent(*this, &coin_control).All()};
 
     WalletCJLogPrint(this, "CWallet::%s -- vCoins.size(): %d\n", __func__, vCoins.size());
 
@@ -104,7 +104,7 @@ bool CWallet::SelectDenominatedAmounts(CAmount nValueMax, std::set<CAmount>& set
     CAmount nValueTotal{0};
     CCoinControl coin_control(CoinType::ONLY_READY_TO_MIX);
     // CompareByPriority() cares about effective value, which is only calculable when supplied a feerate
-    std::vector<COutput> vCoins{AvailableCoins(*this, &coin_control, /*feerate=*/CFeeRate(0)).all()};
+    std::vector<COutput> vCoins{AvailableCoins(*this, &coin_control, /*feerate=*/CFeeRate(0)).All()};
 
     // larger denoms first
     std::sort(vCoins.rbegin(), vCoins.rend(), CompareByPriority());
@@ -235,7 +235,7 @@ bool CWallet::HasCollateralInputs(bool fOnlyConfirmed) const
     CCoinControl coin_control(CoinType::ONLY_COINJOIN_COLLATERAL);
     coin_control.m_include_unsafe_inputs = !fOnlyConfirmed;
 
-    return AvailableCoinsListUnspent(*this, &coin_control).size() > 0;
+    return AvailableCoinsListUnspent(*this, &coin_control).Size() > 0;
 }
 
 int CWallet::CountInputsWithAmount(CAmount nInputAmount) const
diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
@@ -658,7 +658,7 @@ RPCHelpMan listunspent()
         coinControl.m_include_unsafe_inputs = include_unsafe;
 
         LOCK(pwallet->cs_wallet);
-        vecOutputs = AvailableCoinsListUnspent(*pwallet, &coinControl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount).all();
+        vecOutputs = AvailableCoinsListUnspent(*pwallet, &coinControl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount).All();
     }
 
     LOCK(pwallet->cs_wallet);
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
@@ -77,24 +77,62 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wall
     return CalculateMaximumSignedTxSize(tx, wallet, txouts, coin_control);
 }
 
-uint64_t CoinsResult::size() const
+size_t CoinsResult::Size() const
 {
-    return legacy.size() + other.size();
+    size_t size{0};
+    for (const auto& it : coins) {
+        size += it.second.size();
+    }
+    return size;
 }
 
-std::vector<COutput> CoinsResult::all() const
+std::vector<COutput> CoinsResult::All() const
 {
     std::vector<COutput> all;
-    all.reserve(this->size());
-    all.insert(all.end(), legacy.begin(), legacy.end());
-    all.insert(all.end(), other.begin(), other.end());
+    all.reserve(Size());
+    for (const auto& it : coins) {
+        all.insert(all.end(), it.second.begin(), it.second.end());
+    }
     return all;
 }
 
-void CoinsResult::clear()
+void CoinsResult::Clear() {
+    coins.clear();
+}
+
+void CoinsResult::Erase(std::set<COutPoint>& preset_coins)
 {
-    legacy.clear();
-    other.clear();
+    for (auto& it : coins) {
+        auto& vec = it.second;
+        auto i = std::find_if(vec.begin(), vec.end(), [&](const COutput &c) { return preset_coins.count(c.outpoint);});
+        if (i != vec.end()) {
+            vec.erase(i);
+            break;
+        }
+    }
+}
+
+void CoinsResult::Shuffle(FastRandomContext& rng_fast)
+{
+    for (auto& it : coins) {
+        ::Shuffle(it.second.begin(), it.second.end(), rng_fast);
+    }
+}
+
+void CoinsResult::Add(OutputType type, const COutput& out)
+{
+    coins[type].emplace_back(out);
+}
+
+static OutputType GetOutputType(TxoutType type)
+{
+    switch (type) {
+        case TxoutType::SCRIPTHASH:
+        case TxoutType::PUBKEYHASH:
+            return OutputType::LEGACY;
+        default:
+            return OutputType::UNKNOWN;
+    }
 }
 
 CoinsResult AvailableCoins(const CWallet& wallet,
@@ -215,37 +253,30 @@ 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;
-
             // If the Output is P2SH and spendable, we want to know if it is
             // a P2SH (legacy). 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
+            CScript script;
             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))
+                if (!provider->GetCScript(hash, script))
                     continue;
-                type = Solver(redeemScript, return_values_unused);
             } else {
-                type = Solver(output.scriptPubKey, return_values_unused);
+                script = output.scriptPubKey;
             }
 
             COutput coin(outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate);
-            switch (type) {
-            case TxoutType::SCRIPTHASH:
-            case TxoutType::PUBKEYHASH:
-                result.legacy.push_back(coin);
-                break;
-            default:
-                result.other.push_back(coin);
-            };
+
+            // 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;
+            type = Solver(script, return_values_unused);
+            result.Add(GetOutputType(type), coin);
 
             // Cache total amount as we go
             result.total_amount += output.nValue;
@@ -257,7 +288,7 @@ CoinsResult AvailableCoins(const CWallet& wallet,
             }
 
             // Checks the maximum number of UTXO's.
-            if (nMaximumCount > 0 && result.size() >= nMaximumCount) {
+            if (nMaximumCount > 0 && result.Size() >= nMaximumCount) {
                 return result;
             }
         }
@@ -313,7 +344,7 @@ std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet)
 
     std::map<CTxDestination, std::vector<COutput>> result;
 
-    for (COutput& coin : AvailableCoinsListUnspent(wallet).all()) {
+    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)) {
@@ -443,22 +474,25 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
 {
     // Run coin selection on each OutputType and compute the Waste Metric
     std::vector<SelectionResult> results;
-    if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.legacy, coin_selection_params, nCoinType)}) {
-        results.push_back(*result);
+    for (const auto& it : available_coins.coins) {
+        if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, it.second, coin_selection_params, nCoinType)}) {
+            results.push_back(*result);
+        }
     }
-
-    // If we can't fund the transaction from any individual OutputType, run coin selection
-    // over all available coins, else pick the best solution from the results
-    if (results.size() == 0) {
-        if (allow_mixed_output_types) {
-            if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.all(), coin_selection_params, nCoinType)}) {
-                return result;
-            }
+    // If we have at least one solution for funding the transaction without mixing, choose the minimum one according to waste metric
+    // and return the result
+    if (results.size() > 0) return *std::min_element(results.begin(), results.end());
+
+    // If we can't fund the transaction from any individual OutputType, run coin selection one last time
+    // over all available coins, which would allow mixing
+    if (allow_mixed_output_types) {
+        if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.All(), coin_selection_params, nCoinType)}) {
+            return result;
         }
-        return std::optional<SelectionResult>();
-    };
-    std::optional<SelectionResult> result{*std::min_element(results.begin(), results.end())};
-    return result;
+    }
+    // Either mixing is not allowed and we couldn't find a solution from any single OutputType, or mixing was allowed and we still couldn't
+    // find a solution using all available coins
+    return std::nullopt;
 };
 
 std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins,
@@ -576,7 +610,7 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
             // Calculate the smallest set of inputs required to meet nTargetValue from available_coins
             bool success{false};
             OutputGroup preset_candidates(coin_selection_params);
-            for (const COutput& out : available_coins.all()) {
+            for (const COutput& out : available_coins.All()) {
                 if (!out.spendable) continue;
                 if (preset_coins.count(out.outpoint)) {
                     preset_candidates.Insert(out, /*ancestors=*/0, /*descendants=*/0, /*positive_only=*/false);
@@ -600,8 +634,7 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
 
     // remove preset inputs from coins so that Coin Selection doesn't pick them.
     if (coin_control.HasSelected()) {
-        available_coins.legacy.erase(remove_if(available_coins.legacy.begin(), available_coins.legacy.end(), [&](const COutput& c) { return preset_coins.count(c.outpoint); }), available_coins.legacy.end());
-        available_coins.other.erase(remove_if(available_coins.other.begin(), available_coins.other.end(), [&](const COutput& c) { return preset_coins.count(c.outpoint); }), available_coins.other.end());
+        available_coins.Erase(preset_coins);
     }
 
     unsigned int limit_ancestor_count = 0;
@@ -613,12 +646,11 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
 
     // form groups from remaining coins; note that preset coins will not
     // automatically have their associated (same address) coins included
-    if (coin_control.m_avoid_partial_spends && available_coins.size() > OUTPUT_GROUP_MAX_ENTRIES) {
+    if (coin_control.m_avoid_partial_spends && available_coins.Size() > OUTPUT_GROUP_MAX_ENTRIES) {
         // Cases where we have 101+ outputs all pointing to the same destination may result in
         // privacy leaks as they will potentially be deterministically sorted. We solve that by
         // explicitly shuffling the outputs before processing
-        Shuffle(available_coins.legacy.begin(), available_coins.legacy.end(), coin_selection_params.rng_fast);
-        Shuffle(available_coins.other.begin(), available_coins.other.end(), coin_selection_params.rng_fast);
+        available_coins.Shuffle(coin_selection_params.rng_fast);
     }
     // Coin Selection attempts to select inputs from a pool of eligible UTXOs to fund the
     // transaction at a target feerate. If an attempt fails, more attempts may be made using a more
diff --git a/src/wallet/spend.h b/src/wallet/spend.h
@@ -33,23 +33,22 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wall
  * 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
+ * Size(), Clear(), Erase(), Shuffle(), and Add() methods are implemented to
+ * allow easy interaction with the struct.
  */
 struct CoinsResult {
-    /** Vectors for each OutputType */
-    std::vector<COutput> legacy;
-
-    /** Other is a catch-all for anything that doesn't match the known OutputTypes */
-    std::vector<COutput> other;
+    std::map<OutputType, std::vector<COutput>> coins;
 
     /** Concatenate and return all COutputs as one vector */
-    std::vector<COutput> all() const;
+    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();
+    size_t Size() const;
+    void Clear();
+    void Erase(std::set<COutPoint>& preset_coins);
+    void Shuffle(FastRandomContext& rng_fast);
+    void Add(OutputType type, const COutput& out);
 
     /** Sum of all available coins */
     CAmount total_amount{0};
diff --git a/src/wallet/test/availablecoins_tests.cpp b/src/wallet/test/availablecoins_tests.cpp
@@ -62,8 +62,8 @@ BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup)
     // 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);
+    BOOST_CHECK_EQUAL(available_coins.Size(), 1U);
+    BOOST_CHECK_EQUAL(available_coins.coins[OutputType::UNKNOWN].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
@@ -77,7 +77,7 @@ BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup)
     BOOST_ASSERT(dest);
     AddTx(CRecipient{{GetScriptForDestination(*dest)}, 4 * COIN, /*fSubtractFeeFromAmount=*/true});
     available_coins = AvailableCoins(*wallet);
-    BOOST_CHECK_EQUAL(available_coins.legacy.size(), 2U);
+    BOOST_CHECK_EQUAL(available_coins.coins[OutputType::LEGACY].size(), 2U);
 }
 
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp

Original file line number	Diff line number	Diff line change
`@@ -57,7 +57,7 @@ static void CoinSelection(benchmark::Bench& bench)`
`57`	`57`	`wallet::CoinsResult available_coins;`
`58`	`58`	`for (const auto& wtx : wtxs) {`
`59`	`59`	`const auto txout = wtx->tx->vout.at(0);`
`60`		`- available_coins.legacy.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);`
	`60`	`+ available_coins.coins[OutputType::LEGACY].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);`
`61`	`61`	`}`
`62`	`62`	`const CoinEligibilityFilter filter_standard(1, 6, 0);`
`63`	`63`	`FastRandomContext rand{};`
Original file line number	Diff line number	Diff line change
`@@ -18,13 +18,14 @@`
`18`	`18`	`static const std::string OUTPUT_TYPE_STRING_LEGACY = "legacy";`
`19`	`19`	`static const std::string OUTPUT_TYPE_STRING_UNKNOWN = "unknown";`
`20`	`20`
`21`		`-const std::array<OutputType, 1> OUTPUT_TYPES = {OutputType::LEGACY};`
`22`		`-`
`23`	`21`	`bool ParseOutputType(const std::string& type, OutputType& output_type)`
`24`	`22`	`{`
`25`	`23`	`if (type == OUTPUT_TYPE_STRING_LEGACY) {`
`26`	`24`	`output_type = OutputType::LEGACY;`
`27`	`25`	`return true;`
	`26`	`+ } else if (type == OUTPUT_TYPE_STRING_UNKNOWN) {`
	`27`	`+ output_type = OutputType::UNKNOWN;`
	`28`	`+ return true;`
`28`	`29`	`}`
`29`	`30`	`return false;`
`30`	`31`	`}`
Original file line number	Diff line number	Diff line change
`@@ -163,7 +163,7 @@ static RPCHelpMan masternode_outputs()`
`163`	`163`	`CCoinControl coin_control(CoinType::ONLY_MASTERNODE_COLLATERAL);`
`164`	`164`
`165`	`165`	`UniValue outputsArr(UniValue::VARR);`
`166`		`- for (const auto& out : WITH_LOCK(wallet->cs_wallet, return AvailableCoinsListUnspent(*wallet, &coin_control).all())) {`
	`166`	`+ for (const auto& out : WITH_LOCK(wallet->cs_wallet, return AvailableCoinsListUnspent(*wallet, &coin_control).All())) {`
`167`	`167`	`outputsArr.push_back(out.outpoint.ToStringShort());`
`168`	`168`	`}`
`169`	`169`
Original file line number	Diff line number	Diff line change
`@@ -658,7 +658,7 @@ RPCHelpMan listunspent()`
`658`	`658`	`coinControl.m_include_unsafe_inputs = include_unsafe;`
`659`	`659`
`660`	`660`	`LOCK(pwallet->cs_wallet);`
`661`		`- vecOutputs = AvailableCoinsListUnspent(*pwallet, &coinControl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount).all();`
	`661`	`+ vecOutputs = AvailableCoinsListUnspent(*pwallet, &coinControl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount).All();`
`662`	`662`	`}`
`663`	`663`
`664`	`664`	`LOCK(pwallet->cs_wallet);`