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

jonasschnelli commented at 11:15 PM on December 13, 2017: contributor

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

jonasschnelli added the label GUI on Dec 13, 2017

in src/qt/sendcoinsdialog.cpp:352 in 28a47e7854 outdated

 345 | @@ -346,6 +346,13 @@ void SendCoinsDialog::on_sendButton_clicked()
 346 |          questionString.append("</span>");
 347 |      }
 348 |  
 349 | +    if (prepareStatus.m_fallback_fee_used) {
 350 | +        questionString.append("<hr /><span style='color:#aa0000;'>");
 351 | +        questionString.append(tr("WARNING")+":</span> <span>");
 352 | +        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."));

promag commented at 3:29 AM on December 14, 2017:

s/estimations/estimation.

promag commented at 3:31 AM on December 14, 2017:

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

jonasschnelli commented at 6:26 AM on December 14, 2017:

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

in src/wallet/wallet.h:964 in 28a47e7854 outdated

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

promag commented at 3:32 AM on December 14, 2017:

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

in src/qt/walletmodel.h:155 in 28a47e7854 outdated

 152 | +              m_fallback_fee_used(_m_fallback_fee_used)
 153 |          {
 154 |          }
 155 |          StatusCode status;
 156 |          QString reasonCommitFailed;
 157 | +        bool m_fallback_fee_used;

promag commented at 3:33 AM on December 14, 2017:

These should be const?

jonasschnelli commented at 6:27 AM on December 14, 2017:

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

in src/qt/walletmodel.h:147 in 28a47e7854 outdated

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

promag commented at 3:34 AM on December 14, 2017:

Drop _m_ prefix? , bool fallback_fee_used = false).

in src/qt/walletmodel.cpp:303 in 28a47e7854 outdated

 299 | @@ -299,7 +300,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
 300 |              return AbsurdFee;
 301 |      }
 302 |  
 303 | -    return SendCoinsReturn(OK);
 304 | +    return SendCoinsReturn(OK, "", (feeCalc.reason == FeeReason::FALLBACK));

promag commented at 3:34 AM on December 14, 2017:

Drop ()?

in src/qt/walletmodel.cpp:289 in 28a47e7854 outdated

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

promag commented at 3:34 AM on December 14, 2017:

Drop ()?

promag commented at 3:37 AM on December 14, 2017: member

utACK 28a47e7. Some nits though.

Pass back FeeCalculation object in CWallet::CreateTransaction() 76cd4000d5

[Qt] Warn if fallback fee has been used 147dc47c7f

jonasschnelli force-pushed on Dec 14, 2017

jonasschnelli commented at 6:27 AM on December 14, 2017: contributor

Fixed @promag points

laanwj commented at 2:31 PM on December 14, 2017: member

Concept ACK.

morcos commented at 3:11 PM on December 14, 2017: member

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 commented at 7:00 PM on December 14, 2017: contributor

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

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

morcos commented at 7:48 PM on December 15, 2017: member

@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 commented at 7:55 PM on December 15, 2017: contributor

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

morcos commented at 8:32 PM on December 15, 2017: member

Sure you're right

thijstriemstra commented at 10:49 PM on December 20, 2017: none

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

MarcoFalke commented at 10:57 PM on December 20, 2017: member

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

I am assuming #11882.

jonasschnelli commented at 11:00 PM on December 20, 2017: contributor

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

thijstriemstra commented at 11:15 PM on December 20, 2017: none

[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 commented at 11:19 PM on December 20, 2017: member

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

thijstriemstra commented at 11:29 PM on December 20, 2017: none

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 commented at 6:34 PM on March 7, 2018: member

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

laanwj closed this on Mar 7, 2018

MarcoFalke locked this on Sep 8, 2021