diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a1b4eb106..5e9701c71 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2569,7 +2569,43 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT std::vector vAvailableCoins; 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(&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) { @@ -2615,49 +2651,21 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } // Choose coins to use - CAmount nValueIn = 0; - setCoins.clear(); - if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coinControl)) - { - strFailReason = _("Insufficient funds"); - return false; + if (pick_new_inputs) { + nValueIn = 0; + setCoins.clear(); + if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coinControl)) + { + 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; - - // 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) - { - strFailReason = _("Keypool ran out, please call keypoolrefill first"); - return false; - } - - scriptChange = GetScriptForDestination(vchPubKey.GetID()); - } - CTxOut newTxOut(nChange, scriptChange); // Never create dust outputs; if we would, just @@ -2666,7 +2674,6 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT { nChangePosInOut = -1; nFeeRet += nChange; - reservekey.ReturnKey(); } else { @@ -2685,7 +2692,6 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT txNew.vout.insert(position, newTxOut); } } else { - reservekey.ReturnKey(); nChangePosInOut = -1; } @@ -2740,16 +2746,30 @@ bool CWallet::CreateTransaction(const std::vector& 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 */, 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) { CAmount extraFeePaid = nFeeRet - nFeeNeeded; std::vector::iterator change_position = txNew.vout.begin()+nChangePosInOut; @@ -2758,6 +2778,12 @@ bool CWallet::CreateTransaction(const std::vector& 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) { @@ -2777,6 +2803,8 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT } } + if (nChangePosInOut == -1) reservekey.ReturnKey(); // Return any reserved key if we don't have change + if (sign) { CTransaction txNewConst(txNew);