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 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 @@ -1132,15 +1129,15 @@ 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 @@ -1217,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) { @@ -1229,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]; @@ -1272,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}; } @@ -1313,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) { @@ -1335,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 @@ -1345,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"); @@ -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)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 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,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 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 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; }