Merge bitcoin#25273: wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction

0295b44 wallet: return CreatedTransactionResult from FundTransaction (Andrew Chow)
758501b wallet: use optional for change position as an optional in CreateTransaction (Andrew Chow)
2d39db7 wallet: Explicitly preserve scriptSig and scriptWitness in CreateTransaction (Andrew Chow)
14e5074 wallet: Explicitly preserve transaction version in CreateTransaction (Andrew Chow)
0fefcbb wallet: Explicitly preserve transaction locktime in CreateTransaction (Andrew Chow)
4d335bb wallet: Set preset input sequence through coin control (Andrew Chow)
596642c wallet: Replace SelectExternal with SetTxOut (Andrew Chow)
5321786 coincontrol: Replace HasInputWeight with returning optional from Get (Andrew Chow)
e1abfb5 wallet: Introduce and use PreselectedInput class in CCoinControl (Andrew Chow)

Pull request description:

  Currently `FundTransaction` handles transaction locktime and preset input data by extracting the selected inputs and change output from `CreateTransaction`'s results. This means that `CreateTransaction` is actually unaware of any user desired locktime or sequence numbers. This can have an effect on whether and how anti-fee-sniping works.

  This PR makes `CreateTransaction` aware of the locktime and preset input data by providing them to `CCoinControl`. `CreateTransasction` will then set the sequences, scriptSigs, scriptWItnesses, and locktime as appropriate if they are specified. This allows `FundTransaction` to actually use `CreateTransaction`'s result directly instead of having to extract the parts of it that it wants.

  Additionally `FundTransaction` will return a `CreateTransactionResult` as `CreateTransaction` does instead of having several output parameters. Lastly, instead of using `-1` as a magic number for the change output position, the change position is changed to be an optional with no value set indicating no desired change output position (when provided as an input parameter) or no change output present (in the result).

ACKs for top commit:
  josibake:
    ACK bitcoin@0295b44
  S3RK:
    Code review ACK 0295b44

Tree-SHA512: 016be4d41cbf97e1938506e70959bb5335b87006162a1c1c62fa0adb637cbe7aefb76d342b8efad5f37dc693f270c8d0a0839e239fd1ac32c6941a8172f1a710

Author: fanquake

src/wallet/coincontrol.cpp

@@ -14,69 +14,142 @@ CCoinControl::CCoinControl()
 
 bool CCoinControl::HasSelected() const
 {
-    return !m_selected_inputs.empty();
+    return !m_selected.empty();
 }
 
-bool CCoinControl::IsSelected(const COutPoint& output) const
+bool CCoinControl::IsSelected(const COutPoint& outpoint) const
 {
-    return m_selected_inputs.count(output) > 0;
+    return m_selected.count(outpoint) > 0;
 }
 
-bool CCoinControl::IsExternalSelected(const COutPoint& output) const
+bool CCoinControl::IsExternalSelected(const COutPoint& outpoint) const
 {
-    return m_external_txouts.count(output) > 0;
+    const auto it = m_selected.find(outpoint);
+    return it != m_selected.end() && it->second.HasTxOut();
 }
 
 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()) {
+    const auto it = m_selected.find(outpoint);
+    if (it == m_selected.end() || !it->second.HasTxOut()) {
         return std::nullopt;
     }
+    return it->second.GetTxOut();
+}
 
-    return std::make_optional(ext_it->second);
+PreselectedInput& CCoinControl::Select(const COutPoint& outpoint)
+{
+    auto& input = m_selected[outpoint];
+    input.SetPosition(m_selection_pos);
+    ++m_selection_pos;
+    return input;
+}
+void CCoinControl::UnSelect(const COutPoint& outpoint)
+{
+    m_selected.erase(outpoint);
 }
 
-void CCoinControl::Select(const COutPoint& output)
+void CCoinControl::UnSelectAll()
 {
-    m_selected_inputs.insert(output);
+    m_selected.clear();
 }
 
-void CCoinControl::SelectExternal(const COutPoint& outpoint, const CTxOut& txout)
+std::vector<COutPoint> CCoinControl::ListSelected() const
 {
-    m_selected_inputs.insert(outpoint);
-    m_external_txouts.emplace(outpoint, txout);
+    std::vector<COutPoint> outpoints;
+    std::transform(m_selected.begin(), m_selected.end(), std::back_inserter(outpoints),
+        [](const std::map<COutPoint, PreselectedInput>::value_type& pair) {
+            return pair.first;
+        });
+    return outpoints;
 }
 
-void CCoinControl::UnSelect(const COutPoint& output)
+void CCoinControl::SetInputWeight(const COutPoint& outpoint, int64_t weight)
 {
-    m_selected_inputs.erase(output);
+    m_selected[outpoint].SetInputWeight(weight);
 }
 
-void CCoinControl::UnSelectAll()
+std::optional<int64_t> CCoinControl::GetInputWeight(const COutPoint& outpoint) const
 {
-    m_selected_inputs.clear();
+    const auto it = m_selected.find(outpoint);
+    return it != m_selected.end() ? it->second.GetInputWeight() : std::nullopt;
 }
 
-std::vector<COutPoint> CCoinControl::ListSelected() const
+std::optional<uint32_t> CCoinControl::GetSequence(const COutPoint& outpoint) const
 {
-    return {m_selected_inputs.begin(), m_selected_inputs.end()};
+    const auto it = m_selected.find(outpoint);
+    return it != m_selected.end() ? it->second.GetSequence() : std::nullopt;
 }
 
-void CCoinControl::SetInputWeight(const COutPoint& outpoint, int64_t weight)
+std::pair<std::optional<CScript>, std::optional<CScriptWitness>> CCoinControl::GetScripts(const COutPoint& outpoint) const
+{
+    const auto it = m_selected.find(outpoint);
+    return it != m_selected.end() ? m_selected.at(outpoint).GetScripts() : std::make_pair(std::nullopt, std::nullopt);
+}
+
+void PreselectedInput::SetTxOut(const CTxOut& txout)
+{
+    m_txout = txout;
+}
+
+CTxOut PreselectedInput::GetTxOut() const
+{
+    assert(m_txout.has_value());
+    return m_txout.value();
+}
+
+bool PreselectedInput::HasTxOut() const
+{
+    return m_txout.has_value();
+}
+
+void PreselectedInput::SetInputWeight(int64_t weight)
+{
+    m_weight = weight;
+}
+
+std::optional<int64_t> PreselectedInput::GetInputWeight() const
+{
+    return m_weight;
+}
+
+void PreselectedInput::SetSequence(uint32_t sequence)
+{
+    m_sequence = sequence;
+}
+
+std::optional<uint32_t> PreselectedInput::GetSequence() const
+{
+    return m_sequence;
+}
+
+void PreselectedInput::SetScriptSig(const CScript& script)
+{
+    m_script_sig = script;
+}
+
+void PreselectedInput::SetScriptWitness(const CScriptWitness& script_wit)
+{
+    m_script_witness = script_wit;
+}
+
+bool PreselectedInput::HasScripts() const
+{
+    return m_script_sig.has_value() || m_script_witness.has_value();
+}
+
+std::pair<std::optional<CScript>, std::optional<CScriptWitness>> PreselectedInput::GetScripts() const
 {
-    m_input_weights[outpoint] = weight;
+    return {m_script_sig, m_script_witness};
 }
 
-bool CCoinControl::HasInputWeight(const COutPoint& outpoint) const
+void PreselectedInput::SetPosition(unsigned int pos)
 {
-    return m_input_weights.count(outpoint) > 0;
+    m_pos = pos;
 }
 
-int64_t CCoinControl::GetInputWeight(const COutPoint& outpoint) const
+std::optional<unsigned int> PreselectedInput::GetPosition() const
 {
-    auto it = m_input_weights.find(outpoint);
-    assert(it != m_input_weights.end());
-    return it->second;
+    return m_pos;
 }
 } // namespace wallet

src/wallet/coincontrol.h

@@ -24,6 +24,58 @@ const int DEFAULT_MAX_DEPTH = 9999999;
 //! Default for -avoidpartialspends
 static constexpr bool DEFAULT_AVOIDPARTIALSPENDS = false;
 
+class PreselectedInput
+{
+private:
+    //! The previous output being spent by this input
+    std::optional<CTxOut> m_txout;
+    //! The input weight for spending this input
+    std::optional<int64_t> m_weight;
+    //! The sequence number for this input
+    std::optional<uint32_t> m_sequence;
+    //! The scriptSig for this input
+    std::optional<CScript> m_script_sig;
+    //! The scriptWitness for this input
+    std::optional<CScriptWitness> m_script_witness;
+    //! The position in the inputs vector for this input
+    std::optional<unsigned int> m_pos;
+
+public:
+    /**
+     * Set the previous output for this input.
+     * Only necessary if the input is expected to be an external input.
+     */
+    void SetTxOut(const CTxOut& txout);
+    /** Retrieve the previous output for this input. */
+    CTxOut GetTxOut() const;
+    /** Return whether the previous output is set for this input. */
+    bool HasTxOut() const;
+
+    /** Set the weight for this input. */
+    void SetInputWeight(int64_t weight);
+    /** Retrieve the input weight for this input. */
+    std::optional<int64_t> GetInputWeight() const;
+
+    /** Set the sequence for this input. */
+    void SetSequence(uint32_t sequence);
+    /** Retrieve the sequence for this input. */
+    std::optional<uint32_t> GetSequence() const;
+
+    /** Set the scriptSig for this input. */
+    void SetScriptSig(const CScript& script);
+    /** Set the scriptWitness for this input. */
+    void SetScriptWitness(const CScriptWitness& script_wit);
+    /** Return whether either the scriptSig or scriptWitness are set for this input. */
+    bool HasScripts() const;
+    /** Retrieve both the scriptSig and the scriptWitness. */
+    std::pair<std::optional<CScript>, std::optional<CScriptWitness>> GetScripts() const;
+
+    /** Store the position of this input. */
+    void SetPosition(unsigned int pos);
+    /** Retrieve the position of this input. */
+    std::optional<unsigned int> GetPosition() const;
+};
+
 /** Coin Control Features. */
 class CCoinControl
 {
@@ -59,6 +111,10 @@ class CCoinControl
     int m_max_depth = DEFAULT_MAX_DEPTH;
     //! SigningProvider that has pubkeys and scripts to do spend size estimation for external inputs
     FlatSigningProvider m_external_provider;
+    //! Locktime
+    std::optional<uint32_t> m_locktime;
+    //! Version
+    std::optional<uint32_t> m_version;
 
     CCoinControl();
 
@@ -69,11 +125,11 @@ class CCoinControl
     /**
      * Returns true if the given output is pre-selected.
      */
-    bool IsSelected(const COutPoint& output) const;
+    bool IsSelected(const COutPoint& outpoint) const;
     /**
      * Returns true if the given output is selected as an external input.
      */
-    bool IsExternalSelected(const COutPoint& output) const;
+    bool IsExternalSelected(const COutPoint& outpoint) const;
     /**
      * Returns the external output for the given outpoint if it exists.
      */
@@ -82,16 +138,11 @@ class CCoinControl
      * 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);
+    PreselectedInput& Select(const COutPoint& outpoint);
     /**
      * Unselects the given output.
      */
-    void UnSelect(const COutPoint& output);
+    void UnSelect(const COutPoint& outpoint);
     /**
      * Unselects all outputs.
      */
@@ -104,23 +155,33 @@ class CCoinControl
      * 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;
+    std::optional<int64_t> GetInputWeight(const COutPoint& outpoint) const;
+    /** Retrieve the sequence for an input */
+    std::optional<uint32_t> GetSequence(const COutPoint& outpoint) const;
+    /** Retrieves the scriptSig and scriptWitness for an input. */
+    std::pair<std::optional<CScript>, std::optional<CScriptWitness>> GetScripts(const COutPoint& outpoint) const;
+
+    bool HasSelectedOrder() const
+    {
+        return m_selection_pos > 0;
+    }
+
+    std::optional<unsigned int> GetSelectionPos(const COutPoint& outpoint) const
+    {
+        const auto it = m_selected.find(outpoint);
+        if (it == m_selected.end()) {
+            return std::nullopt;
+        }
+        return it->second.GetPosition();
+    }
 
 private:
     //! 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;
+    std::map<COutPoint, PreselectedInput> m_selected;
+    unsigned int m_selection_pos{0};
 };
 } // namespace wallet
 

src/wallet/feebumper.cpp

@@ -203,10 +203,9 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
             errors.push_back(Untranslated(strprintf("%s:%u is already spent", txin.prevout.hash.GetHex(), txin.prevout.n)));
             return Result::MISC_ERROR;
         }
-        if (wallet.IsMine(txin.prevout)) {
-            new_coin_control.Select(txin.prevout);
-        } else {
-            new_coin_control.SelectExternal(txin.prevout, coin.out);
+        PreselectedInput& preset_txin = new_coin_control.Select(txin.prevout);
+        if (!wallet.IsMine(txin.prevout)) {
+            preset_txin.SetTxOut(coin.out);
         }
         input_value += coin.out.nValue;
         spent_outputs.push_back(coin.out);
@@ -317,8 +316,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
     // We cannot source new unconfirmed inputs(bip125 rule 2)
     new_coin_control.m_min_depth = 1;
 
-    constexpr int RANDOM_CHANGE_POSITION = -1;
-    auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, new_coin_control, false);
+    auto res = CreateTransaction(wallet, recipients, std::nullopt, new_coin_control, false);
     if (!res) {
         errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res));
         return Result::WALLET_ERROR;

src/wallet/interfaces.cpp

@@ -281,12 +281,12 @@ class WalletImpl : public Wallet
         CAmount& fee) override
     {
         LOCK(m_wallet->cs_wallet);
-        auto res = CreateTransaction(*m_wallet, recipients, change_pos,
+        auto res = CreateTransaction(*m_wallet, recipients, change_pos == -1 ? std::nullopt : std::make_optional(change_pos),
                                      coin_control, sign);
         if (!res) return util::Error{util::ErrorString(res)};
         const auto& txr = *res;
         fee = txr.fee;
-        change_pos = txr.change_pos;
+        change_pos = txr.change_pos ? *txr.change_pos : -1;
 
         return txr.tx;
     }