diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f07a6cb5a50..201cf971e10 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2501,6 +2501,13 @@ bool CWallet::CreateTransaction(const std::vector& 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& 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(&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(&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::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()) + { + strFailReason = _("Change index out of range"); + return false; + } + + std::vector::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& 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::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; + 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::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; } }