[Qt] Warn if fallback fee has been used #11892

jonasschnelli · 2017-12-13T23:15:31Z

This PR appends a warning at the send coins confirmation text in case the fallback fee was used.

promag

utACK 28a47e7. Some nits though.

promag · 2017-12-14T03:29:05Z

src/qt/sendcoinsdialog.cpp

+    if (prepareStatus.m_fallback_fee_used) {
+        questionString.append("<hr /><span style='color:#aa0000;'>");
+        questionString.append(tr("WARNING")+":</span> <span>");
+        questionString.append(tr("A static fallback transaction fee has been used due to missing fee estimations. Please consider waiting a few blocks until fee estimation is ready. Your transaction will very likely get stuck or you overpay in fees."));


s/estimations/estimation.

I would reorder the sentenses: 1, 2, 3 -> 3, 1, 2. Also, avoid you/your?

I think the order is correct (cause, fix, possible issue).

promag · 2017-12-14T03:32:45Z

src/wallet/wallet.h

@@ -961,7 +961,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
     * @note passing nChangePosInOut as -1 will result in setting a random position
     */
    bool CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet, int& nChangePosInOut,
-                           std::string& strFailReason, const CCoinControl& coin_control, bool sign = true);
+                           std::string& strFailReason, const CCoinControl& coin_control, FeeCalculation& feeCalc, bool sign = true);


fee_calc? This line already uses new convention. Same for others.

promag · 2017-12-14T03:33:44Z

src/qt/walletmodel.h

        {
        }
        StatusCode status;
        QString reasonCommitFailed;
+        bool m_fallback_fee_used;


These should be const?

I keep it non const for now to avoid changing code that receives that struct, also, other variables are also non-const.

promag · 2017-12-14T03:34:26Z

src/qt/walletmodel.h

@@ -144,13 +144,15 @@ class WalletModel : public QObject
    // Return status record for SendCoins, contains error id + information
    struct SendCoinsReturn
    {
-        SendCoinsReturn(StatusCode _status = OK, QString _reasonCommitFailed = "")
+        SendCoinsReturn(StatusCode _status = OK, QString _reasonCommitFailed = "", bool _m_fallback_fee_used = false)


Drop _m_ prefix? , bool fallback_fee_used = false).

promag · 2017-12-14T03:34:46Z

src/qt/walletmodel.cpp

@@ -299,7 +300,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
            return AbsurdFee;
    }

-    return SendCoinsReturn(OK);
+    return SendCoinsReturn(OK, "", (feeCalc.reason == FeeReason::FALLBACK));


promag · 2017-12-14T03:34:58Z

src/qt/walletmodel.cpp

@@ -285,7 +286,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
        {
            if(!fSubtractFeeFromAmount && (total + nFeeRequired) > nBalance)
            {
-                return SendCoinsReturn(AmountWithFeeExceedsBalance);
+                return SendCoinsReturn(AmountWithFeeExceedsBalance, "", (feeCalc.reason == FeeReason::FALLBACK));


jonasschnelli · 2017-12-14T06:27:59Z

Fixed @promag points

laanwj · 2017-12-14T14:31:15Z

Concept ACK.

morcos · 2017-12-14T15:11:12Z

Another option for the GUI would be to just prevent using a fallback fee at all. We already have the option to manually set the fee. We could instead refuse to send the transaction unless the user manually sets the fee. I think I would prefer that approach.

Actually it might not be terrible to do a similar thing for RPC. Where any sendto call fails if fee estimation is needed and doesn't work with an error message telling you you must specify the fee (hopefully the option to specify the fee exists for all RPC sending calls). But at the very least we could do it for QT.

jonasschnelli · 2017-12-14T19:00:42Z

@morcos see #11882.
My idea was to follow these steps:

Allow to disable fallbackfee (but off by default) (Disable default fallbackfee on mainnet #11882)
Force RBF BIP125 if fallbackfee is active (Disable default fallbackfee on mainnet #11882)
Warn in the GUI if fallbackfee is present (this PR)
Disable fallback fee by default in GUI (upcoming if Disable default fallbackfee on mainnet #11882 gets merged)
Disable fallback fee by default in RPC (upcoming if Disable default fallbackfee on mainnet #11882 gets merged, but later)

morcos · 2017-12-15T19:48:27Z

@jonasschnelli commented there.
too many options. lets just be more forceful about defaulting to the "right" thing. and if people want something different they have a way of manually doing that.

EDIT. And I think the release notes and help could say that if you are setting a fallbackfee it is highly recommend, but not required that you set walletrbf=1, but adding two more config options seems overkill.

jonasschnelli · 2017-12-15T19:55:12Z

@morcos. Yes. Sounds good.
This PR though is completely disconnected from the default value,... lets continue the default value discussion in #11882.

morcos · 2017-12-15T20:32:44Z

Sure you're right

thijstriemstra · 2017-12-20T22:49:18Z

"static fallback transaction fee" does not mean a lot to me (and most people). Can this be rephrased to something human readable?

maflcko · 2017-12-20T22:57:40Z

Is this still needed, since the user explicitly set the fallbackfee (and thus is aware of it)?

I am assuming #11882.

jonasschnelli · 2017-12-20T23:00:15Z

I think as long as a fallback fee is possible, a warning should appear in case such was used.

thijstriemstra · 2017-12-20T23:15:37Z

[I don't want to block progress on any PRs but] I'd have to google this 'static fallback transaction fee' and doublecheck before i'd feel happy to make the transaction after reading such a warning..

sipa · 2017-12-20T23:19:44Z

@thijstriemstra Would it help to put the "Please consider waiting a few blocks until fee estimation is ready" first?

thijstriemstra · 2017-12-20T23:29:09Z

In a way, but 'a few', can we be more specific? or less detailed? :) as long as cryptic statements like 'static fallback' are avoided I think. Thanks for the feedback!

laanwj · 2018-03-07T18:34:51Z

I think this can be closed now that the fallback fee has been disabled on mainnet (#11882).

jonasschnelli added the GUI label Dec 13, 2017

promag reviewed Dec 14, 2017

View reviewed changes

jonasschnelli added 2 commits December 13, 2017 20:25

Pass back FeeCalculation object in CWallet::CreateTransaction()

76cd400

[Qt] Warn if fallback fee has been used

147dc47

jonasschnelli force-pushed the 2017/12/qt_warn_fallbackfee branch from 28a47e7 to 147dc47 Compare December 14, 2017 06:27

maflcko mentioned this pull request Dec 20, 2017

cost too many fees? #10247

Closed

jonasschnelli mentioned this pull request Feb 9, 2018

Disable default fallbackfee on mainnet #11882

Merged

laanwj closed this Mar 7, 2018

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

[Qt] Warn if fallback fee has been used #11892

[Qt] Warn if fallback fee has been used #11892

Uh oh!

Conversation

jonasschnelli commented Dec 13, 2017

Uh oh!

promag 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!

jonasschnelli commented Dec 14, 2017

Uh oh!

laanwj commented Dec 14, 2017

Uh oh!

morcos commented Dec 14, 2017

Uh oh!

jonasschnelli commented Dec 14, 2017

Uh oh!

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

Uh oh!

Uh oh!

jonasschnelli commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morcos commented Dec 15, 2017

Uh oh!

thijstriemstra commented Dec 20, 2017

Uh oh!

maflcko commented Dec 20, 2017

Uh oh!

jonasschnelli commented Dec 20, 2017

Uh oh!

thijstriemstra commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Dec 20, 2017

Uh oh!

thijstriemstra commented Dec 20, 2017

Uh oh!

laanwj commented Mar 7, 2018

Uh oh!

Uh oh!

morcos commented Dec 15, 2017 •

edited

Loading

jonasschnelli commented Dec 15, 2017 •

edited

Loading

thijstriemstra commented Dec 20, 2017 •

edited

Loading