diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index baa051d24b731..1a31f9a92bc47 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3590,7 +3590,6 @@ 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 for (auto out : vAvailableCoins) { if (out.fSpendable) { @@ -3636,52 +3635,37 @@ 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; + coin_selection_params.use_bnb = false; // 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 nFeeRet = 0; - bool pick_new_inputs = true; CAmount nValueIn = 0; - 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; - while (--nMaxTries > 0) + // Start with no fee and loop until there is enough fee + while (true) { - 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) { + 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) { 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. + 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); @@ -3689,173 +3673,133 @@ 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); - 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; - } + if (nChangePosInOut == -1) + { + // Insert change txn at random position: + nChangePosInOut = GetRandInt(txNew.vout.size()+1); + } + else if ((unsigned int)nChangePosInOut > txNew.vout.size()) + { + error = _("Change index out of range"); + return false; + } - 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() && nChangePosInOut != -1) { + nChangePosInOut = std::distance(txNew.vout.begin(), it); } + }; - nFee = coin_selection_params.m_effective_feerate.GetFee(nBytes); + assert(nChangePosInOut != -1); + auto change_position = txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); - return true; - }; + // Dummy fill vin for maximum size estimation + // + for (const auto& coin : setCoins) { + txNew.vin.push_back(CTxIn(coin.outpoint, CScript())); + } - if (!calculateFee(nFeeRet)) { + // 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 change_and_fee = [&]() { - 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 (change_and_fee() > 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 += change_and_fee(); - } 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 = change_and_fee(); + // 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 += change_and_fee(); + // 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 = _("Change index out of range"); + std::cout << txout.ToString() << std::endl; + // 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 (change_and_fee() < 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(change_and_fee()); - } 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); - } + nFeeRet = fee_needed; + break; // The fee has been deducted from the recipients, nothing left to do here } - - 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; - } - } - - if (nMaxTries == 0) { - error = _("Exceeded max tries."); - return false; } // Give up if change keypool ran out and change is required