Merge #10712: Add change output if necessary to reduce excess fee
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 #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
This commit is contained in:
commit
e8b95239ee
1 changed files with 74 additions and 46 deletions
|
@ -2569,7 +2569,43 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
|
||||||
std::vector<COutput> vAvailableCoins;
|
std::vector<COutput> vAvailableCoins;
|
||||||
AvailableCoins(vAvailableCoins, true, coinControl);
|
AvailableCoins(vAvailableCoins, true, coinControl);
|
||||||
|
|
||||||
|
// 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;
|
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)
|
||||||
{
|
{
|
||||||
|
@ -2615,49 +2651,21 @@ 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
|
||||||
// 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 newTxOut(nChange, scriptChange);
|
CTxOut newTxOut(nChange, scriptChange);
|
||||||
|
|
||||||
// Never create dust outputs; if we would, just
|
// Never create dust outputs; if we would, just
|
||||||
|
@ -2666,7 +2674,6 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
|
||||||
{
|
{
|
||||||
nChangePosInOut = -1;
|
nChangePosInOut = -1;
|
||||||
nFeeRet += nChange;
|
nFeeRet += nChange;
|
||||||
reservekey.ReturnKey();
|
|
||||||
}
|
}
|
||||||
else
|
else
|
||||||
{
|
{
|
||||||
|
@ -2685,7 +2692,6 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
|
||||||
txNew.vout.insert(position, newTxOut);
|
txNew.vout.insert(position, newTxOut);
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
reservekey.ReturnKey();
|
|
||||||
nChangePosInOut = -1;
|
nChangePosInOut = -1;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -2740,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;
|
||||||
|
@ -2758,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) {
|
||||||
|
@ -2777,6 +2803,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)
|
if (sign)
|
||||||
{
|
{
|
||||||
CTransaction txNewConst(txNew);
|
CTransaction txNewConst(txNew);
|
||||||
|
|
Loading…
Reference in a new issue