From e1abfb5b2037ca4fe5a05aa578030c8016491c8b Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Tue, 31 May 2022 17:39:19 -0400
Subject: [PATCH 1/9] wallet: Introduce and use PreselectedInput class in
 CCoinControl

Instead of having different maps for selected inputs, external inputs,
and input weight in CCoinControl, have a class PreselectedInput which
tracks stores that information for each input.
---
 src/wallet/coincontrol.cpp | 79 +++++++++++++++++++++++++++-----------
 src/wallet/coincontrol.h   | 42 +++++++++++++++-----
 2 files changed, 89 insertions(+), 32 deletions(-)

diff --git a/src/wallet/coincontrol.cpp b/src/wallet/coincontrol.cpp
index 2087119db90..a4f3bdac233 100644
--- a/src/wallet/coincontrol.cpp
+++ b/src/wallet/coincontrol.cpp
@@ -14,69 +14,104 @@ 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 std::make_optional(ext_it->second);
+    return it->second.GetTxOut();
 }
 
-void CCoinControl::Select(const COutPoint& output)
+PreselectedInput& CCoinControl::Select(const COutPoint& outpoint)
 {
-    m_selected_inputs.insert(output);
+    return m_selected[outpoint];
 }
 
 void CCoinControl::SelectExternal(const COutPoint& outpoint, const CTxOut& txout)
 {
-    m_selected_inputs.insert(outpoint);
-    m_external_txouts.emplace(outpoint, txout);
+    m_selected[outpoint].SetTxOut(txout);
 }
 
-void CCoinControl::UnSelect(const COutPoint& output)
+void CCoinControl::UnSelect(const COutPoint& outpoint)
 {
-    m_selected_inputs.erase(output);
+    m_selected.erase(outpoint);
 }
 
 void CCoinControl::UnSelectAll()
 {
-    m_selected_inputs.clear();
+    m_selected.clear();
 }
 
 std::vector<COutPoint> CCoinControl::ListSelected() const
 {
-    return {m_selected_inputs.begin(), m_selected_inputs.end()};
+    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::SetInputWeight(const COutPoint& outpoint, int64_t weight)
 {
-    m_input_weights[outpoint] = weight;
+    m_selected[outpoint].SetInputWeight(weight);
 }
 
 bool CCoinControl::HasInputWeight(const COutPoint& outpoint) const
 {
-    return m_input_weights.count(outpoint) > 0;
+    const auto it = m_selected.find(outpoint);
+    return it != m_selected.end() && it->second.HasInputWeight();
 }
 
 int64_t CCoinControl::GetInputWeight(const COutPoint& outpoint) const
 {
-    auto it = m_input_weights.find(outpoint);
-    assert(it != m_input_weights.end());
-    return it->second;
+    return m_selected.at(outpoint).GetInputWeight();
+}
+
+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;
+}
+
+int64_t PreselectedInput::GetInputWeight() const
+{
+    assert(m_weight.has_value());
+    return m_weight.value();
+}
+
+bool PreselectedInput::HasInputWeight() const
+{
+    return m_weight.has_value();
 }
 } // namespace wallet
diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h
index 71593e236f7..4f87d13d998 100644
--- a/src/wallet/coincontrol.h
+++ b/src/wallet/coincontrol.h
@@ -24,6 +24,33 @@ 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;
+
+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. */
+    int64_t GetInputWeight() const;
+    /** Return whether the input weight is set. */
+    bool HasInputWeight() const;
+};
+
 /** Coin Control Features. */
 class CCoinControl
 {
@@ -69,11 +96,11 @@ public:
     /**
      * 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,7 +109,7 @@ public:
      * 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);
+    PreselectedInput& Select(const COutPoint& outpoint);
     /**
      * 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.
@@ -91,7 +118,7 @@ public:
     /**
      * Unselects the given output.
      */
-    void UnSelect(const COutPoint& output);
+    void UnSelect(const COutPoint& outpoint);
     /**
      * Unselects all outputs.
      */
@@ -115,12 +142,7 @@ public:
 
 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;
 };
 } // namespace wallet
 

From 5321786b9d90eaf35059bb07e6beaaa2cbb257ac Mon Sep 17 00:00:00 2001
From: Andrew Chow <github@achow101.com>
Date: Mon, 23 Jan 2023 16:59:25 -0500
Subject: [PATCH 2/9] coincontrol: Replace HasInputWeight with returning
 optional from Get

---
 src/wallet/coincontrol.cpp           | 19 ++++---------------
 src/wallet/coincontrol.h             | 10 ++--------
 src/wallet/spend.cpp                 | 19 ++++++++++---------
 src/wallet/test/fuzz/coincontrol.cpp |  5 +----
 4 files changed, 17 insertions(+), 36 deletions(-)

diff --git a/src/wallet/coincontrol.cpp b/src/wallet/coincontrol.cpp
index a4f3bdac233..94218b45c11 100644
--- a/src/wallet/coincontrol.cpp
+++ b/src/wallet/coincontrol.cpp
@@ -72,15 +72,10 @@ void CCoinControl::SetInputWeight(const COutPoint& outpoint, int64_t weight)
     m_selected[outpoint].SetInputWeight(weight);
 }
 
-bool CCoinControl::HasInputWeight(const COutPoint& outpoint) const
+std::optional<int64_t> CCoinControl::GetInputWeight(const COutPoint& outpoint) const
 {
     const auto it = m_selected.find(outpoint);
-    return it != m_selected.end() && it->second.HasInputWeight();
-}
-
-int64_t CCoinControl::GetInputWeight(const COutPoint& outpoint) const
-{
-    return m_selected.at(outpoint).GetInputWeight();
+    return it != m_selected.end() ? it->second.GetInputWeight() : std::nullopt;
 }
 
 void PreselectedInput::SetTxOut(const CTxOut& txout)
@@ -104,14 +99,8 @@ void PreselectedInput::SetInputWeight(int64_t weight)
     m_weight = weight;
 }
 
-int64_t PreselectedInput::GetInputWeight() const
+std::optional<int64_t> PreselectedInput::GetInputWeight() const
 {
-    assert(m_weight.has_value());
-    return m_weight.value();
-}
-
-bool PreselectedInput::HasInputWeight() const
-{
-    return m_weight.has_value();
+    return m_weight;
 }
 } // namespace wallet
diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h
index 4f87d13d998..1d1f5c4da25 100644
--- a/src/wallet/coincontrol.h
+++ b/src/wallet/coincontrol.h
@@ -46,9 +46,7 @@ public:
     /** Set the weight for this input. */
     void SetInputWeight(int64_t weight);
     /** Retrieve the input weight for this input. */
-    int64_t GetInputWeight() const;
-    /** Return whether the input weight is set. */
-    bool HasInputWeight() const;
+    std::optional<int64_t> GetInputWeight() const;
 };
 
 /** Coin Control Features. */
@@ -131,14 +129,10 @@ public:
      * 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;
 
 private:
     //! Selected inputs (inputs that will be used, regardless of whether they're optimal or not)
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 35583642a5a..2fb238fccdc 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -117,8 +117,9 @@ static std::optional<int64_t> GetSignedTxinWeight(const CWallet* wallet, const C
                                                   const bool can_grind_r)
 {
     // If weight was provided, use that.
-    if (coin_control && coin_control->HasInputWeight(txin.prevout)) {
-        return coin_control->GetInputWeight(txin.prevout);
+    std::optional<int64_t> weight;
+    if (coin_control && (weight = coin_control->GetInputWeight(txin.prevout))) {
+        return weight.value();
     }
 
     // Otherwise, use the maximum satisfaction size provided by the descriptor.
@@ -261,7 +262,10 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
     const bool can_grind_r = wallet.CanGrindR();
     std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().calculateIndividualBumpFees(coin_control.ListSelected(), coin_selection_params.m_effective_feerate);
     for (const COutPoint& outpoint : coin_control.ListSelected()) {
-        int input_bytes = -1;
+        int64_t input_bytes = coin_control.GetInputWeight(outpoint).value_or(-1);
+        if (input_bytes != -1) {
+            input_bytes = GetVirtualTransactionSize(input_bytes, 0, 0);
+        }
         CTxOut txout;
         if (auto ptr_wtx = wallet.GetWalletTx(outpoint.hash)) {
             // Clearly invalid input, fail
@@ -269,7 +273,9 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
                 return util::Error{strprintf(_("Invalid pre-selected input %s"), outpoint.ToString())};
             }
             txout = ptr_wtx->tx->vout.at(outpoint.n);
-            input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control);
+            if (input_bytes == -1) {
+                input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control);
+            }
         } else {
             // The input is external. We did not find the tx in mapWallet.
             const auto out{coin_control.GetExternalOutput(outpoint)};
@@ -284,11 +290,6 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
             input_bytes = CalculateMaximumSignedInputSize(txout, outpoint, &coin_control.m_external_provider, can_grind_r, &coin_control);
         }
 
-        // If available, override calculated size with coin control specified size
-        if (coin_control.HasInputWeight(outpoint)) {
-            input_bytes = GetVirtualTransactionSize(coin_control.GetInputWeight(outpoint), 0, 0);
-        }
-
         if (input_bytes == -1) {
             return util::Error{strprintf(_("Not solvable pre-selected input %s"), outpoint.ToString())}; // Not solvable, can't estimate size for fee
         }
diff --git a/src/wallet/test/fuzz/coincontrol.cpp b/src/wallet/test/fuzz/coincontrol.cpp
index 0f71f28df2b..64ed4f245d4 100644
--- a/src/wallet/test/fuzz/coincontrol.cpp
+++ b/src/wallet/test/fuzz/coincontrol.cpp
@@ -76,10 +76,7 @@ FUZZ_TARGET(coincontrol, .init = initialize_coincontrol)
                 (void)coin_control.SetInputWeight(out_point, weight);
             },
             [&] {
-                // Condition to avoid the assertion in GetInputWeight
-                if (coin_control.HasInputWeight(out_point)) {
-                    (void)coin_control.GetInputWeight(out_point);
-                }
+                (void)coin_control.GetInputWeight(out_point);
             });
     }
 }

From 596642c5a9f52dda2599b0bde424366bb22b3c6e Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Mon, 20 Jun 2022 12:03:03 -0400
Subject: [PATCH 3/9] wallet: Replace SelectExternal with SetTxOut

Instead of having a separate CCoinControl::SelectExternal function, we
can use the normal CCoinControl::Select function and explicitly use
PreselectedInput::SetTxOut in the caller. The semantics of what an
external input is remains.
---
 src/wallet/coincontrol.cpp             |  6 ------
 src/wallet/coincontrol.h               |  5 -----
 src/wallet/feebumper.cpp               |  7 +++----
 src/wallet/spend.cpp                   | 16 ++++++++--------
 src/wallet/test/coinselector_tests.cpp |  2 +-
 src/wallet/test/fuzz/coincontrol.cpp   |  2 +-
 6 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/src/wallet/coincontrol.cpp b/src/wallet/coincontrol.cpp
index 94218b45c11..00852b5f857 100644
--- a/src/wallet/coincontrol.cpp
+++ b/src/wallet/coincontrol.cpp
@@ -41,12 +41,6 @@ PreselectedInput& CCoinControl::Select(const COutPoint& outpoint)
 {
     return m_selected[outpoint];
 }
-
-void CCoinControl::SelectExternal(const COutPoint& outpoint, const CTxOut& txout)
-{
-    m_selected[outpoint].SetTxOut(txout);
-}
-
 void CCoinControl::UnSelect(const COutPoint& outpoint)
 {
     m_selected.erase(outpoint);
diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h
index 1d1f5c4da25..e7a16e37df5 100644
--- a/src/wallet/coincontrol.h
+++ b/src/wallet/coincontrol.h
@@ -108,11 +108,6 @@ public:
      * The output will be included in the transaction even if it's not the most optimal choice.
      */
     PreselectedInput& Select(const COutPoint& outpoint);
-    /**
-     * 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.
      */
diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp
index d9a08310a87..d3df9ceefff 100644
--- a/src/wallet/feebumper.cpp
+++ b/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);
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 2fb238fccdc..c3fd695eb96 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -1347,15 +1347,15 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
 
     for (const CTxIn& txin : tx.vin) {
         const auto& outPoint = txin.prevout;
-        if (wallet.IsMine(outPoint)) {
-            // The input was found in the wallet, so select as internal
-            coinControl.Select(outPoint);
-        } else if (coins[outPoint].out.IsNull()) {
-            error = _("Unable to find UTXO for external input");
-            return false;
-        } else {
+        PreselectedInput& preset_txin = coinControl.Select(outPoint);
+        if (!wallet.IsMine(outPoint)) {
+            if (coins[outPoint].out.IsNull()) {
+                error = _("Unable to find UTXO for external input");
+                return false;
+            }
+
             // The input was not in the wallet, but is in the UTXO set, so select as external
-            coinControl.SelectExternal(outPoint, coins[outPoint].out);
+            preset_txin.SetTxOut(coins[outPoint].out);
         }
     }
 
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index 9569210ba06..fa0dfa55569 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -1282,7 +1282,7 @@ BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test)
     cc.m_allow_other_inputs = false;
     COutput output = available_coins.All().at(0);
     cc.SetInputWeight(output.outpoint, 148);
-    cc.SelectExternal(output.outpoint, output.txout);
+    cc.Select(output.outpoint).SetTxOut(output.txout);
 
     LOCK(wallet->cs_wallet);
     const auto preset_inputs = *Assert(FetchSelectedInputs(*wallet, cc, cs_params));
diff --git a/src/wallet/test/fuzz/coincontrol.cpp b/src/wallet/test/fuzz/coincontrol.cpp
index 64ed4f245d4..f1efbc1cb81 100644
--- a/src/wallet/test/fuzz/coincontrol.cpp
+++ b/src/wallet/test/fuzz/coincontrol.cpp
@@ -60,7 +60,7 @@ FUZZ_TARGET(coincontrol, .init = initialize_coincontrol)
             },
             [&] {
                 const CTxOut tx_out{ConsumeMoney(fuzzed_data_provider), ConsumeScript(fuzzed_data_provider)};
-                (void)coin_control.SelectExternal(out_point, tx_out);
+                (void)coin_control.Select(out_point).SetTxOut(tx_out);
             },
             [&] {
                 (void)coin_control.UnSelect(out_point);

From 4d335bb1e00a414a4740007d5a192a73179b2262 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Tue, 31 May 2022 23:51:49 -0400
Subject: [PATCH 4/9] wallet: Set preset input sequence through coin control

---
 src/wallet/coincontrol.cpp | 16 ++++++++++++++++
 src/wallet/coincontrol.h   |  9 +++++++++
 src/wallet/spend.cpp       | 15 ++++++++++++---
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/src/wallet/coincontrol.cpp b/src/wallet/coincontrol.cpp
index 00852b5f857..f13b7073bee 100644
--- a/src/wallet/coincontrol.cpp
+++ b/src/wallet/coincontrol.cpp
@@ -72,6 +72,12 @@ std::optional<int64_t> CCoinControl::GetInputWeight(const COutPoint& outpoint) c
     return it != m_selected.end() ? it->second.GetInputWeight() : std::nullopt;
 }
 
+std::optional<uint32_t> CCoinControl::GetSequence(const COutPoint& outpoint) const
+{
+    const auto it = m_selected.find(outpoint);
+    return it != m_selected.end() ? it->second.GetSequence() : std::nullopt;
+}
+
 void PreselectedInput::SetTxOut(const CTxOut& txout)
 {
     m_txout = txout;
@@ -97,4 +103,14 @@ 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;
+}
 } // namespace wallet
diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h
index e7a16e37df5..32595955f05 100644
--- a/src/wallet/coincontrol.h
+++ b/src/wallet/coincontrol.h
@@ -31,6 +31,8 @@ private:
     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;
 
 public:
     /**
@@ -47,6 +49,11 @@ public:
     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;
 };
 
 /** Coin Control Features. */
@@ -128,6 +135,8 @@ public:
      * Returns the input weight.
      */
     std::optional<int64_t> GetInputWeight(const COutPoint& outpoint) const;
+    /** Retrieve the sequence for an input */
+    std::optional<uint32_t> GetSequence(const COutPoint& outpoint) const;
 
 private:
     //! Selected inputs (inputs that will be used, regardless of whether they're optimal or not)
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index c3fd695eb96..7ec2775fb12 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -1150,11 +1150,19 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     // to avoid conflicting with other possible uses of nSequence,
     // and in the spirit of "smallest possible change from prior
     // behavior."
-    const uint32_t nSequence{coin_control.m_signal_bip125_rbf.value_or(wallet.m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL};
+    bool use_anti_fee_sniping = true;
+    const uint32_t default_sequence{coin_control.m_signal_bip125_rbf.value_or(wallet.m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL};
     for (const auto& coin : selected_coins) {
-        txNew.vin.emplace_back(coin->outpoint, CScript(), nSequence);
+        std::optional<uint32_t> sequence = coin_control.GetSequence(coin->outpoint);
+        if (sequence) {
+            // If an input has a preset sequence, we can't do anti-fee-sniping
+            use_anti_fee_sniping = false;
+        }
+        txNew.vin.emplace_back(coin->outpoint, CScript(), sequence.value_or(default_sequence));
+    }
+    if (use_anti_fee_sniping) {
+        DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight());
     }
-    DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight());
 
     // Calculate the transaction fee
     TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control);
@@ -1357,6 +1365,7 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
             // The input was not in the wallet, but is in the UTXO set, so select as external
             preset_txin.SetTxOut(coins[outPoint].out);
         }
+        preset_txin.SetSequence(txin.nSequence);
     }
 
     auto res = CreateTransaction(wallet, vecSend, nChangePosInOut, coinControl, false);

From 0fefcbb776063b7fcc03c28e544d830a2f540250 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Wed, 1 Jun 2022 12:35:00 -0400
Subject: [PATCH 5/9] wallet: Explicitly preserve transaction locktime in
 CreateTransaction

We provide the preset nLockTime to CCoinControl so that
CreateTransactionInternal can be aware of it and set it in the produced
transaction.
---
 src/wallet/coincontrol.h | 2 ++
 src/wallet/spend.cpp     | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h
index 32595955f05..c99a3bfd0f6 100644
--- a/src/wallet/coincontrol.h
+++ b/src/wallet/coincontrol.h
@@ -91,6 +91,8 @@ public:
     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;
 
     CCoinControl();
 
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 7ec2775fb12..9dfc8c2879a 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -1160,6 +1160,11 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
         }
         txNew.vin.emplace_back(coin->outpoint, CScript(), sequence.value_or(default_sequence));
     }
+    if (coin_control.m_locktime) {
+        txNew.nLockTime = coin_control.m_locktime.value();
+        // If we have a locktime set, we can't use anti-fee-sniping
+        use_anti_fee_sniping = false;
+    }
     if (use_anti_fee_sniping) {
         DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight());
     }
@@ -1341,6 +1346,9 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
         vecSend.push_back(recipient);
     }
 
+    // Set the user desired locktime
+    coinControl.m_locktime = tx.nLockTime;
+
     // Acquire the locks to prevent races to the new locked unspents between the
     // CreateTransaction call and LockCoin calls (when lockUnspents is true).
     LOCK(wallet.cs_wallet);

From 14e50746f683361f4d511d384d6f1dc44ed2bf10 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Wed, 1 Jun 2022 14:14:17 -0400
Subject: [PATCH 6/9] wallet: Explicitly preserve transaction version in
 CreateTransaction

We provide the preset nVersion to CCoinControl so that
CreateTransactionInternal can be aware of it and set it in the produced
transaction.
---
 src/wallet/coincontrol.h | 2 ++
 src/wallet/spend.cpp     | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h
index c99a3bfd0f6..66f9dcb53c0 100644
--- a/src/wallet/coincontrol.h
+++ b/src/wallet/coincontrol.h
@@ -93,6 +93,8 @@ public:
     FlatSigningProvider m_external_provider;
     //! Locktime
     std::optional<uint32_t> m_locktime;
+    //! Version
+    std::optional<uint32_t> m_version;
 
     CCoinControl();
 
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 9dfc8c2879a..c560c711373 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -977,6 +977,10 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     FastRandomContext rng_fast;
     CMutableTransaction txNew; // The resulting transaction that we make
 
+    if (coin_control.m_version) {
+        txNew.nVersion = coin_control.m_version.value();
+    }
+
     CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy
     coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
     coin_selection_params.m_include_unsafe_inputs = coin_control.m_include_unsafe_inputs;
@@ -1349,6 +1353,9 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
     // Set the user desired locktime
     coinControl.m_locktime = tx.nLockTime;
 
+    // Set the user desired version
+    coinControl.m_version = tx.nVersion;
+
     // Acquire the locks to prevent races to the new locked unspents between the
     // CreateTransaction call and LockCoin calls (when lockUnspents is true).
     LOCK(wallet.cs_wallet);

From 2d39db7aa128a948b6ad11242591ef26a342f5b1 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Wed, 1 Jun 2022 17:39:58 -0400
Subject: [PATCH 7/9] wallet: Explicitly preserve scriptSig and scriptWitness
 in CreateTransaction

When creating a transaction with preset inputs, also preserve the
scriptSig and scriptWitness for those preset inputs if they are provided
(e.g. in fundrawtransaction).
---
 src/wallet/coincontrol.cpp | 41 +++++++++++++++++++++++++++++++++++++-
 src/wallet/coincontrol.h   | 37 ++++++++++++++++++++++++++++++++++
 src/wallet/spend.cpp       | 31 +++++++++++++++++++++++++++-
 3 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/src/wallet/coincontrol.cpp b/src/wallet/coincontrol.cpp
index f13b7073bee..873c5ab3837 100644
--- a/src/wallet/coincontrol.cpp
+++ b/src/wallet/coincontrol.cpp
@@ -39,7 +39,10 @@ std::optional<CTxOut> CCoinControl::GetExternalOutput(const COutPoint& outpoint)
 
 PreselectedInput& CCoinControl::Select(const COutPoint& outpoint)
 {
-    return m_selected[outpoint];
+    auto& input = m_selected[outpoint];
+    input.SetPosition(m_selection_pos);
+    ++m_selection_pos;
+    return input;
 }
 void CCoinControl::UnSelect(const COutPoint& outpoint)
 {
@@ -78,6 +81,12 @@ std::optional<uint32_t> CCoinControl::GetSequence(const COutPoint& outpoint) con
     return it != m_selected.end() ? it->second.GetSequence() : std::nullopt;
 }
 
+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;
@@ -113,4 +122,34 @@ 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
+{
+    return {m_script_sig, m_script_witness};
+}
+
+void PreselectedInput::SetPosition(unsigned int pos)
+{
+    m_pos = pos;
+}
+
+std::optional<unsigned int> PreselectedInput::GetPosition() const
+{
+    return m_pos;
+}
 } // namespace wallet
diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h
index 66f9dcb53c0..b2f813383dc 100644
--- a/src/wallet/coincontrol.h
+++ b/src/wallet/coincontrol.h
@@ -33,6 +33,12 @@ private:
     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:
     /**
@@ -54,6 +60,20 @@ public:
     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. */
@@ -141,10 +161,27 @@ public:
     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::map<COutPoint, PreselectedInput> m_selected;
+    unsigned int m_selection_pos{0};
 };
 } // namespace wallet
 
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index c560c711373..72ec719f226 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -1146,6 +1146,25 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     // Shuffle selected coins and fill in final vin
     std::vector<std::shared_ptr<COutput>> selected_coins = result.GetShuffledInputVector();
 
+    if (coin_control.HasSelected() && coin_control.HasSelectedOrder()) {
+        // When there are preselected inputs, we need to move them to be the first UTXOs
+        // and have them be in the order selected. We can use stable_sort for this, where we
+        // compare with the positions stored in coin_control. The COutputs that have positions
+        // will be placed before those that don't, and those positions will be in order.
+        std::stable_sort(selected_coins.begin(), selected_coins.end(),
+            [&coin_control](const std::shared_ptr<COutput>& a, const std::shared_ptr<COutput>& b) {
+                auto a_pos = coin_control.GetSelectionPos(a->outpoint);
+                auto b_pos = coin_control.GetSelectionPos(b->outpoint);
+                if (a_pos.has_value() && b_pos.has_value()) {
+                    return a_pos.value() < b_pos.value();
+                } else if (a_pos.has_value() && !b_pos.has_value()) {
+                    return true;
+                } else {
+                    return false;
+                }
+            });
+    }
+
     // The sequence number is set to non-maxint so that DiscourageFeeSniping
     // works.
     //
@@ -1162,7 +1181,15 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
             // If an input has a preset sequence, we can't do anti-fee-sniping
             use_anti_fee_sniping = false;
         }
-        txNew.vin.emplace_back(coin->outpoint, CScript(), sequence.value_or(default_sequence));
+        txNew.vin.emplace_back(coin->outpoint, CScript{}, sequence.value_or(default_sequence));
+
+        auto scripts = coin_control.GetScripts(coin->outpoint);
+        if (scripts.first) {
+            txNew.vin.back().scriptSig = *scripts.first;
+        }
+        if (scripts.second) {
+            txNew.vin.back().scriptWitness = *scripts.second;
+        }
     }
     if (coin_control.m_locktime) {
         txNew.nLockTime = coin_control.m_locktime.value();
@@ -1381,6 +1408,8 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
             preset_txin.SetTxOut(coins[outPoint].out);
         }
         preset_txin.SetSequence(txin.nSequence);
+        preset_txin.SetScriptSig(txin.scriptSig);
+        preset_txin.SetScriptWitness(txin.scriptWitness);
     }
 
     auto res = CreateTransaction(wallet, vecSend, nChangePosInOut, coinControl, false);

From 758501b71391136c33b525b1a0109b990d4f463e Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Wed, 1 Jun 2022 14:32:26 -0400
Subject: [PATCH 8/9] wallet: use optional for change position as an optional
 in CreateTransaction

Instead of making -1 a magic number meaning no change or random change
position, use an optional to have that meaning.
---
 src/wallet/feebumper.cpp         |  3 +--
 src/wallet/interfaces.cpp        |  4 +--
 src/wallet/rpc/spend.cpp         |  3 +--
 src/wallet/spend.cpp             | 42 +++++++++++++++-----------------
 src/wallet/spend.h               |  6 ++---
 src/wallet/test/spend_tests.cpp  |  3 +--
 src/wallet/test/wallet_tests.cpp |  3 +--
 7 files changed, 28 insertions(+), 36 deletions(-)

diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp
index d3df9ceefff..c6ed0fe11cf 100644
--- a/src/wallet/feebumper.cpp
+++ b/src/wallet/feebumper.cpp
@@ -316,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;
diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
index 4ca5e292294..d15273dfc9b 100644
--- a/src/wallet/interfaces.cpp
+++ b/src/wallet/interfaces.cpp
@@ -281,12 +281,12 @@ public:
         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;
     }
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index e7884af8b08..7cde644d9bc 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -155,8 +155,7 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto
     std::shuffle(recipients.begin(), recipients.end(), FastRandomContext());
 
     // Send
-    constexpr int RANDOM_CHANGE_POSITION = -1;
-    auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, coin_control, true);
+    auto res = CreateTransaction(wallet, recipients, std::nullopt, coin_control, true);
     if (!res) {
         throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, util::ErrorString(res).original);
     }
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 72ec719f226..f911e52ee18 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -965,15 +965,12 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng
 static util::Result<CreatedTransactionResult> CreateTransactionInternal(
         CWallet& wallet,
         const std::vector<CRecipient>& vecSend,
-        int change_pos,
+        std::optional<unsigned int> change_pos,
         const CCoinControl& coin_control,
         bool sign) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
 {
     AssertLockHeld(wallet.cs_wallet);
 
-    // out variables, to be packed into returned result structure
-    int nChangePosInOut = change_pos;
-
     FastRandomContext rng_fast;
     CMutableTransaction txNew; // The resulting transaction that we make
 
@@ -1132,15 +1129,15 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     const CAmount change_amount = result.GetChange(coin_selection_params.min_viable_change, coin_selection_params.m_change_fee);
     if (change_amount > 0) {
         CTxOut newTxOut(change_amount, scriptChange);
-        if (nChangePosInOut == -1) {
+        if (!change_pos) {
             // Insert change txn at random position:
-            nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1);
-        } else if ((unsigned int)nChangePosInOut > txNew.vout.size()) {
+            change_pos = rng_fast.randrange(txNew.vout.size() + 1);
+        } else if ((unsigned int)*change_pos > txNew.vout.size()) {
             return util::Error{_("Transaction change output index out of range")};
         }
-        txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut);
+        txNew.vout.insert(txNew.vout.begin() + *change_pos, newTxOut);
     } else {
-        nChangePosInOut = -1;
+        change_pos = std::nullopt;
     }
 
     // Shuffle selected coins and fill in final vin
@@ -1217,8 +1214,8 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     }
 
     // If there is a change output and we overpay the fees then increase the change to match the fee needed
-    if (nChangePosInOut != -1 && fee_needed < current_fee) {
-        auto& change = txNew.vout.at(nChangePosInOut);
+    if (change_pos && fee_needed < current_fee) {
+        auto& change = txNew.vout.at(*change_pos);
         change.nValue += current_fee - fee_needed;
         current_fee = result.GetSelectedValue() - CalculateOutputValue(txNew);
         if (fee_needed != current_fee) {
@@ -1229,11 +1226,11 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     // Reduce output values for subtractFeeFromAmount
     if (coin_selection_params.m_subtract_fee_outputs) {
         CAmount to_reduce = fee_needed - current_fee;
-        int i = 0;
+        unsigned int i = 0;
         bool fFirst = true;
         for (const auto& recipient : vecSend)
         {
-            if (i == nChangePosInOut) {
+            if (change_pos && i == *change_pos) {
                 ++i;
             }
             CTxOut& txout = txNew.vout[i];
@@ -1272,7 +1269,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     }
 
     // Give up if change keypool ran out and change is required
-    if (scriptChange.empty() && nChangePosInOut != -1) {
+    if (scriptChange.empty() && change_pos) {
         return util::Error{error};
     }
 
@@ -1313,13 +1310,13 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
               feeCalc.est.fail.start, feeCalc.est.fail.end,
               (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) > 0.0 ? 100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) : 0.0,
               feeCalc.est.fail.withinTarget, feeCalc.est.fail.totalConfirmed, feeCalc.est.fail.inMempool, feeCalc.est.fail.leftMempool);
-    return CreatedTransactionResult(tx, current_fee, nChangePosInOut, feeCalc);
+    return CreatedTransactionResult(tx, current_fee, change_pos, feeCalc);
 }
 
 util::Result<CreatedTransactionResult> CreateTransaction(
         CWallet& wallet,
         const std::vector<CRecipient>& vecSend,
-        int change_pos,
+        std::optional<unsigned int> change_pos,
         const CCoinControl& coin_control,
         bool sign)
 {
@@ -1335,7 +1332,7 @@ util::Result<CreatedTransactionResult> CreateTransaction(
 
     auto res = CreateTransactionInternal(wallet, vecSend, change_pos, coin_control, sign);
     TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), bool(res),
-           res ? res->fee : 0, res ? res->change_pos : 0);
+           res ? res->fee : 0, res && res->change_pos.has_value() ? *res->change_pos : 0);
     if (!res) return res;
     const auto& txr_ungrouped = *res;
     // try with avoidpartialspends unless it's enabled already
@@ -1345,16 +1342,15 @@ util::Result<CreatedTransactionResult> CreateTransaction(
         tmp_cc.m_avoid_partial_spends = true;
 
         // Reuse the change destination from the first creation attempt to avoid skipping BIP44 indexes
-        const int ungrouped_change_pos = txr_ungrouped.change_pos;
-        if (ungrouped_change_pos != -1) {
-            ExtractDestination(txr_ungrouped.tx->vout[ungrouped_change_pos].scriptPubKey, tmp_cc.destChange);
+        if (txr_ungrouped.change_pos) {
+            ExtractDestination(txr_ungrouped.tx->vout[*txr_ungrouped.change_pos].scriptPubKey, tmp_cc.destChange);
         }
 
         auto txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, tmp_cc, sign);
         // if fee of this alternative one is within the range of the max fee, we use this one
         const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped.fee + wallet.m_max_aps_fee) : false};
         TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, txr_grouped.has_value(),
-               txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() ? txr_grouped->change_pos : 0);
+               txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? *txr_grouped->change_pos : 0);
         if (txr_grouped) {
             wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n",
                 txr_ungrouped.fee, txr_grouped->fee, use_aps ? "grouped" : "non-grouped");
@@ -1412,7 +1408,7 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
         preset_txin.SetScriptWitness(txin.scriptWitness);
     }
 
-    auto res = CreateTransaction(wallet, vecSend, nChangePosInOut, coinControl, false);
+    auto res = CreateTransaction(wallet, vecSend, nChangePosInOut == -1 ? std::nullopt : std::optional<unsigned int>((unsigned int)nChangePosInOut), coinControl, false);
     if (!res) {
         error = util::ErrorString(res);
         return false;
@@ -1420,7 +1416,7 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
     const auto& txr = *res;
     CTransactionRef tx_new = txr.tx;
     nFeeRet = txr.fee;
-    nChangePosInOut = txr.change_pos;
+    nChangePosInOut = txr.change_pos ? *txr.change_pos : -1;
 
     if (nChangePosInOut != -1) {
         tx.vout.insert(tx.vout.begin() + nChangePosInOut, tx_new->vout[nChangePosInOut]);
diff --git a/src/wallet/spend.h b/src/wallet/spend.h
index 407627b5f18..ccadbed1f56 100644
--- a/src/wallet/spend.h
+++ b/src/wallet/spend.h
@@ -207,9 +207,9 @@ struct CreatedTransactionResult
     CTransactionRef tx;
     CAmount fee;
     FeeCalculation fee_calc;
-    int change_pos;
+    std::optional<unsigned int> change_pos;
 
-    CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, int _change_pos, const FeeCalculation& _fee_calc)
+    CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, std::optional<unsigned int> _change_pos, const FeeCalculation& _fee_calc)
         : tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos) {}
 };
 
@@ -218,7 +218,7 @@ struct CreatedTransactionResult
  * selected by SelectCoins(); Also create the change output, when needed
  * @note passing change_pos as -1 will result in setting a random position
  */
-util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, int change_pos, const CCoinControl& coin_control, bool sign = true);
+util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, std::optional<unsigned int> change_pos, const CCoinControl& coin_control, bool sign = true);
 
 /**
  * Insert additional inputs into the transaction by
diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
index 5926d881290..b2d252b3f95 100644
--- a/src/wallet/test/spend_tests.cpp
+++ b/src/wallet/test/spend_tests.cpp
@@ -28,13 +28,12 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
     // instead of the miner.
     auto check_tx = [&wallet](CAmount leftover_input_amount) {
         CRecipient recipient{PubKeyDestination({}), 50 * COIN - leftover_input_amount, /*subtract_fee=*/true};
-        constexpr int RANDOM_CHANGE_POSITION = -1;
         CCoinControl coin_control;
         coin_control.m_feerate.emplace(10000);
         coin_control.fOverrideFeeRate = true;
         // We need to use a change type with high cost of change so that the leftover amount will be dropped to fee instead of added as a change output
         coin_control.m_change_type = OutputType::LEGACY;
-        auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, coin_control);
+        auto res = CreateTransaction(*wallet, {recipient}, std::nullopt, coin_control);
         BOOST_CHECK(res);
         const auto& txr = *res;
         BOOST_CHECK_EQUAL(txr.tx->vout.size(), 1);
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index dd43705a840..3d1cbe36a87 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -558,8 +558,7 @@ public:
         CTransactionRef tx;
         CCoinControl dummy;
         {
-            constexpr int RANDOM_CHANGE_POSITION = -1;
-            auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, dummy);
+            auto res = CreateTransaction(*wallet, {recipient}, std::nullopt, dummy);
             BOOST_CHECK(res);
             tx = res->tx;
         }

From 0295b44c257fe23f1ad344aff11372d67864f0db Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Wed, 1 Jun 2022 17:04:03 -0400
Subject: [PATCH 9/9] wallet: return CreatedTransactionResult from
 FundTransaction

Instead of using the output parameters, return CreatedTransactionResult
from FundTransaction in the same way that CreateTransaction does.
Additionally, instead of modifying the original CMutableTransaction, the
result from CreateTransactionInternal is used.
---
 src/wallet/rpc/spend.cpp               | 47 ++++++++++++--------------
 src/wallet/spend.cpp                   | 36 ++++----------------
 src/wallet/spend.h                     |  2 +-
 src/wallet/test/fuzz/notifications.cpp |  3 +-
 4 files changed, 30 insertions(+), 58 deletions(-)

diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index 7cde644d9bc..b121c8e1a70 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -488,13 +488,13 @@ static std::vector<RPCArg> FundTxDoc(bool solving_data = true)
     return args;
 }
 
-void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl, bool override_min_fee)
+CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const UniValue& options, CCoinControl& coinControl, bool override_min_fee)
 {
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
     wallet.BlockUntilSyncedToCurrentChain();
 
-    change_position = -1;
+    std::optional<unsigned int> change_position;
     bool lockUnspents = false;
     UniValue subtractFeeFromOutputs;
     std::set<int> setSubtractFeeFromOutputs;
@@ -552,7 +552,11 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
         }
 
         if (options.exists("changePosition") || options.exists("change_position")) {
-            change_position = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt<int>();
+            int pos = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt<int>();
+            if (pos < 0 || (unsigned int)pos > tx.vout.size()) {
+                throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds");
+            }
+            change_position = (unsigned int)pos;
         }
 
         if (options.exists("change_type")) {
@@ -702,9 +706,6 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
     if (tx.vout.size() == 0)
         throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output");
 
-    if (change_position != -1 && (change_position < 0 || (unsigned int)change_position > tx.vout.size()))
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds");
-
     for (unsigned int idx = 0; idx < subtractFeeFromOutputs.size(); idx++) {
         int pos = subtractFeeFromOutputs[idx].getInt<int>();
         if (setSubtractFeeFromOutputs.count(pos))
@@ -716,11 +717,11 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
         setSubtractFeeFromOutputs.insert(pos);
     }
 
-    bilingual_str error;
-
-    if (!FundTransaction(wallet, tx, fee_out, change_position, error, lockUnspents, setSubtractFeeFromOutputs, coinControl)) {
-        throw JSONRPCError(RPC_WALLET_ERROR, error.original);
+    auto txr = FundTransaction(wallet, tx, change_position, lockUnspents, setSubtractFeeFromOutputs, coinControl);
+    if (!txr) {
+        throw JSONRPCError(RPC_WALLET_ERROR, ErrorString(txr).original);
     }
+    return *txr;
 }
 
 static void SetOptionsInputWeights(const UniValue& inputs, UniValue& options)
@@ -843,17 +844,15 @@ RPCHelpMan fundrawtransaction()
         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
     }
 
-    CAmount fee;
-    int change_position;
     CCoinControl coin_control;
     // Automatically select (additional) coins. Can be overridden by options.add_inputs.
     coin_control.m_allow_other_inputs = true;
-    FundTransaction(*pwallet, tx, fee, change_position, request.params[1], coin_control, /*override_min_fee=*/true);
+    auto txr = FundTransaction(*pwallet, tx, request.params[1], coin_control, /*override_min_fee=*/true);
 
     UniValue result(UniValue::VOBJ);
-    result.pushKV("hex", EncodeHexTx(CTransaction(tx)));
-    result.pushKV("fee", ValueFromAmount(fee));
-    result.pushKV("changepos", change_position);
+    result.pushKV("hex", EncodeHexTx(*txr.tx));
+    result.pushKV("fee", ValueFromAmount(txr.fee));
+    result.pushKV("changepos", txr.change_pos ? (int)*txr.change_pos : -1);
 
     return result;
 },
@@ -1275,8 +1274,6 @@ RPCHelpMan send()
             PreventOutdatedOptions(options);
 
 
-            CAmount fee;
-            int change_position;
             bool rbf{options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf};
             CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf);
             CCoinControl coin_control;
@@ -1284,9 +1281,9 @@ RPCHelpMan send()
             // be overridden by options.add_inputs.
             coin_control.m_allow_other_inputs = rawTx.vin.size() == 0;
             SetOptionsInputWeights(options["inputs"], options);
-            FundTransaction(*pwallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/false);
+            auto txr = FundTransaction(*pwallet, rawTx, options, coin_control, /*override_min_fee=*/false);
 
-            return FinishTransaction(pwallet, options, rawTx);
+            return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx));
         }
     };
 }
@@ -1711,8 +1708,6 @@ RPCHelpMan walletcreatefundedpsbt()
 
     UniValue options{request.params[3].isNull() ? UniValue::VOBJ : request.params[3]};
 
-    CAmount fee;
-    int change_position;
     const UniValue &replaceable_arg = options["replaceable"];
     const bool rbf{replaceable_arg.isNull() ? wallet.m_signal_rbf : replaceable_arg.get_bool()};
     CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf);
@@ -1721,10 +1716,10 @@ RPCHelpMan walletcreatefundedpsbt()
     // be overridden by options.add_inputs.
     coin_control.m_allow_other_inputs = rawTx.vin.size() == 0;
     SetOptionsInputWeights(request.params[0], options);
-    FundTransaction(wallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/true);
+    auto txr = FundTransaction(wallet, rawTx, options, coin_control, /*override_min_fee=*/true);
 
     // Make a blank psbt
-    PartiallySignedTransaction psbtx(rawTx);
+    PartiallySignedTransaction psbtx(CMutableTransaction(*txr.tx));
 
     // Fill transaction with out data but don't sign
     bool bip32derivs = request.params[4].isNull() ? true : request.params[4].get_bool();
@@ -1740,8 +1735,8 @@ RPCHelpMan walletcreatefundedpsbt()
 
     UniValue result(UniValue::VOBJ);
     result.pushKV("psbt", EncodeBase64(ssTx.str()));
-    result.pushKV("fee", ValueFromAmount(fee));
-    result.pushKV("changepos", change_position);
+    result.pushKV("fee", ValueFromAmount(txr.fee));
+    result.pushKV("changepos", txr.change_pos ? (int)*txr.change_pos : -1);
     return result;
 },
     };
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index f911e52ee18..5b28d38c37d 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -1360,7 +1360,7 @@ util::Result<CreatedTransactionResult> CreateTransaction(
     return res;
 }
 
-bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl)
+util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional<unsigned int> change_pos, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl)
 {
     std::vector<CRecipient> vecSend;
 
@@ -1396,8 +1396,7 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
         PreselectedInput& preset_txin = coinControl.Select(outPoint);
         if (!wallet.IsMine(outPoint)) {
             if (coins[outPoint].out.IsNull()) {
-                error = _("Unable to find UTXO for external input");
-                return false;
+                return util::Error{_("Unable to find UTXO for external input")};
             }
 
             // The input was not in the wallet, but is in the UTXO set, so select as external
@@ -1408,38 +1407,17 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
         preset_txin.SetScriptWitness(txin.scriptWitness);
     }
 
-    auto res = CreateTransaction(wallet, vecSend, nChangePosInOut == -1 ? std::nullopt : std::optional<unsigned int>((unsigned int)nChangePosInOut), coinControl, false);
+    auto res = CreateTransaction(wallet, vecSend, change_pos, coinControl, false);
     if (!res) {
-        error = util::ErrorString(res);
-        return false;
-    }
-    const auto& txr = *res;
-    CTransactionRef tx_new = txr.tx;
-    nFeeRet = txr.fee;
-    nChangePosInOut = txr.change_pos ? *txr.change_pos : -1;
-
-    if (nChangePosInOut != -1) {
-        tx.vout.insert(tx.vout.begin() + nChangePosInOut, tx_new->vout[nChangePosInOut]);
+        return res;
     }
 
-    // Copy output sizes from new transaction; they may have had the fee
-    // subtracted from them.
-    for (unsigned int idx = 0; idx < tx.vout.size(); idx++) {
-        tx.vout[idx].nValue = tx_new->vout[idx].nValue;
-    }
-
-    // Add new txins while keeping original txin scriptSig/order.
-    for (const CTxIn& txin : tx_new->vin) {
-        if (!coinControl.IsSelected(txin.prevout)) {
-            tx.vin.push_back(txin);
-
-        }
-        if (lockUnspents) {
+    if (lockUnspents) {
+        for (const CTxIn& txin : res->tx->vin) {
             wallet.LockCoin(txin.prevout);
         }
-
     }
 
-    return true;
+    return res;
 }
 } // namespace wallet
diff --git a/src/wallet/spend.h b/src/wallet/spend.h
index ccadbed1f56..504c078b803 100644
--- a/src/wallet/spend.h
+++ b/src/wallet/spend.h
@@ -224,7 +224,7 @@ util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const
  * Insert additional inputs into the transaction by
  * calling CreateTransaction();
  */
-bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl);
+util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional<unsigned int> change_pos, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl);
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_SPEND_H
diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp
index 82cdd323027..203ab5f606a 100644
--- a/src/wallet/test/fuzz/notifications.cpp
+++ b/src/wallet/test/fuzz/notifications.cpp
@@ -156,10 +156,9 @@ struct FuzzedWallet {
         coin_control.fOverrideFeeRate = fuzzed_data_provider.ConsumeBool();
         // Add solving data (m_external_provider and SelectExternal)?
 
-        CAmount fee_out;
         int change_position{fuzzed_data_provider.ConsumeIntegralInRange<int>(-1, tx.vout.size() - 1)};
         bilingual_str error;
-        (void)FundTransaction(*wallet, tx, fee_out, change_position, error, /*lockUnspents=*/false, subtract_fee_from_outputs, coin_control);
+        (void)FundTransaction(*wallet, tx, change_position, /*lockUnspents=*/false, subtract_fee_from_outputs, coin_control);
     }
 };