diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cf3bcae33be3..70ce07a25835 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3771,17 +3771,22 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT assert(txNew.nLockTime < LOCKTIME_THRESHOLD); FeeCalculation feeCalc; CFeeRate discard_rate = coin_control.m_discard_feerate ? *coin_control.m_discard_feerate : GetDiscardRate(::feeEstimator); - CAmount nFeeNeeded; unsigned int nBytes; { - std::set setCoins; - std::vector vecTxDSInTmp; + std::vector vecCoins; LOCK2(cs_main, mempool.cs); LOCK(cs_wallet); { + CAmount nAmountAvailable{0}; std::vector vAvailableCoins; AvailableCoins(vAvailableCoins, true, &coin_control); + 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 reservekey so // change transaction isn't always pay-to-bitcoin-address @@ -3810,24 +3815,26 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT scriptChange = GetScriptForDestination(vchPubKey.GetID()); } - CTxOut change_prototype_txout(0, scriptChange); - size_t change_prototype_size = GetSerializeSize(change_prototype_txout, SER_DISK, 0); nFeeRet = 0; bool pick_new_inputs = true; CAmount nValueIn = 0; - // Start with no fee and loop until there is enough fee - while (true) + 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) { - nChangePosInOut = nChangePosRequest; + nChangePosInOut = std::numeric_limits::max(); txNew.vin.clear(); txNew.vout.clear(); wtxNew.fFromMe = true; bool fFirst = true; CAmount nValueToSelect = nValue; - if (nSubtractFeeFromAmount == 0) - nValueToSelect += nFeeRet; + if (nSubtractFeeFromAmount == 0) { + assert(nAmountToSelectAdditional >= 0); + nValueToSelect += nAmountToSelectAdditional; + } // vouts to the payees for (const auto& recipient : vecSend) { @@ -3864,8 +3871,8 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // Choose coins to use if (pick_new_inputs) { nValueIn = 0; - setCoins.clear(); - if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, &coin_control)) { + std::set setCoinsTmp; + if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoinsTmp, nValueIn, &coin_control)) { if (coin_control.nCoinType == CoinType::ONLY_NONDENOMINATED) { strFailReason = _("Unable to locate enough PrivateSend non-denominated funds for this transaction."); } else if (coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { @@ -3876,40 +3883,124 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } return false; } + vecCoins.assign(setCoinsTmp.begin(), setCoinsTmp.end()); + } + + // Fill vin + // + // 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); + } + + auto calculateFee = [&](CAmount& nFee) -> bool { + // Fill in dummy signatures for fee calculation. + int nIn = 0; + for (const auto& coin : vecCoins) { + const CScript& scriptPubKey = coin.txout.scriptPubKey; + SignatureData sigdata; + if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata)) { + strFailReason = _("Signing transaction failed"); + return false; + } else { + UpdateTransaction(txNew, nIn, sigdata); + } + + nIn++; + } + + nBytes = ::GetSerializeSize(txNew, SER_NETWORK, PROTOCOL_VERSION); + + if (nExtraPayloadSize != 0) { + // account for extra payload in fee calculation + nBytes += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize; + } + + if (nBytes > MAX_STANDARD_TX_SIZE) { + // Do not create oversized transactions (bad-txns-oversize). + strFailReason = _("Transaction too large"); + return false; + } + + // Remove scriptSigs to eliminate the fee calculation dummy signatures + for (auto& txin : txNew.vin) { + txin.scriptSig = CScript(); + } + + nFee = GetMinimumFee(nBytes, coin_control, ::mempool, ::feeEstimator, &feeCalc); + + // If we made it here and we aren't even able to meet the relay fee on the next pass, give up + // because we must be at the maximum allowed fee. + if (nFee < ::minRelayTxFee.GetFee(nBytes)) { + strFailReason = _("Transaction too large for fee policy"); + return false; + } + + return true; + }; + + if (!calculateFee(nFeeRet)) { + return false; } - const CAmount nChange = nValueIn - nValueToSelect; CTxOut newTxOut; + const CAmount nAmountLeft = nValueIn - nValue; + auto getChange = [&]() { + if (nSubtractFeeFromAmount > 0) { + return nAmountLeft; + } else { + return nAmountLeft - nFeeRet; + } + }; - if (nChange > 0) + if (getChange() > 0) { //over pay for denominated transactions if (coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { nChangePosInOut = -1; - nFeeRet += nChange; + nFeeRet += getChange(); } else { - // Fill a vout to ourself - newTxOut = CTxOut(nChange, scriptChange); + // 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; + } + + // Remove the change output again, it will be added later again if required + txNew.vout.pop_back(); + + // Set the change amount properly + newTxOut.nValue = getChange(); // Never create dust outputs; if we would, just // add the dust to the fee. if (IsDust(newTxOut, discard_rate)) { + nFeeRet = nFeePrev; nChangePosInOut = -1; - nFeeRet += nChange; - + nFeeRet += getChange(); } else { - if (nChangePosInOut == -1) + if (nChangePosRequest == -1) { // Insert change txn at random position: nChangePosInOut = GetRandInt(txNew.vout.size()+1); } - else if ((unsigned int)nChangePosInOut > txNew.vout.size()) + else if ((unsigned int)nChangePosRequest > txNew.vout.size()) { strFailReason = _("Change index out of range"); return false; + } else { + nChangePosInOut = nChangePosRequest; } std::vector::iterator position = txNew.vout.begin()+nChangePosInOut; @@ -3920,157 +4011,83 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT nChangePosInOut = -1; } - // Fill vin - // - // Note how the sequence number is set to max()-1 so that the - // nLockTime set above actually works. - vecTxDSInTmp.clear(); - for (const auto& coin : setCoins) { - CTxIn txin = CTxIn(coin.outpoint,CScript(), - CTxIn::SEQUENCE_FINAL - 1); - vecTxDSInTmp.push_back(CTxDSIn(txin, coin.txout.scriptPubKey)); - txNew.vin.push_back(txin); + 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 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()); - std::sort(vecTxDSInTmp.begin(), vecTxDSInTmp.end(), CompareInputBIP69()); std::sort(txNew.vout.begin(), txNew.vout.end(), CompareOutputBIP69()); - } - // If there was 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) + // 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) { - nChangePosInOut = i; - break; + if (txOut == newTxOut) + { + nChangePosInOut = i; + break; + } + i++; } - i++; } } - // Fill in dummy signatures for fee calculation. - int nIn = 0; - for (const auto& txdsin : vecTxDSInTmp) - { - const CScript& scriptPubKey = txdsin.prevPubKey; - SignatureData sigdata; - if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata)) - { - strFailReason = _("Signing transaction failed"); - return false; - } else { - UpdateTransaction(txNew, nIn, sigdata); - } - - nIn++; - } - - nBytes = ::GetSerializeSize(txNew, SER_NETWORK, PROTOCOL_VERSION); - - if (nExtraPayloadSize != 0) { - // account for extra payload in fee calculation - nBytes += GetSizeOfCompactSize(nExtraPayloadSize) + nExtraPayloadSize; - } - - if (nBytes > MAX_STANDARD_TX_SIZE) { - // Do not create oversized transactions (bad-txns-oversize). - strFailReason = _("Transaction too large"); - return false; - } - - // Remove scriptSigs to eliminate the fee calculation dummy signatures - for (auto& txin : txNew.vin) { - txin.scriptSig = CScript(); - } - - nFeeNeeded = GetMinimumFee(nBytes, coin_control, ::mempool, ::feeEstimator, &feeCalc); - - // If we made it here and we aren't even able to meet the relay fee on the next pass, give up - // because we must be at the maximum allowed fee. - if (nFeeNeeded < ::minRelayTxFee.GetFee(nBytes)) - { - strFailReason = _("Transaction too large for fee policy"); - return false; - } - - if (nFeeRet >= nFeeNeeded) { - // Reduce fee to only the needed amount if possible. This - // prevents potential overpayment in fees if the coins - // selected to meet nFeeNeeded result in a transaction that - // requires less fee than the prior iteration. - - // If we have no change and a big enough excess fee, then - // try to construct transaction again only without picking - // new inputs. We now know we only need the smaller fee - // (because of reduced tx size) and so we should add a - // change output. Only try this once. - if (nChangePosInOut == -1 && nSubtractFeeFromAmount == 0 && pick_new_inputs) { - unsigned int tx_size_with_change = nBytes + change_prototype_size + 2; // Add 2 as a buffer in case increasing # of outputs changes compact size - CAmount fee_needed_with_change = GetMinimumFee(tx_size_with_change, coin_control, ::mempool, ::feeEstimator, nullptr); - CAmount minimum_value_for_change = GetDustThreshold(change_prototype_txout, discard_rate); - if (nFeeRet >= fee_needed_with_change + minimum_value_for_change) { - pick_new_inputs = false; - nFeeRet = fee_needed_with_change; - continue; - } - } - - // If we have change output already, just increase it - if (nFeeRet > nFeeNeeded && nChangePosInOut != -1 && nSubtractFeeFromAmount == 0) { - CAmount extraFeePaid = nFeeRet - nFeeNeeded; - std::vector::iterator change_position = txNew.vout.begin()+nChangePosInOut; - change_position->nValue += extraFeePaid; - nFeeRet -= extraFeePaid; - } - break; // Done, enough fee included. - } - else if (!pick_new_inputs) { - // This shouldn't happen, we should have had enough excess - // fee to pay for the new output and still meet nFeeNeeded - // Or we should have just subtracted fee from recipients and - // nFeeNeeded should not have changed - strFailReason = _("Transaction fee and change calculation failed"); - 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. + // Either way, we used the total amount of the inputs we picked and the transaction is ready. + break; } - // Try to reduce change to include necessary fee + // We have a change output and we don't need to subtruct fees, which means the transaction is ready. if (nChangePosInOut != -1 && nSubtractFeeFromAmount == 0) { - CAmount additionalFeeNeeded = nFeeNeeded - nFeeRet; - std::vector::iterator change_position = txNew.vout.begin()+nChangePosInOut; - // Only reduce change if remaining amount is still a large enough output. - if (change_position->nValue >= MIN_FINAL_CHANGE + additionalFeeNeeded) { - change_position->nValue -= additionalFeeNeeded; - nFeeRet += additionalFeeNeeded; - break; // Done, able to increase fee from change - } + 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; } + } - // Include more fee and try again. - nFeeRet = nFeeNeeded; - continue; + if (nMaxTries == 0) { + strFailReason = _("Exceeded max tries."); + return false; } } + // Make sure change position was updated one way or another + assert(nChangePosInOut != std::numeric_limits::max()); + if (nChangePosInOut == -1) reservekey.ReturnKey(); // Return any reserved key if we don't have change if (sign) { CTransaction txNewConst(txNew); int nIn = 0; - for(const auto& txdsin : vecTxDSInTmp) + for(const auto& coin : vecCoins) { - const CScript& scriptPubKey = txdsin.prevPubKey; + const CScript& scriptPubKey = coin.txout.scriptPubKey; SignatureData sigdata; if (!ProduceSignature(TransactionSignatureCreator(this, &txNewConst, nIn, SIGHASH_ALL), scriptPubKey, sigdata)) @@ -4105,8 +4122,8 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } } - LogPrintf("Fee Calculation: Fee:%d Bytes:%u Needed:%d Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n", - nFeeRet, nBytes, nFeeNeeded, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay, + LogPrintf("Fee Calculation: Fee:%d Bytes:%u Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n", + nFeeRet, nBytes, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay, feeCalc.est.pass.start, feeCalc.est.pass.end, 100 * feeCalc.est.pass.withinTarget / (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool), feeCalc.est.pass.withinTarget, feeCalc.est.pass.totalConfirmed, feeCalc.est.pass.inMempool, feeCalc.est.pass.leftMempool, diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c94ff9b771f5..a3ab09e286f2 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -547,6 +547,16 @@ class CInputCoin { } }; +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)); + } +}; + class COutput { public: