Skip to content

Commit

Permalink
Merge bitcoin#10712: Add change output if necessary to reduce excess fee
Browse files Browse the repository at this point in the history 
0f402b9 Fix rare edge case of paying too many fees when transaction has no change. (Alex Morcos)
253cd7e Only reserve key for scriptChange once in CreateTransaction (Alex Morcos)

Pull request description:

  This is an alternative to bitcoin#10333

  See commit messages.

  The first commit is mostly code move, it just moves the change creation code out of the loop.

  @instagibbs

Tree-SHA512: f16287ae0f0c6f09cf8b1f0db5880bb567ffa74a50898e3d1ef549ba592c6309ae1a9b251739f63a8bb622d48f03ce2dff9e7a57a6bac4afb4b95b0a86613ea8
Signed-off-by: Pasta <pasta@dashboost.org>
  • Loading branch information
@laanwj @PastaPastaPasta
laanwj authored and PastaPastaPasta committed Aug 7, 2019
1 parent 70c9ffc commit b5b7868
Showing 1 changed file with 71 additions and 43 deletions.
114 changes: 71 additions & 43 deletions src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3559,7 +3559,43 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
std::vector<COutput> vAvailableCoins;
AvailableCoins(vAvailableCoins, true, coinControl, 1, MAX_MONEY, MAX_MONEY, 0, 0, 9999999, nCoinType);

// 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
CScript scriptChange;

// coin control: send change to custom address
if (coinControl && !boost::get<CNoDestination>(&coinControl->destChange))
scriptChange = GetScriptForDestination(coinControl->destChange);

// no coin control: send change to newly generated address
else
{
// Note: We use a new key here to keep it from being obvious which side is the change.
// The drawback is that by not reusing a previous key, the change may be lost if a
// backup is restored, if the backup doesn't have the new private key for the change.
// If we reused the old key, it would be possible to add code to look for and
// rediscover unknown transactions that were written with keys of ours to recover
// post-backup change.

// Reserve a new key pair from key pool
CPubKey vchPubKey;
bool ret;
ret = reservekey.GetReservedKey(vchPubKey, true);
if (!ret)
{
strFailReason = _("Keypool ran out, please call keypoolrefill first");
return false;
}

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)
{
Expand Down Expand Up @@ -3605,9 +3641,10 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
}

// Choose coins to use
CAmount nValueIn = 0;
setCoins.clear();
if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coinControl, nCoinType))
if (pick_new_inputs) {
nValueIn = 0;
setCoins.clear();
if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coinControl, nCoinType))
{
if (nCoinType == ONLY_NONDENOMINATED) {
strFailReason = _("Unable to locate enough PrivateSend non-denominated funds for this transaction.");
Expand All @@ -3618,50 +3655,20 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
strFailReason = _("Insufficient funds.");
}

return false;
return false;}
}

const CAmount nChange = nValueIn - nValueToSelect;
CTxOut newTxOut;

if (nChange > 0)
{

//over pay for denominated transactions
if (nCoinType == ONLY_DENOMINATED) {
nFeeRet += nChange;
wtxNew.mapValue["DS"] = "1";
} else {

// Fill a vout to ourself
// TODO: pass in scriptChange instead of reservekey so
// change transaction isn't always pay-to-dash-address
CScript scriptChange;

// coin control: send change to custom address
if (coinControl && !boost::get<CNoDestination>(&coinControl->destChange))
scriptChange = GetScriptForDestination(coinControl->destChange);

// no coin control: send change to newly generated address
else
{
// Note: We use a new key here to keep it from being obvious which side is the change.
// The drawback is that by not reusing a previous key, the change may be lost if a
// backup is restored, if the backup doesn't have the new private key for the change.
// If we reused the old key, it would be possible to add code to look for and
// rediscover unknown transactions that were written with keys of ours to recover
// post-backup change.

// Reserve a new key pair from key pool
CPubKey vchPubKey;
if (!reservekey.GetReservedKey(vchPubKey, true))
{
strFailReason = _("Keypool ran out, please call keypoolrefill first");
return false;
}
scriptChange = GetScriptForDestination(vchPubKey.GetID());
}

newTxOut = CTxOut(nChange, scriptChange);

// Never create dust outputs; if we would, just
Expand All @@ -3670,7 +3677,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
{
nChangePosInOut = -1;
nFeeRet += nChange;
reservekey.ReturnKey();

}
else
{
Expand All @@ -3690,7 +3697,6 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
}
}
} else {
reservekey.ReturnKey();
nChangePosInOut = -1;
}

Expand Down Expand Up @@ -3776,16 +3782,30 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
}

if (nFeeRet >= nFeeNeeded) {
// Reduce fee to only the needed amount if we have change
// output to increase. 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.
// 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.

// TODO: The case where nSubtractFeeFromAmount > 0 remains
// to be addressed because it requires returning the fee to
// 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 */);
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) {
CAmount extraFeePaid = nFeeRet - nFeeNeeded;
std::vector<CTxOut>::iterator change_position = txNew.vout.begin()+nChangePosInOut;
Expand All @@ -3794,6 +3814,12 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
}
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
if (nChangePosInOut != -1 && nSubtractFeeFromAmount == 0) {
Expand All @@ -3813,6 +3839,8 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
}
}

if (nChangePosInOut == -1) reservekey.ReturnKey(); // Return any reserved key if we don't have change

if (sign)
{
CTransaction txNewConst(txNew);
Expand Down

0 comments on commit b5b7868

Please sign in to comment.