diff --git a/src/wallet/coincontrol.cpp b/src/wallet/coincontrol.cpp index 2087119db9..873c5ab383 100644 --- a/src/wallet/coincontrol.cpp +++ b/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 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); + auto& input = m_selected[outpoint]; + input.SetPosition(m_selection_pos); + ++m_selection_pos; + return input; } - -void CCoinControl::SelectExternal(const COutPoint& outpoint, const CTxOut& txout) +void CCoinControl::UnSelect(const COutPoint& outpoint) { - m_selected_inputs.insert(outpoint); - m_external_txouts.emplace(outpoint, txout); -} - -void CCoinControl::UnSelect(const COutPoint& output) -{ - m_selected_inputs.erase(output); + m_selected.erase(outpoint); } void CCoinControl::UnSelectAll() { - m_selected_inputs.clear(); + m_selected.clear(); } std::vector CCoinControl::ListSelected() const { - return {m_selected_inputs.begin(), m_selected_inputs.end()}; + std::vector outpoints; + std::transform(m_selected.begin(), m_selected.end(), std::back_inserter(outpoints), + [](const std::map::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 +std::optional CCoinControl::GetInputWeight(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.GetInputWeight() : std::nullopt; } -int64_t CCoinControl::GetInputWeight(const COutPoint& outpoint) const +std::optional CCoinControl::GetSequence(const COutPoint& outpoint) const { - auto it = m_input_weights.find(outpoint); - assert(it != m_input_weights.end()); - return it->second; + const auto it = m_selected.find(outpoint); + return it != m_selected.end() ? it->second.GetSequence() : std::nullopt; +} + +std::pair, std::optional> 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 PreselectedInput::GetInputWeight() const +{ + return m_weight; +} + +void PreselectedInput::SetSequence(uint32_t sequence) +{ + m_sequence = sequence; +} + +std::optional 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> PreselectedInput::GetScripts() const +{ + return {m_script_sig, m_script_witness}; +} + +void PreselectedInput::SetPosition(unsigned int pos) +{ + m_pos = pos; +} + +std::optional PreselectedInput::GetPosition() const +{ + return m_pos; } } // namespace wallet diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index 71593e236f..b2f813383d 100644 --- a/src/wallet/coincontrol.h +++ b/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 m_txout; + //! The input weight for spending this input + std::optional m_weight; + //! The sequence number for this input + std::optional m_sequence; + //! The scriptSig for this input + std::optional m_script_sig; + //! The scriptWitness for this input + std::optional m_script_witness; + //! The position in the inputs vector for this input + std::optional 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 GetInputWeight() const; + + /** Set the sequence for this input. */ + void SetSequence(uint32_t sequence); + /** Retrieve the sequence for this input. */ + std::optional 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> GetScripts() const; + + /** Store the position of this input. */ + void SetPosition(unsigned int pos); + /** Retrieve the position of this input. */ + std::optional GetPosition() const; +}; + /** Coin Control Features. */ class CCoinControl { @@ -59,6 +111,10 @@ 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 m_locktime; + //! Version + std::optional m_version; CCoinControl(); @@ -69,11 +125,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,16 +138,11 @@ 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); - /** - * 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 @@ 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 GetInputWeight(const COutPoint& outpoint) const; + /** Retrieve the sequence for an input */ + std::optional GetSequence(const COutPoint& outpoint) const; + /** Retrieves the scriptSig and scriptWitness for an input. */ + std::pair, std::optional> GetScripts(const COutPoint& outpoint) const; + + bool HasSelectedOrder() const + { + return m_selection_pos > 0; + } + + std::optional 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 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 m_external_txouts; - //! Map of COutPoints to the maximum weight for that input - std::map m_input_weights; + std::map m_selected; + unsigned int m_selection_pos{0}; }; } // namespace wallet diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index d9a08310a8..c6ed0fe11c 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); @@ -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; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 4ca5e29229..d15273dfc9 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 e7884af8b0..b121c8e1a7 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); } @@ -489,13 +488,13 @@ static std::vector 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 change_position; bool lockUnspents = false; UniValue subtractFeeFromOutputs; std::set setSubtractFeeFromOutputs; @@ -553,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 pos = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt(); + 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")) { @@ -703,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(); if (setSubtractFeeFromOutputs.count(pos)) @@ -717,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) @@ -844,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; }, @@ -1276,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; @@ -1285,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)); } }; } @@ -1712,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); @@ -1722,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(); @@ -1741,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 35583642a5..5b28d38c37 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -117,8 +117,9 @@ static std::optional 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 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 FetchSelectedInputs(const CWallet& wallet, const const bool can_grind_r = wallet.CanGrindR(); std::map 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 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 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 } @@ -964,18 +965,19 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng static util::Result CreateTransactionInternal( CWallet& wallet, const std::vector& vecSend, - int change_pos, + std::optional 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 + 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; @@ -1127,20 +1129,39 @@ static util::Result 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 std::vector> 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& a, const std::shared_ptr& 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. // @@ -1149,11 +1170,32 @@ static util::Result 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 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)); + + 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(); + // 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()); } - DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight()); // Calculate the transaction fee TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control); @@ -1172,8 +1214,8 @@ static util::Result 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) { @@ -1184,11 +1226,11 @@ static util::Result 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]; @@ -1227,7 +1269,7 @@ static util::Result 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}; } @@ -1268,13 +1310,13 @@ static util::Result 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 CreateTransaction( CWallet& wallet, const std::vector& vecSend, - int change_pos, + std::optional change_pos, const CCoinControl& coin_control, bool sign) { @@ -1290,7 +1332,7 @@ util::Result 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 @@ -1300,16 +1342,15 @@ util::Result 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"); @@ -1319,7 +1360,7 @@ util::Result CreateTransaction( return res; } -bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl coinControl) +util::Result FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional change_pos, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl coinControl) { std::vector vecSend; @@ -1332,6 +1373,12 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, vecSend.push_back(recipient); } + // 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); @@ -1346,50 +1393,31 @@ 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()) { + 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 - coinControl.SelectExternal(outPoint, coins[outPoint].out); + 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); + 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; - - 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 407627b5f1..504c078b80 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 change_pos; - CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, int _change_pos, const FeeCalculation& _fee_calc) + CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, std::optional _change_pos, const FeeCalculation& _fee_calc) : tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos) {} }; @@ -218,13 +218,13 @@ 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 CreateTransaction(CWallet& wallet, const std::vector& vecSend, int change_pos, const CCoinControl& coin_control, bool sign = true); +util::Result CreateTransaction(CWallet& wallet, const std::vector& vecSend, std::optional change_pos, const CCoinControl& coin_control, bool sign = true); /** * 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& setSubtractFeeFromOutputs, CCoinControl); +util::Result FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional change_pos, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl); } // namespace wallet #endif // BITCOIN_WALLET_SPEND_H diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 9569210ba0..fa0dfa5556 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 0f71f28df2..f1efbc1cb8 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); @@ -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); }); } } diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 82cdd32302..203ab5f606 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(-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); } }; diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index 5926d88129..b2d252b3f9 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 dd43705a84..3d1cbe36a8 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; }