Merge bitcoin/bitcoin#24091: wallet: Consolidate CInputCoin and COutput

049003f coinselection: Remove COutput operators == and != (Andrew Chow)
f6c39c6 coinselection: Remove CInputCoin (Andrew Chow)
70f31f1 coinselection: Use COutput instead of CInputCoin (Andrew Chow)
14fbb57 coinselection: Add effective value and fees to COutput (Andrew Chow)
f082123 moveonly: move COutput to coinselection.h (Andrew Chow)
42e974e wallet: Remove CWallet and CWalletTx from COutput's constructor (Andrew Chow)
14d04d5 wallet: Replace CWalletTx in COutput with COutPoint and CTxOut (Andrew Chow)
0ba4d19 wallet: Provide input bytes to COutput (Andrew Chow)
d51f27d wallet: Store whether a COutput is from the wallet (Andrew Chow)
b799814 wallet: Store tx time in COutput (Andrew Chow)
4602295 wallet: Remove use_max_sig default value (Andrew Chow)
10379f0 scripted-diff: Rename COutput member variables (Andrew Chow)
c7c64db wallet: cleanup COutput constructor (Andrew Chow)

Pull request description:

  While working on coin selection code, it occurred to me that `CInputCoin` is really a subset of `COutput` and the conversion of a `COutput` to a `CInputCoin` does not appear to be all that useful. So this PR adds fields that are present in `CInputCoin` to `COutput` and replaces the usage of `CInputCoin` with `COutput`.

  `COutput` is also moved to coinselection.h. As part of this move, the usage of `CWalletTx` is removed from `COutput`. It is instead replaced by storing a `COutPoint` and the `CTxOut` rather than the entire `CWalletTx` as coin selection does not really need the full `CWalletTx`. The `CWalletTx` was only used for figuring out whether the transaction containing the output was from the current wallet, and for the transaction's time. These are now parameters to `COutput`'s constructor.

ACKs for top commit:
  ryanofsky:
    Code review ACK 049003f, just adding comments and removing == operators since last review
  w0xlt:
    reACK 049003f
  Xekyo:
    reACK 049003f

Tree-SHA512: 048b4cd620a0415e1d9fe8597257ee4bc64656566e1d28a9bdd147d6d72dc87c3f34a3339fa9ab6acf42c388df7901fc4ee900ccaabc3de790ffad162b544c15

Author: fanquake

src/bench/coin_selection.cpp

@@ -13,7 +13,6 @@
 
 using node::NodeContext;
 using wallet::AttemptSelection;
-using wallet::CInputCoin;
 using wallet::COutput;
 using wallet::CWallet;
 using wallet::CWalletTx;
@@ -58,7 +57,7 @@ static void CoinSelection(benchmark::Bench& bench)
     // Create coins
     std::vector<COutput> coins;
     for (const auto& wtx : wtxs) {
-        coins.emplace_back(wallet, *wtx, 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */);
+        coins.emplace_back(COutPoint(wtx->GetHash(), 0), wtx->tx->vout.at(0), /*depth=*/ 6 * 24, GetTxSpendSize(wallet, *wtx, 0), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx->GetTxTime(), /*from_me=*/ true);
     }
 
     const CoinEligibilityFilter filter_standard(1, 6, 0);
@@ -81,17 +80,15 @@ static void CoinSelection(benchmark::Bench& bench)
     });
 }
 
-typedef std::set<CInputCoin> CoinSet;
-
 // Copied from src/wallet/test/coinselector_tests.cpp
 static void add_coin(const CAmount& nValue, int nInput, std::vector<OutputGroup>& set)
 {
     CMutableTransaction tx;
     tx.vout.resize(nInput + 1);
     tx.vout[nInput].nValue = nValue;
-    CInputCoin coin(MakeTransactionRef(tx), nInput);
+    COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 0, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ true);
     set.emplace_back();
-    set.back().Insert(coin, 0, true, 0, 0, false);
+    set.back().Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false);
 }
 // Copied from src/wallet/test/coinselector_tests.cpp
 static CAmount make_hard_case(int utxos, std::vector<OutputGroup>& utxo_pool)

src/wallet/coinselection.cpp

@@ -50,8 +50,8 @@ struct {
  * The Branch and Bound algorithm is described in detail in Murch's Master Thesis:
  * https://murch.one/wp-content/uploads/2016/11/erhardt2016coinselection.pdf
  *
- * @param const std::vector<CInputCoin>& utxo_pool The set of UTXOs that we are choosing from.
- *        These UTXOs will be sorted in descending order by effective value and the CInputCoins'
+ * @param const std::vector<OutputGroup>& utxo_pool The set of UTXO groups that we are choosing from.
+ *        These UTXO groups will be sorted in descending order by effective value and the OutputGroups'
  *        values are their effective values.
  * @param const CAmount& selection_target This is the value that we want to select. It is the lower
  *        bound of the range.
@@ -309,29 +309,29 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
 
  ******************************************************************************/
 
-void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants, bool positive_only) {
+void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descendants, bool positive_only) {
     // Compute the effective value first
-    const CAmount coin_fee = output.m_input_bytes < 0 ? 0 : m_effective_feerate.GetFee(output.m_input_bytes);
+    const CAmount coin_fee = output.input_bytes < 0 ? 0 : m_effective_feerate.GetFee(output.input_bytes);
     const CAmount ev = output.txout.nValue - coin_fee;
 
     // Filter for positive only here before adding the coin
     if (positive_only && ev <= 0) return;
 
     m_outputs.push_back(output);
-    CInputCoin& coin = m_outputs.back();
+    COutput& coin = m_outputs.back();
 
-    coin.m_fee = coin_fee;
-    fee += coin.m_fee;
+    coin.fee = coin_fee;
+    fee += coin.fee;
 
-    coin.m_long_term_fee = coin.m_input_bytes < 0 ? 0 : m_long_term_feerate.GetFee(coin.m_input_bytes);
-    long_term_fee += coin.m_long_term_fee;
+    coin.long_term_fee = coin.input_bytes < 0 ? 0 : m_long_term_feerate.GetFee(coin.input_bytes);
+    long_term_fee += coin.long_term_fee;
 
     coin.effective_value = ev;
     effective_value += coin.effective_value;
 
-    m_from_me &= from_me;
-    m_value += output.txout.nValue;
-    m_depth = std::min(m_depth, depth);
+    m_from_me &= coin.from_me;
+    m_value += coin.txout.nValue;
+    m_depth = std::min(m_depth, coin.depth);
     // ancestors here express the number of ancestors the new coin will end up having, which is
     // the sum, rather than the max; this will overestimate in the cases where multiple inputs
     // have common ancestors
@@ -353,16 +353,16 @@ CAmount OutputGroup::GetSelectionAmount() const
     return m_subtract_fee_outputs ? m_value : effective_value;
 }
 
-CAmount GetSelectionWaste(const std::set<CInputCoin>& inputs, CAmount change_cost, CAmount target, bool use_effective_value)
+CAmount GetSelectionWaste(const std::set<COutput>& inputs, CAmount change_cost, CAmount target, bool use_effective_value)
 {
     // This function should not be called with empty inputs as that would mean the selection failed
     assert(!inputs.empty());
 
     // Always consider the cost of spending an input now vs in the future.
     CAmount waste = 0;
     CAmount selected_effective_value = 0;
-    for (const CInputCoin& coin : inputs) {
-        waste += coin.m_fee - coin.m_long_term_fee;
+    for (const COutput& coin : inputs) {
+        waste += coin.fee - coin.long_term_fee;
         selected_effective_value += use_effective_value ? coin.effective_value : coin.txout.nValue;
     }
 
@@ -407,14 +407,14 @@ void SelectionResult::AddInput(const OutputGroup& group)
     m_use_effective = !group.m_subtract_fee_outputs;
 }
 
-const std::set<CInputCoin>& SelectionResult::GetInputSet() const
+const std::set<COutput>& SelectionResult::GetInputSet() const
 {
     return m_selected_inputs;
 }
 
-std::vector<CInputCoin> SelectionResult::GetShuffledInputVector() const
+std::vector<COutput> SelectionResult::GetShuffledInputVector() const
 {
-    std::vector<CInputCoin> coins(m_selected_inputs.begin(), m_selected_inputs.end());
+    std::vector<COutput> coins(m_selected_inputs.begin(), m_selected_inputs.end());
     Shuffle(coins.begin(), coins.end(), FastRandomContext());
     return coins;
 }
@@ -426,4 +426,9 @@ bool SelectionResult::operator<(SelectionResult other) const
     // As this operator is only used in std::min_element, we want the result that has more inputs when waste are equal.
     return *m_waste < *other.m_waste || (*m_waste == *other.m_waste && m_selected_inputs.size() > other.m_selected_inputs.size());
 }
+
+std::string COutput::ToString() const
+{
+    return strprintf("COutput(%s, %d, %d) [%s]", outpoint.hash.ToString(), outpoint.n, depth, FormatMoney(txout.nValue));
+}
 } // namespace wallet

src/wallet/coinselection.h

@@ -19,56 +19,70 @@ static constexpr CAmount MIN_CHANGE{COIN / 100};
 static const CAmount MIN_FINAL_CHANGE = MIN_CHANGE/2;
 
 /** A UTXO under consideration for use in funding a new transaction. */
-class CInputCoin {
+class COutput
+{
 public:
-    CInputCoin(const CTransactionRef& tx, unsigned int i)
-    {
-        if (!tx)
-            throw std::invalid_argument("tx should not be null");
-        if (i >= tx->vout.size())
-            throw std::out_of_range("The output index is out of range");
-
-        outpoint = COutPoint(tx->GetHash(), i);
-        txout = tx->vout[i];
-        effective_value = txout.nValue;
-    }
+    /** The outpoint identifying this UTXO */
+    COutPoint outpoint;
 
-    CInputCoin(const CTransactionRef& tx, unsigned int i, int input_bytes) : CInputCoin(tx, i)
-    {
-        m_input_bytes = input_bytes;
-    }
+    /** The output itself */
+    CTxOut txout;
 
-    CInputCoin(const COutPoint& outpoint_in, const CTxOut& txout_in)
-    {
-        outpoint = outpoint_in;
-        txout = txout_in;
-        effective_value = txout.nValue;
-    }
+    /**
+     * Depth in block chain.
+     * If > 0: the tx is on chain and has this many confirmations.
+     * If = 0: the tx is waiting confirmation.
+     * If < 0: a conflicting tx is on chain and has this many confirmations. */
+    int depth;
 
-    CInputCoin(const COutPoint& outpoint_in, const CTxOut& txout_in, int input_bytes) : CInputCoin(outpoint_in, txout_in)
-    {
-        m_input_bytes = input_bytes;
-    }
+    /** Pre-computed estimated size of this output as a fully-signed input in a transaction. Can be -1 if it could not be calculated */
+    int input_bytes;
 
-    COutPoint outpoint;
-    CTxOut txout;
+    /** Whether we have the private keys to spend this output */
+    bool spendable;
+
+    /** Whether we know how to spend this output, ignoring the lack of keys */
+    bool solvable;
+
+    /**
+     * Whether this output is considered safe to spend. Unconfirmed transactions
+     * from outside keys and unconfirmed replacement transactions are considered
+     * unsafe and will not be used to fund new spending transactions.
+     */
+    bool safe;
+
+    /** The time of the transaction containing this output as determined by CWalletTx::nTimeSmart */
+    int64_t time;
+
+    /** Whether the transaction containing this output is sent from the owning wallet */
+    bool from_me;
+
+    /** The output's value minus fees required to spend it. Initialized as the output's absolute value. */
     CAmount effective_value;
-    CAmount m_fee{0};
-    CAmount m_long_term_fee{0};
 
-    /** Pre-computed estimated size of this output as a fully-signed input in a transaction. Can be -1 if it could not be calculated */
-    int m_input_bytes{-1};
+    /** The fee required to spend this output at the transaction's target feerate. */
+    CAmount fee{0};
 
-    bool operator<(const CInputCoin& rhs) const {
-        return outpoint < rhs.outpoint;
-    }
+    /** The fee required to spend this output at the consolidation feerate. */
+    CAmount long_term_fee{0};
 
-    bool operator!=(const CInputCoin& rhs) const {
-        return outpoint != rhs.outpoint;
-    }
+    COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me)
+        : outpoint(outpoint),
+        txout(txout),
+        depth(depth),
+        input_bytes(input_bytes),
+        spendable(spendable),
+        solvable(solvable),
+        safe(safe),
+        time(time),
+        from_me(from_me),
+        effective_value(txout.nValue)
+    {}
+
+    std::string ToString() const;
 
-    bool operator==(const CInputCoin& rhs) const {
-        return outpoint == rhs.outpoint;
+    bool operator<(const COutput& rhs) const {
+        return outpoint < rhs.outpoint;
     }
 };
 
@@ -143,7 +157,7 @@ struct CoinEligibilityFilter
 struct OutputGroup
 {
     /** The list of UTXOs contained in this output group. */
-    std::vector<CInputCoin> m_outputs;
+    std::vector<COutput> m_outputs;
     /** Whether the UTXOs were sent by the wallet to itself. This is relevant because we may want at
      * least a certain number of confirmations on UTXOs received from outside wallets while trusting
      * our own UTXOs more. */
@@ -180,7 +194,7 @@ struct OutputGroup
         m_subtract_fee_outputs(params.m_subtract_fee_outputs)
     {}
 
-    void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants, bool positive_only);
+    void Insert(const COutput& output, size_t ancestors, size_t descendants, bool positive_only);
     bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const;
     CAmount GetSelectionAmount() const;
 };
@@ -202,13 +216,13 @@ struct OutputGroup
  * @param[in] use_effective_value Whether to use the input's effective value (when true) or the real value (when false).
  * @return The waste
  */
-[[nodiscard]] CAmount GetSelectionWaste(const std::set<CInputCoin>& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true);
+[[nodiscard]] CAmount GetSelectionWaste(const std::set<COutput>& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true);
 
 struct SelectionResult
 {
 private:
     /** Set of inputs selected by the algorithm to use in the transaction */
-    std::set<CInputCoin> m_selected_inputs;
+    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 CAmount m_target;
     /** Whether the input values for calculations should be the effective value (true) or normal value (false) */
@@ -234,9 +248,9 @@ struct SelectionResult
     [[nodiscard]] CAmount GetWaste() const;
 
     /** Get m_selected_inputs */
-    const std::set<CInputCoin>& GetInputSet() const;
-    /** Get the vector of CInputCoins that will be used to fill in a CTransaction's vin */
-    std::vector<CInputCoin> GetShuffledInputVector() const;
+    const std::set<COutput>& GetInputSet() const;
+    /** Get the vector of COutputs that will be used to fill in a CTransaction's vin */
+    std::vector<COutput> GetShuffledInputVector() const;
 
     bool operator<(SelectionResult other) const;
 };

src/wallet/interfaces.cpp

@@ -111,6 +111,17 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet,
     return result;
 }
 
+WalletTxOut MakeWalletTxOut(const CWallet& wallet,
+    const COutput& output) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
+{
+    WalletTxOut result;
+    result.txout = output.txout;
+    result.time = output.time;
+    result.depth_in_main_chain = output.depth;
+    result.is_spent = wallet.IsSpent(output.outpoint.hash, output.outpoint.n);
+    return result;
+}
+
 class WalletImpl : public Wallet
 {
 public:
@@ -419,8 +430,8 @@ class WalletImpl : public Wallet
         for (const auto& entry : ListCoins(*m_wallet)) {
             auto& group = result[entry.first];
             for (const auto& coin : entry.second) {
-                group.emplace_back(COutPoint(coin.tx->GetHash(), coin.i),
-                    MakeWalletTxOut(*m_wallet, *coin.tx, coin.i, coin.nDepth));
+                group.emplace_back(coin.outpoint,
+                    MakeWalletTxOut(*m_wallet, coin));
             }
         }
         return result;

src/wallet/rpc/coins.cpp

@@ -648,16 +648,16 @@ RPCHelpMan listunspent()
 
     for (const COutput& out : vecOutputs) {
         CTxDestination address;
-        const CScript& scriptPubKey = out.tx->tx->vout[out.i].scriptPubKey;
+        const CScript& scriptPubKey = out.txout.scriptPubKey;
         bool fValidAddress = ExtractDestination(scriptPubKey, address);
-        bool reused = avoid_reuse && pwallet->IsSpentKey(out.tx->GetHash(), out.i);
+        bool reused = avoid_reuse && pwallet->IsSpentKey(out.outpoint.hash, out.outpoint.n);
 
         if (destinations.size() && (!fValidAddress || !destinations.count(address)))
             continue;
 
         UniValue entry(UniValue::VOBJ);
-        entry.pushKV("txid", out.tx->GetHash().GetHex());
-        entry.pushKV("vout", out.i);
+        entry.pushKV("txid", out.outpoint.hash.GetHex());
+        entry.pushKV("vout", (int)out.outpoint.n);
 
         if (fValidAddress) {
             entry.pushKV("address", EncodeDestination(address));
@@ -702,29 +702,29 @@ RPCHelpMan listunspent()
         }
 
         entry.pushKV("scriptPubKey", HexStr(scriptPubKey));
-        entry.pushKV("amount", ValueFromAmount(out.tx->tx->vout[out.i].nValue));
-        entry.pushKV("confirmations", out.nDepth);
-        if (!out.nDepth) {
+        entry.pushKV("amount", ValueFromAmount(out.txout.nValue));
+        entry.pushKV("confirmations", out.depth);
+        if (!out.depth) {
             size_t ancestor_count, descendant_count, ancestor_size;
             CAmount ancestor_fees;
-            pwallet->chain().getTransactionAncestry(out.tx->GetHash(), ancestor_count, descendant_count, &ancestor_size, &ancestor_fees);
+            pwallet->chain().getTransactionAncestry(out.outpoint.hash, ancestor_count, descendant_count, &ancestor_size, &ancestor_fees);
             if (ancestor_count) {
                 entry.pushKV("ancestorcount", uint64_t(ancestor_count));
                 entry.pushKV("ancestorsize", uint64_t(ancestor_size));
                 entry.pushKV("ancestorfees", uint64_t(ancestor_fees));
             }
         }
-        entry.pushKV("spendable", out.fSpendable);
-        entry.pushKV("solvable", out.fSolvable);
-        if (out.fSolvable) {
+        entry.pushKV("spendable", out.spendable);
+        entry.pushKV("solvable", out.solvable);
+        if (out.solvable) {
             std::unique_ptr<SigningProvider> provider = pwallet->GetSolvingProvider(scriptPubKey);
             if (provider) {
                 auto descriptor = InferDescriptor(scriptPubKey, *provider);
                 entry.pushKV("desc", descriptor->ToString());
             }
         }
         if (avoid_reuse) entry.pushKV("reused", reused);
-        entry.pushKV("safe", out.fSafe);
+        entry.pushKV("safe", out.safe);
         results.push_back(entry);
     }