Merge 260ede1 into merged_master (Bitcoin PR bitcoin/bitcoin#24644)

Author: jamesdorfman

doc/tracing.md

@@ -168,6 +168,49 @@ Arguments passed:
 4. Value of the coin as `int64`
 5. If the coin is a coinbase as `bool`
 
+### Context `coin_selection`
+
+#### Tracepoint `coin_selection:selected_coins`
+
+Is called when `SelectCoins` completes.
+
+Arguments passed:
+1. Wallet name as `pointer to C-style string`
+2. Coin selection algorithm name as `pointer to C-style string`
+3. Selection target value as `int64`
+4. Calculated waste metric of the solution as `int64`
+5. Total value of the selected inputs as `int64`
+
+#### Tracepoint `coin_selection:normal_create_tx_internal`
+
+Is called when the first `CreateTransactionInternal` completes.
+
+Arguments passed:
+1. Wallet name as `pointer to C-style string`
+2. Whether `CreateTransactionInternal` succeeded as `bool`
+3. The expected transaction fee as an `int64`
+4. The position of the change output as an `int32`
+
+#### Tracepoint `coin_selection:attempting_aps_create_tx`
+
+Is called when `CreateTransactionInternal` is called the second time for the optimistic
+Avoid Partial Spends selection attempt. This is used to determine whether the next
+tracepoints called are for the Avoid Partial Spends solution, or a different transaction.
+
+Arguments passed:
+1. Wallet name as `pointer to C-style string`
+
+#### Tracepoint `coin_selection:aps_create_tx_internal`
+
+Is called when the second `CreateTransactionInternal` with Avoid Partial Spends enabled completes.
+
+Arguments passed:
+1. Wallet name as `pointer to C-style string`
+2. Whether the Avoid Partial Spends solution will be used as `bool`
+3. Whether `CreateTransactionInternal` succeeded as` bool`
+4. The expected transaction fee as an `int64`
+5. The position of the change output as an `int32`
+
 ## Adding tracepoints to Bitcoin Core
 
 To add a new tracepoint, `#include <util/trace.h>` in the compilation unit where

src/wallet/coinselection.cpp

@@ -106,7 +106,7 @@ static const size_t TOTAL_TRIES = 100000;
 std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change)
 {
     CAmountMap map_target{{ ::policyAsset, selection_target}};
-    SelectionResult result(map_target);
+    SelectionResult result(map_target, SelectionAlgorithm::BNB);
     CAmount curr_value = 0;
     std::vector<size_t> curr_selection; // selected utxo indexes
 
@@ -210,7 +210,7 @@ std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_poo
 std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value, FastRandomContext& rng)
 {
     CAmountMap map_target{{ ::policyAsset, target_value}};
-    SelectionResult result(map_target);
+    SelectionResult result(map_target, SelectionAlgorithm::SRD);
 
     std::vector<size_t> indexes;
     indexes.resize(utxo_pool.size());
@@ -293,7 +293,7 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v
 std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmountMap& mapTargetValue,
                                               CAmount change_target, FastRandomContext& rng)
 {
-    SelectionResult result(mapTargetValue);
+    SelectionResult result(mapTargetValue, SelectionAlgorithm::KNAPSACK);
 
     std::vector<OutputGroup> inner_groups;
     // ELEMENTS: this is not used
@@ -404,7 +404,7 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
                                               CAmount change_target, FastRandomContext& rng, const CAsset& asset)
 {
     CAmountMap map_target{{ asset, nTargetValue }};
-    SelectionResult result(map_target);
+    SelectionResult result(map_target, SelectionAlgorithm::KNAPSACK);
 
     // List of values less than target
     std::optional<OutputGroup> lowest_larger;
@@ -677,4 +677,17 @@ std::string COutput::ToString() const
 {
     return strprintf("COutput(%s, %d, %d) [%s] [%s]", outpoint.hash.ToString(), outpoint.n, depth, FormatMoney(value), asset.GetHex());
 }
+
+std::string GetAlgorithmName(const SelectionAlgorithm algo)
+{
+    switch (algo)
+    {
+    case SelectionAlgorithm::BNB: return "bnb";
+    case SelectionAlgorithm::KNAPSACK: return "knapsack";
+    case SelectionAlgorithm::SRD: return "srd";
+    case SelectionAlgorithm::MANUAL: return "manual";
+    // No default case to allow for compiler to warn
+    }
+    assert(false);
+}
 } // namespace wallet

src/wallet/coinselection.h

@@ -254,21 +254,34 @@ struct OutputGroup
  */
 [[nodiscard]] CAmount GenerateChangeTarget(CAmount payment_value, FastRandomContext& rng);
 
+enum class SelectionAlgorithm : uint8_t
+{
+    BNB = 0,
+    KNAPSACK = 1,
+    SRD = 2,
+    MANUAL = 3,
+};
+
+std::string GetAlgorithmName(const SelectionAlgorithm algo);
+
 struct SelectionResult
 {
 private:
     /** Set of inputs selected by the algorithm to use in the transaction */
     std::set<COutput> m_selected_inputs;
-    /** The target the algorithm selected for. Note that this may not be equal to the recipient amount as it can include non-input fees */
-    const CAmountMap m_target;
     /** Whether the input values for calculations should be the effective value (true) or normal value (false) */
     bool m_use_effective{false};
     /** The computed waste */
     std::optional<CAmount> m_waste;
 
 public:
-    explicit SelectionResult(const CAmountMap target)
-        : m_target(target) {}
+    /** The target the algorithm selected for. Note that this may not be equal to the recipient amount as it can include non-input fees */
+    const CAmountMap m_target;
+    /** The algorithm used to produce this result */
+    const SelectionAlgorithm m_algo;
+
+    explicit SelectionResult(const CAmountMap target, SelectionAlgorithm algo)
+        : m_target(target), m_algo(algo) {}
 
     SelectionResult() = delete;
 

src/wallet/spend.cpp

@@ -14,6 +14,7 @@
 #include <util/fees.h>
 #include <util/moneystr.h>
 #include <util/rbf.h>
+#include <util/trace.h>
 #include <util/translation.h>
 #include <wallet/coincontrol.h>
 #include <wallet/fees.h>
@@ -581,9 +582,10 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vec
              */
             preset_inputs.Insert(out, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false);
         }
-        SelectionResult result(mapTargetValue);
+        SelectionResult result(mapTargetValue, SelectionAlgorithm::MANUAL);
         result.AddInput(preset_inputs);
         if (result.GetSelectedValue() < mapTargetValue) return std::nullopt;
+        result.ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
         return result;
     }
 
@@ -706,7 +708,7 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vec
     // permissive CoinEligibilityFilter.
     std::optional<SelectionResult> res = [&] {
         // Pre-selected inputs already cover the target amount.
-        if (value_to_select <= CAmountMap{}) return std::make_optional(SelectionResult(mapTargetValue));
+        if (value_to_select <= CAmountMap{}) return std::make_optional(SelectionResult(mapTargetValue, SelectionAlgorithm::MANUAL));
 
         // If possible, fund the transaction with confirmed UTXOs only. Prefer at least six
         // confirmations on outputs received from other wallets and only spend confirmed change.
@@ -761,6 +763,9 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vec
     // add preset inputs to the total value selected
     // Add preset inputs to result
     res->AddInput(preset_inputs);
+    if (res->m_algo == SelectionAlgorithm::MANUAL) {
+        res->ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
+    }
 
     return res;
 }
@@ -1240,6 +1245,7 @@ static bool CreateTransactionInternal(
         error = _("Insufficient funds");
         return false;
     }
+    TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result->m_algo).c_str(), result->m_target, result->GetWaste(), result->GetSelectedValue());
 
     // If all of our inputs are explicit, we don't need a blinded dummy
     if (may_need_blinded_dummy) {
@@ -1793,8 +1799,10 @@ bool CreateTransaction(
     int nChangePosIn = nChangePosInOut;
     Assert(!tx); // tx is an out-param. TODO change the return type from bool to tx (or nullptr)
     bool res = CreateTransactionInternal(wallet, vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, fee_calc_out, sign, blind_details, issuance_details);
+    TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), res, nFeeRet, nChangePosInOut);
     // try with avoidpartialspends unless it's enabled already
     if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && wallet.m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) {
+        TRACE1(coin_selection, attempting_aps_create_tx, wallet.GetName().c_str());
         CCoinControl tmp_cc = coin_control;
         tmp_cc.m_avoid_partial_spends = true;
         CAmount nFeeRet2;
@@ -1807,6 +1815,7 @@ bool CreateTransaction(
             // if fee of this alternative one is within the range of the max fee, we use this one
             const bool use_aps = nFeeRet2 <= nFeeRet + wallet.m_max_aps_fee;
             wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", nFeeRet, nFeeRet2, use_aps ? "grouped" : "non-grouped");
+            TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, res, nFeeRet2, nChangePosInOut2);
             if (use_aps) {
                 tx = tx2;
                 nFeeRet = nFeeRet2;

src/wallet/test/coinselector_tests.cpp

@@ -175,7 +175,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
     FastRandomContext rand{};
     // Setup
     std::vector<COutput> utxo_pool;
-    SelectionResult expected_result(CAmountMap{{::policyAsset, 0}});
+    SelectionResult expected_result(CAmountMap{{::policyAsset, 0}}, SelectionAlgorithm::BNB);
 
     /////////////////////////
     // Known Outcome tests //