Improve wallet fee logic and fix GUI bugs #10706

morcos · 2017-06-29T18:18:21Z

This builds on #10589 (first 5 commits from that PR, last 5 commits are new)

The first couple commits refactor to use the CCoinControl class to pass fee calculation parameters around.

This allows for fixing the buggy interaction in QT between the global payTxFee which can be modified by the RPC call settxfee or temporarily modified by the QT custom fee settings. Before these changes the GUI could sometimes send a transaction with a recently set payTxFee and not respect the settings displayed in the GUI. After these changes, using the GUI does not involve the global transaction confirm target or payTxFee.

The prospective fee displays in the smart fee slider and the coin control dialog are changed to use the fee calculation from GetMinimumFee, this simplifies the code and makes them slightly more correct in edge cases.

Maxing the fee calculation with the mempool min fee is move from estimateSmartFee to GetMinimumFee.

This fixes a long standing bug, and should be tagged for 0.15 as it is holding up finalizing the estimatesmartfee RPC API before release.

TheBlueMatt

Looks good, definitely want this to go in with #10589 (I'm super not a fan of CalculateEstimateType as an GetMinimumFee helper, but its easy to get rid of after this PR, less so after 10589).

TheBlueMatt · 2017-07-09T17:02:50Z

src/wallet/coincontrol.h

+    //! Override the default payTxFee if set, 0 = use smart fee estimation
+    boost::optional<CFeeRate> m_feerate;
+    //! Override the default confirmation target
+    boost::optional<unsigned int> m_confirm_target;


Gah, I'd really rather keep the old magic value of just 0 means default instead of adding boost::optional (and kinda would prefer to skip boost::option in favor of more descriptive names for m_feerate, too).

I hate the magic values of 0. Actually I think I should make the logic even slightly more clear.
There should just be a clear parameter precedence.

coin_control.m_feerate

coin_control.m_confirm_target

::payTxFee (global)

::nTxConfirmTarget (globa)

It goes down the list until it fines one that is set and all the first 3 should be boost::optional's. Why do you not like boost::optional?
I could skip changing the global payTxFee for now, and still have that work via the 0 magic value.
The code to process this will be slightly longer in GetMininumFee but I think it'll be far more clear no?

Hmm, maybe I shouldnt argue against boost::optional. I'm not a fan of optional types, but they do make sense here. Any further simplification would be nice, this stuff is gross (thanks for cleaning it up!).

TheBlueMatt · 2017-07-09T22:18:41Z

src/wallet/coincontrol.h

-    //! Override the default confirmation target, 0 = use default
-    int nConfirmTarget;
+    //! Override the default payTxFee if set, 0 = use smart fee estimation
+    boost::optional<CFeeRate> m_feerate;


Can you write out more docs here?
"m_feerate unset uses global paytxfee (if set) otherwise uses smart fee estimation. m_feerate set to 0 ignores global paytxfee and uses smart fee estimation. m_feerate set skips fee estimation but applies minTxFee and maxTxFee"
"fOverrideFeeRate overrides all other feerate options (global paytxfee, smart fee estimation, txconfirmtarget, min+maxTxFee, etc) and uses m_feerate"

(see above, will document when we have a final logic)

TheBlueMatt · 2017-07-09T22:25:48Z

src/qt/walletmodel.cpp

        LOCK2(cs_main, wallet->cs_wallet);
-        feeBump.reset(new CFeeBumper(wallet, hash, nTxConfirmTarget, false, 0, true));
+        feeBump.reset(new CFeeBumper(wallet, hash, no_coin_control, 0));


This changes the newTxReplaceable option to fWalletRbf instead of true. Maybe leave it the way it was, or was this on purpose?

Good catch. Will fix. I think I just assumed how the code worked and didn't even look at what I was replacing.

morcos · 2017-07-10T16:45:40Z

Rebased against the updated #10589 and fixed bug as well as improved parameter precedence logic for Coin Control.

ryanofsky · 2017-07-10T22:58:06Z

src/wallet/wallet.cpp

@@ -2567,7 +2567,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
        LOCK2(cs_main, cs_wallet);
        {
            std::vector<COutput> vAvailableCoins;
-            AvailableCoins(vAvailableCoins, true, coinControl);
+            AvailableCoins(vAvailableCoins, true, &coin_control);


In commit "Make CoinControl a required argument to CreateTransaction"

Would be nice to make coin control a required argument to AvailableCoins as well. (To get rid of more multiply-defined default values.)

I agree, but decided it would make this PR a bit too big.

~~Which multiply defined values are you referring to though?~~ (edit: i think you just mean too many arguments with default values defined)

Which multiply defined values are you referring to though?

I was just referring to the fact that functions accepting null coincontrol pointers have to figure out their own default values instead of using the ones in CoinControl::SetNull.

ryanofsky · 2017-07-10T22:58:24Z

src/wallet/wallet.cpp

@@ -2617,7 +2617,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
                // Choose coins to use
                CAmount nValueIn = 0;
                setCoins.clear();
-                if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coinControl))
+                if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, &coin_control))


In commit "Make CoinControl a required argument to CreateTransaction"

Would be nice if to make coin control a required argument to SelectCoins as well.

ryanofsky · 2017-07-10T23:11:40Z

src/wallet/coincontrol.h

@@ -41,9 +43,9 @@ class CCoinControl
        fAllowOtherInputs = false;
        fAllowWatchOnly = false;
        setSelected.clear();
-        nFeeRate = CFeeRate(0);
+        m_feerate = boost::none;


In commit "Refactor to use CoinControl in GetMinimumFee and FeeBumper"

Maybe replace = boost::none with .reset() here and other places to remove boost reference and make it a little easier to port to std::optional in the future.

Actually turns out .reset() is deprecated so left it as is.

Actually turns out .reset() is deprecated so left it as is.

.reset is part of c++17 (http://en.cppreference.com/w/cpp/utility/optional/reset) so I still think it would be better to switch to this. (I don't think we'd be using boost::optional at all now if we weren't planning to switch to std::optional in the future.)

ryanofsky · 2017-07-10T23:17:45Z

src/wallet/coincontrol.h

@@ -19,12 +21,12 @@ class CCoinControl
    bool fAllowOtherInputs;
    //! Includes watch only addresses which match the ISMINE_WATCH_SOLVABLE criteria
    bool fAllowWatchOnly;
-    //! Override estimated feerate
+    //! Override automatic min/max checks on fee, m_feerate must be set if true
    bool fOverrideFeeRate;


In commit "Refactor to use CoinControl in GetMinimumFee and FeeBumper"

You should definitely get rid of the fOverrideFeeRate member now that m_feerate is optional.

Well.. almost
fOverrideFeeRate is used from fundrawtransaction and it seems to me that it might make sense that you don't want your specified fee rate subject to clamping by various min fees or the maxTxFee when you are using it from there but you do generally.

In any case it would be a change of functionality to change that, so we should do it separately.

fOverrideFeeRate is used from fundrawtransaction

Sorry, misread fOverrideFeeRate comment. And this is now pretty clear with fOverrideFeeRate handled in GetMinimumFee.

morcos · 2017-07-11T14:47:13Z

Rebased cleanly so only new commits are left after #10589 was merged

TheBlueMatt · 2017-07-11T15:31:04Z

src/wallet/wallet.cpp

-                CAmount nFeeNeeded = GetMinimumFee(nBytes, currentConfirmationTarget, ::mempool, ::feeEstimator, &feeCalc, false /* ignoreGlobalPayTxFee */, conservative_estimate);
-                if (coinControl && coinControl->fOverrideFeeRate)
-                    nFeeNeeded = coinControl->nFeeRate.GetFee(nBytes);
+                // Allow to override automatic fee calculation (min/max checks) over coin control instance


It would be nice to move this check into GetMinimumFee so that everything is ompletely in the same place.

morcos · 2017-07-11T17:49:12Z

Rebased to accomodate #10712 and moved fOverrideFeeRate inside GetMinimumFee

ryanofsky

utACK fb98c64

ryanofsky · 2017-07-11T14:20:22Z

src/wallet/wallet.cpp

@@ -2567,7 +2567,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
        LOCK2(cs_main, cs_wallet);
        {
            std::vector<COutput> vAvailableCoins;
-            AvailableCoins(vAvailableCoins, true, coinControl);
+            AvailableCoins(vAvailableCoins, true, &coin_control);


Which multiply defined values are you referring to though?

I was just referring to the fact that functions accepting null coincontrol pointers have to figure out their own default values instead of using the ones in CoinControl::SetNull.

ryanofsky · 2017-07-11T15:23:49Z

src/wallet/coincontrol.h

@@ -41,9 +43,9 @@ class CCoinControl
        fAllowOtherInputs = false;
        fAllowWatchOnly = false;
        setSelected.clear();
-        nFeeRate = CFeeRate(0);
+        m_feerate = boost::none;


Actually turns out .reset() is deprecated so left it as is.

.reset is part of c++17 (http://en.cppreference.com/w/cpp/utility/optional/reset) so I still think it would be better to switch to this. (I don't think we'd be using boost::optional at all now if we weren't planning to switch to std::optional in the future.)

ryanofsky · 2017-07-11T18:28:07Z

src/wallet/rpcwallet.cpp

-            newConfirmTarget = options["confTarget"].get_int();
-            if (newConfirmTarget <= 0) { // upper-bound will be checked by estimatefee/smartfee
+            int target = options["confTarget"].get_int();
+            if (target <= 0) { // FIXME: Check upper bound too


In commit "Refactor to use CoinControl in GetMinimumFee and FeeBumper"

Why this fixme? Is the previous comment about upper bound not actually true?

Correct, that comment about upper-bound is not true, I don't know why it was put there.
At the time I wrote these PR's I had a lot of important stuff I wanted to make sure got in, so I didn't want to fix every last little thing, but I think I could easily add a commit now that checks the bounds everywhere necessary.

ryanofsky · 2017-07-11T18:31:58Z

src/wallet/coincontrol.h

@@ -19,12 +21,12 @@ class CCoinControl
    bool fAllowOtherInputs;
    //! Includes watch only addresses which match the ISMINE_WATCH_SOLVABLE criteria
    bool fAllowWatchOnly;
-    //! Override estimated feerate
+    //! Override automatic min/max checks on fee, m_feerate must be set if true
    bool fOverrideFeeRate;


fOverrideFeeRate is used from fundrawtransaction

Sorry, misread fOverrideFeeRate comment. And this is now pretty clear with fOverrideFeeRate handled in GetMinimumFee.

ryanofsky · 2017-07-11T18:49:00Z

src/qt/coincontroldialog.cpp

@@ -587,7 +587,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
    if (payTxFee.GetFeePerK() > 0)
        dFeeVary = (double)std::max(CWallet::GetRequiredFee(1000), payTxFee.GetFeePerK()) / 1000;
    else {
-        dFeeVary = (double)std::max(CWallet::GetRequiredFee(1000), ::feeEstimator.estimateSmartFee(coinControl->nConfirmTarget, NULL, ::mempool, conservative_estimate).GetFeePerK()) / 1000;
+        dFeeVary = (double)std::max(CWallet::GetRequiredFee(1000), ::feeEstimator.estimateSmartFee(*coinControl->m_confirm_target, NULL, ::mempool, conservative_estimate).GetFeePerK()) / 1000;


In commit "Refactor to use CoinControl in GetMinimumFee and FeeBumper"

Maybe assert(coinControl->m_confirm_target) somewhere in this function. The event handling code which sets this seems a little precarious, so it'd be good to know if coinControl hasn't been initialized correctly.

seems reasonable

this one went away, but addressed a similar one in qt/sendcoinsdialog.cpp

ryanofsky · 2017-07-11T20:31:45Z

src/qt/sendcoinsdialog.cpp

-                                                                std::max(CWallet::fallbackFee.GetFeePerK(), CWallet::GetRequiredFee(1000))) + "/kB");
+    ui->labelSmartFee->setText(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), feeRate.GetFeePerK()) + "/kB");
+
+    if (feeCalc.reason == FeeReason::FALLBACK) {


In commit "Make QT fee displays use GetMinimumFee instead of estimateSmartFee":

Is this sufficient to know fee estimation succeeded? For example would it be possible for estimate to fail, and then fallbackFee to be less than RequiredFee, so reason would be REQUIRED not FALLBACK?

Also can you expand commit description to say a little bit about why it's better to display minimum fee instead of estimated fee directly here. It seems perfectly reasonable, I'd just like to understand why the min fee wasn't being being used all along, and whether one value is clearly more useful than the other or if this is more intended to be a minor tweak that simplifies the code.

I think the behavior in rare edge cases is maybe not precisely correct, but there is likely no harm in it. The only thing you are missing is a warning that fee estimation is not up to date yet, and before this PR that could already happen if there was a mempool min fee (in which case you wouldn't even get maxed with the fallback fee). After this PR, you only miss the warning if your fallback fee is less than mempool min fee or minRelayTxFee or minTxFee. The first case is a strict improvement over prior behavior. The second two cases can only result from very poor configuration of command line options. In short, I think it could be improved to report even more precise information to the user but it's outside the scope of what I wanted to do here.

I can expand commit description, but the idea is the displays should correspond to the fees that your actually about to put on the transaction if you click send. I think it's essentially a bug that it didn't do that before.

I think the behavior in rare edge cases is maybe not precisely correct, but there is likely no harm in it.

I see. This is less significant than I thought because the fee is shown either way, just the labeling is different.

I can expand commit description, but the idea is the displays should correspond to the fees that your actually about to put on the transaction if you click send. I think it's essentially a bug that it didn't do that before.

Thanks, I didn't know if the motivation was to simplify code or if min fee was actually preferable to display. Fact that min fee is used to send the transaction definitely makes sense as a reason to display it.

TheBlueMatt · 2017-07-12T01:29:39Z

utACK fb98c64

morcos · 2017-07-12T18:58:30Z

Addressed @ryanofsky feedback and added a commit to do proper bounds checking. boost::none's were just all removed in one commit for simplicity.

(feelogic.ver2) -> c494da9 (feelogic.ver2.squash)

ryanofsky

utACK c494da9. Changes were adding qt coincontrol asserts, removing uses of boost::none, and adding a new commit with more conf_target checking.

jnewbery

utACK c494da9 . I'm not very familiar with the qt code, so I can't give you much feedback there, but the changes all look sensible.

A few nits. All could be addressed after merge (or left entirely).

jnewbery · 2017-07-13T17:44:05Z

src/wallet/wallet.cpp

        if (feeCalc) feeCalc->reason = FeeReason::PAYTXFEE;
+        // Allow to override automatic min/max check over coin control instance
+        if (coin_control.fOverrideFeeRate) return fee_needed;
+    } else {


nit: I think this is clearer as:

if { ... } else if { ... } else { ... }

rather than:

if { ... } else { if { ... } else { ... } }

(to match the logical construction of the comment above)

I wondered same thing initially, but I think it was written this way to unify the logic behind cases #2 and #4 which are very similar.

jnewbery · 2017-07-13T17:45:06Z

src/wallet/wallet.cpp

@@ -2469,7 +2469,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC

    CReserveKey reservekey(this);
    CWalletTx wtx;
-    if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, &coinControl, false))
+    if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, coinControl, false))


supernit: braces

jnewbery · 2017-07-13T17:45:21Z

src/wallet/wallet.cpp


            // 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<CNoDestination>(&coinControl->destChange))
-                scriptChange = GetScriptForDestination(coinControl->destChange);
+            if (!boost::get<CNoDestination>(&coin_control.destChange))


nit: braces

jnewbery · 2017-07-13T17:55:20Z

src/wallet/wallet.cpp

+
+            fee_needed = estimator.estimateSmartFee(target, feeCalc, conservative_estimate).GetFee(nTxBytes);
+            if (fee_needed == 0) {
+                // ... unless we don't have enough mempool data for estimatefee, then use fallbackFee


The ... unless comment doesn't make sense now that you've removed the earlier use -txconfirmtarget to estimate... comment. Perhaps just remove the ... unless?

jnewbery · 2017-07-13T18:57:20Z

src/rpc/mining.h

@@ -12,4 +12,7 @@
 /** Generate blocks (mine) */
 UniValue generateBlocks(std::shared_ptr<CReserveScript> coinbaseScript, int nGenerate, uint64_t nMaxTries, bool keepScript);

+/** Check bounds on a command line confirm target */


Why is this in rpc/mining.cpp? All of the calls to this function are in wallet/rpcwallet.cpp. Can you just put the function there?

Also the comment is a bit misleading - this function checks confirm target in RPCs, not command line arguments.

#10707 adds it to the RPC calls in rpc/mining.cpp and rpcwallet.cpp already includes rpc/mining.h

morcos · 2017-07-13T21:13:23Z

@jnewbery See if you like the reorganization of GetMinimumFee and if so I'll squash both commits

jnewbery · 2017-07-13T21:34:51Z

See if you like the reorganization of GetMinimumFee

I wasn't necessarily advocating for changing the order of the conditionals, just for flattening the nested ifs. I'm happy in either order.

Definitely like the extra comments referring to which of the 4 branches the blocks match up to.

The indentation on LL2942-2943 is incorrect. Fix that and I'm happy!

morcos · 2017-07-14T15:07:36Z

addressed @jnewbery nits and slight refactor of logic in GetMinimumFee (same end result)
(feelogic.ver4) -> 759db91 (feelogic.ver4.squash)

Improve parameter precedence in coin_control

This fixes buggy behavior where we were temporarily setting and unsetting the global payTxFee when trying to send a transaction with a custom fee from the GUI. The previous behavior was inconsistent depending on the order of using the RPC call settxfee and clicking various radio buttons in the sendcoinsdialog. The new behavior is that transactions sent with the GUI will always use either the smartfee slider value or the custom fee set on the GUI and they will not affect the global defaults which are only for RPC and initial GUI values.

Remove helper function (CalculateEstimateType) for determining whether estimates should be conservative or not, now that this is only called once from GetMinimumFee and incorporate the logic directly there.

This check has been moved to the wallet logic GetMinimumFee. The rpc call to estimatesmartfee will now no longer return a result maxed with the mempool min fee, but automated fee calculations from the wallet will produce the same result as before and coincontrol and sendcoins dialogs in the GUI will correctly display the right prospective fee. changes to policy/fees.cpp include a big whitespace indentation change.

morcos · 2017-07-15T03:48:52Z

Unfortunately this required a bit of a rebase due to conflict with #10769 in qt/sendcoinsdialog.cpp
Other files had clean rebase.

gmaxwell

concept ACK

sipa

Concept ACK. I very much like making everything uniformly use a coin control object for guiding estimation, and making the GUI's coin control independent from global settings.

Code looks good, but I haven't had time to review all logic changes.

sipa · 2017-07-15T18:42:41Z

src/rpc/mining.cpp

@@ -815,7 +815,6 @@ UniValue estimatesmartfee(const JSONRPCRequest& request)
            "\n"
            "A negative value is returned if not enough transactions and blocks\n"
            "have been observed to make an estimate for any number of blocks.\n"
-            "However it will not return a value below the mempool reject fee.\n"


Perhaps worth pointing out that the result may not be sufficient to be acceptable for the mempool?

I think we should at least flag the change in the release notes but I don't think that's necessarily something users need to worry about. In the event that the tx isn't accepted to the mempool it's still likely'ish to be confirmed within the target by being resubmitted after mempool min fee decays. Which makes me wonder if we should even be worrying about mempool min fee in the GetMinimumFee logic but enough changes for now.

sipa · 2017-07-15T18:44:30Z

src/wallet/wallet.cpp

+        // Allow to override automatic min/max check over coin control instance
+        if (coin_control.fOverrideFeeRate) return fee_needed;
+    }
+    else if (!coin_control.m_confirm_target && ::payTxFee != CFeeRate(0)) { // 3. TODO: remove magic value of 0 for global payTxFee


Nit: else on the same line

TheBlueMatt · 2017-07-16T22:18:41Z

re-utACK 11590d3

gmaxwell

ACK

laanwj · 2017-07-17T07:24:58Z

src/qt/sendcoinsdialog.cpp

@@ -274,14 +268,10 @@ void SendCoinsDialog::on_sendButton_clicked()
    CCoinControl ctrl;
    if (model->getOptionsModel()->getCoinControlFeatures())
        ctrl = *CoinControlDialog::coinControl;


Thanks. Definitely seems an improvement with regard to CCoinControl handling.
Hopefully in a next PR we can finally get rid of this global object too.

laanwj · 2017-07-17T07:25:30Z

utACK 11590d3

11590d3 Properly bound check conf_target in wallet RPC calls (Alex Morcos) fd29d3d Remove checking of mempool min fee from estimateSmartFee. (Alex Morcos) 2fffaa9 Make QT fee displays use GetMinimumFee instead of estimateSmartFee (Alex Morcos) 1983ca6 Use CoinControl to pass custom fee setting from QT. (Alex Morcos) 03ee701 Refactor to use CoinControl in GetMinimumFee and FeeBumper (Alex Morcos) ecd81df Make CoinControl a required argument to CreateTransaction (Alex Morcos) Pull request description: This builds on #10589 (first 5 commits from that PR, last 5 commits are new) The first couple commits refactor to use the CCoinControl class to pass fee calculation parameters around. This allows for fixing the buggy interaction in QT between the global payTxFee which can be modified by the RPC call settxfee or temporarily modified by the QT custom fee settings. Before these changes the GUI could sometimes send a transaction with a recently set payTxFee and not respect the settings displayed in the GUI. After these changes, using the GUI does not involve the global transaction confirm target or payTxFee. The prospective fee displays in the smart fee slider and the coin control dialog are changed to use the fee calculation from GetMinimumFee, this simplifies the code and makes them slightly more correct in edge cases. Maxing the fee calculation with the mempool min fee is move from estimateSmartFee to GetMinimumFee. This fixes a long standing bug, and should be tagged for 0.15 as it is holding up finalizing the estimatesmartfee RPC API before release. Tree-SHA512: 4d36a1bd5934aa62f3806d380fcafbef73e9fe5bdf190fc5259a3e3a13349e5ce796e50e7068c46dc630ccf56d061bce5804f0bfe2e082bb01ca725b63efd4c1

06bcdb8 Convert named argument from nblocks to conf_target (Alex Morcos) 439c4e8 Improve api to estimatesmartfee (Alex Morcos) Pull request description: Through 0.14 branch, the estimatesmartfee API was tagged "WARNING: This interface is unstable and may disappear or change!" and this warning is removed for 0.15, so any wanted API updates should happen now. The changes here are to make the additional parameter for conservative estimates a more general estimate_mode string , to omit the feerate and include an error string instead of returning -1 on error, and to do better parameter checking initially. ~It is only the last 2 commits, but it's built on #10706 and #10543~. See #10707 (comment) for renaming of nblocks argument to conf_target. Will need to be included before string freeze. PR description edited for clarity Tree-SHA512: 6d8ebee8bb410e2950ffd59663eebfed8d1611d995dc935bb91e430d9da7e2f306796f45631458376027d26341c660f09e825e61748103d2f2736ec6dc3df3ae

…fundrawtx 99c7fc3 Prevent user from specifying conflicting parameters to fundrawtx (Matt Corallo) Pull request description: estimate_mode/conf_target both are overridden by feeRate, so should not be specified together with feeRate. Based on #10706 Tree-SHA512: 8ccd08575fd1f2a0d45112538ffbbc73983ee172963230b0cc7ac41d13c6f3c740917f82b212c41ded3a64d873452e7f2c7af49f3b47cab897f8e85117f21333

Contains RBF stuff to be removed in a later commit 11590d3 Properly bound check conf_target in wallet RPC calls (Alex Morcos) fd29d3d Remove checking of mempool min fee from estimateSmartFee. (Alex Morcos) 2fffaa9 Make QT fee displays use GetMinimumFee instead of estimateSmartFee (Alex Morcos) 1983ca6 Use CoinControl to pass custom fee setting from QT. (Alex Morcos) 03ee701 Refactor to use CoinControl in GetMinimumFee and FeeBumper (Alex Morcos) ecd81df Make CoinControl a required argument to CreateTransaction (Alex Morcos) Pull request description: This builds on bitcoin#10589 (first 5 commits from that PR, last 5 commits are new) The first couple commits refactor to use the CCoinControl class to pass fee calculation parameters around. This allows for fixing the buggy interaction in QT between the global payTxFee which can be modified by the RPC call settxfee or temporarily modified by the QT custom fee settings. Before these changes the GUI could sometimes send a transaction with a recently set payTxFee and not respect the settings displayed in the GUI. After these changes, using the GUI does not involve the global transaction confirm target or payTxFee. The prospective fee displays in the smart fee slider and the coin control dialog are changed to use the fee calculation from GetMinimumFee, this simplifies the code and makes them slightly more correct in edge cases. Maxing the fee calculation with the mempool min fee is move from estimateSmartFee to GetMinimumFee. This fixes a long standing bug, and should be tagged for 0.15 as it is holding up finalizing the estimatesmartfee RPC API before release. Tree-SHA512: 4d36a1bd5934aa62f3806d380fcafbef73e9fe5bdf190fc5259a3e3a13349e5ce796e50e7068c46dc630ccf56d061bce5804f0bfe2e082bb01ca725b63efd4c1

…ers to fundrawtx 99c7fc3 Prevent user from specifying conflicting parameters to fundrawtx (Matt Corallo) Pull request description: estimate_mode/conf_target both are overridden by feeRate, so should not be specified together with feeRate. Based on bitcoin#10706 Tree-SHA512: 8ccd08575fd1f2a0d45112538ffbbc73983ee172963230b0cc7ac41d13c6f3c740917f82b212c41ded3a64d873452e7f2c7af49f3b47cab897f8e85117f21333

06bcdb8 Convert named argument from nblocks to conf_target (Alex Morcos) 439c4e8 Improve api to estimatesmartfee (Alex Morcos) Pull request description: Through 0.14 branch, the estimatesmartfee API was tagged "WARNING: This interface is unstable and may disappear or change!" and this warning is removed for 0.15, so any wanted API updates should happen now. The changes here are to make the additional parameter for conservative estimates a more general estimate_mode string , to omit the feerate and include an error string instead of returning -1 on error, and to do better parameter checking initially. ~It is only the last 2 commits, but it's built on bitcoin#10706 and bitcoin#10543~. See bitcoin#10707 (comment) for renaming of nblocks argument to conf_target. Will need to be included before string freeze. PR description edited for clarity Tree-SHA512: 6d8ebee8bb410e2950ffd59663eebfed8d1611d995dc935bb91e430d9da7e2f306796f45631458376027d26341c660f09e825e61748103d2f2736ec6dc3df3ae

Contains RBF stuff to be removed in a later commit 11590d3 Properly bound check conf_target in wallet RPC calls (Alex Morcos) fd29d3d Remove checking of mempool min fee from estimateSmartFee. (Alex Morcos) 2fffaa9 Make QT fee displays use GetMinimumFee instead of estimateSmartFee (Alex Morcos) 1983ca6 Use CoinControl to pass custom fee setting from QT. (Alex Morcos) 03ee701 Refactor to use CoinControl in GetMinimumFee and FeeBumper (Alex Morcos) ecd81df Make CoinControl a required argument to CreateTransaction (Alex Morcos) Pull request description: This builds on bitcoin#10589 (first 5 commits from that PR, last 5 commits are new) The first couple commits refactor to use the CCoinControl class to pass fee calculation parameters around. This allows for fixing the buggy interaction in QT between the global payTxFee which can be modified by the RPC call settxfee or temporarily modified by the QT custom fee settings. Before these changes the GUI could sometimes send a transaction with a recently set payTxFee and not respect the settings displayed in the GUI. After these changes, using the GUI does not involve the global transaction confirm target or payTxFee. The prospective fee displays in the smart fee slider and the coin control dialog are changed to use the fee calculation from GetMinimumFee, this simplifies the code and makes them slightly more correct in edge cases. Maxing the fee calculation with the mempool min fee is move from estimateSmartFee to GetMinimumFee. This fixes a long standing bug, and should be tagged for 0.15 as it is holding up finalizing the estimatesmartfee RPC API before release. Tree-SHA512: 4d36a1bd5934aa62f3806d380fcafbef73e9fe5bdf190fc5259a3e3a13349e5ce796e50e7068c46dc630ccf56d061bce5804f0bfe2e082bb01ca725b63efd4c1

…ers to fundrawtx 99c7fc3 Prevent user from specifying conflicting parameters to fundrawtx (Matt Corallo) Pull request description: estimate_mode/conf_target both are overridden by feeRate, so should not be specified together with feeRate. Based on bitcoin#10706 Tree-SHA512: 8ccd08575fd1f2a0d45112538ffbbc73983ee172963230b0cc7ac41d13c6f3c740917f82b212c41ded3a64d873452e7f2c7af49f3b47cab897f8e85117f21333

06bcdb8 Convert named argument from nblocks to conf_target (Alex Morcos) 439c4e8 Improve api to estimatesmartfee (Alex Morcos) Pull request description: Through 0.14 branch, the estimatesmartfee API was tagged "WARNING: This interface is unstable and may disappear or change!" and this warning is removed for 0.15, so any wanted API updates should happen now. The changes here are to make the additional parameter for conservative estimates a more general estimate_mode string , to omit the feerate and include an error string instead of returning -1 on error, and to do better parameter checking initially. ~It is only the last 2 commits, but it's built on bitcoin#10706 and bitcoin#10543~. See bitcoin#10707 (comment) for renaming of nblocks argument to conf_target. Will need to be included before string freeze. PR description edited for clarity Tree-SHA512: 6d8ebee8bb410e2950ffd59663eebfed8d1611d995dc935bb91e430d9da7e2f306796f45631458376027d26341c660f09e825e61748103d2f2736ec6dc3df3ae

in RPCs fundrawtransaction and walletcreatefundedpsbt only. This maintains the feeRate behavior in these two RPCs. See this GitHub discussion for more: bitcoin#10706

in RPCs fundrawtransaction and walletcreatefundedpsbt only. This provides the existing feeRate (BTC/kvB) behavior in these two RPCs to the new fee_rate (sat/vB) param also. See these two GitHub review discussions for more info: bitcoin#10706 bitcoin#20305 (comment)

in RPCs fundrawtransaction and walletcreatefundedpsbt only. This provides the existing feeRate (BTC/kvB) behavior in these two RPCs to the new fee_rate (sat/vB) param also. See these two GitHub review discussions for more info: bitcoin/bitcoin#10706 bitcoin/bitcoin#20305 (comment)

laanwj added this to the 0.15.0 milestone Jun 29, 2017

morcos mentioned this pull request Jun 29, 2017

Better API for estimatesmartfee RPC #10707

Merged

fanquake added TX fees and policy Wallet labels Jun 30, 2017

This was referenced Jun 30, 2017

Add change output if necessary to reduce excess fee #10712

Merged

More economical fee estimates for RBF and RPC options to control #10589

Merged

TheBlueMatt reviewed Jul 9, 2017

View reviewed changes

morcos force-pushed the improveWalletFeeLogic branch from 19d71b7 to abf98e6 Compare July 10, 2017 16:44

ryanofsky reviewed Jul 10, 2017

View reviewed changes

morcos force-pushed the improveWalletFeeLogic branch from abf98e6 to d72b3a5 Compare July 11, 2017 14:45

TheBlueMatt reviewed Jul 11, 2017

View reviewed changes

morcos force-pushed the improveWalletFeeLogic branch 2 times, most recently from 3b2dd93 to fb98c64 Compare July 11, 2017 17:48

ryanofsky reviewed Jul 11, 2017

View reviewed changes

TheBlueMatt mentioned this pull request Jul 12, 2017

Prevent user from specifying conflicting parameters to fundrawtx #10799

Merged

morcos force-pushed the improveWalletFeeLogic branch 2 times, most recently from 80afcf0 to c494da9 Compare July 12, 2017 18:55

ryanofsky reviewed Jul 12, 2017

View reviewed changes

morcos mentioned this pull request Jul 13, 2017

Redefine Dust and add a discard_rate #10817

Merged

jnewbery reviewed Jul 13, 2017

View reviewed changes

morcos force-pushed the improveWalletFeeLogic branch from 29fd69c to 759db91 Compare July 14, 2017 15:05

Make CoinControl a required argument to CreateTransaction

ecd81df

morcos added 5 commits July 14, 2017 23:10

Refactor to use CoinControl in GetMinimumFee and FeeBumper

03ee701

Improve parameter precedence in coin_control

Make QT fee displays use GetMinimumFee instead of estimateSmartFee

2fffaa9

Remove helper function (CalculateEstimateType) for determining whether estimates should be conservative or not, now that this is only called once from GetMinimumFee and incorporate the logic directly there.

Properly bound check conf_target in wallet RPC calls

11590d3

morcos force-pushed the improveWalletFeeLogic branch from 759db91 to 11590d3 Compare July 15, 2017 03:47

gmaxwell reviewed Jul 15, 2017

View reviewed changes

sipa reviewed Jul 15, 2017

View reviewed changes

gmaxwell approved these changes Jul 17, 2017

View reviewed changes

laanwj reviewed Jul 17, 2017

View reviewed changes

laanwj merged commit 11590d3 into bitcoin:master Jul 17, 2017

jnewbery mentioned this pull request Aug 1, 2017

TODO for release notes 0.15.0 #9889

Closed

12 tasks

jonatack mentioned this pull request Nov 11, 2020

wallet: introduce fee_rate sat/vB param/option #20305

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Improve wallet fee logic and fix GUI bugs #10706

Improve wallet fee logic and fix GUI bugs #10706

Uh oh!

Conversation

morcos commented Jun 29, 2017

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 11, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos Jul 11, 2017 •

edited

Loading