diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2869,47 +2869,8 @@ 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(Amount::zero(), scriptChange); - size_t change_prototype_size = - GetSerializeSize(change_prototype_txout, SER_DISK, 0); - nFeeRet = Amount::zero(); - bool pick_new_inputs = true; - Amount nValueIn = Amount::zero(); - // Start with no fee and loop until there is enough fee + // Start with no fee and loop until there is enough fee. while (true) { nChangePosInOut = nChangePosRequest; txNew.vin.clear(); @@ -2960,15 +2921,13 @@ txNew.vout.push_back(txout); } - // Choose coins to use - if (pick_new_inputs) { - nValueIn = Amount::zero(); - setCoins.clear(); - if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, - nValueIn, coinControl)) { - strFailReason = _("Insufficient funds"); - return false; - } + // Choose coins to use. + Amount nValueIn = Amount::zero(); + setCoins.clear(); + if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, + nValueIn, coinControl)) { + strFailReason = _("Insufficient funds"); + return false; } for (const auto &pcoin : setCoins) { @@ -2990,6 +2949,40 @@ const Amount nChange = nValueIn - nValueToSelect; if (nChange > Amount::zero()) { // 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); // We do not move dust-change to fees, because the sender would @@ -3024,6 +3017,7 @@ if (newTxOut.IsDust(dustRelayFee)) { nChangePosInOut = -1; nFeeRet += nChange; + reservekey.ReturnKey(); } else { if (nChangePosInOut == -1) { // Insert change txn at random position: @@ -3039,6 +3033,7 @@ txNew.vout.insert(position, newTxOut); } } else { + reservekey.ReturnKey(); nChangePosInOut = -1; } @@ -3091,36 +3086,15 @@ } if (nFeeRet >= nFeeNeeded) { - // 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. - + // 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. - - // 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. - Amount fee_needed_for_change = - GetMinimumFee(change_prototype_size, - currentConfirmationTarget, g_mempool); - Amount minimum_value_for_change = - change_prototype_txout.GetDustThreshold(dustRelayFee); - Amount 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 + // 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) { Amount extraFeePaid = nFeeRet - nFeeNeeded; @@ -3132,12 +3106,6 @@ // Done, enough fee included. break; - } 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. @@ -3161,11 +3129,6 @@ continue; } - if (nChangePosInOut == -1) { - // Return any reserved key if we don't have change - reservekey.ReturnKey(); - } - if (sign) { SigHashType sigHashType = SigHashType().withForkId();