From 829b0376b673efd0a802e094d5abd42f6faf4e75 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Tue, 18 Aug 2020 18:47:17 +0200 Subject: [PATCH 01/22] wallet: Remove unused vecTxDSInTmp in CWallet::CreateTransaction --- src/wallet/wallet.cpp | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cf3bcae33be3..2b175f7a1840 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3775,7 +3775,6 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT unsigned int nBytes; { std::set setCoins; - std::vector vecTxDSInTmp; LOCK2(cs_main, mempool.cs); LOCK(cs_wallet); { @@ -3924,18 +3923,16 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // // Note how the sequence number is set to max()-1 so that the // nLockTime set above actually works. - vecTxDSInTmp.clear(); + txNew.vin.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 no specific change position was requested, apply BIP69 if (nChangePosRequest == -1) { 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()); } @@ -3955,9 +3952,9 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // Fill in dummy signatures for fee calculation. int nIn = 0; - for (const auto& txdsin : vecTxDSInTmp) + for (const auto& coin : setCoins) { - const CScript& scriptPubKey = txdsin.prevPubKey; + const CScript& scriptPubKey = coin.txout.scriptPubKey; SignatureData sigdata; if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata)) { @@ -4068,9 +4065,9 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT { CTransaction txNewConst(txNew); int nIn = 0; - for(const auto& txdsin : vecTxDSInTmp) + for(const auto& coin : setCoins) { - const CScript& scriptPubKey = txdsin.prevPubKey; + const CScript& scriptPubKey = coin.txout.scriptPubKey; SignatureData sigdata; if (!ProduceSignature(TransactionSignatureCreator(this, &txNewConst, nIn, SIGHASH_ALL), scriptPubKey, sigdata)) From 1a24fd4dc10f0ac01b5366780bc2bae1e61c52ad Mon Sep 17 00:00:00 2001 From: xdustinface Date: Tue, 18 Aug 2020 22:15:03 +0200 Subject: [PATCH 02/22] wallet: Calculate fees earlier and respect them in change determination --- src/wallet/wallet.cpp | 142 ++++++++++++++++++++++++------------------ 1 file changed, 83 insertions(+), 59 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 2b175f7a1840..1035d1c5cab7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3877,8 +3877,73 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } } - const CAmount nChange = nValueIn - nValueToSelect; + // 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 : setCoins) { + CTxIn txin = CTxIn(coin.outpoint,CScript(), + CTxIn::SEQUENCE_FINAL - 1); + txNew.vin.push_back(txin); + } + + auto calculateFee = [&](CAmount& nFee) -> bool { + // Fill in dummy signatures for fee calculation. + int nIn = 0; + for (const auto& coin : setCoins) + { + 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(nFeeNeeded)) { + return false; + } + CTxOut newTxOut; + const CAmount nAmountLeft = nValueIn - nValueToSelect; + CAmount nChange = nAmountLeft - nFeeNeeded; if (nChange > 0) { @@ -3887,8 +3952,21 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT nChangePosInOut = -1; nFeeRet += nChange; } 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 + CAmount nFeeNeededWithChange{0}; + if (!calculateFee(nFeeNeededWithChange)) { + 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 = nAmountLeft - nFeeNeededWithChange; // Never create dust outputs; if we would, just // add the dust to the fee. @@ -3913,23 +3991,14 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT std::vector::iterator position = txNew.vout.begin()+nChangePosInOut; txNew.vout.insert(position, newTxOut); + nFeeNeeded = nFeeNeededWithChange; + nChange = nAmountLeft - nFeeNeeded; } } } else { nChangePosInOut = -1; } - // 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 : setCoins) { - CTxIn txin = CTxIn(coin.outpoint,CScript(), - CTxIn::SEQUENCE_FINAL - 1); - txNew.vin.push_back(txin); - } - // If no specific change position was requested, apply BIP69 if (nChangePosRequest == -1) { std::sort(txNew.vin.begin(), txNew.vin.end(), CompareInputBIP69()); @@ -3950,51 +4019,6 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } } - // Fill in dummy signatures for fee calculation. - int nIn = 0; - for (const auto& coin : setCoins) - { - 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(); - } - - 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 From a11a5cb14b8e197ecf0b9dd59a2e20b3c1b33215 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Thu, 20 Aug 2020 20:25:44 +0200 Subject: [PATCH 03/22] wallet: Cleanup obsolete code in CreateTransaction This parts were needed before when the fee was calculated after the change was assigned. But now with the previous commit the fee is calculated upfront and respected properly from the begining. So there is no longer a need of increasing or decreasing the change depending on the fee as it has the correct value directly after its added. --- src/wallet/wallet.cpp | 70 +++++-------------------------------------- 1 file changed, 8 insertions(+), 62 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1035d1c5cab7..507c34ac6274 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3771,7 +3771,6 @@ 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; @@ -3809,8 +3808,6 @@ 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; @@ -3937,13 +3934,13 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT return true; }; - if (!calculateFee(nFeeNeeded)) { + if (!calculateFee(nFeeRet)) { return false; } CTxOut newTxOut; const CAmount nAmountLeft = nValueIn - nValueToSelect; - CAmount nChange = nAmountLeft - nFeeNeeded; + CAmount nChange = nAmountLeft - nFeeRet; if (nChange > 0) { @@ -3991,8 +3988,8 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT std::vector::iterator position = txNew.vout.begin()+nChangePosInOut; txNew.vout.insert(position, newTxOut); - nFeeNeeded = nFeeNeededWithChange; - nChange = nAmountLeft - nFeeNeeded; + nFeeRet = nFeeNeededWithChange; + nChange = nAmountLeft - nFeeRet; } } } else { @@ -4019,56 +4016,9 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } } - 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; - } - - // Try to reduce change to include necessary fee + // We have a change output, means we can just break as the transaction is done. 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 @@ -4076,10 +4026,6 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT if (nSubtractFeeFromAmount > 0) { pick_new_inputs = false; } - - // Include more fee and try again. - nFeeRet = nFeeNeeded; - continue; } } @@ -4126,8 +4072,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, From 82e253b323a3bc77902062249b7fb2dd23fa7fa1 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Thu, 20 Aug 2020 23:36:17 +0200 Subject: [PATCH 04/22] wallet: Try to pick other inputs if the selected ones are too small If nChange is negative in this cases it means that the selected inputs can't cover the amount to send and the required transaction fee. So we just add the missing amount to nFeeRet. This leads to the algo trying to pick larger inputs in the next loop (with nFeeRet more duffs than in the previous loop). This process gets repeated until the selected amount is enough to cover all the costs or until the requested amount can't be selected anymore (not enough utxos to cover it). --- src/wallet/wallet.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 507c34ac6274..4da639f871aa 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3996,6 +3996,13 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT nChangePosInOut = -1; } + if (nChange < 0 && !nSubtractFeeFromAmount) { + // nValueIn is not enough to cover nValueToSelect + nFeeRet. Add the missing amount abs(nChange) to the fee + // and try to select other inputs in the next loop to cover the full required amount. + nFeeRet += abs(nChange); + continue; + } + // If no specific change position was requested, apply BIP69 if (nChangePosRequest == -1) { std::sort(txNew.vin.begin(), txNew.vin.end(), CompareInputBIP69()); From 3583a2fa061e4f48bc0e4a12a891d367dca7d856 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Fri, 21 Aug 2020 00:06:03 +0200 Subject: [PATCH 05/22] wallet: Break the loop if the transaction is ready --- src/wallet/wallet.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4da639f871aa..cf281aa64117 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4031,8 +4031,21 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // 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 nAmountLeft == nFeeRet it means we either added nChange to nFeeRet since nChange 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. + if (nAmountLeft == nFeeRet) { + break; + } } } From a49d4ca1fdb4034de3536cc3a6cf162258151daa Mon Sep 17 00:00:00 2001 From: xdustinface Date: Sat, 22 Aug 2020 09:59:14 +0200 Subject: [PATCH 06/22] wallet: Respect additional amount from previous cycles --- src/wallet/wallet.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cf281aa64117..391824108a83 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3812,6 +3812,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT nFeeRet = 0; bool pick_new_inputs = true; CAmount nValueIn = 0; + CAmount nAmountToSelectAdditional{0}; // Start with no fee and loop until there is enough fee while (true) { @@ -3822,8 +3823,9 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT bool fFirst = true; CAmount nValueToSelect = nValue; - if (nSubtractFeeFromAmount == 0) - nValueToSelect += nFeeRet; + if (nSubtractFeeFromAmount == 0) { + nValueToSelect += nAmountToSelectAdditional; + } // vouts to the payees for (const auto& recipient : vecSend) { @@ -3939,7 +3941,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } CTxOut newTxOut; - const CAmount nAmountLeft = nValueIn - nValueToSelect; + const CAmount nAmountLeft = nValueIn - nValue; CAmount nChange = nAmountLeft - nFeeRet; if (nChange > 0) @@ -3997,9 +3999,9 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } if (nChange < 0 && !nSubtractFeeFromAmount) { - // nValueIn is not enough to cover nValueToSelect + nFeeRet. Add the missing amount abs(nChange) to the fee + // 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 to cover the full required amount. - nFeeRet += abs(nChange); + nAmountToSelectAdditional += abs(nChange); continue; } From b4c2d0bc9e53f87db1dbb83d80bee8ac34acf50d Mon Sep 17 00:00:00 2001 From: xdustinface Date: Sat, 22 Aug 2020 10:03:17 +0200 Subject: [PATCH 07/22] wallet: Respect available in coin selection, try all coins in last round --- src/wallet/wallet.cpp | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 391824108a83..2b91a6de35f6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3777,9 +3777,16 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT 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 @@ -3826,6 +3833,12 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT if (nSubtractFeeFromAmount == 0) { nValueToSelect += nAmountToSelectAdditional; } + // If the resulting amount to select exceeds the + // balance from the available coins just try it one + // last time with whole available amount. + if (nValueToSelect > nAmountAvailable) { + nValueToSelect = nAmountAvailable; + } // vouts to the payees for (const auto& recipient : vecSend) { @@ -3998,11 +4011,16 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT nChangePosInOut = -1; } - if (nChange < 0 && !nSubtractFeeFromAmount) { - // 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 to cover the full required amount. - nAmountToSelectAdditional += abs(nChange); - continue; + if (nChange < 0) { + if (!nSubtractFeeFromAmount) { + // 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 to cover the full required amount. + nAmountToSelectAdditional += abs(nChange); + continue; + } else if (nAmountToSelectAdditional && nValueToSelect == nAmountAvailable) { + strFailReason = _("Insufficient funds."); + return false; + } } // If no specific change position was requested, apply BIP69 From 1587b03acb4953beb1fef1a0add20b89db77dc7c Mon Sep 17 00:00:00 2001 From: xdustinface Date: Sat, 22 Aug 2020 10:14:13 +0200 Subject: [PATCH 08/22] wallet: Avoid potential infinite loop, just in case.. --- src/wallet/wallet.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 2b91a6de35f6..b1294c88aa54 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3820,8 +3820,9 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT bool pick_new_inputs = true; CAmount nValueIn = 0; CAmount nAmountToSelectAdditional{0}; - // Start with no fee and loop until there is enough fee - while (true) + // Start with no fee and loop until there is enough fee, try it 500 times. + int nMaxTries = 500; + while (--nMaxTries) { nChangePosInOut = nChangePosRequest; txNew.vin.clear(); @@ -4067,6 +4068,11 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT break; } } + + if (!nMaxTries) { + strFailReason = _("Exceeded max tries."); + return false; + } } if (nChangePosInOut == -1) reservekey.ReturnKey(); // Return any reserved key if we don't have change From 02c394c30ac80623fb1935ca010eaf8ae726963c Mon Sep 17 00:00:00 2001 From: xdustinface Date: Sun, 23 Aug 2020 11:13:26 +0200 Subject: [PATCH 09/22] wallet: Fix signing in CreateTransaction Prior it was messed because of 60d96a1a28b55f071c6144f248e136679c44337e. Set coins isn't sorted the same way as txNew.vin is so it sometimes may pick wrong coins for signing the input. --- src/wallet/wallet.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b1294c88aa54..9066d340d5fc 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3820,7 +3820,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT bool pick_new_inputs = true; CAmount nValueIn = 0; CAmount nAmountToSelectAdditional{0}; - // Start with no fee and loop until there is enough fee, try it 500 times. + // Start with no nAmountToSelectAdditional and loop until there is enough to cover the request, try it 500 times. int nMaxTries = 500; while (--nMaxTries) { @@ -4079,11 +4079,18 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT if (sign) { + auto getScriptPubKey = [&](const CTxIn& txin) -> const CScript& { + for (const auto& coin : setCoins) { + if (txin.prevout == coin.outpoint) { + return coin.txout.scriptPubKey; + } + } + assert(false); + }; CTransaction txNewConst(txNew); - int nIn = 0; - for(const auto& coin : setCoins) + for (int nIn = 0; nIn < txNew.vin.size(); ++nIn) { - const CScript& scriptPubKey = coin.txout.scriptPubKey; + const CScript& scriptPubKey = getScriptPubKey(txNew.vin[nIn]); SignatureData sigdata; if (!ProduceSignature(TransactionSignatureCreator(this, &txNewConst, nIn, SIGHASH_ALL), scriptPubKey, sigdata)) @@ -4093,8 +4100,6 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } else { UpdateTransaction(txNew, nIn, sigdata); } - - nIn++; } } From 6345c4a1a5bd74dc99c0c749bf1c151b68d04db4 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Sun, 23 Aug 2020 13:39:13 +0200 Subject: [PATCH 10/22] wallet: Fix change calculation if "subtract fee from amount" is enabled --- src/wallet/wallet.cpp | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9066d340d5fc..0c0ab5fb0010 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3956,22 +3956,30 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT CTxOut newTxOut; const CAmount nAmountLeft = nValueIn - nValue; - CAmount nChange = nAmountLeft - nFeeRet; + auto getChange = [&]() { + if (nSubtractFeeFromAmount) { + 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 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 - CAmount nFeeNeededWithChange{0}; - if (!calculateFee(nFeeNeededWithChange)) { + // 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; } @@ -3979,15 +3987,15 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT txNew.vout.pop_back(); // Set the change amount properly - newTxOut.nValue = nAmountLeft - nFeeNeededWithChange; + 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 { @@ -4004,19 +4012,17 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT std::vector::iterator position = txNew.vout.begin()+nChangePosInOut; txNew.vout.insert(position, newTxOut); - nFeeRet = nFeeNeededWithChange; - nChange = nAmountLeft - nFeeRet; } } } else { nChangePosInOut = -1; } - if (nChange < 0) { + if (getChange() < 0) { if (!nSubtractFeeFromAmount) { // 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 to cover the full required amount. - nAmountToSelectAdditional += abs(nChange); + nAmountToSelectAdditional += abs(getChange()); continue; } else if (nAmountToSelectAdditional && nValueToSelect == nAmountAvailable) { strFailReason = _("Insufficient funds."); From d46cd3228ff59de16655594f2cc816e0c0ea032a Mon Sep 17 00:00:00 2001 From: xdustinface Date: Sun, 23 Aug 2020 16:39:50 +0200 Subject: [PATCH 11/22] wallet: Return after fee calc if no or not enough amount available Return the proper fail reason. Prior to this commit it run into "Exceeded max tried". Note: Only return if not enough amount is available if we can't subtract fee from amount. --- src/wallet/wallet.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0c0ab5fb0010..33d2f88575e2 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4018,6 +4018,18 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT nChangePosInOut = -1; } + if (nAmountAvailable == 0 || (!nSubtractFeeFromAmount && nValueIn < nValue)) { + 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) { + strFailReason = _("Unable to locate enough PrivateSend denominated funds for this transaction."); + strFailReason += " " + _("PrivateSend uses exact denominated amounts to send funds, you might simply need to mix some more coins."); + } else { + strFailReason = _("Insufficient funds."); + } + return false; + } + if (getChange() < 0) { if (!nSubtractFeeFromAmount) { // nValueIn is not enough to cover nValue + nFeeRet. Add the missing amount abs(nChange) to the fee From 6dc126bc8dc21ba65788fed59d89c40b1908bb36 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Mon, 24 Aug 2020 13:04:51 +0200 Subject: [PATCH 12/22] wallet: Fix break logic if available amount is not enough --- src/wallet/wallet.cpp | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 33d2f88575e2..8c84040058df 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4018,26 +4018,21 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT nChangePosInOut = -1; } - if (nAmountAvailable == 0 || (!nSubtractFeeFromAmount && nValueIn < nValue)) { - 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) { - strFailReason = _("Unable to locate enough PrivateSend denominated funds for this transaction."); - strFailReason += " " + _("PrivateSend uses exact denominated amounts to send funds, you might simply need to mix some more coins."); - } else { - strFailReason = _("Insufficient funds."); - } - return false; - } - - if (getChange() < 0) { - if (!nSubtractFeeFromAmount) { + if (nAmountAvailable == 0 || getChange() < 0) { + if (!nSubtractFeeFromAmount && nValueToSelect < nAmountAvailable) { // 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 to cover the full required amount. nAmountToSelectAdditional += abs(getChange()); continue; - } else if (nAmountToSelectAdditional && nValueToSelect == nAmountAvailable) { - strFailReason = _("Insufficient funds."); + } else { + 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) { + strFailReason = _("Unable to locate enough PrivateSend denominated funds for this transaction."); + strFailReason += " " + _("PrivateSend uses exact denominated amounts to send funds, you might simply need to mix some more coins."); + } else { + strFailReason = _("Insufficient funds."); + } return false; } } From d8b4a8cc676e038d794523c4e3ac4044134a4f3c Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 24 Aug 2020 14:56:51 +0300 Subject: [PATCH 13/22] Revert "wallet: Fix signing in CreateTransaction" This reverts commit 5fcdc0f00e7b961ebb62c94d17d585537e911309. --- src/wallet/wallet.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8c84040058df..022ddbd15269 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3820,7 +3820,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT bool pick_new_inputs = true; CAmount nValueIn = 0; CAmount nAmountToSelectAdditional{0}; - // Start with no nAmountToSelectAdditional and loop until there is enough to cover the request, try it 500 times. + // Start with no fee and loop until there is enough fee, try it 500 times. int nMaxTries = 500; while (--nMaxTries) { @@ -4092,18 +4092,11 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT if (sign) { - auto getScriptPubKey = [&](const CTxIn& txin) -> const CScript& { - for (const auto& coin : setCoins) { - if (txin.prevout == coin.outpoint) { - return coin.txout.scriptPubKey; - } - } - assert(false); - }; CTransaction txNewConst(txNew); - for (int nIn = 0; nIn < txNew.vin.size(); ++nIn) + int nIn = 0; + for(const auto& coin : setCoins) { - const CScript& scriptPubKey = getScriptPubKey(txNew.vin[nIn]); + const CScript& scriptPubKey = coin.txout.scriptPubKey; SignatureData sigdata; if (!ProduceSignature(TransactionSignatureCreator(this, &txNewConst, nIn, SIGHASH_ALL), scriptPubKey, sigdata)) @@ -4113,6 +4106,8 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } else { UpdateTransaction(txNew, nIn, sigdata); } + + nIn++; } } From 7e9da4105cbc45a48604de252eecd31e3c0c7e7e Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 24 Aug 2020 15:53:33 +0300 Subject: [PATCH 14/22] Use a vector of coins instead of a set in CreateTransaction and sort it in a BIP69 compliant way when needed --- src/wallet/wallet.cpp | 14 ++++++++------ src/wallet/wallet.h | 10 ++++++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 022ddbd15269..e8062488b9b7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3773,7 +3773,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT CFeeRate discard_rate = coin_control.m_discard_feerate ? *coin_control.m_discard_feerate : GetDiscardRate(::feeEstimator); unsigned int nBytes; { - std::set setCoins; + std::vector vecCoins; LOCK2(cs_main, mempool.cs); LOCK(cs_wallet); { @@ -3876,8 +3876,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) { @@ -3888,6 +3888,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } return false; } + vecCoins.assign(setCoinsTmp.begin(), setCoinsTmp.end()); } // Fill vin @@ -3895,7 +3896,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // 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 : setCoins) { + for (const auto& coin : vecCoins) { CTxIn txin = CTxIn(coin.outpoint,CScript(), CTxIn::SEQUENCE_FINAL - 1); txNew.vin.push_back(txin); @@ -3904,7 +3905,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT auto calculateFee = [&](CAmount& nFee) -> bool { // Fill in dummy signatures for fee calculation. int nIn = 0; - for (const auto& coin : setCoins) + for (const auto& coin : vecCoins) { const CScript& scriptPubKey = coin.txout.scriptPubKey; SignatureData sigdata; @@ -4039,6 +4040,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // 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(txNew.vout.begin(), txNew.vout.end(), CompareOutputBIP69()); } @@ -4094,7 +4096,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT { CTransaction txNewConst(txNew); int nIn = 0; - for(const auto& coin : setCoins) + for(const auto& coin : vecCoins) { const CScript& scriptPubKey = coin.txout.scriptPubKey; SignatureData sigdata; 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: From b9c7796e33cfa26ca2857a1b5d55596580e80b3c Mon Sep 17 00:00:00 2001 From: xdustinface Date: Mon, 24 Aug 2020 18:41:40 +0200 Subject: [PATCH 15/22] wallet: Adjust comment --- src/wallet/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e8062488b9b7..8e6d3a9c1f11 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3820,7 +3820,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT bool pick_new_inputs = true; CAmount nValueIn = 0; CAmount nAmountToSelectAdditional{0}; - // Start with no fee and loop until there is enough fee, try it 500 times. + // Start with nAmountToSelectAdditional=0 and loop until there is enough to cover the request + fees, try it 500 times. int nMaxTries = 500; while (--nMaxTries) { From 2da84120f20a1a5b0c92e5d40c5d21436a40d083 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Tue, 25 Aug 2020 02:18:47 +0300 Subject: [PATCH 16/22] Cleaner usage of nChangePosRequest/InOut --- src/wallet/wallet.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8e6d3a9c1f11..a34626eddb89 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3824,7 +3824,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT int nMaxTries = 500; while (--nMaxTries) { - nChangePosInOut = nChangePosRequest; + nChangePosInOut = std::numeric_limits::max(); txNew.vin.clear(); txNew.vout.clear(); wtxNew.fFromMe = true; @@ -4000,15 +4000,17 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } 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; @@ -4090,6 +4092,9 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } } + // 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) From 7bcc9e2393d0ca89b76a0d0550c3ead0d3ab441f Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Tue, 25 Aug 2020 02:22:45 +0300 Subject: [PATCH 17/22] Simplify some fail/try-again conditions (fixed) --- src/wallet/wallet.cpp | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a34626eddb89..864eaefe9726 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3832,14 +3832,9 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT CAmount nValueToSelect = nValue; if (nSubtractFeeFromAmount == 0) { + assert(nAmountToSelectAdditional >= 0); nValueToSelect += nAmountToSelectAdditional; } - // If the resulting amount to select exceeds the - // balance from the available coins just try it one - // last time with whole available amount. - if (nValueToSelect > nAmountAvailable) { - nValueToSelect = nAmountAvailable; - } // vouts to the payees for (const auto& recipient : vecSend) { @@ -4021,23 +4016,17 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT nChangePosInOut = -1; } - if (nAmountAvailable == 0 || getChange() < 0) { - if (!nSubtractFeeFromAmount && nValueToSelect < nAmountAvailable) { + 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 to cover the full required amount. + // and try to select other inputs in the next loop step to cover the full required amount. nAmountToSelectAdditional += abs(getChange()); - continue; - } else { - 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) { - strFailReason = _("Unable to locate enough PrivateSend denominated funds for this transaction."); - strFailReason += " " + _("PrivateSend uses exact denominated amounts to send funds, you might simply need to mix some more coins."); - } else { - strFailReason = _("Insufficient funds."); - } - return false; + } 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 From 3f31d601eea1e5195193dce635c950a1206864e0 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Tue, 25 Aug 2020 02:24:04 +0300 Subject: [PATCH 18/22] Loop through outputs to update change output position only when outputs are sorted for BIP69 No need to do this for non-bip69 cases (i.e. when a specific change output position was requested and assigned) --- src/wallet/wallet.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 864eaefe9726..aff3cb8accda 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4034,19 +4034,19 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT std::sort(vecCoins.begin(), vecCoins.end(), CompareInputCoinBIP69()); std::sort(txNew.vin.begin(), txNew.vin.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++; } } From ddd6af38221b212734ea20db6c745a6d76e439a2 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Tue, 25 Aug 2020 02:35:17 +0300 Subject: [PATCH 19/22] Avoid implicit conversions of int-s into bool-s --- src/wallet/wallet.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index aff3cb8accda..3e685ab71ceb 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3822,7 +3822,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT 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) + while (--nMaxTries > 0) { nChangePosInOut = std::numeric_limits::max(); txNew.vin.clear(); @@ -3953,7 +3953,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT CTxOut newTxOut; const CAmount nAmountLeft = nValueIn - nValue; auto getChange = [&]() { - if (nSubtractFeeFromAmount) { + if (nSubtractFeeFromAmount > 0) { return nAmountLeft; } else { return nAmountLeft - nFeeRet; @@ -4075,7 +4075,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } } - if (!nMaxTries) { + if (nMaxTries == 0) { strFailReason = _("Exceeded max tries."); return false; } From 66216e153ce15cfcc6eba7ec2fe5a999a8b12b36 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Tue, 25 Aug 2020 02:37:10 +0300 Subject: [PATCH 20/22] Move `nAmountLeft == nFeeRet` check higher, tweak comments --- src/wallet/wallet.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3e685ab71ceb..d1d7d917d67f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4050,7 +4050,14 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } } - // We have a change output, means we can just break as the transaction is done. + 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; } @@ -4066,13 +4073,6 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } pick_new_inputs = false; } - - // If nAmountLeft == nFeeRet it means we either added nChange to nFeeRet since nChange 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. - if (nAmountLeft == nFeeRet) { - break; - } } if (nMaxTries == 0) { From 31c137a33a3713f5b3729fe123e0a91c38a0eb3a Mon Sep 17 00:00:00 2001 From: xdustinface Date: Sun, 30 Aug 2020 12:56:38 +0200 Subject: [PATCH 21/22] wallet: Fix some formatting --- src/wallet/wallet.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d1d7d917d67f..b16a14568569 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3900,12 +3900,10 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT auto calculateFee = [&](CAmount& nFee) -> bool { // Fill in dummy signatures for fee calculation. int nIn = 0; - for (const auto& coin : vecCoins) - { + for (const auto& coin : vecCoins) { const CScript& scriptPubKey = coin.txout.scriptPubKey; SignatureData sigdata; - if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata)) - { + if (!ProduceSignature(DummySignatureCreator(this), scriptPubKey, sigdata)) { strFailReason = _("Signing transaction failed"); return false; } else { @@ -3937,8 +3935,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // 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)) - { + if (nFee < ::minRelayTxFee.GetFee(nBytes)) { strFailReason = _("Transaction too large for fee policy"); return false; } From a05963337fda6fafad144d2a89d091b2e53aa380 Mon Sep 17 00:00:00 2001 From: xdustinface Date: Tue, 1 Sep 2020 17:34:04 +0200 Subject: [PATCH 22/22] wallet: Improve CTxIn creation in CreateTransaction --- src/wallet/wallet.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b16a14568569..70ce07a25835 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3892,9 +3892,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // nLockTime set above actually works. txNew.vin.clear(); for (const auto& coin : vecCoins) { - CTxIn txin = CTxIn(coin.outpoint,CScript(), - CTxIn::SEQUENCE_FINAL - 1); - txNew.vin.push_back(txin); + txNew.vin.emplace_back(coin.outpoint, CScript(), CTxIn::SEQUENCE_FINAL - 1); } auto calculateFee = [&](CAmount& nFee) -> bool {