Merge bitcoin/bitcoin#26066: wallet: Refactor and document CoinControl

daba957 refactor: Make ListSelected return vector (Sebastian Falbesoner)
9477662 wallet: Move CoinCointrol definitions to .cpp (Aurèle Oulès)
1db23da wallet: Use std::optional for GetExternalOutput and fixups (Aurèle Oulès)
becc45b scripted-diff: Rename setSelected->m_selected_inputs (Aurèle Oulès)

Pull request description:

  - Moves CoinControl function definitions from `coincontrol.h` to `coincontrol.cpp`
  - Adds more documentation
  - Renames class member for an improved comprehension
  - Use `std::optional` for `GetExternalOutput`

ACKs for top commit:
  achow101:
    ACK daba957
  Xekyo:
    ACK daba957

Tree-SHA512: 3bf2dc834a3246c2f53f8c55154258e605fcb169431d3f7b156931f33c7e3b1ae28e03e16b37f9140a827890eb7798be485b2c36bfc23ff29bb01763f289a07c

Author: achow101

src/qt/coincontroldialog.cpp

@@ -412,8 +412,7 @@ void CoinControlDialog::updateLabels(CCoinControl& m_coin_control, WalletModel *
     unsigned int nQuantity      = 0;
     bool fWitness               = false;
 
-    std::vector<COutPoint> vCoinControl;
-    m_coin_control.ListSelected(vCoinControl);
+    auto vCoinControl{m_coin_control.ListSelected()};
 
     size_t i = 0;
     for (const auto& out : model->wallet().getCoins(vCoinControl)) {

src/wallet/coincontrol.cpp

@@ -11,4 +11,72 @@ CCoinControl::CCoinControl()
 {
     m_avoid_partial_spends = gArgs.GetBoolArg("-avoidpartialspends", DEFAULT_AVOIDPARTIALSPENDS);
 }
+
+bool CCoinControl::HasSelected() const
+{
+    return !m_selected_inputs.empty();
+}
+
+bool CCoinControl::IsSelected(const COutPoint& output) const
+{
+    return m_selected_inputs.count(output) > 0;
+}
+
+bool CCoinControl::IsExternalSelected(const COutPoint& output) const
+{
+    return m_external_txouts.count(output) > 0;
+}
+
+std::optional<CTxOut> CCoinControl::GetExternalOutput(const COutPoint& outpoint) const
+{
+    const auto ext_it = m_external_txouts.find(outpoint);
+    if (ext_it == m_external_txouts.end()) {
+        return std::nullopt;
+    }
+
+    return std::make_optional(ext_it->second);
+}
+
+void CCoinControl::Select(const COutPoint& output)
+{
+    m_selected_inputs.insert(output);
+}
+
+void CCoinControl::SelectExternal(const COutPoint& outpoint, const CTxOut& txout)
+{
+    m_selected_inputs.insert(outpoint);
+    m_external_txouts.emplace(outpoint, txout);
+}
+
+void CCoinControl::UnSelect(const COutPoint& output)
+{
+    m_selected_inputs.erase(output);
+}
+
+void CCoinControl::UnSelectAll()
+{
+    m_selected_inputs.clear();
+}
+
+std::vector<COutPoint> CCoinControl::ListSelected() const
+{
+    return {m_selected_inputs.begin(), m_selected_inputs.end()};
+}
+
+void CCoinControl::SetInputWeight(const COutPoint& outpoint, int64_t weight)
+{
+    m_input_weights[outpoint] = weight;
+}
+
+bool CCoinControl::HasInputWeight(const COutPoint& outpoint) const
+{
+    return m_input_weights.count(outpoint) > 0;
+}
+
+int64_t CCoinControl::GetInputWeight(const COutPoint& outpoint) const
+{
+    auto it = m_input_weights.find(outpoint);
+    assert(it != m_input_weights.end());
+    return it->second;
+}
 } // namespace wallet

src/wallet/coincontrol.h

@@ -13,9 +13,9 @@
 #include <script/signingprovider.h>
 #include <script/standard.h>
 
-#include <optional>
 #include <algorithm>
 #include <map>
+#include <optional>
 #include <set>
 
 namespace wallet {
@@ -63,76 +63,62 @@ class CCoinControl
 
     CCoinControl();
 
-    bool HasSelected() const
-    {
-        return (setSelected.size() > 0);
-    }
-
-    bool IsSelected(const COutPoint& output) const
-    {
-        return (setSelected.count(output) > 0);
-    }
-
-    bool IsExternalSelected(const COutPoint& output) const
-    {
-        return (m_external_txouts.count(output) > 0);
-    }
-
-    bool GetExternalOutput(const COutPoint& outpoint, CTxOut& txout) const
-    {
-        const auto ext_it = m_external_txouts.find(outpoint);
-        if (ext_it == m_external_txouts.end()) {
-            return false;
-        }
-        txout = ext_it->second;
-        return true;
-    }
-
-    void Select(const COutPoint& output)
-    {
-        setSelected.insert(output);
-    }
-
-    void SelectExternal(const COutPoint& outpoint, const CTxOut& txout)
-    {
-        setSelected.insert(outpoint);
-        m_external_txouts.emplace(outpoint, txout);
-    }
-
-    void UnSelect(const COutPoint& output)
-    {
-        setSelected.erase(output);
-    }
-
-    void UnSelectAll()
-    {
-        setSelected.clear();
-    }
-
-    void ListSelected(std::vector<COutPoint>& vOutpoints) const
-    {
-        vOutpoints.assign(setSelected.begin(), setSelected.end());
-    }
-
-    void SetInputWeight(const COutPoint& outpoint, int64_t weight)
-    {
-        m_input_weights[outpoint] = weight;
-    }
-
-    bool HasInputWeight(const COutPoint& outpoint) const
-    {
-        return m_input_weights.count(outpoint) > 0;
-    }
-
-    int64_t GetInputWeight(const COutPoint& outpoint) const
-    {
-        auto it = m_input_weights.find(outpoint);
-        assert(it != m_input_weights.end());
-        return it->second;
-    }
+    /**
+     * Returns true if there are pre-selected inputs.
+     */
+    bool HasSelected() const;
+    /**
+     * Returns true if the given output is pre-selected.
+     */
+    bool IsSelected(const COutPoint& output) const;
+    /**
+     * Returns true if the given output is selected as an external input.
+     */
+    bool IsExternalSelected(const COutPoint& output) const;
+    /**
+     * Returns the external output for the given outpoint if it exists.
+     */
+    std::optional<CTxOut> GetExternalOutput(const COutPoint& outpoint) const;
+    /**
+     * Lock-in the given output for spending.
+     * The output will be included in the transaction even if it's not the most optimal choice.
+     */
+    void Select(const COutPoint& output);
+    /**
+     * Lock-in the given output as an external input for spending because it is not in the wallet.
+     * The output will be included in the transaction even if it's not the most optimal choice.
+     */
+    void SelectExternal(const COutPoint& outpoint, const CTxOut& txout);
+    /**
+     * Unselects the given output.
+     */
+    void UnSelect(const COutPoint& output);
+    /**
+     * Unselects all outputs.
+     */
+    void UnSelectAll();
+    /**
+     * List the selected inputs.
+     */
+    std::vector<COutPoint> ListSelected() const;
+    /**
+     * Set an input's weight.
+     */
+    void SetInputWeight(const COutPoint& outpoint, int64_t weight);
+    /**
+     * Returns true if the input weight is set.
+     */
+    bool HasInputWeight(const COutPoint& outpoint) const;
+    /**
+     * Returns the input weight.
+     */
+    int64_t GetInputWeight(const COutPoint& outpoint) const;
 
 private:
-    std::set<COutPoint> setSelected;
+    //! Selected inputs (inputs that will be used, regardless of whether they're optimal or not)
+    std::set<COutPoint> m_selected_inputs;
+    //! Map of external inputs to include in the transaction
+    //! These are not in the wallet, so we need to track them separately
     std::map<COutPoint, CTxOut> m_external_txouts;
     //! Map of COutPoints to the maximum weight for that input
     std::map<COutPoint, int64_t> m_input_weights;

src/wallet/spend.cpp

@@ -52,9 +52,7 @@ int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* wallet,
 TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, const CCoinControl* coin_control)
 {
     CMutableTransaction txNew(tx);
-    if (!wallet->DummySignTx(txNew, txouts, coin_control)) {
-        return TxSize{-1, -1};
-    }
+    if (!wallet->DummySignTx(txNew, txouts, coin_control)) return TxSize{-1, -1};
     CTransaction ctx(txNew);
     int64_t vsize = GetVirtualTransactionSize(ctx);
     int64_t weight = GetTransactionWeight(ctx);
@@ -72,11 +70,9 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *walle
             assert(input.prevout.n < mi->second.tx->vout.size());
             txouts.emplace_back(mi->second.tx->vout.at(input.prevout.n));
         } else if (coin_control) {
-            CTxOut txout;
-            if (!coin_control->GetExternalOutput(input.prevout, txout)) {
-                return TxSize{-1, -1};
-            }
-            txouts.emplace_back(txout);
+            const auto& txout{coin_control->GetExternalOutput(input.prevout)};
+            if (!txout) return TxSize{-1, -1};
+            txouts.emplace_back(*txout);
         } else {
             return TxSize{-1, -1};
         }
@@ -163,10 +159,8 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
                                             const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
 {
     PreSelectedInputs result;
-    std::vector<COutPoint> vPresetInputs;
-    coin_control.ListSelected(vPresetInputs);
     const bool can_grind_r = wallet.CanGrindR();
-    for (const COutPoint& outpoint : vPresetInputs) {
+    for (const COutPoint& outpoint : coin_control.ListSelected()) {
         int input_bytes = -1;
         CTxOut txout;
         if (auto ptr_wtx = wallet.GetWalletTx(outpoint.hash)) {
@@ -178,9 +172,12 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
             input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control);
         } else {
             // The input is external. We did not find the tx in mapWallet.
-            if (!coin_control.GetExternalOutput(outpoint, txout)) {
+            const auto out{coin_control.GetExternalOutput(outpoint)};
+            if (!out) {
                 return util::Error{strprintf(_("Not found pre-selected input %s"), outpoint.ToString())};
             }
+
+            txout = *out;
         }
 
         if (input_bytes == -1) {