From 6ca51dfd44182117fd128fc5652a881c8de2cc9b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 28 Dec 2024 13:42:05 +0000 Subject: [PATCH 01/13] wallet: Use existing feerate instead of getting a new one Courtesy of 1a6a0b0d from bitcoin#21083 --- src/wallet/spend.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 4f22d0bcd84fa..801650d9dec20 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -651,6 +651,11 @@ bool CWallet::CreateTransactionInternal( error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::DUFF_B), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::DUFF_B)); return false; } + if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) { + // eventually allow a fallback fee + error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); + return false; + } int nBytes{0}; { @@ -802,7 +807,7 @@ bool CWallet::CreateTransactionInternal( txin.scriptSig = CScript(); } - nFee = GetMinimumFee(*this, nBytes, coin_control, &feeCalc); + nFee = coin_selection_params.m_effective_feerate.GetFee(nBytes); return true; }; @@ -911,12 +916,6 @@ bool CWallet::CreateTransactionInternal( } } - if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) { - // eventually allow a fallback fee - error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); - return false; - } - if (nAmountLeft == nFeeRet) { // We either added the change amount to nFeeRet because the change amount was considered // to be dust or the input exactly matches output + fee. From 05c319e2ab2c879d04500271cecccca6f00a4fdb Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 28 Dec 2024 14:00:07 +0000 Subject: [PATCH 02/13] refactor: move oversized transaction check to tail end of scope Change needed so that next commit can extend check to include post-signature size. --- src/wallet/spend.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 801650d9dec20..da239c39a0e47 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -796,12 +796,6 @@ bool CWallet::CreateTransactionInternal( nBytes += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize; } - if (static_cast(nBytes) > MAX_STANDARD_TX_SIZE) { - // Do not create oversized transactions (bad-txns-oversize). - error = _("Transaction too large"); - return false; - } - // Remove scriptSigs to eliminate the fee calculation dummy signatures for (auto& txin : txNew.vin) { txin.scriptSig = CScript(); @@ -962,6 +956,12 @@ bool CWallet::CreateTransactionInternal( // Return the constructed transaction data. tx = MakeTransactionRef(std::move(txNew)); + + // Limit size + if (static_cast(nBytes) > MAX_STANDARD_TX_SIZE) { + error = _("Transaction too large"); + return false; + } } if (nFeeRet > m_default_max_tx_fee) { From ab756ba31c926825e924bdb0fc90b4382b47e9b6 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 28 Dec 2024 14:59:20 +0000 Subject: [PATCH 03/13] wallet: Fail if maximum weight is too large Courtesy of 3e69939b from bitcoin#20536 --- src/wallet/spend.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index da239c39a0e47..651c781e5830b 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -958,7 +958,8 @@ bool CWallet::CreateTransactionInternal( tx = MakeTransactionRef(std::move(txNew)); // Limit size - if (static_cast(nBytes) > MAX_STANDARD_TX_SIZE) { + if ((sign && ::GetSerializeSize(*tx, PROTOCOL_VERSION) > MAX_STANDARD_TX_SIZE) || + (!sign && static_cast(nBytes) > MAX_STANDARD_TX_SIZE)) { error = _("Transaction too large"); return false; } From 1cf1c581f1642cac875a1317b6346ac3d9589e15 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 3 Jan 2025 07:20:08 +0000 Subject: [PATCH 04/13] refactor: move selected coin and txin sorting to the end of the scope Co-authored-by: UdjinM6 --- src/wallet/spend.cpp | 46 +++++++++++++++++++++----------------------- src/wallet/spend.h | 10 ---------- 2 files changed, 22 insertions(+), 34 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 651c781e5830b..54792cedcb2e7 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -618,6 +618,7 @@ bool CWallet::CreateTransactionInternal( CAmount nValue = 0; ReserveDestination reservedest(this); int nChangePosRequest = nChangePosInOut; + const bool sort_bip69{nChangePosRequest == -1}; unsigned int nSubtractFeeFromAmount = 0; for (const auto& recipient : vecSend) { @@ -659,7 +660,7 @@ bool CWallet::CreateTransactionInternal( int nBytes{0}; { - std::vector vecCoins; + std::set setCoins; LOCK(cs_wallet); txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight()); { @@ -759,8 +760,8 @@ bool CWallet::CreateTransactionInternal( bool bnb_used = false; if (pick_new_inputs) { nValueIn = 0; - std::set setCoinsTmp; - if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoinsTmp, nValueIn, coin_control, coin_selection_params, bnb_used)) { + setCoins.clear(); + if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coin_control, coin_selection_params, bnb_used)) { if (coin_control.nCoinType == CoinType::ONLY_NONDENOMINATED) { error = _("Unable to locate enough non-denominated funds for this transaction."); } else if (coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { @@ -771,16 +772,13 @@ bool CWallet::CreateTransactionInternal( } return false; } - vecCoins.assign(setCoinsTmp.begin(), setCoinsTmp.end()); } - // Fill vin + // Dummy fill vin for maximum size estimation // - // Note how the sequence number is set to max()-1 so that the - // nLockTime set above actually works. txNew.vin.clear(); - for (const auto& coin : vecCoins) { - txNew.vin.emplace_back(coin.outpoint, CScript(), CTxIn::SEQUENCE_FINAL - 1); + for (const auto& coin : setCoins) { + txNew.vin.push_back(CTxIn(coin.outpoint, CScript())); } auto calculateFee = [&](CAmount& nFee) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) -> bool { @@ -889,24 +887,13 @@ bool CWallet::CreateTransactionInternal( continue; } - // If no specific change position was requested, apply BIP69 - if (nChangePosRequest == -1) { - std::sort(vecCoins.begin(), vecCoins.end(), CompareInputCoinBIP69()); - std::sort(txNew.vin.begin(), txNew.vin.end(), CompareInputBIP69()); + if (sort_bip69) { std::sort(txNew.vout.begin(), txNew.vout.end(), CompareOutputBIP69()); // If there was a change output added before, we must update its position now - if (nChangePosInOut != -1) { - int i = 0; - for (const CTxOut& txOut : txNew.vout) - { - if (txOut == newTxOut) - { - nChangePosInOut = i; - break; - } - i++; - } + if (const auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); + it != txNew.vout.end() && nChangePosInOut != -1) { + nChangePosInOut = std::distance(txNew.vout.begin(), it); } } @@ -949,6 +936,17 @@ bool CWallet::CreateTransactionInternal( // Make sure change position was updated one way or another assert(nChangePosInOut != std::numeric_limits::max()); + // Fill in final vin and shuffle/sort it + txNew.vin.clear(); + + // Note how the sequence number is set to non-maxint so that + // the nLockTime set above actually works. + const uint32_t nSequence = CTxIn::SEQUENCE_FINAL - 1; + for (const auto& coin : setCoins) { + txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence)); + } + if (sort_bip69) { std::sort(txNew.vin.begin(), txNew.vin.end(), CompareInputBIP69()); } + if (sign && !SignTransaction(txNew)) { error = _("Signing transaction failed"); return false; diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 3f01698e88761..03f9a7c2b5023 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -61,14 +61,4 @@ class COutput } }; -struct CompareInputCoinBIP69 -{ - inline bool operator()(const CInputCoin& a, const CInputCoin& b) const - { - // Note: CInputCoin-s are essentially inputs, their txouts are used for informational purposes only - // that's why we use CompareInputBIP69 to sort them in a BIP69 compliant way. - return CompareInputBIP69()(CTxIn(a.outpoint), CTxIn(b.outpoint)); - } -}; - #endif // BITCOIN_WALLET_SPEND_H From 0711e673411ff62237668a436e4ec97417e20dd3 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 29 Dec 2024 11:23:35 +0000 Subject: [PATCH 05/13] wallet: shuffle transaction inputs if we aren't using BIP69 --- src/wallet/spend.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 54792cedcb2e7..783248225299c 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -946,6 +946,7 @@ bool CWallet::CreateTransactionInternal( txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence)); } if (sort_bip69) { std::sort(txNew.vin.begin(), txNew.vin.end(), CompareInputBIP69()); } + else { Shuffle(txNew.vin.begin(), txNew.vin.end(), FastRandomContext()); } if (sign && !SignTransaction(txNew)) { error = _("Signing transaction failed"); From bd35042a16b29df8e27aaf77309c032c25e60b21 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 29 Dec 2024 11:31:41 +0000 Subject: [PATCH 06/13] wallet: move `CoinSelectionParams` to positions expected upstream Not using BnB is a Dash-specific decision and has been commented accordingly. --- src/wallet/spend.cpp | 43 +++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 783248225299c..94791e1d58a17 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -640,24 +640,6 @@ bool CWallet::CreateTransactionInternal( CMutableTransaction txNew; FeeCalculation feeCalc; - - CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy - coin_selection_params.m_discard_feerate = coin_control.m_discard_feerate ? *coin_control.m_discard_feerate : GetDiscardRate(*this); - - // Get the fee rate to use effective values in coin selection - coin_selection_params.m_effective_feerate = GetMinimumFeeRate(*this, coin_control, &feeCalc); - // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly - // provided one - if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) { - error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::DUFF_B), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::DUFF_B)); - return false; - } - if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) { - // eventually allow a fallback fee - error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); - return false; - } - int nBytes{0}; { std::set setCoins; @@ -667,14 +649,14 @@ bool CWallet::CreateTransactionInternal( CAmount nAmountAvailable{0}; std::vector vAvailableCoins; AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); - coin_selection_params.use_bnb = false; // never use BnB + CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy + coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; for (auto out : vAvailableCoins) { if (out.fSpendable) { nAmountAvailable += out.tx->tx->vout[out.i].nValue; } } - coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; // Create change script that will be used if we need change // TODO: pass in scriptChange instead of reservedest so @@ -705,9 +687,30 @@ bool CWallet::CreateTransactionInternal( CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty()); } + // Set discard feerate + coin_selection_params.m_discard_feerate = coin_control.m_discard_feerate ? *coin_control.m_discard_feerate : GetDiscardRate(*this); + + // Get the fee rate to use effective values in coin selection + coin_selection_params.m_effective_feerate = GetMinimumFeeRate(*this, coin_control, &feeCalc); + // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly + // provided one + if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) { + error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::DUFF_B), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::DUFF_B)); + return false; + } + if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) { + // eventually allow a fallback fee + error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); + return false; + } + nFeeRet = 0; bool pick_new_inputs = true; CAmount nValueIn = 0; + + // BnB selector is the only selector used when this is true. + coin_selection_params.use_bnb = false; // Dash: never use BnB + CAmount nAmountToSelectAdditional{0}; // Start with nAmountToSelectAdditional=0 and loop until there is enough to cover the request + fees, try it 500 times. int nMaxTries = 500; From 6e4d7898f54117cf0fac9f01e761560ccd62f23e Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 29 Dec 2024 15:21:27 +0000 Subject: [PATCH 07/13] wallet: add back missing `CoinSelectionParams` assignments They were removed as part of dash#3668 and omitted in related backports but as bitcoin#17331 will be a logical partial revert of the former, we need to add them back in preparation for it. --- src/wallet/spend.cpp | 27 +++++++++++++++++++++++++++ src/wallet/test/wallet_tests.cpp | 1 - src/wallet/wallet.h | 2 ++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 94791e1d58a17..e01317f0cfb0d 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -686,6 +686,18 @@ bool CWallet::CreateTransactionInternal( // change keypool ran out, but change is required. CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty()); } + CTxOut change_prototype_txout(0, scriptChange); + coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout); + + // Get size of spending the change output + int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this); + // If the wallet doesn't know how to sign change output, assume p2sh-p2pkh + // as lower-bound to allow BnB to do it's thing + if (change_spend_size == -1) { + coin_selection_params.change_spend_size = DUMMY_NESTED_P2PKH_INPUT_SIZE; + } else { + coin_selection_params.change_spend_size = (size_t)change_spend_size; + } // Set discard feerate coin_selection_params.m_discard_feerate = coin_control.m_discard_feerate ? *coin_control.m_discard_feerate : GetDiscardRate(*this); @@ -704,12 +716,18 @@ bool CWallet::CreateTransactionInternal( return false; } + // Get long term estimate + CCoinControl cc_temp; + cc_temp.m_confirm_target = chain().estimateMaxBlocks(); + coin_selection_params.m_long_term_feerate = GetMinimumFeeRate(*this, cc_temp, nullptr); + nFeeRet = 0; bool pick_new_inputs = true; CAmount nValueIn = 0; // BnB selector is the only selector used when this is true. coin_selection_params.use_bnb = false; // Dash: never use BnB + coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values CAmount nAmountToSelectAdditional{0}; // Start with nAmountToSelectAdditional=0 and loop until there is enough to cover the request + fees, try it 500 times. @@ -726,7 +744,11 @@ bool CWallet::CreateTransactionInternal( assert(nAmountToSelectAdditional >= 0); nValueToSelect += nAmountToSelectAdditional; } + // vouts to the payees + if (!coin_selection_params.m_subtract_fee_outputs) { + coin_selection_params.tx_noinputs_size = 10; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count + } for (const auto& recipient : vecSend) { CTxOut txout(recipient.nAmount, recipient.scriptPubKey); @@ -743,6 +765,11 @@ bool CWallet::CreateTransactionInternal( } } + // Include the fee cost for outputs. Note this is only used for BnB right now + if (!coin_selection_params.m_subtract_fee_outputs) { + coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); + } + if (IsDust(txout, chain().relayDustFee())) { if (recipient.fSubtractFeeFromAmount && nFeeRet > 0) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 54bc114aaa247..a6bb3ea3c9501 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -1353,7 +1353,6 @@ static size_t CalculateNestedKeyhashInputSize(bool use_max_sig) return ::GetSerializeSize(tx_in, PROTOCOL_VERSION); } -static constexpr size_t DUMMY_NESTED_P2PKH_INPUT_SIZE = 113; BOOST_FIXTURE_TEST_CASE(dummy_input_size_test, TestChain100Setup) { BOOST_CHECK_EQUAL(CalculateNestedKeyhashInputSize(false), DUMMY_NESTED_P2PKH_INPUT_SIZE); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 50b06c635b6da..9f97d8d3bd022 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -104,6 +104,8 @@ static const CAmount DEFAULT_TRANSACTION_MAXFEE = COIN / 10; static const CAmount HIGH_TX_FEE_PER_KB = COIN / 100; //! -maxtxfee will warn if called with a higher fee than this amount (in satoshis) static const CAmount HIGH_MAX_TX_FEE = 100 * HIGH_TX_FEE_PER_KB; +//! Pre-calculated constants for input size estimation in *virtual size* +static constexpr size_t DUMMY_NESTED_P2PKH_INPUT_SIZE = 113; //! if set, all keys will be derived by using BIP39/BIP44 static const bool DEFAULT_USE_HD_WALLET = true; From 66fe2d4a82ce941f067656f48e1d7eff8b9e6931 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 29 Dec 2024 15:18:14 +0000 Subject: [PATCH 08/13] merge bitcoin#25497: more accurate target for large transactions --- src/wallet/spend.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index e01317f0cfb0d..f111732042f1e 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -747,7 +747,8 @@ bool CWallet::CreateTransactionInternal( // vouts to the payees if (!coin_selection_params.m_subtract_fee_outputs) { - coin_selection_params.tx_noinputs_size = 10; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count + coin_selection_params.tx_noinputs_size = 9; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count + coin_selection_params.tx_noinputs_size += GetSizeOfCompactSize(vecSend.size()); // bytes for output count } for (const auto& recipient : vecSend) { From 9e9e66f596b57b114e742ec984941a8e2dd905ce Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 28 Dec 2024 11:21:20 +0000 Subject: [PATCH 09/13] partial bitcoin#17331: Use effective values throughout coin selection includes: - 1bf4a62cb61bd4b91d9cd4e379fea2b914786342 - af5867c89688b06173b295b7c32a42845ea455da - d97d25d95006725e705635530b27643363d6b2a4 - cc3f14b27c06b7a0da1472f5c7100c3f0b76fd98 This pull request logically reverts portions of dash#3668 but breaks BIP69 sorting, which will be fixed in an upcoming commit. It has been disabled for now. --- src/wallet/coinselection.cpp | 14 +- src/wallet/spend.cpp | 320 ++++++++++++++--------------------- src/wallet/wallet.h | 4 + 3 files changed, 139 insertions(+), 199 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 6bd9044da8fa9..e3919105023fc 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -72,7 +72,7 @@ bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_v std::vector curr_selection; // select the utxo at this index curr_selection.reserve(utxo_pool.size()); - CAmount actual_target = not_input_fees + target_value; + CAmount selection_target = not_input_fees + target_value; // Calculate curr_available_value CAmount curr_available_value = 0; @@ -81,7 +81,7 @@ bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_v assert(utxo.effective_value > 0); curr_available_value += utxo.effective_value; } - if (curr_available_value < actual_target) { + if (curr_available_value < selection_target) { return false; } @@ -96,12 +96,12 @@ bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_v for (size_t i = 0; i < TOTAL_TRIES; ++i) { // Conditions for starting a backtrack bool backtrack = false; - if (curr_value + curr_available_value < actual_target || // Cannot possibly reach target with the amount remaining in the curr_available_value. - curr_value > actual_target + cost_of_change || // Selected value is out of range, go back and try other branch + if (curr_value + curr_available_value < selection_target || // Cannot possibly reach target with the amount remaining in the curr_available_value. + curr_value > selection_target + cost_of_change || // Selected value is out of range, go back and try other branch (curr_waste > best_waste && (utxo_pool.at(0).fee - utxo_pool.at(0).long_term_fee) > 0)) { // Don't select things which we know will be more wasteful if the waste is increasing backtrack = true; - } else if (curr_value >= actual_target) { // Selected value is within range - curr_waste += (curr_value - actual_target); // This is the excess value which is added to the waste for the below comparison + } else if (curr_value >= selection_target) { // Selected value is within range + curr_waste += (curr_value - selection_target); // This is the excess value which is added to the waste for the below comparison // Adding another UTXO after this check could bring the waste down if the long term fee is higher than the current fee. // However we are not going to explore that because this optimization for the waste is only done when we have hit our target // value. Adding any more UTXOs will be just burning the UTXO; it will go entirely to fees. Thus we aren't going to @@ -114,7 +114,7 @@ bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_v break; } } - curr_waste -= (curr_value - actual_target); // Remove the excess value as we will be selecting different coins now + curr_waste -= (curr_value - selection_target); // Remove the excess value as we will be selecting different coins now backtrack = true; } diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index f111732042f1e..f8f04ffa5e386 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -365,24 +365,23 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil setCoinsRet.clear(); nValueRet = 0; - if (coin_selection_params.use_bnb) { - // Get the feerate for effective value. - // When subtracting the fee from the outputs, we want the effective feerate to be 0 - CFeeRate effective_feerate{0}; - if (!coin_selection_params.m_subtract_fee_outputs) { - effective_feerate = coin_selection_params.m_effective_feerate; - } - - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, coin_selection_params.m_long_term_feerate, eligibility_filter, true /* positive_only */); - - // Calculate cost of change - CAmount cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size); + // Calculate the fees for things that aren't inputs, excluding the change output + const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); + + // Get the feerate for effective value. + // When subtracting the fee from the outputs, we want the effective feerate to be 0 + CFeeRate effective_feerate{0}; + if (!coin_selection_params.m_subtract_fee_outputs) { + effective_feerate = coin_selection_params.m_effective_feerate; + } - // Calculate the fees for things that aren't inputs - CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); + if (coin_selection_params.use_bnb) { + std::vector positive_groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, coin_selection_params.m_long_term_feerate, eligibility_filter, true /* positive_only */); bnb_used = true; - return SelectCoinsBnB(groups, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees); + return SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change, setCoinsRet, nValueRet, not_input_fees); } else { + // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here. + // The knapsack solver currently does not use effective values, so we give GroupOutputs feerates of 0 so it sets the effective values to be the same as the real value. std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, CFeeRate(0), CFeeRate(0), eligibility_filter, false /* positive_only */); bnb_used = false; return KnapsackSolver(nTargetValue, groups, setCoinsRet, nValueRet, nCoinType == CoinType::ONLY_FULLY_MIXED, m_default_max_tx_fee); @@ -618,7 +617,7 @@ bool CWallet::CreateTransactionInternal( CAmount nValue = 0; ReserveDestination reservedest(this); int nChangePosRequest = nChangePosInOut; - const bool sort_bip69{nChangePosRequest == -1}; + const bool sort_bip69{false}; unsigned int nSubtractFeeFromAmount = 0; for (const auto& recipient : vecSend) { @@ -646,18 +645,11 @@ bool CWallet::CreateTransactionInternal( LOCK(cs_wallet); txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight()); { - CAmount nAmountAvailable{0}; std::vector vAvailableCoins; AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; - for (auto out : vAvailableCoins) { - if (out.fSpendable) { - nAmountAvailable += out.tx->tx->vout[out.i].nValue; - } - } - // Create change script that will be used if we need change // TODO: pass in scriptChange instead of reservedest so // change transaction isn't always pay-to-bitcoin-address @@ -721,29 +713,32 @@ bool CWallet::CreateTransactionInternal( cc_temp.m_confirm_target = chain().estimateMaxBlocks(); coin_selection_params.m_long_term_feerate = GetMinimumFeeRate(*this, cc_temp, nullptr); + // Calculate the cost of change + // Cost of change is the cost of creating the change output + cost of spending the change output in the future. + // For creating the change output now, we use the effective feerate. + // For spending the change output in the future, we use the discard feerate for now. + // So cost of change = (change output size * effective feerate) + (size of spending change output * discard feerate) + coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size); + coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_change_fee; + nFeeRet = 0; - bool pick_new_inputs = true; CAmount nValueIn = 0; // BnB selector is the only selector used when this is true. coin_selection_params.use_bnb = false; // Dash: never use BnB coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values - CAmount nAmountToSelectAdditional{0}; - // Start with nAmountToSelectAdditional=0 and loop until there is enough to cover the request + fees, try it 500 times. + // Start with no fee and loop until there is enough fee, try it 500 times. int nMaxTries = 500; while (--nMaxTries > 0) { - nChangePosInOut = std::numeric_limits::max(); + nChangePosInOut = nChangePosRequest; txNew.vin.clear(); txNew.vout.clear(); - bool fFirst = true; CAmount nValueToSelect = nValue; - if (nSubtractFeeFromAmount == 0) { - assert(nAmountToSelectAdditional >= 0); - nValueToSelect += nAmountToSelectAdditional; - } + if (nSubtractFeeFromAmount == 0) + nValueToSelect += nFeeRet; // vouts to the payees if (!coin_selection_params.m_subtract_fee_outputs) { @@ -754,34 +749,14 @@ bool CWallet::CreateTransactionInternal( { CTxOut txout(recipient.nAmount, recipient.scriptPubKey); - if (recipient.fSubtractFeeFromAmount) - { - assert(nSubtractFeeFromAmount != 0); - txout.nValue -= nFeeRet / nSubtractFeeFromAmount; // Subtract fee equally from each selected recipient - - if (fFirst) // first receiver pays the remainder not divisible by output count - { - fFirst = false; - txout.nValue -= nFeeRet % nSubtractFeeFromAmount; - } - } - - // Include the fee cost for outputs. Note this is only used for BnB right now + // Include the fee cost for outputs. if (!coin_selection_params.m_subtract_fee_outputs) { coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); } if (IsDust(txout, chain().relayDustFee())) { - if (recipient.fSubtractFeeFromAmount && nFeeRet > 0) - { - if (txout.nValue < 0) - error = _("The transaction amount is too small to pay the fee"); - else - error = _("The transaction amount is too small to send after the fee has been deducted"); - } - else - error = _("Transaction amount too small"); + error = _("Transaction amount too small"); return false; } txNew.vout.push_back(txout); @@ -789,167 +764,131 @@ bool CWallet::CreateTransactionInternal( // Choose coins to use bool bnb_used = false; - if (pick_new_inputs) { - nValueIn = 0; - setCoins.clear(); - if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coin_control, coin_selection_params, bnb_used)) { - if (coin_control.nCoinType == CoinType::ONLY_NONDENOMINATED) { - error = _("Unable to locate enough non-denominated funds for this transaction."); - } else if (coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { - error = _("Unable to locate enough mixed funds for this transaction."); - error = error + Untranslated(" ") + strprintf(_("%s uses exact denominated amounts to send funds, you might simply need to mix some more coins."), gCoinJoinName); - } else if (nValueIn < nValueToSelect) { - error = _("Insufficient funds."); - } - return false; + nValueIn = 0; + setCoins.clear(); + if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coin_control, coin_selection_params, bnb_used)) { + if (coin_control.nCoinType == CoinType::ONLY_NONDENOMINATED) { + error = _("Unable to locate enough non-denominated funds for this transaction."); + } else if (coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { + error = _("Unable to locate enough mixed funds for this transaction."); + error = error + Untranslated(" ") + strprintf(_("%s uses exact denominated amounts to send funds, you might simply need to mix some more coins."), gCoinJoinName); + } else { + error = _("Insufficient funds."); } + return false; } - // Dummy fill vin for maximum size estimation - // - txNew.vin.clear(); - for (const auto& coin : setCoins) { - txNew.vin.push_back(CTxIn(coin.outpoint, CScript())); + // Always make a change output + // We will reduce the fee from this change output later, and remove the output if it is too small. + const CAmount change_and_fee = nValueIn - nValue; + assert(change_and_fee >= 0); + CTxOut newTxOut(change_and_fee, scriptChange); + + if (nChangePosInOut == -1) + { + // Insert change txn at random position: + nChangePosInOut = GetRandInt(txNew.vout.size()+1); + } + else if ((unsigned int)nChangePosInOut > txNew.vout.size()) + { + error = _("Transaction change output index out of range"); + return false; } - auto calculateFee = [&](CAmount& nFee) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) -> bool { - AssertLockHeld(cs_wallet); - nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); - if (nBytes < 0) { - error = _("Signing transaction failed"); - return false; - } + assert(nChangePosInOut != -1); + auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); - if (nExtraPayloadSize != 0) { - // account for extra payload in fee calculation - nBytes += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize; - } + if (sort_bip69) { + std::sort(txNew.vout.begin(), txNew.vout.end(), CompareOutputBIP69()); - // Remove scriptSigs to eliminate the fee calculation dummy signatures - for (auto& txin : txNew.vin) { - txin.scriptSig = CScript(); + // If there was a change output added before, we must update its position now + if (const auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); it != txNew.vout.end()) { + change_position = it; + nChangePosInOut = std::distance(txNew.vout.begin(), change_position); } - - nFee = coin_selection_params.m_effective_feerate.GetFee(nBytes); - - return true; }; - if (!calculateFee(nFeeRet)) { + // Dummy fill vin for maximum size estimation + // + for (const auto& coin : setCoins) { + txNew.vin.push_back(CTxIn(coin.outpoint, CScript())); + } + + // Calculate the transaction fee + nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); + if (nBytes < 0) { + error = _("Signing transaction failed"); return false; } - CTxOut newTxOut; - const CAmount nAmountLeft = nValueIn - nValue; - auto getChange = [&]() { - if (nSubtractFeeFromAmount > 0) { - return nAmountLeft; - } else { - return nAmountLeft - nFeeRet; - } - }; + if (nExtraPayloadSize != 0) { + // account for extra payload in fee calculation + nBytes += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize; + } + + nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes); + + CAmount fee_needed = nFeeRet; + if (nSubtractFeeFromAmount == 0) { + change_position->nValue -= fee_needed; + } - if (getChange() > 0) + // We want to drop the change to fees if: + // 1. The change output would be dust + // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change) + // 3. We are working with fully mixed CoinJoin denominations + CAmount change_amount = change_position->nValue; + if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change || coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { - //over pay for denominated transactions - if (coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { - nChangePosInOut = -1; - nFeeRet += getChange(); - } else { - // Fill a vout to ourself with zero amount until we know the correct change - newTxOut = CTxOut(0, scriptChange); - txNew.vout.push_back(newTxOut); - - // Calculate the fee with the change output added, store the - // current fee to reset it in case the remainder is dust and we - // don't need to fee with change output added. - CAmount nFeePrev = nFeeRet; - if (!calculateFee(nFeeRet)) { - return false; - } + nChangePosInOut = -1; + change_amount = 0; + txNew.vout.erase(change_position); - // Remove the change output again, it will be added later again if required - txNew.vout.pop_back(); + nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); + fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); + } - // Set the change amount properly - newTxOut.nValue = getChange(); + // If the fee is covered, there's no need to loop or subtract from recipients + if (fee_needed <= change_and_fee - change_amount) { + nFeeRet = change_and_fee - change_amount; + break; + } - // Never create dust outputs; if we would, just - // add the dust to the fee. - if (IsDust(newTxOut, coin_selection_params.m_discard_feerate)) - { - nFeeRet = nFeePrev; - nChangePosInOut = -1; - nFeeRet += getChange(); + // Reduce output values for subtractFeeFromAmount + if (nSubtractFeeFromAmount != 0) { + CAmount to_reduce = fee_needed + change_amount - change_and_fee; + int i = 0; + bool fFirst = true; + for (const auto& recipient : vecSend) + { + if (i == nChangePosInOut) { + ++i; } - else + CTxOut& txout = txNew.vout[i]; + + if (recipient.fSubtractFeeFromAmount) { - if (nChangePosRequest == -1) + txout.nValue -= to_reduce / nSubtractFeeFromAmount; // Subtract fee equally from each selected recipient + + if (fFirst) // first receiver pays the remainder not divisible by output count { - // Insert change txn at random position: - nChangePosInOut = GetRandInt(txNew.vout.size()+1); + fFirst = false; + txout.nValue -= to_reduce % nSubtractFeeFromAmount; } - else if ((unsigned int)nChangePosRequest > txNew.vout.size()) - { - error = _("Transaction change output index out of range"); + // Error if this output is reduced to be below dust + if (IsDust(txout, chain().relayDustFee())) { + if (txout.nValue < 0) { + error = _("The transaction amount is too small to pay the fee"); + } else { + error = _("The transaction amount is too small to send after the fee has been deducted"); + } return false; - } else { - nChangePosInOut = nChangePosRequest; } - - std::vector::iterator position = txNew.vout.begin()+nChangePosInOut; - txNew.vout.insert(position, newTxOut); } + ++i; } - } else { - nChangePosInOut = -1; - } - - if (getChange() < 0) { - if (nSubtractFeeFromAmount == 0) { - // nValueIn is not enough to cover nValue + nFeeRet. Add the missing amount abs(nChange) to the fee - // and try to select other inputs in the next loop step to cover the full required amount. - nAmountToSelectAdditional += abs(getChange()); - } else if (nAmountToSelectAdditional > 0 && nValueToSelect == nAmountAvailable) { - // We tried selecting more and failed. We have no extra funds left, - // so just add 1 duff to fail in the next loop step with a correct reason - nAmountToSelectAdditional += 1; - } - continue; - } - - if (sort_bip69) { - std::sort(txNew.vout.begin(), txNew.vout.end(), CompareOutputBIP69()); - - // If there was a change output added before, we must update its position now - if (const auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); - it != txNew.vout.end() && nChangePosInOut != -1) { - nChangePosInOut = std::distance(txNew.vout.begin(), it); - } - } - - if (nAmountLeft == nFeeRet) { - // We either added the change amount to nFeeRet because the change amount was considered - // to be dust or the input exactly matches output + fee. - // Either way, we used the total amount of the inputs we picked and the transaction is ready. - break; - } - - // We have a change output and we don't need to subtruct fees, which means the transaction is ready. - if (nChangePosInOut != -1 && nSubtractFeeFromAmount == 0) { - break; - } - - // If subtracting fee from recipients, we now know what fee we - // need to subtract, we have no reason to reselect inputs - if (nSubtractFeeFromAmount > 0) { - // If we are in here the second time it means we already subtracted the fee from the - // output(s) and there weren't any issues while doing that. So the transaction is ready now - // and we can break. - if (!pick_new_inputs) { - break; - } - pick_new_inputs = false; + nFeeRet = fee_needed; + break; // The fee has been deducted from the recipients, nothing left to do here } } @@ -964,9 +903,6 @@ bool CWallet::CreateTransactionInternal( } } - // Make sure change position was updated one way or another - assert(nChangePosInOut != std::numeric_limits::max()); - // Fill in final vin and shuffle/sort it txNew.vin.clear(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 9f97d8d3bd022..1d03d8f4f1473 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -262,6 +262,10 @@ struct CoinSelectionParams size_t change_output_size = 0; /** Size of the input to spend a change output in virtual bytes. */ size_t change_spend_size = 0; + /** Cost of creating the change output. */ + CAmount m_change_fee{0}; + /** Cost of creating the change output + cost of spending the change output in the future. */ + CAmount m_cost_of_change{0}; /** The fee to spend these UTXOs at the long term feerate. */ CFeeRate m_effective_feerate; /** The feerate estimate used to estimate an upper bound on what should be sufficient to spend From 7e54bd96254f9f3b0900346d306e48cb087725a2 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 2 Jan 2025 11:55:22 +0000 Subject: [PATCH 10/13] wallet: copy and sort `vecSend` if BIP69 sorting enabled, enable sorting --- src/wallet/spend.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index f8f04ffa5e386..2d42e9232a015 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -617,7 +617,7 @@ bool CWallet::CreateTransactionInternal( CAmount nValue = 0; ReserveDestination reservedest(this); int nChangePosRequest = nChangePosInOut; - const bool sort_bip69{false}; + const bool sort_bip69{nChangePosRequest == -1}; unsigned int nSubtractFeeFromAmount = 0; for (const auto& recipient : vecSend) { @@ -798,8 +798,17 @@ bool CWallet::CreateTransactionInternal( assert(nChangePosInOut != -1); auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); + // We're making a copy of vecSend because it's const, sortedVecSend should be used + // in place of vecSend in all subsequent usage. + std::vector sortedVecSend{vecSend}; if (sort_bip69) { std::sort(txNew.vout.begin(), txNew.vout.end(), CompareOutputBIP69()); + // The output reduction loop uses vecSend to map to txNew.vout, we need to + // shuffle them both to ensure this mapping remains consistent + std::sort(sortedVecSend.begin(), sortedVecSend.end(), + [](const CRecipient& a, const CRecipient& b) { + return a.nAmount < b.nAmount || (a.nAmount == b.nAmount && a.scriptPubKey < b.scriptPubKey); + }); // If there was a change output added before, we must update its position now if (const auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); it != txNew.vout.end()) { @@ -859,7 +868,7 @@ bool CWallet::CreateTransactionInternal( CAmount to_reduce = fee_needed + change_amount - change_and_fee; int i = 0; bool fFirst = true; - for (const auto& recipient : vecSend) + for (const auto& recipient : sortedVecSend) { if (i == nChangePosInOut) { ++i; From 445f489452b914d81b90d0b0ced1f75d2b0cd806 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 9 Feb 2025 09:21:44 +0000 Subject: [PATCH 11/13] merge bitcoin#17331: Use effective values throughout coin selection excludes: - 1bf4a62cb61bd4b91d9cd4e379fea2b914786342 - af5867c89688b06173b295b7c32a42845ea455da - d97d25d95006725e705635530b27643363d6b2a4 - cc3f14b27c06b7a0da1472f5c7100c3f0b76fd98 --- src/bench/coin_selection.cpp | 8 +- src/wallet/coinselection.cpp | 40 +-- src/wallet/coinselection.h | 52 +++- src/wallet/spend.cpp | 361 +++++++++++-------------- src/wallet/test/coinselector_tests.cpp | 148 +++++----- src/wallet/wallet.h | 48 +--- 6 files changed, 305 insertions(+), 352 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index e53755c046c19..da29c8bddbb92 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -48,15 +48,14 @@ static void CoinSelection(benchmark::Bench& bench) coins.emplace_back(wtx.get(), 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */); } const CoinEligibilityFilter filter_standard(1, 6, 0); - const CoinSelectionParams coin_selection_params(/* use_bnb= */ true, /* change_output_size= */ 34, + const CoinSelectionParams coin_selection_params(/* change_output_size= */ 34, /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); bench.run([&] { std::set setCoinsRet; CAmount nValueRet; - bool bnb_used; - bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used); + bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params); assert(success); assert(nValueRet == 1003 * COIN); assert(setCoinsRet.size() == 2); @@ -94,12 +93,11 @@ static void BnBExhaustion(benchmark::Bench& bench) std::vector utxo_pool; CoinSet selection; CAmount value_ret = 0; - CAmount not_input_fees = 0; bench.run([&] { // Benchmark CAmount target = make_hard_case(17, utxo_pool); - SelectCoinsBnB(utxo_pool, target, 0, selection, value_ret, not_input_fees); // Should exhaust + SelectCoinsBnB(utxo_pool, target, 0, selection, value_ret); // Should exhaust // Cleanup utxo_pool.clear(); diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index e3919105023fc..a657b282c3b07 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -16,7 +16,7 @@ struct { bool operator()(const OutputGroup& a, const OutputGroup& b) const { - return a.effective_value > b.effective_value; + return a.GetSelectionAmount() > b.GetSelectionAmount(); } } descending; @@ -51,35 +51,32 @@ struct { * @param const std::vector& utxo_pool The set of UTXOs that we are choosing from. * These UTXOs will be sorted in descending order by effective value and the CInputCoins' * values are their effective values. - * @param const CAmount& target_value This is the value that we want to select. It is the lower + * @param const CAmount& selection_target This is the value that we want to select. It is the lower * bound of the range. * @param const CAmount& cost_of_change This is the cost of creating and spending a change output. - * This plus target_value is the upper bound of the range. + * This plus selection_target is the upper bound of the range. * @param std::set& out_set -> This is an output parameter for the set of CInputCoins * that have been selected. * @param CAmount& value_ret -> This is an output parameter for the total value of the CInputCoins * that were selected. - * @param CAmount not_input_fees -> The fees that need to be paid for the outputs and fixed size - * overhead (version, locktime, marker and flag) */ static const size_t TOTAL_TRIES = 100000; -bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret, CAmount not_input_fees) +bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret) { out_set.clear(); CAmount curr_value = 0; std::vector curr_selection; // select the utxo at this index curr_selection.reserve(utxo_pool.size()); - CAmount selection_target = not_input_fees + target_value; // Calculate curr_available_value CAmount curr_available_value = 0; for (const OutputGroup& utxo : utxo_pool) { // Assert that this utxo is not negative. It should never be negative, effective value calculation should have removed it - assert(utxo.effective_value > 0); - curr_available_value += utxo.effective_value; + assert(utxo.GetSelectionAmount() > 0); + curr_available_value += utxo.GetSelectionAmount(); } if (curr_available_value < selection_target) { return false; @@ -123,7 +120,7 @@ bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_v // Walk backwards to find the last included UTXO that still needs to have its omission branch traversed. while (!curr_selection.empty() && !curr_selection.back()) { curr_selection.pop_back(); - curr_available_value += utxo_pool.at(curr_selection.size()).effective_value; + curr_available_value += utxo_pool.at(curr_selection.size()).GetSelectionAmount(); } if (curr_selection.empty()) { // We have walked back to the first utxo and no branch is untraversed. All solutions searched @@ -133,24 +130,24 @@ bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_v // Output was included on previous iterations, try excluding now. curr_selection.back() = false; OutputGroup& utxo = utxo_pool.at(curr_selection.size() - 1); - curr_value -= utxo.effective_value; + curr_value -= utxo.GetSelectionAmount(); curr_waste -= utxo.fee - utxo.long_term_fee; } else { // Moving forwards, continuing down this branch OutputGroup& utxo = utxo_pool.at(curr_selection.size()); // Remove this utxo from the curr_available_value utxo amount - curr_available_value -= utxo.effective_value; + curr_available_value -= utxo.GetSelectionAmount(); // Avoid searching a branch if the previous UTXO has the same value and same waste and was excluded. Since the ratio of fee to // long term fee is the same, we only need to check if one of those values match in order to know that the waste is the same. if (!curr_selection.empty() && !curr_selection.back() && - utxo.effective_value == utxo_pool.at(curr_selection.size() - 1).effective_value && + utxo.GetSelectionAmount() == utxo_pool.at(curr_selection.size() - 1).GetSelectionAmount() && utxo.fee == utxo_pool.at(curr_selection.size() - 1).fee) { curr_selection.push_back(false); } else { // Inclusion branch first (Largest First Exploration) curr_selection.push_back(true); - curr_value += utxo.effective_value; + curr_value += utxo.GetSelectionAmount(); curr_waste += utxo.fee - utxo.long_term_fee; } } @@ -283,14 +280,14 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group if (tryDenom == 0 && CoinJoin::IsDenominatedAmount(group.m_value)) { continue; // we don't want denom values on first run } - if (group.m_value == nTargetValue) { + if (group.GetSelectionAmount() == nTargetValue) { util::insert(setCoinsRet, group.m_outputs); nValueRet += group.m_value; return true; - } else if (group.m_value < nTargetValue + nMinChange) { + } else if (group.GetSelectionAmount() < nTargetValue + nMinChange) { applicable_groups.push_back(group); - nTotalLower += group.m_value; - } else if (!lowest_larger || group.m_value < lowest_larger->m_value) { + nTotalLower += group.GetSelectionAmount(); + } else if (!lowest_larger || group.GetSelectionAmount() < lowest_larger->GetSelectionAmount()) { lowest_larger = group; } } @@ -336,7 +333,7 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group // If we have a bigger coin and (either the stochastic approximation didn't find a good solution, // or the next bigger coin is closer), return the bigger coin if (lowest_larger && - ((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || lowest_larger->m_value <= nBest)) { + ((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || lowest_larger->GetSelectionAmount() <= nBest)) { util::insert(setCoinsRet, lowest_larger->m_outputs); nValueRet += lowest_larger->m_value; } else { @@ -400,3 +397,8 @@ bool OutputGroup::EligibleForSpending(const CoinEligibilityFilter& eligibility_f && m_ancestors <= eligibility_filter.max_ancestors && m_descendants <= eligibility_filter.max_descendants; } + +CAmount OutputGroup::GetSelectionAmount() const +{ + return m_subtract_fee_outputs ? m_value : effective_value; +} diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 487636ffa413c..cbe80d87c682b 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -57,6 +57,45 @@ class CInputCoin { } }; +/** Parameters for one iteration of Coin Selection. */ +struct CoinSelectionParams +{ + /** Size of a change output in bytes, determined by the output type. */ + size_t change_output_size = 0; + /** Size of the input to spend a change output in virtual bytes. */ + size_t change_spend_size = 0; + /** Cost of creating the change output. */ + CAmount m_change_fee{0}; + /** Cost of creating the change output + cost of spending the change output in the future. */ + CAmount m_cost_of_change{0}; + /** The fee to spend these UTXOs at the long term feerate. */ + CFeeRate m_effective_feerate; + /** The feerate estimate used to estimate an upper bound on what should be sufficient to spend + * the change output sometime in the future. */ + CFeeRate m_long_term_feerate; + /** If the cost to spend a change output at the discard feerate exceeds its value, drop it to fees. */ + CFeeRate m_discard_feerate; + size_t tx_noinputs_size = 0; + /** Indicate that we are subtracting the fee from outputs */ + bool m_subtract_fee_outputs = false; + /** When true, always spend all (up to OUTPUT_GROUP_MAX_ENTRIES) or none of the outputs + * associated with the same address. This helps reduce privacy leaks resulting from address + * reuse. Dust outputs are not eligible to be added to output groups and thus not considered. */ + bool m_avoid_partial_spends = false; + + CoinSelectionParams(size_t change_output_size, size_t change_spend_size, CFeeRate effective_feerate, + CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial) : + change_output_size(change_output_size), + change_spend_size(change_spend_size), + m_effective_feerate(effective_feerate), + m_long_term_feerate(long_term_feerate), + m_discard_feerate(discard_feerate), + tx_noinputs_size(tx_noinputs_size), + m_avoid_partial_spends(avoid_partial) + {} + CoinSelectionParams() {} +}; + /** Parameters for filtering which OutputGroups we may use in coin selection. * We start by being very selective and requiring multiple confirmations and * then get more permissive if we cannot fund the transaction. */ @@ -109,18 +148,23 @@ struct OutputGroup * a lower feerate). Calculated using long term fee estimate. This is used to decide whether * it could be economical to create a change output. */ CFeeRate m_long_term_feerate{0}; + /** Indicate that we are subtracting the fee from outputs. + * When true, the value that is used for coin selection is the UTXO's real value rather than effective value */ + bool m_subtract_fee_outputs{false}; OutputGroup() {} - OutputGroup(const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate) : - m_effective_feerate(effective_feerate), - m_long_term_feerate(long_term_feerate) + OutputGroup(const CoinSelectionParams& params) : + m_effective_feerate(params.m_effective_feerate), + m_long_term_feerate(params.m_long_term_feerate), + m_subtract_fee_outputs(params.m_subtract_fee_outputs) {} void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants, bool positive_only); bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter, bool isISLocked) const; + CAmount GetSelectionAmount() const; }; -bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret, CAmount not_input_fees); +bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret); // Original coin selection algorithm as a fallback bool KnapsackSolver(const CAmount& nTargetValue, std::vector& groups, std::set& setCoinsRet, CAmount& nValueRet, bool fFulyMixedOnly, CAmount maxTxFee); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 2d42e9232a015..61d5ab1e8f0bd 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -271,12 +271,12 @@ static bool isGroupISLocked(const OutputGroup& group, interfaces::Chain& chain) }); } -std::vector CWallet::GroupOutputs(const std::vector& outputs, bool separate_coins, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const +std::vector CWallet::GroupOutputs(const std::vector& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only) const { std::vector groups_out; - if (separate_coins) { - // Single coin means no grouping. Each COutput gets its own OutputGroup. + if (!coin_sel_params.m_avoid_partial_spends) { + // Allowing partial spends means no grouping. Each COutput gets its own OutputGroup. for (const COutput& output : outputs) { // Skip outputs we cannot spend if (!output.fSpendable) continue; @@ -286,11 +286,11 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu CInputCoin input_coin = output.GetInputCoin(); // Make an OutputGroup containing just this output - OutputGroup group{effective_feerate, long_term_feerate}; + OutputGroup group{coin_sel_params}; group.Insert(input_coin, output.nDepth, output.tx->IsFromMe(ISMINE_ALL), ancestors, descendants, positive_only); // Check the OutputGroup's eligibility. Only add the eligible ones. - if (positive_only && group.effective_value <= 0) continue; + if (positive_only && group.GetSelectionAmount() <= 0) continue; bool isISLocked = isGroupISLocked(group, chain()); if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter, isISLocked)) groups_out.push_back(group); } @@ -317,7 +317,7 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu if (groups.size() == 0) { // No OutputGroups for this scriptPubKey yet, add one - groups.emplace_back(effective_feerate, long_term_feerate); + groups.emplace_back(coin_sel_params); } // Get the last OutputGroup in the vector so that we can add the CInputCoin to it @@ -328,7 +328,7 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu // to avoid surprising users with very high fees. if (group->m_outputs.size() >= OUTPUT_GROUP_MAX_ENTRIES) { // The last output group is full, add a new group to the vector and use that group for the insertion - groups.emplace_back(effective_feerate, long_term_feerate); + groups.emplace_back(coin_sel_params); group = &groups.back(); } @@ -350,7 +350,7 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu } // Check the OutputGroup's eligibility. Only add the eligible ones. - if (positive_only && group.effective_value <= 0) continue; + if (positive_only && group.GetSelectionAmount() <= 0) continue; bool isISLocked = isGroupISLocked(group, chain()); if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter, isISLocked)) groups_out.push_back(group); } @@ -360,35 +360,24 @@ std::vector CWallet::GroupOutputs(const std::vector& outpu } bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector coins, - std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool& bnb_used, CoinType nCoinType) const + std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType) const { setCoinsRet.clear(); nValueRet = 0; - // Calculate the fees for things that aren't inputs, excluding the change output - const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); - - // Get the feerate for effective value. - // When subtracting the fee from the outputs, we want the effective feerate to be 0 - CFeeRate effective_feerate{0}; - if (!coin_selection_params.m_subtract_fee_outputs) { - effective_feerate = coin_selection_params.m_effective_feerate; - } - - if (coin_selection_params.use_bnb) { - std::vector positive_groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, coin_selection_params.m_long_term_feerate, eligibility_filter, true /* positive_only */); - bnb_used = true; - return SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change, setCoinsRet, nValueRet, not_input_fees); - } else { - // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here. - // The knapsack solver currently does not use effective values, so we give GroupOutputs feerates of 0 so it sets the effective values to be the same as the real value. - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, CFeeRate(0), CFeeRate(0), eligibility_filter, false /* positive_only */); - bnb_used = false; - return KnapsackSolver(nTargetValue, groups, setCoinsRet, nValueRet, nCoinType == CoinType::ONLY_FULLY_MIXED, m_default_max_tx_fee); + // Note that unlike KnapsackSolver, we do not include the fee for creating a change output as BnB will not create a change output. + std::vector positive_groups = GroupOutputs(coins, coin_selection_params, eligibility_filter, true /* positive_only */); + if (SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change, setCoinsRet, nValueRet)) { + return true; } + // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here. + std::vector all_groups = GroupOutputs(coins, coin_selection_params, eligibility_filter, false /* positive_only */); + // While nTargetValue includes the transaction fees for non-input things, it does not include the fee for creating a change output. + // So we need to include that for KnapsackSolver as well, as we are expecting to create a change output. + return KnapsackSolver(nTargetValue + coin_selection_params.m_change_fee, all_groups, setCoinsRet, nValueRet, nCoinType == CoinType::ONLY_FULLY_MIXED, m_default_max_tx_fee); } -bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params, bool& bnb_used) const +bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params) const { // Note: this function should never be used for "always free" tx types like dstx @@ -396,9 +385,6 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm CoinType nCoinType = coin_control.nCoinType; CAmount value_to_select = nTargetValue; - // Default to bnb was not used. If we use it, we set it later - bnb_used = false; - // coin control -> return all selected outputs (we want all selected to go into the transaction for sure) if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs) { @@ -447,10 +433,10 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm return false; // Not solvable, can't estimate size for fee } coin.effective_value = coin.txout.nValue - coin_selection_params.m_effective_feerate.GetFee(coin.m_input_bytes); - if (coin_selection_params.use_bnb) { - value_to_select -= coin.effective_value; - } else { + if (coin_selection_params.m_subtract_fee_outputs) { value_to_select -= coin.txout.nValue; + } else { + value_to_select -= coin.effective_value; } setPresetCoins.insert(coin); } else { @@ -491,26 +477,26 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm // If possible, fund the transaction with confirmed UTXOs only. Prefer at least six // confirmations on outputs received from other wallets and only spend confirmed change. - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) return true; - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) return true; + if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) return true; + if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) return true; // Fall back to using zero confirmation change (but with as few ancestors in the mempool as // possible) if we cannot fund the transaction otherwise. if (m_spend_zero_conf_change) { - if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) return true; + if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) return true; if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), - vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) { + vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { return true; } if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), - vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) { + vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { return true; } // If partial groups are allowed, relax the requirement of spending OutputGroups (groups // of UTXOs sent to the same address, which are obviously controlled by a single wallet) // in their entirety. if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), - vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) { + vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { return true; } // Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs @@ -518,7 +504,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm if (coin_control.m_include_unsafe_inputs && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0 /* conf_mine */, 0 /* conf_theirs */, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), - vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) { + vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { return true; } // Try with unlimited ancestors/descendants. The transaction will still need to meet @@ -526,7 +512,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm // OutputGroups use heuristics that may overestimate ancestor/descendant counts. if (!fRejectLongChains && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits::max(), std::numeric_limits::max(), true /* include_partial_groups */), - vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) { + vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { return true; } } @@ -616,8 +602,7 @@ bool CWallet::CreateTransactionInternal( { CAmount nValue = 0; ReserveDestination reservedest(this); - int nChangePosRequest = nChangePosInOut; - const bool sort_bip69{nChangePosRequest == -1}; + const bool sort_bip69{nChangePosInOut == -1}; unsigned int nSubtractFeeFromAmount = 0; for (const auto& recipient : vecSend) { @@ -721,189 +706,167 @@ bool CWallet::CreateTransactionInternal( coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size); coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_change_fee; - nFeeRet = 0; - CAmount nValueIn = 0; - - // BnB selector is the only selector used when this is true. - coin_selection_params.use_bnb = false; // Dash: never use BnB coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values - // Start with no fee and loop until there is enough fee, try it 500 times. - int nMaxTries = 500; - while (--nMaxTries > 0) + // vouts to the payees + if (!coin_selection_params.m_subtract_fee_outputs) { + coin_selection_params.tx_noinputs_size = 9; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count + coin_selection_params.tx_noinputs_size += GetSizeOfCompactSize(vecSend.size()); // bytes for output count + } + for (const auto& recipient : vecSend) { - nChangePosInOut = nChangePosRequest; - txNew.vin.clear(); - txNew.vout.clear(); + CTxOut txout(recipient.nAmount, recipient.scriptPubKey); - CAmount nValueToSelect = nValue; - if (nSubtractFeeFromAmount == 0) - nValueToSelect += nFeeRet; - - // vouts to the payees + // Include the fee cost for outputs. if (!coin_selection_params.m_subtract_fee_outputs) { - coin_selection_params.tx_noinputs_size = 9; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count - coin_selection_params.tx_noinputs_size += GetSizeOfCompactSize(vecSend.size()); // bytes for output count + coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); } - for (const auto& recipient : vecSend) - { - CTxOut txout(recipient.nAmount, recipient.scriptPubKey); - // Include the fee cost for outputs. - if (!coin_selection_params.m_subtract_fee_outputs) { - coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); - } - - if (IsDust(txout, chain().relayDustFee())) - { - error = _("Transaction amount too small"); - return false; - } - txNew.vout.push_back(txout); + if (IsDust(txout, chain().relayDustFee())) + { + error = _("Transaction amount too small"); + return false; } + txNew.vout.push_back(txout); + } - // Choose coins to use - bool bnb_used = false; - nValueIn = 0; - setCoins.clear(); - if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coin_control, coin_selection_params, bnb_used)) { - if (coin_control.nCoinType == CoinType::ONLY_NONDENOMINATED) { - error = _("Unable to locate enough non-denominated funds for this transaction."); - } else if (coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { - error = _("Unable to locate enough mixed funds for this transaction."); - error = error + Untranslated(" ") + strprintf(_("%s uses exact denominated amounts to send funds, you might simply need to mix some more coins."), gCoinJoinName); - } else { - error = _("Insufficient funds."); - } - return false; + // Include the fees for things that aren't inputs, excluding the change output + const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); + CAmount nValueToSelect = nValue + not_input_fees; + + // Choose coins to use + CAmount inputs_sum = 0; + setCoins.clear(); + if (!SelectCoins(vAvailableCoins, /* nTargetValue */ nValueToSelect, setCoins, inputs_sum, coin_control, coin_selection_params)) { + if (coin_control.nCoinType == CoinType::ONLY_NONDENOMINATED) { + error = _("Unable to locate enough non-denominated funds for this transaction."); + } else if (coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { + error = _("Unable to locate enough mixed funds for this transaction."); + error = error + Untranslated(" ") + strprintf(_("%s uses exact denominated amounts to send funds, you might simply need to mix some more coins."), gCoinJoinName); + } else { + error = _("Insufficient funds."); } + return false; + } - // Always make a change output - // We will reduce the fee from this change output later, and remove the output if it is too small. - const CAmount change_and_fee = nValueIn - nValue; - assert(change_and_fee >= 0); - CTxOut newTxOut(change_and_fee, scriptChange); + // Always make a change output + // We will reduce the fee from this change output later, and remove the output if it is too small. + const CAmount change_and_fee = inputs_sum - nValue; + assert(change_and_fee >= 0); + CTxOut newTxOut(change_and_fee, scriptChange); - if (nChangePosInOut == -1) - { - // Insert change txn at random position: - nChangePosInOut = GetRandInt(txNew.vout.size()+1); - } - else if ((unsigned int)nChangePosInOut > txNew.vout.size()) - { - error = _("Transaction change output index out of range"); - return false; + if (nChangePosInOut == -1) + { + // Insert change txn at random position: + nChangePosInOut = GetRandInt(txNew.vout.size()+1); + } + else if ((unsigned int)nChangePosInOut > txNew.vout.size()) + { + error = _("Transaction change output index out of range"); + return false; + } + + assert(nChangePosInOut != -1); + auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); + + // We're making a copy of vecSend because it's const, sortedVecSend should be used + // in place of vecSend in all subsequent usage. + std::vector sortedVecSend{vecSend}; + if (sort_bip69) { + std::sort(txNew.vout.begin(), txNew.vout.end(), CompareOutputBIP69()); + // The output reduction loop uses vecSend to map to txNew.vout, we need to + // shuffle them both to ensure this mapping remains consistent + std::sort(sortedVecSend.begin(), sortedVecSend.end(), + [](const CRecipient& a, const CRecipient& b) { + return a.nAmount < b.nAmount || (a.nAmount == b.nAmount && a.scriptPubKey < b.scriptPubKey); + }); + + // If there was a change output added before, we must update its position now + if (const auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); it != txNew.vout.end()) { + change_position = it; + nChangePosInOut = std::distance(txNew.vout.begin(), change_position); } + }; - assert(nChangePosInOut != -1); - auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); - - // We're making a copy of vecSend because it's const, sortedVecSend should be used - // in place of vecSend in all subsequent usage. - std::vector sortedVecSend{vecSend}; - if (sort_bip69) { - std::sort(txNew.vout.begin(), txNew.vout.end(), CompareOutputBIP69()); - // The output reduction loop uses vecSend to map to txNew.vout, we need to - // shuffle them both to ensure this mapping remains consistent - std::sort(sortedVecSend.begin(), sortedVecSend.end(), - [](const CRecipient& a, const CRecipient& b) { - return a.nAmount < b.nAmount || (a.nAmount == b.nAmount && a.scriptPubKey < b.scriptPubKey); - }); - - // If there was a change output added before, we must update its position now - if (const auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); it != txNew.vout.end()) { - change_position = it; - nChangePosInOut = std::distance(txNew.vout.begin(), change_position); - } - }; + // Dummy fill vin for maximum size estimation + // + for (const auto& coin : setCoins) { + txNew.vin.push_back(CTxIn(coin.outpoint, CScript())); + } - // Dummy fill vin for maximum size estimation - // - for (const auto& coin : setCoins) { - txNew.vin.push_back(CTxIn(coin.outpoint, CScript())); - } + // Calculate the transaction fee + nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); + if (nBytes < 0) { + error = _("Signing transaction failed"); + return false; + } - // Calculate the transaction fee - nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); - if (nBytes < 0) { - error = _("Signing transaction failed"); - return false; - } + if (nExtraPayloadSize != 0) { + // account for extra payload in fee calculation + nBytes += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize; + } - if (nExtraPayloadSize != 0) { - // account for extra payload in fee calculation - nBytes += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize; - } + nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes); - nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes); + CAmount fee_needed = nFeeRet; + if (nSubtractFeeFromAmount == 0) { + change_position->nValue -= fee_needed; + } - CAmount fee_needed = nFeeRet; - if (nSubtractFeeFromAmount == 0) { - change_position->nValue -= fee_needed; - } + // We want to drop the change to fees if: + // 1. The change output would be dust + // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change) + // 3. We are working with fully mixed CoinJoin denominations + CAmount change_amount = change_position->nValue; + if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change || coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) + { + nChangePosInOut = -1; + change_amount = 0; + txNew.vout.erase(change_position); - // We want to drop the change to fees if: - // 1. The change output would be dust - // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change) - // 3. We are working with fully mixed CoinJoin denominations - CAmount change_amount = change_position->nValue; - if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change || coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) - { - nChangePosInOut = -1; - change_amount = 0; - txNew.vout.erase(change_position); + nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); + fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); + } - nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); - fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); - } + // Update nFeeRet in case fee_needed changed due to dropping the change output + if (fee_needed <= change_and_fee - change_amount) { + nFeeRet = change_and_fee - change_amount; + } - // If the fee is covered, there's no need to loop or subtract from recipients - if (fee_needed <= change_and_fee - change_amount) { - nFeeRet = change_and_fee - change_amount; - break; - } + // Reduce output values for subtractFeeFromAmount + if (nSubtractFeeFromAmount != 0) { + CAmount to_reduce = fee_needed + change_amount - change_and_fee; + int i = 0; + bool fFirst = true; + for (const auto& recipient : sortedVecSend) + { + if (i == nChangePosInOut) { + ++i; + } + CTxOut& txout = txNew.vout[i]; - // Reduce output values for subtractFeeFromAmount - if (nSubtractFeeFromAmount != 0) { - CAmount to_reduce = fee_needed + change_amount - change_and_fee; - int i = 0; - bool fFirst = true; - for (const auto& recipient : sortedVecSend) + if (recipient.fSubtractFeeFromAmount) { - if (i == nChangePosInOut) { - ++i; - } - CTxOut& txout = txNew.vout[i]; + txout.nValue -= to_reduce / nSubtractFeeFromAmount; // Subtract fee equally from each selected recipient - if (recipient.fSubtractFeeFromAmount) + if (fFirst) // first receiver pays the remainder not divisible by output count { - txout.nValue -= to_reduce / nSubtractFeeFromAmount; // Subtract fee equally from each selected recipient - - if (fFirst) // first receiver pays the remainder not divisible by output count - { - fFirst = false; - txout.nValue -= to_reduce % nSubtractFeeFromAmount; - } - // Error if this output is reduced to be below dust - if (IsDust(txout, chain().relayDustFee())) { - if (txout.nValue < 0) { - error = _("The transaction amount is too small to pay the fee"); - } else { - error = _("The transaction amount is too small to send after the fee has been deducted"); - } - return false; + fFirst = false; + txout.nValue -= to_reduce % nSubtractFeeFromAmount; + } + // Error if this output is reduced to be below dust + if (IsDust(txout, chain().relayDustFee())) { + if (txout.nValue < 0) { + error = _("The transaction amount is too small to pay the fee"); + } else { + error = _("The transaction amount is too small to send after the fee has been deducted"); } + return false; } - ++i; } - nFeeRet = fee_needed; - break; // The fee has been deducted from the recipients, nothing left to do here + ++i; } - } - - if (nMaxTries == 0) { - error = _("Exceeded max tries."); - return false; + nFeeRet = fee_needed; } // Give up if change keypool ran out and change is required diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index e7731a9c4db3c..a6d26482464a6 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -34,10 +34,10 @@ static const CoinEligibilityFilter filter_standard(1, 6, 0); static const CoinEligibilityFilter filter_confirmed(1, 1, 0); static const CoinEligibilityFilter filter_standard_extra(6, 6, 0); -CoinSelectionParams coin_selection_params(/* use_bnb= */ false, /* change_output_size= */ 0, +CoinSelectionParams coin_selection_params(/* change_output_size= */ 0, /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(0), /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), - /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); + /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); static void add_coin(const CAmount& nValue, int nInput, std::vector& set) { @@ -132,14 +132,13 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) CoinSet selection; CoinSet actual_selection; CAmount value_ret = 0; - CAmount not_input_fees = 0; ///////////////////////// // Known Outcome tests // ///////////////////////// // Empty utxo pool - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret)); selection.clear(); // Add utxos @@ -150,7 +149,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) // Select 1 Cent add_coin(1 * CENT, 1, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret)); BOOST_CHECK(equal_sets(selection, actual_selection)); BOOST_CHECK_EQUAL(value_ret, 1 * CENT); actual_selection.clear(); @@ -158,7 +157,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) // Select 2 Cent add_coin(2 * CENT, 2, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 2 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 2 * CENT, 0.5 * CENT, selection, value_ret)); BOOST_CHECK(equal_sets(selection, actual_selection)); BOOST_CHECK_EQUAL(value_ret, 2 * CENT); actual_selection.clear(); @@ -167,27 +166,27 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) // Select 5 Cent add_coin(4 * CENT, 4, actual_selection); add_coin(1 * CENT, 1, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 5 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 5 * CENT, 0.5 * CENT, selection, value_ret)); BOOST_CHECK(equal_sets(selection, actual_selection)); BOOST_CHECK_EQUAL(value_ret, 5 * CENT); actual_selection.clear(); selection.clear(); // Select 11 Cent, not possible - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 11 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 11 * CENT, 0.5 * CENT, selection, value_ret)); actual_selection.clear(); selection.clear(); // Cost of change is greater than the difference between target value and utxo sum add_coin(1 * CENT, 1, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0.5 * CENT, selection, value_ret)); BOOST_CHECK_EQUAL(value_ret, 1 * CENT); BOOST_CHECK(equal_sets(selection, actual_selection)); actual_selection.clear(); selection.clear(); // Cost of change is less than the difference between target value and utxo sum - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0, selection, value_ret, not_input_fees)); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0, selection, value_ret)); actual_selection.clear(); selection.clear(); @@ -196,7 +195,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(5 * CENT, 5, actual_selection); add_coin(4 * CENT, 4, actual_selection); add_coin(1 * CENT, 1, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 0.5 * CENT, selection, value_ret)); BOOST_CHECK(equal_sets(selection, actual_selection)); BOOST_CHECK_EQUAL(value_ret, 10 * CENT); actual_selection.clear(); @@ -207,21 +206,21 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(5 * CENT, 5, actual_selection); add_coin(3 * CENT, 3, actual_selection); add_coin(2 * CENT, 2, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 5000, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 5000, selection, value_ret)); BOOST_CHECK_EQUAL(value_ret, 10 * CENT); // FIXME: this test is redundant with the above, because 1 Cent is selected, not "too small" // BOOST_CHECK(equal_sets(selection, actual_selection)); // Select 0.25 Cent, not possible - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.25 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.25 * CENT, 0.5 * CENT, selection, value_ret)); actual_selection.clear(); selection.clear(); // Iteration exhaustion test CAmount target = make_hard_case(17, utxo_pool); - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret, not_input_fees)); // Should exhaust + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret)); // Should exhaust target = make_hard_case(14, utxo_pool); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret, not_input_fees)); // Should not exhaust + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret)); // Should not exhaust // Test same value early bailout optimization utxo_pool.clear(); @@ -238,7 +237,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) for (int i = 0; i < 50000; ++i) { add_coin(5 * CENT, 7, utxo_pool); } - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 30 * CENT, 5000, selection, value_ret, not_input_fees)); + BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 30 * CENT, 5000, selection, value_ret)); BOOST_CHECK_EQUAL(value_ret, 30 * CENT); BOOST_CHECK(equal_sets(selection, actual_selection)); @@ -252,35 +251,34 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) } // Run 100 times, to make sure it is never finding a solution for (int i = 0; i < 100; ++i) { - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 2 * CENT, selection, value_ret, not_input_fees)); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 2 * CENT, selection, value_ret)); } // Make sure that effective value is working in SelectCoinsMinConf when BnB is used - CoinSelectionParams coin_selection_params_bnb(/* use_bnb= */ true, /* change_output_size= */ 0, + CoinSelectionParams coin_selection_params_bnb(/* change_output_size= */ 0, /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(3000), /* long_term_feerate= */ CFeeRate(1000), /* discard_feerate= */ CFeeRate(1000), - /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); + /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); { std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); wallet->LoadWallet(); LOCK(wallet->cs_wallet); - bool bnb_used; std::vector coins; CoinSet setCoinsRet; CAmount nValueRet; add_coin(coins, *wallet, 1); coins.at(0).nInputBytes = 40; // Make sure that it has a negative effective value. The next check should assert if this somehow got through. Otherwise it will fail - BOOST_CHECK(!wallet->SelectCoinsMinConf(1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); + BOOST_CHECK(!wallet->SelectCoinsMinConf(1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params_bnb)); // Test fees subtracted from output: coins.clear(); add_coin(coins, *wallet, 1 * CENT); coins.at(0).nInputBytes = 40; coin_selection_params_bnb.m_subtract_fee_outputs = true; - BOOST_CHECK(wallet->SelectCoinsMinConf(1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params_bnb)); BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); } @@ -290,7 +288,6 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) wallet->SetupLegacyScriptPubKeyMan(); LOCK(wallet->cs_wallet); - bool bnb_used; std::vector coins; CoinSet setCoinsRet; CAmount nValueRet; @@ -302,9 +299,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_control.fAllowOtherInputs = true; coin_control.Select(COutPoint(coins.at(0).tx->GetHash(), coins.at(0).i)); coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); - BOOST_CHECK(wallet->SelectCoins(coins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb, bnb_used)); - BOOST_CHECK(bnb_used); - BOOST_CHECK(coin_selection_params_bnb.use_bnb); + BOOST_CHECK(wallet->SelectCoins(coins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb)); } } @@ -317,7 +312,6 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) CoinSet setCoinsRet, setCoinsRet2; CAmount nValueRet; - bool bnb_used; std::vector coins; // test multiple times to allow for differences in the shuffle order @@ -326,24 +320,24 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) coins.clear(); // with an empty wallet we can't even pay one cent - BOOST_CHECK(!wallet->SelectCoinsMinConf( 1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!wallet->SelectCoinsMinConf( 1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params)); add_coin(coins, *wallet, 1*CENT, 4); // add a new 1 cent coin // with a new 1 cent coin, we still can't find a mature 1 cent - BOOST_CHECK(!wallet->SelectCoinsMinConf( 1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!wallet->SelectCoinsMinConf( 1 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params)); // but we can find a new 1 cent - BOOST_CHECK(wallet->SelectCoinsMinConf( 1 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf( 1 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); add_coin(coins, *wallet, 2*CENT); // add a mature 2 cent coin // we can't make 3 cents of mature coins - BOOST_CHECK(!wallet->SelectCoinsMinConf( 3 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!wallet->SelectCoinsMinConf( 3 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params)); // we can make 3 cents of new coins - BOOST_CHECK(wallet->SelectCoinsMinConf( 3 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf( 3 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 3 * CENT); add_coin(coins, *wallet, 5*CENT); // add a mature 5 cent coin, @@ -353,33 +347,33 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // now we have new: 1+10=11 (of which 10 was self-sent), and mature: 2+5+20=27. total = 38 // we can't make 38 cents only if we disallow new coins: - BOOST_CHECK(!wallet->SelectCoinsMinConf(38 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!wallet->SelectCoinsMinConf(38 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params)); // we can't even make 37 cents if we don't allow new coins even if they're from us - BOOST_CHECK(!wallet->SelectCoinsMinConf(38 * CENT, filter_standard_extra, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(!wallet->SelectCoinsMinConf(38 * CENT, filter_standard_extra, coins, setCoinsRet, nValueRet, coin_selection_params)); // but we can make 37 cents if we accept new coins from ourself - BOOST_CHECK(wallet->SelectCoinsMinConf(37 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(37 * CENT, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 37 * CENT); // and we can make 38 cents if we accept all new coins - BOOST_CHECK(wallet->SelectCoinsMinConf(38 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(38 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 38 * CENT); // try making 34 cents from 1,2,5,10,20 - we can't do it exactly - BOOST_CHECK(wallet->SelectCoinsMinConf(34 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(34 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 35 * CENT); // but 35 cents is closest BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // the best should be 20+10+5. it's incredibly unlikely the 1 or 2 got included (but possible) // when we try making 7 cents, the smaller coins (1,2,5) are enough. We should see just 2+5 - BOOST_CHECK(wallet->SelectCoinsMinConf( 7 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf( 7 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 7 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); // when we try making 8 cents, the smaller coins (1,2,5) are exactly enough. - BOOST_CHECK(wallet->SelectCoinsMinConf( 8 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf( 8 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK(nValueRet == 8 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // when we try making 9 cents, no subset of smaller coins is enough, and we get the next bigger coin (10) - BOOST_CHECK(wallet->SelectCoinsMinConf( 9 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf( 9 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 10 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -393,30 +387,30 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, 30*CENT); // now we have 6+7+8+20+30 = 71 cents total // check that we have 71 and not 72 - BOOST_CHECK(wallet->SelectCoinsMinConf(71 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); - BOOST_CHECK(!wallet->SelectCoinsMinConf(72 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(71 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); + BOOST_CHECK(!wallet->SelectCoinsMinConf(72 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); // now try making 16 cents. the best smaller coins can do is 6+7+8 = 21; not as good at the next biggest coin, 20 - BOOST_CHECK(wallet->SelectCoinsMinConf(16 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(16 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 20 * CENT); // we should get 20 in one coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); add_coin(coins, *wallet, 5*CENT); // now we have 5+6+7+8+20+30 = 75 cents total // now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, better than the next biggest coin, 20 - BOOST_CHECK(wallet->SelectCoinsMinConf(16 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(16 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 18 * CENT); // we should get 18 in 3 coins BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); add_coin(coins, *wallet, 18*CENT); // now we have 5+6+7+8+18+20+30 // and now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, the same as the next biggest coin, 18 - BOOST_CHECK(wallet->SelectCoinsMinConf(16 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(16 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 18 * CENT); // we should get 18 in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); // because in the event of a tie, the biggest coin wins // now try making 11 cents. we should get 5+6 - BOOST_CHECK(wallet->SelectCoinsMinConf(11 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(11 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 11 * CENT); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); @@ -425,11 +419,11 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, 2*COIN); add_coin(coins, *wallet, 3*COIN); add_coin(coins, *wallet, 4*COIN); // now we have 5+6+7+8+18+20+30+100+200+300+400 = 1094 cents - BOOST_CHECK(wallet->SelectCoinsMinConf(95 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(95 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * COIN); // we should get 1 BTC in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); - BOOST_CHECK(wallet->SelectCoinsMinConf(195 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(195 * CENT, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 2 * COIN); // we should get 2 BTC in 1 coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -444,14 +438,14 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // try making 1 * MIN_CHANGE from the 1.5 * MIN_CHANGE // we'll get change smaller than MIN_CHANGE whatever happens, so can expect MIN_CHANGE exactly - BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); // but if we add a bigger coin, small change is avoided add_coin(coins, *wallet, 1111*MIN_CHANGE); // try making 1 from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 + 1111 = 1112.5 - BOOST_CHECK(wallet->SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount // if we add more small coins: @@ -459,7 +453,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, MIN_CHANGE * 7 / 10); // and try again to make 1.0 * MIN_CHANGE - BOOST_CHECK(wallet->SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount // run the 'mtgox' test (see https://blockexplorer.com/tx/29a3efd3ef04f9153d47a990bd7b048a4b2d213daaa5fb8ed670fb85f13bdbcf) @@ -468,7 +462,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int j = 0; j < 20; j++) add_coin(coins, *wallet, 50000 * COIN); - BOOST_CHECK(wallet->SelectCoinsMinConf(500000 * COIN, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(500000 * COIN, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 500000 * COIN); // we should get the exact amount BOOST_CHECK_EQUAL(setCoinsRet.size(), 10U); // in ten coins @@ -481,7 +475,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, MIN_CHANGE * 6 / 10); add_coin(coins, *wallet, MIN_CHANGE * 7 / 10); add_coin(coins, *wallet, 1111 * MIN_CHANGE); - BOOST_CHECK(wallet->SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(1 * MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1111 * MIN_CHANGE); // we get the bigger coin BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); @@ -491,7 +485,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, MIN_CHANGE * 6 / 10); add_coin(coins, *wallet, MIN_CHANGE * 8 / 10); add_coin(coins, *wallet, 1111 * MIN_CHANGE); - BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); // we should get the exact amount BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); // in two coins 0.4+0.6 @@ -502,12 +496,12 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, MIN_CHANGE * 100); // trying to make 100.01 from these three coins - BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE * 10001 / 100, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE * 10001 / 100, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE * 10105 / 100); // we should get all coins BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // but if we try to make 99.9, we should take the bigger of the two small coins to avoid small change - BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE * 9990 / 100, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(MIN_CHANGE * 9990 / 100, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 101 * MIN_CHANGE); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); } @@ -521,7 +515,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // We only create the wallet once to save time, but we still run the coin selection RUN_TESTS times. for (int i = 0; i < RUN_TESTS; i++) { - BOOST_CHECK(wallet->SelectCoinsMinConf(2000, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(2000, filter_confirmed, coins, setCoinsRet, nValueRet, coin_selection_params)); if (amt - 2000 < MIN_CHANGE) { // needs more than one input: @@ -547,17 +541,19 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int i = 0; i < RUN_TESTS; i++) { // picking 50 from 100 coins doesn't depend on the shuffle, // but does depend on randomness in the stochastic approximation code - BOOST_CHECK(wallet->SelectCoinsMinConf(50 * COIN, filter_standard, coins, setCoinsRet , nValueRet, coin_selection_params, bnb_used)); - BOOST_CHECK(wallet->SelectCoinsMinConf(50 * COIN, filter_standard, coins, setCoinsRet2, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(KnapsackSolver(50 * COIN, GroupCoins(coins), setCoinsRet, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE)); + BOOST_CHECK(KnapsackSolver(50 * COIN, GroupCoins(coins), setCoinsRet2, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE)); BOOST_CHECK(!equal_sets(setCoinsRet, setCoinsRet2)); int fails = 0; for (int j = 0; j < RANDOM_REPEATS; j++) { - // selecting 1 from 100 identical coins depends on the shuffle; this test will fail 1% of the time - // run the test RANDOM_REPEATS times and only complain if all of them fail - BOOST_CHECK(wallet->SelectCoinsMinConf(COIN, filter_standard, coins, setCoinsRet , nValueRet, coin_selection_params, bnb_used)); - BOOST_CHECK(wallet->SelectCoinsMinConf(COIN, filter_standard, coins, setCoinsRet2, nValueRet, coin_selection_params, bnb_used)); + // Test that the KnapsackSolver selects randomly from equivalent coins (same value and same input size). + // When choosing 1 from 100 identical coins, 1% of the time, this test will choose the same coin twice + // which will cause it to fail. + // To avoid that issue, run the test RANDOM_REPEATS times and only complain if all of them fail + BOOST_CHECK(KnapsackSolver(COIN, GroupCoins(coins), setCoinsRet, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE)); + BOOST_CHECK(KnapsackSolver(COIN, GroupCoins(coins), setCoinsRet2, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE)); if (equal_sets(setCoinsRet, setCoinsRet2)) fails++; } @@ -577,10 +573,8 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) int fails = 0; for (int j = 0; j < RANDOM_REPEATS; j++) { - // selecting 1 from 100 identical coins depends on the shuffle; this test will fail 1% of the time - // run the test RANDOM_REPEATS times and only complain if all of them fail - BOOST_CHECK(wallet->SelectCoinsMinConf(90*CENT, filter_standard, coins, setCoinsRet , nValueRet, coin_selection_params, bnb_used)); - BOOST_CHECK(wallet->SelectCoinsMinConf(90*CENT, filter_standard, coins, setCoinsRet2, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(KnapsackSolver(90*CENT, GroupCoins(coins), setCoinsRet, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE)); + BOOST_CHECK(KnapsackSolver(90*CENT, GroupCoins(coins), setCoinsRet2, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE)); if (equal_sets(setCoinsRet, setCoinsRet2)) fails++; } @@ -598,7 +592,6 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) CoinSet setCoinsRet; CAmount nValueRet; - bool bnb_used; std::vector coins; // Test vValue sort order @@ -606,7 +599,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) add_coin(coins, *wallet, 1000 * COIN); add_coin(coins, *wallet, 3 * COIN); - BOOST_CHECK(wallet->SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); + BOOST_CHECK(wallet->SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params)); BOOST_CHECK_EQUAL(nValueRet, 1003 * COIN); BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); } @@ -645,19 +638,14 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test) CAmount target = rand.randrange(balance - 1000) + 1000; // Perform selection - CoinSelectionParams coin_selection_params_knapsack(/* use_bnb= */ false, /* change_output_size= */ 34, - /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), - /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), - /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); - CoinSelectionParams coin_selection_params_bnb(/* use_bnb= */ true, /* change_output_size= */ 34, - /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), - /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), - /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); + CoinSelectionParams cs_params(/* change_output_size= */ 34, + /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), + /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), + /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); CoinSet out_set; CAmount out_value = 0; - bool bnb_used = false; - BOOST_CHECK(wallet->SelectCoinsMinConf(target, filter_standard, coins, out_set, out_value, coin_selection_params_bnb, bnb_used) || - wallet->SelectCoinsMinConf(target, filter_standard, coins, out_set, out_value, coin_selection_params_knapsack, bnb_used)); + CCoinControl cc; + BOOST_CHECK(wallet->SelectCoins(coins, target, out_set, out_value, cc, cs_params)); BOOST_CHECK_GE(out_value, target); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 1d03d8f4f1473..77483eeddcdc5 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -253,48 +253,6 @@ struct WalletTxHasher } }; -/** Parameters for one iteration of Coin Selection. */ -struct CoinSelectionParams -{ - /** Toggles use of Branch and Bound instead of Knapsack solver. */ - bool use_bnb = true; - /** Size of a change output in bytes, determined by the output type. */ - size_t change_output_size = 0; - /** Size of the input to spend a change output in virtual bytes. */ - size_t change_spend_size = 0; - /** Cost of creating the change output. */ - CAmount m_change_fee{0}; - /** Cost of creating the change output + cost of spending the change output in the future. */ - CAmount m_cost_of_change{0}; - /** The fee to spend these UTXOs at the long term feerate. */ - CFeeRate m_effective_feerate; - /** The feerate estimate used to estimate an upper bound on what should be sufficient to spend - * the change output sometime in the future. */ - CFeeRate m_long_term_feerate; - /** If the cost to spend a change output at the discard feerate exceeds its value, drop it to fees. */ - CFeeRate m_discard_feerate; - size_t tx_noinputs_size = 0; - /** Indicate that we are subtracting the fee from outputs */ - bool m_subtract_fee_outputs = false; - /** When true, always spend all (up to OUTPUT_GROUP_MAX_ENTRIES) or none of the outputs - * associated with the same address. This helps reduce privacy leaks resulting from address - * reuse. Dust outputs are not eligible to be added to output groups and thus not considered. */ - bool m_avoid_partial_spends = false; - - CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_feerate, - CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial) : - use_bnb(use_bnb), - change_output_size(change_output_size), - change_spend_size(change_spend_size), - m_effective_feerate(effective_feerate), - m_long_term_feerate(long_term_feerate), - m_discard_feerate(discard_feerate), - tx_noinputs_size(tx_noinputs_size), - m_avoid_partial_spends(avoid_partial) - {} - CoinSelectionParams() {} -}; - class WalletRescanReserver; //forward declarations for ScanForWalletTransactions/RescanFromTime /** * A CWallet maintains a set of transactions and balances, and provides the ability to create new transactions. @@ -474,7 +432,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati * from coin_control and Coin Selection if successful. */ bool SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, - const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params, bool& bnb_used) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** Get a name for this wallet for logging/debugging purposes. */ @@ -588,7 +546,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati * param@[out] setCoinsRet Populated with the coins selected if successful. * param@[out] nValueRet Used to return the total value of selected coins. */ - bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector coins, std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, bool& bnb_used, CoinType nCoinType = CoinType::ALL_COINS) const; + bool SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector coins, std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType = CoinType::ALL_COINS) const; // Coin selection bool SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::vector& vecTxDSInRet); @@ -615,7 +573,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool IsSpentKey(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set& tx_destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::vector GroupOutputs(const std::vector& outputs, bool separate_coins, const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate, const CoinEligibilityFilter& filter, bool positive_only) const; + std::vector GroupOutputs(const std::vector& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only) const; void RecalculateMixedCredit(const uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From 0185847a43d13fea2fa31ccb2bbe7e1de9f837e5 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 2 Jan 2025 00:57:14 +0300 Subject: [PATCH 12/13] fix: correct fee calculations in `CreateTransactionInternal` Adapted from 87e0ef90 in bitcoin#25647 and c1a84f10 in bitcoin#26643 Co-authored-by: UdjinM6 --- src/wallet/spend.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 61d5ab1e8f0bd..b03c3267081df 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -625,6 +625,7 @@ bool CWallet::CreateTransactionInternal( CMutableTransaction txNew; FeeCalculation feeCalc; int nBytes{0}; + CAmount fee_needed{0}; { std::set setCoins; LOCK(cs_wallet); @@ -806,9 +807,8 @@ bool CWallet::CreateTransactionInternal( nBytes += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize; } - nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes); + fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); - CAmount fee_needed = nFeeRet; if (nSubtractFeeFromAmount == 0) { change_position->nValue -= fee_needed; } @@ -828,6 +828,8 @@ bool CWallet::CreateTransactionInternal( fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); } + nFeeRet = inputs_sum - nValue - change_amount; + // Update nFeeRet in case fee_needed changed due to dropping the change output if (fee_needed <= change_and_fee - change_amount) { nFeeRet = change_and_fee - change_amount; @@ -903,6 +905,11 @@ bool CWallet::CreateTransactionInternal( } } + if (fee_needed > nFeeRet) { + error = _("Fee needed > fee paid"); + return false; + } + if (nFeeRet > m_default_max_tx_fee) { error = TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED); return false; From 153bdc2cb21572ed3cb0e0258de1b6b0f710b785 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 4 Jun 2021 17:28:46 -0400 Subject: [PATCH 13/13] merge bitcoin#22155: Add test for subtract fee from recipient behavior --- src/Makefile.test.include | 1 + src/wallet/test/spend_tests.cpp | 61 +++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 src/wallet/test/spend_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index c1fd27e80443e..0540c35bf4c1b 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -194,6 +194,7 @@ BITCOIN_TESTS += \ wallet/test/bip39_tests.cpp \ wallet/test/coinjoin_tests.cpp \ wallet/test/psbt_wallet_tests.cpp \ + wallet/test/spend_tests.cpp \ wallet/test/wallet_tests.cpp \ wallet/test/walletdb_tests.cpp \ wallet/test/wallet_crypto_tests.cpp \ diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp new file mode 100644 index 0000000000000..ad739c6cc0872 --- /dev/null +++ b/src/wallet/test/spend_tests.cpp @@ -0,0 +1,61 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include + +#include + +BOOST_FIXTURE_TEST_SUITE(spend_tests, WalletTestingSetup) + +BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup) +{ + CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + auto wallet = CreateSyncedWallet(*m_node.chain, *m_node.coinjoin_loader, m_node.chainman->ActiveChain(), coinbaseKey); + + // Check that a subtract-from-recipient transaction slightly less than the + // coinbase input amount does not create a change output (because it would + // be uneconomical to add and spend the output), and make sure it pays the + // leftover input amount which would have been change to the recipient + // instead of the miner. + auto check_tx = [&wallet](CAmount leftover_input_amount) { + CRecipient recipient{GetScriptForRawPubKey({}), 500 * COIN - leftover_input_amount, true /* subtract fee */}; + CTransactionRef tx; + CAmount fee; + int change_pos = -1; + bilingual_str error; + CCoinControl coin_control; + coin_control.m_feerate.emplace(10000); + coin_control.fOverrideFeeRate = true; + FeeCalculation fee_calc; + BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc)); + BOOST_CHECK_EQUAL(tx->vout.size(), 1); + BOOST_CHECK_EQUAL(tx->vout[0].nValue, recipient.nAmount + leftover_input_amount - fee); + BOOST_CHECK_GT(fee, 0); + return fee; + }; + + // Send full input amount to recipient, check that only nonzero fee is + // subtracted (to_reduce == fee). + const CAmount fee{check_tx(0)}; + + // Send slightly less than full input amount to recipient, check leftover + // input amount is paid to recipient not the miner (to_reduce == fee - 123) + BOOST_CHECK_EQUAL(fee, check_tx(123)); + + // Send full input minus fee amount to recipient, check leftover input + // amount is paid to recipient not the miner (to_reduce == 0) + BOOST_CHECK_EQUAL(fee, check_tx(fee)); + + // Send full input minus more than the fee amount to recipient, check + // leftover input amount is paid to recipient not the miner (to_reduce == + // -123). This overpays the recipient instead of overpaying the miner more + // than double the neccesary fee. + BOOST_CHECK_EQUAL(fee, check_tx(fee + 123)); +} + +BOOST_AUTO_TEST_SUITE_END()