/bitcoin

[wallet] fee fixes: always create change, adjust value, and p… #10333

Closed
wants to merge 2 commits into
from
Commits
Jump to file or symbol
Failed to load files and symbols.
+102 −85
Split
View
187 src/wallet/wallet.cpp
@@ -2501,6 +2501,13 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
wtxNew.fTimeReceivedIsTxTime = true;
wtxNew.BindWallet(this);
CMutableTransaction txNew;
+ // This transaction is used to track the best transaction that has change
+ // falling between uneconomical dust and MIN_FINAL_CHANGE. If the wallet
+ // runs out of funds trying to find a transaction that has pruneable
+ // change dust or change larger than MIN_FINAL_CHANGE, it will
+ // return this transaction.
+ CMutableTransaction tx_cached;
+ bool have_cached_txn = false;
// Discourage fee sniping.
//
@@ -2592,73 +2599,69 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
setCoins.clear();
if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coinControl))
{
+ // We previously succeeded with smaller change we can keep
+ if (have_cached_txn) {
+ txNew = tx_cached;
+ break;
+ }
strFailReason = _("Insufficient funds");
return false;
}
const CAmount nChange = nValueIn - nValueToSelect;
- if (nChange > 0)
- {
- // Fill a vout to ourself
- // TODO: pass in scriptChange instead of reservekey so
- // change transaction isn't always pay-to-bitcoin-address
- CScript scriptChange;
+ assert(nChange >= 0);
- // coin control: send change to custom address
- if (coinControl && !boost::get<CNoDestination>(&coinControl->destChange))
- scriptChange = GetScriptForDestination(coinControl->destChange);
+ // Fill a vout to ourself, may be removed later on final fee adjustment
+ // TODO: pass in scriptChange instead of reservekey so
+ // change transaction isn't always pay-to-bitcoin-address
+ CScript scriptChange;
- // no coin control: send change to newly generated address
- else
+ // 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)
{
- // 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());
+ strFailReason = _("Keypool ran out, please call keypoolrefill first");
+ return false;
}
- CTxOut newTxOut(nChange, scriptChange);
+ scriptChange = GetScriptForDestination(vchPubKey.GetID());
+ }
- // Never create dust outputs; if we would, just
- // add the dust to the fee.
- if (IsDust(newTxOut, ::dustRelayFee))
- {
- nChangePosInOut = -1;
- nFeeRet += nChange;
- reservekey.ReturnKey();
- }
- else
- {
- if (nChangePosInOut == -1)
- {
- // Insert change txn at random position:
- nChangePosInOut = GetRandInt(txNew.vout.size()+1);
- }
- else if ((unsigned int)nChangePosInOut > txNew.vout.size())
- {
- strFailReason = _("Change index out of range");
- return false;
- }
+ CTxOut newTxOut(nChange, scriptChange);
- std::vector<CTxOut>::iterator position = txNew.vout.begin()+nChangePosInOut;
- txNew.vout.insert(position, newTxOut);
- }
+ // TODO move this section near fee-adjustment area.
+ if (nChangePosInOut == -1)
+ {
+ // Insert change txn at random position:
+ nChangePosInOut = GetRandInt(txNew.vout.size()+1);
+ }
+ else if ((unsigned int)nChangePosInOut > txNew.vout.size())
@achow101

achow101 Jun 23, 2017

Contributor

I don't see how this case matters. Wouldn't nChangePosInOut never be set to an out of range index?

@instagibbs

instagibbs Jun 23, 2017

Member

Caller may request a too-high change position. This is also a simple code move, not going to change this logic.
+ {
+ strFailReason = _("Change index out of range");
+ return false;
+ }
+
+ std::vector<CTxOut>::iterator position = txNew.vout.begin()+nChangePosInOut;
+ // We don't want to add a change output for 0-value with subtractFeeFromAmount
+ if (nSubtractFeeFromAmount == 0 || nChange > 0) {
+ txNew.vout.insert(position, newTxOut);
} else {
- reservekey.ReturnKey();
nChangePosInOut = -1;
}
@@ -2709,41 +2712,55 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
return false;
}
- 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.
- // 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 (nFeeRet > nFeeNeeded && nChangePosInOut != -1 && nSubtractFeeFromAmount == 0) {
- CAmount extraFeePaid = nFeeRet - nFeeNeeded;
- std::vector<CTxOut>::iterator change_position = txNew.vout.begin()+nChangePosInOut;
- change_position->nValue += extraFeePaid;
- nFeeRet -= extraFeePaid;
+ // Modify fee to the needed amount from the change
+ // output, or recipients in the case of subtractFeeFromAmount.
+ // 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.
+ CAmount excessFee = nFeeRet - nFeeNeeded;
+
+ if (nSubtractFeeFromAmount == 0) {
+ // Take fee excess
+ txNew.vout[nChangePosInOut].nValue += excessFee;;
+ nFeeRet -= excessFee;
+
+ // Negative change value means include more fee and try again.
+ if (txNew.vout[nChangePosInOut].nValue < 0) {
+ reservekey.ReturnKey();
+ nFeeRet = nFeeNeeded;
+ continue;
}
- break; // Done, enough fee included.
+ // Drop change if dust
+ // TODO replace with economical change calc
+ else if (IsDust(txNew.vout[nChangePosInOut], ::dustRelayFee)) {
+ nFeeRet += txNew.vout[nChangePosInOut].nValue;
+ txNew.vout.erase(txNew.vout.begin() + nChangePosInOut);
+ nChangePosInOut = -1;
+ reservekey.ReturnKey();
+ // If larger than dust, but still small, increase fee target and try again
+ } else if (txNew.vout[nChangePosInOut].nValue < MIN_FINAL_CHANGE) {
+ // Save this transaction, use if we cannot get large-enough change
+ if (!have_cached_txn) {
+ tx_cached = txNew;
@achow101

achow101 Jun 23, 2017

Contributor

have_cached_txn should probably be set to true here as it is never actually set anywhere
+ have_cached_txn = true;
+ }
+ reservekey.ReturnKey();
+ // Excess fee will be given to change
+ nFeeRet = nFeeNeeded + (MIN_FINAL_CHANGE/2);
+ continue;
+ }
+ break;
}
-
- // Try to reduce change to include necessary fee
- if (nChangePosInOut != -1 && nSubtractFeeFromAmount == 0) {
- CAmount additionalFeeNeeded = nFeeNeeded - nFeeRet;
- std::vector<CTxOut>::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
+ // TODO: The case where nSubtractFeeFromAmount > 0 remains
+ // to be addressed because it requires returning the fee to
+ // the payees and not the change output.
+ else {
+ if (excessFee >= 0) {
+ break;
}
+ nFeeRet = nFeeNeeded;
}
-
- // Include more fee and try again.
- nFeeRet = nFeeNeeded;
- continue;
}
}