Skip to content

Commit 03ee701

Browse files
morcosmorcos
committed
Refactor to use CoinControl in GetMinimumFee and FeeBumper
Improve parameter precedence in coin_control
1 parent ecd81df commit 03ee701

File tree

9 files changed

+79
-73
lines changed

9 files changed

+79
-73
lines changed

‎src/qt/coincontroldialog.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -512,7 +512,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
512512
nBytes -= 34;
513513

514514
// Fee
515-
nPayFee = CWallet::GetMinimumFee(nBytes, coinControl->nConfirmTarget, ::mempool, ::feeEstimator, nullptr /* FeeCalculation */, false /* ignoreGlobalPayTxFee */, conservative_estimate);
515+
nPayFee = CWallet::GetMinimumFee(nBytes, *coinControl, ::mempool, ::feeEstimator, nullptr /* FeeCalculation */);
516516

517517
if (nPayAmount > 0)
518518
{
@@ -587,7 +587,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
587587
if (payTxFee.GetFeePerK() > 0)
588588
dFeeVary = (double)std::max(CWallet::GetRequiredFee(1000), payTxFee.GetFeePerK()) / 1000;
589589
else {
590-
dFeeVary = (double)std::max(CWallet::GetRequiredFee(1000), ::feeEstimator.estimateSmartFee(coinControl->nConfirmTarget, NULL, ::mempool, conservative_estimate).GetFeePerK()) / 1000;
590+
dFeeVary = (double)std::max(CWallet::GetRequiredFee(1000), ::feeEstimator.estimateSmartFee(*coinControl->m_confirm_target, NULL, ::mempool, conservative_estimate).GetFeePerK()) / 1000;
591591
}
592592
QString toolTip4 = tr("Can vary +/- %1 satoshi(s) per input.").arg(dFeeVary);
593593

‎src/qt/sendcoinsdialog.cpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -274,11 +274,11 @@ void SendCoinsDialog::on_sendButton_clicked()
274274
CCoinControl ctrl;
275275
if (model->getOptionsModel()->getCoinControlFeatures())
276276
ctrl = *CoinControlDialog::coinControl;
277-
if (ui->radioSmartFee->isChecked())
278-
ctrl.nConfirmTarget = getConfTargetForIndex(ui->confTargetSelector->currentIndex());
279-
else
280-
ctrl.nConfirmTarget = 0;
281-
277+
if (ui->radioSmartFee->isChecked()) {
278+
ctrl.m_confirm_target = getConfTargetForIndex(ui->confTargetSelector->currentIndex());
279+
} else {
280+
ctrl.m_confirm_target = boost::none;
281+
}
282282
ctrl.signalRbf = ui->optInRBF->isChecked();
283283

284284
prepareStatus = model->prepareTransaction(currentTransaction, ctrl);
@@ -848,9 +848,9 @@ void SendCoinsDialog::coinControlUpdateLabels()
848848
CoinControlDialog::payAmounts.clear();
849849
CoinControlDialog::fSubtractFeeFromAmount = false;
850850
if (ui->radioSmartFee->isChecked()) {
851-
CoinControlDialog::coinControl->nConfirmTarget = getConfTargetForIndex(ui->confTargetSelector->currentIndex());
851+
CoinControlDialog::coinControl->m_confirm_target = getConfTargetForIndex(ui->confTargetSelector->currentIndex());
852852
} else {
853-
CoinControlDialog::coinControl->nConfirmTarget = model->getDefaultConfirmTarget();
853+
CoinControlDialog::coinControl->m_confirm_target = boost::none;
854854
}
855855
CoinControlDialog::coinControl->signalRbf = ui->optInRBF->isChecked();
856856

‎src/qt/walletmodel.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include "sync.h"
2525
#include "ui_interface.h"
2626
#include "util.h" // for GetBoolArg
27+
#include "wallet/coincontrol.h"
2728
#include "wallet/feebumper.h"
2829
#include "wallet/wallet.h"
2930
#include "wallet/walletdb.h" // for BackupWallet
@@ -667,8 +668,10 @@ bool WalletModel::bumpFee(uint256 hash)
667668
{
668669
std::unique_ptr<CFeeBumper> feeBump;
669670
{
671+
CCoinControl coin_control;
672+
coin_control.signalRbf = true;
670673
LOCK2(cs_main, wallet->cs_wallet);
671-
feeBump.reset(new CFeeBumper(wallet, hash, nTxConfirmTarget, false, 0, true, FeeEstimateMode::UNSET));
674+
feeBump.reset(new CFeeBumper(wallet, hash, coin_control, 0));
672675
}
673676
if (feeBump->getResult() != BumpFeeResult::OK)
674677
{

‎src/wallet/coincontrol.h

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010
#include "primitives/transaction.h"
1111
#include "wallet/wallet.h"
1212

13+
#include <boost/optional.hpp>
14+
1315
/** Coin Control Features. */
1416
class CCoinControl
1517
{
@@ -19,12 +21,12 @@ class CCoinControl
1921
bool fAllowOtherInputs;
2022
//! Includes watch only addresses which match the ISMINE_WATCH_SOLVABLE criteria
2123
bool fAllowWatchOnly;
22-
//! Override estimated feerate
24+
//! Override automatic min/max checks on fee, m_feerate must be set if true
2325
bool fOverrideFeeRate;
24-
//! Feerate to use if overrideFeeRate is true
25-
CFeeRate nFeeRate;
26-
//! Override the default confirmation target, 0 = use default
27-
int nConfirmTarget;
26+
//! Override the default payTxFee if set
27+
boost::optional<CFeeRate> m_feerate;
28+
//! Override the default confirmation target if set
29+
boost::optional<unsigned int> m_confirm_target;
2830
//! Signal BIP-125 replace by fee.
2931
bool signalRbf;
3032
//! Fee estimation mode to control arguments to estimateSmartFee
@@ -41,9 +43,9 @@ class CCoinControl
4143
fAllowOtherInputs = false;
4244
fAllowWatchOnly = false;
4345
setSelected.clear();
44-
nFeeRate = CFeeRate(0);
46+
m_feerate = boost::none;
4547
fOverrideFeeRate = false;
46-
nConfirmTarget = 0;
48+
m_confirm_target = boost::none;
4749
signalRbf = fWalletRbf;
4850
m_fee_mode = FeeEstimateMode::UNSET;
4951
}

‎src/wallet/feebumper.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
44

55
#include "consensus/validation.h"
6+
#include "wallet/coincontrol.h"
67
#include "wallet/feebumper.h"
78
#include "wallet/wallet.h"
89
#include "policy/fees.h"
@@ -66,7 +67,7 @@ bool CFeeBumper::preconditionChecks(const CWallet *pWallet, const CWalletTx& wtx
6667
return true;
6768
}
6869

69-
CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, int newConfirmTarget, bool ignoreGlobalPayTxFee, CAmount totalFee, bool newTxReplaceable, FeeEstimateMode fee_mode)
70+
CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, const CCoinControl& coin_control, CAmount totalFee)
7071
:
7172
txid(std::move(txidIn)),
7273
nOldFee(0),
@@ -165,8 +166,7 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, int newConf
165166
nNewFee = totalFee;
166167
nNewFeeRate = CFeeRate(totalFee, maxNewTxSize);
167168
} else {
168-
bool conservative_estimate = CalculateEstimateType(fee_mode, newTxReplaceable);
169-
nNewFee = CWallet::GetMinimumFee(maxNewTxSize, newConfirmTarget, mempool, ::feeEstimator, nullptr /* FeeCalculation */, ignoreGlobalPayTxFee, conservative_estimate);
169+
nNewFee = CWallet::GetMinimumFee(maxNewTxSize, coin_control, mempool, ::feeEstimator, nullptr /* FeeCalculation */);
170170
nNewFeeRate = CFeeRate(nNewFee, maxNewTxSize);
171171

172172
// New fee rate must be at least old rate + minimum incremental relay rate
@@ -221,7 +221,7 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, int newConf
221221
}
222222

223223
// Mark new tx not replaceable, if requested.
224-
if (!newTxReplaceable) {
224+
if (!coin_control.signalRbf) {
225225
for (auto& input : mtx.vin) {
226226
if (input.nSequence < 0xfffffffe) input.nSequence = 0xfffffffe;
227227
}

‎src/wallet/feebumper.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
class CWallet;
1111
class CWalletTx;
1212
class uint256;
13+
class CCoinControl;
1314
enum class FeeEstimateMode;
1415

1516
enum class BumpFeeResult
@@ -25,7 +26,7 @@ enum class BumpFeeResult
2526
class CFeeBumper
2627
{
2728
public:
28-
CFeeBumper(const CWallet *pWalletIn, const uint256 txidIn, int newConfirmTarget, bool ignoreGlobalPayTxFee, CAmount totalFee, bool newTxReplaceable, FeeEstimateMode fee_mode);
29+
CFeeBumper(const CWallet *pWalletIn, const uint256 txidIn, const CCoinControl& coin_control, CAmount totalFee);
2930
BumpFeeResult getResult() const { return currentResult; }
3031
const std::vector<std::string>& getErrors() const { return vErrors; }
3132
CAmount getOldFee() const { return nOldFee; }

‎src/wallet/rpcwallet.cpp

Lines changed: 12 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -460,7 +460,7 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
460460
}
461461

462462
if (request.params.size() > 6 && !request.params[6].isNull()) {
463-
coin_control.nConfirmTarget = request.params[6].get_int();
463+
coin_control.m_confirm_target = request.params[6].get_int();
464464
}
465465

466466
if (request.params.size() > 7 && !request.params[7].isNull()) {
@@ -981,7 +981,7 @@ UniValue sendmany(const JSONRPCRequest& request)
981981
}
982982

983983
if (request.params.size() > 6 && !request.params[6].isNull()) {
984-
coin_control.nConfirmTarget = request.params[6].get_int();
984+
coin_control.m_confirm_target = request.params[6].get_int();
985985
}
986986

987987
if (request.params.size() > 7 && !request.params[7].isNull()) {
@@ -2730,13 +2730,9 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
27302730
RPCTypeCheck(request.params, {UniValue::VSTR});
27312731

27322732
CCoinControl coinControl;
2733-
coinControl.destChange = CNoDestination();
27342733
int changePosition = -1;
2735-
coinControl.fAllowWatchOnly = false; // include watching
27362734
bool lockUnspents = false;
27372735
bool reserveChangeKey = true;
2738-
coinControl.nFeeRate = CFeeRate(0);
2739-
coinControl.fOverrideFeeRate = false;
27402736
UniValue subtractFeeFromOutputs;
27412737
std::set<int> setSubtractFeeFromOutputs;
27422738

@@ -2788,7 +2784,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
27882784

27892785
if (options.exists("feeRate"))
27902786
{
2791-
coinControl.nFeeRate = CFeeRate(AmountFromValue(options["feeRate"]));
2787+
coinControl.m_feerate = CFeeRate(AmountFromValue(options["feeRate"]));
27922788
coinControl.fOverrideFeeRate = true;
27932789
}
27942790

@@ -2799,7 +2795,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
27992795
coinControl.signalRbf = options["replaceable"].get_bool();
28002796
}
28012797
if (options.exists("conf_target")) {
2802-
coinControl.nConfirmTarget = options["conf_target"].get_int();
2798+
coinControl.m_confirm_target = options["conf_target"].get_int();
28032799
}
28042800
if (options.exists("estimate_mode")) {
28052801
if (!FeeModeFromString(options["estimate_mode"].get_str(), coinControl.m_fee_mode)) {
@@ -2905,11 +2901,9 @@ UniValue bumpfee(const JSONRPCRequest& request)
29052901
hash.SetHex(request.params[0].get_str());
29062902

29072903
// optional parameters
2908-
bool ignoreGlobalPayTxFee = false;
2909-
int newConfirmTarget = nTxConfirmTarget;
29102904
CAmount totalFee = 0;
2911-
bool replaceable = true;
2912-
FeeEstimateMode fee_mode = FeeEstimateMode::UNSET;
2905+
CCoinControl coin_control;
2906+
coin_control.signalRbf = true;
29132907
if (request.params.size() > 1) {
29142908
UniValue options = request.params[1];
29152909
RPCTypeCheckObj(options,
@@ -2924,14 +2918,11 @@ UniValue bumpfee(const JSONRPCRequest& request)
29242918
if (options.exists("confTarget") && options.exists("totalFee")) {
29252919
throw JSONRPCError(RPC_INVALID_PARAMETER, "confTarget and totalFee options should not both be set. Please provide either a confirmation target for fee estimation or an explicit total fee for the transaction.");
29262920
} else if (options.exists("confTarget")) {
2927-
// If the user has explicitly set a confTarget in this rpc call,
2928-
// then override the default logic that uses the global payTxFee
2929-
// instead of the confirmation target.
2930-
ignoreGlobalPayTxFee = true;
2931-
newConfirmTarget = options["confTarget"].get_int();
2932-
if (newConfirmTarget <= 0) { // upper-bound will be checked by estimatefee/smartfee
2921+
int target = options["confTarget"].get_int();
2922+
if (target <= 0) { // FIXME: Check upper bound too
29332923
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid confTarget (cannot be <= 0)");
29342924
}
2925+
coin_control.m_confirm_target = target;
29352926
} else if (options.exists("totalFee")) {
29362927
totalFee = options["totalFee"].get_int64();
29372928
if (totalFee <= 0) {
@@ -2940,10 +2931,10 @@ UniValue bumpfee(const JSONRPCRequest& request)
29402931
}
29412932

29422933
if (options.exists("replaceable")) {
2943-
replaceable = options["replaceable"].get_bool();
2934+
coin_control.signalRbf = options["replaceable"].get_bool();
29442935
}
29452936
if (options.exists("estimate_mode")) {
2946-
if (!FeeModeFromString(options["estimate_mode"].get_str(), fee_mode)) {
2937+
if (!FeeModeFromString(options["estimate_mode"].get_str(), coin_control.m_fee_mode)) {
29472938
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter");
29482939
}
29492940
}
@@ -2952,7 +2943,7 @@ UniValue bumpfee(const JSONRPCRequest& request)
29522943
LOCK2(cs_main, pwallet->cs_wallet);
29532944
EnsureWalletIsUnlocked(pwallet);
29542945

2955-
CFeeBumper feeBump(pwallet, hash, newConfirmTarget, ignoreGlobalPayTxFee, totalFee, replaceable, fee_mode);
2946+
CFeeBumper feeBump(pwallet, hash, coin_control, totalFee);
29562947
BumpFeeResult res = feeBump.getResult();
29572948
if (res != BumpFeeResult::OK)
29582949
{

‎src/wallet/wallet.cpp

Lines changed: 38 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -2721,17 +2721,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
27212721
vin.scriptWitness.SetNull();
27222722
}
27232723

2724-
// Allow to override the default confirmation target over the CoinControl instance
2725-
int currentConfirmationTarget = nTxConfirmTarget;
2726-
if (coin_control.nConfirmTarget > 0)
2727-
currentConfirmationTarget = coin_control.nConfirmTarget;
2728-
2729-
// Allow to override the default fee estimate mode over the CoinControl instance
2730-
bool conservative_estimate = CalculateEstimateType(coin_control.m_fee_mode, coin_control.signalRbf);
2731-
2732-
CAmount nFeeNeeded = GetMinimumFee(nBytes, currentConfirmationTarget, ::mempool, ::feeEstimator, &feeCalc, false /* ignoreGlobalPayTxFee */, conservative_estimate);
2733-
if (coin_control.fOverrideFeeRate)
2734-
nFeeNeeded = coin_control.nFeeRate.GetFee(nBytes);
2724+
CAmount nFeeNeeded = GetMinimumFee(nBytes, coin_control, ::mempool, ::feeEstimator, &feeCalc);
27352725

27362726
// If we made it here and we aren't even able to meet the relay fee on the next pass, give up
27372727
// because we must be at the maximum allowed fee.
@@ -2756,7 +2746,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
27562746
// new inputs. We now know we only need the smaller fee
27572747
// (because of reduced tx size) and so we should add a
27582748
// change output. Only try this once.
2759-
CAmount fee_needed_for_change = GetMinimumFee(change_prototype_size, currentConfirmationTarget, ::mempool, ::feeEstimator, nullptr, false /* ignoreGlobalPayTxFee */, conservative_estimate);
2749+
CAmount fee_needed_for_change = GetMinimumFee(change_prototype_size, coin_control, ::mempool, ::feeEstimator, nullptr);
27602750
CAmount minimum_value_for_change = GetDustThreshold(change_prototype_txout, ::dustRelayFee);
27612751
CAmount max_excess_fee = fee_needed_for_change + minimum_value_for_change;
27622752
if (nFeeRet > nFeeNeeded + max_excess_fee && nChangePosInOut == -1 && nSubtractFeeFromAmount == 0 && pick_new_inputs) {
@@ -2932,33 +2922,52 @@ CAmount CWallet::GetRequiredFee(unsigned int nTxBytes)
29322922
return std::max(minTxFee.GetFee(nTxBytes), ::minRelayTxFee.GetFee(nTxBytes));
29332923
}
29342924

2935-
CAmount CWallet::GetMinimumFee(unsigned int nTxBytes, unsigned int nConfirmTarget, const CTxMemPool& pool, const CBlockPolicyEstimator& estimator, FeeCalculation *feeCalc, bool ignoreGlobalPayTxFee, bool conservative_estimate)
2925+
CAmount CWallet::GetMinimumFee(unsigned int nTxBytes, const CCoinControl& coin_control, const CTxMemPool& pool, const CBlockPolicyEstimator& estimator, FeeCalculation *feeCalc)
29362926
{
2937-
// payTxFee is the user-set global for desired feerate
2938-
CAmount nFeeNeeded = payTxFee.GetFee(nTxBytes);
2939-
// User didn't set: use -txconfirmtarget to estimate...
2940-
if (nFeeNeeded == 0 || ignoreGlobalPayTxFee) {
2941-
nFeeNeeded = estimator.estimateSmartFee(nConfirmTarget, feeCalc, pool, conservative_estimate).GetFee(nTxBytes);
2942-
// ... unless we don't have enough mempool data for estimatefee, then use fallbackFee
2943-
if (nFeeNeeded == 0) {
2944-
nFeeNeeded = fallbackFee.GetFee(nTxBytes);
2927+
/* User control of how to calculate fee uses the following parameter precedence:
2928+
1. coin_control.m_feerate
2929+
2. coin_control.m_confirm_target
2930+
3. payTxFee (user-set global variable)
2931+
4. nTxConfirmTarget (user-set global variable)
2932+
The first parameter that is set is used.
2933+
*/
2934+
CAmount fee_needed;
2935+
if (coin_control.m_feerate) { // 1.
2936+
fee_needed = coin_control.m_feerate->GetFee(nTxBytes);
2937+
if (feeCalc) feeCalc->reason = FeeReason::PAYTXFEE;
2938+
// Allow to override automatic min/max check over coin control instance
2939+
if (coin_control.fOverrideFeeRate) return fee_needed;
2940+
}
2941+
else if (!coin_control.m_confirm_target && ::payTxFee != CFeeRate(0)) { // 3. TODO: remove magic value of 0 for global payTxFee
2942+
fee_needed = ::payTxFee.GetFee(nTxBytes);
2943+
if (feeCalc) feeCalc->reason = FeeReason::PAYTXFEE;
2944+
}
2945+
else { // 2. or 4.
2946+
// We will use smart fee estimation
2947+
unsigned int target = coin_control.m_confirm_target ? *coin_control.m_confirm_target : ::nTxConfirmTarget;
2948+
// Allow to override the default fee estimate mode over the CoinControl instance
2949+
bool conservative_estimate = CalculateEstimateType(coin_control.m_fee_mode, coin_control.signalRbf);
2950+
2951+
fee_needed = estimator.estimateSmartFee(target, feeCalc, pool, conservative_estimate).GetFee(nTxBytes);
2952+
if (fee_needed == 0) {
2953+
// if we don't have enough data for estimateSmartFee, then use fallbackFee
2954+
fee_needed = fallbackFee.GetFee(nTxBytes);
29452955
if (feeCalc) feeCalc->reason = FeeReason::FALLBACK;
29462956
}
2947-
} else {
2948-
if (feeCalc) feeCalc->reason = FeeReason::PAYTXFEE;
29492957
}
2958+
29502959
// prevent user from paying a fee below minRelayTxFee or minTxFee
2951-
CAmount requiredFee = GetRequiredFee(nTxBytes);
2952-
if (requiredFee > nFeeNeeded) {
2953-
nFeeNeeded = requiredFee;
2960+
CAmount required_fee = GetRequiredFee(nTxBytes);
2961+
if (required_fee > fee_needed) {
2962+
fee_needed = required_fee;
29542963
if (feeCalc) feeCalc->reason = FeeReason::REQUIRED;
29552964
}
29562965
// But always obey the maximum
2957-
if (nFeeNeeded > maxTxFee) {
2958-
nFeeNeeded = maxTxFee;
2966+
if (fee_needed > maxTxFee) {
2967+
fee_needed = maxTxFee;
29592968
if (feeCalc) feeCalc->reason = FeeReason::MAXTXFEE;
29602969
}
2961-
return nFeeNeeded;
2970+
return fee_needed;
29622971
}
29632972

29642973

‎src/wallet/wallet.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -964,7 +964,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
964964
* Estimate the minimum fee considering user set parameters
965965
* and the required fee
966966
*/
967-
static CAmount GetMinimumFee(unsigned int nTxBytes, unsigned int nConfirmTarget, const CTxMemPool& pool, const CBlockPolicyEstimator& estimator, FeeCalculation *feeCalc, bool ignoreGlobalPayTxFee, bool conservative_estimate);
967+
static CAmount GetMinimumFee(unsigned int nTxBytes, const CCoinControl& coin_control, const CTxMemPool& pool, const CBlockPolicyEstimator& estimator, FeeCalculation *feeCalc);
968968
/**
969969
* Return the minimum required fee taking into account the
970970
* floating relay fee and user set minimum transaction fee

0 commit comments

Comments
 (0)