sikkienl/dash
1
0
Fork 0
mirror of https://github.com/dashpay/dash.git synced 2024-12-27 04:52:59 +01:00
Code Issues Actions Packages Projects Releases Wiki Activity

Fix rare edge case of paying too many fees when transaction has no change.

Browse Source 
Due to the iterative process of selecting new coins in each loop a new fee is
calculated that needs to be met each time.  In the typical case if the most
recent iteration of the loop produced a much smaller transaction and we have now
gathered inputs with too many fees, we can just reduce the change.  However in
the case where there is no change output, it is possible to end up with a
transaction which drastically overpays fees.  This commit addresses that case,
by creating a change output if the overpayment is large enough to support it,
this is accomplished by rerunning the transaction creation loop without
selecting new coins.

Thanks to instagibbs for working on this as well
This commit is contained in:
Alex Morcos 2017-06-30 13:16:53 -04:00
parent 253cd7ec4f
commit 0f402b9263
1 changed files with 40 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
src/wallet/wallet.cpp
View File

@ -2600,8 +2600,12 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
scriptChange = GetScriptForDestination(vchPubKey.GetID()); scriptChange = GetScriptForDestination(vchPubKey.GetID());
} }
CTxOut change_prototype_txout(0, scriptChange);
size_t change_prototype_size = GetSerializeSize(change_prototype_txout, SER_DISK, 0);
nFeeRet = 0; nFeeRet = 0;
bool pick_new_inputs = true;
CAmount nValueIn = 0;
// Start with no fee and loop until there is enough fee // Start with no fee and loop until there is enough fee
while (true) while (true)
{ {
@ -2647,15 +2651,18 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
} }
// Choose coins to use // Choose coins to use
CAmount nValueIn = 0; if (pick_new_inputs) {
setCoins.clear(); nValueIn = 0;
if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coinControl)) setCoins.clear();
{ if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coinControl))
strFailReason = _("Insufficient funds"); {
return false; strFailReason = _("Insufficient funds");
return false;
}
} }
const CAmount nChange = nValueIn - nValueToSelect; const CAmount nChange = nValueIn - nValueToSelect;
if (nChange > 0) if (nChange > 0)
{ {
// Fill a vout to ourself // Fill a vout to ourself
@ -2739,16 +2746,30 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
} }
if (nFeeRet >= nFeeNeeded) { if (nFeeRet >= nFeeNeeded) {
// Reduce fee to only the needed amount if we have change // Reduce fee to only the needed amount if possible. This
// output to increase. This prevents potential overpayment // prevents potential overpayment in fees if the coins
// in fees if the coins selected to meet nFeeNeeded result // selected to meet nFeeNeeded result in a transaction that
// in a transaction that requires less fee than the prior // requires less fee than the prior iteration.
// iteration.
// TODO: The case where nSubtractFeeFromAmount > 0 remains // TODO: The case where nSubtractFeeFromAmount > 0 remains
// to be addressed because it requires returning the fee to // to be addressed because it requires returning the fee to
// the payees and not the change output. // the payees and not the change output.
// TODO: The case where there is no change output remains
// to be addressed so we avoid creating too small an output. // 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.
CAmount fee_needed_for_change = GetMinimumFee(change_prototype_size, currentConfirmationTarget, ::mempool, ::feeEstimator, nullptr, false /* ignoreGlobalPayTxFee */, conservative_estimate);
CAmount minimum_value_for_change = GetDustThreshold(change_prototype_txout, ::dustRelayFee);
CAmount max_excess_fee = fee_needed_for_change + minimum_value_for_change;
if (nFeeRet > nFeeNeeded + max_excess_fee && nChangePosInOut == -1 && nSubtractFeeFromAmount == 0 && pick_new_inputs) {
pick_new_inputs = false;
nFeeRet = nFeeNeeded + fee_needed_for_change;
continue;
}
// If we have change output already, just increase it
if (nFeeRet > nFeeNeeded && nChangePosInOut != -1 && nSubtractFeeFromAmount == 0) { if (nFeeRet > nFeeNeeded && nChangePosInOut != -1 && nSubtractFeeFromAmount == 0) {
CAmount extraFeePaid = nFeeRet - nFeeNeeded; CAmount extraFeePaid = nFeeRet - nFeeNeeded;
std::vector<CTxOut>::iterator change_position = txNew.vout.begin()+nChangePosInOut; std::vector<CTxOut>::iterator change_position = txNew.vout.begin()+nChangePosInOut;
@ -2757,6 +2778,12 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
} }
break; // Done, enough fee included. 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
strFailReason = _("Transaction fee and change calculation failed");
return false;
}
// Try to reduce change to include necessary fee // Try to reduce change to include necessary fee
if (nChangePosInOut != -1 && nSubtractFeeFromAmount == 0) { if (nChangePosInOut != -1 && nSubtractFeeFromAmount == 0) {