wallet: error if an explicit fee rate was given but the needed fee rate differed

kallewoof commented at 5:12 am on March 6, 2020: member

This ensures that the code doesn’t silently ignore too low fee reates. It will now trigger an error in the QT client, if the user provides a fee rate below the minimum, and becomes a necessary check for #11413.

fanquake added the label Wallet on Mar 6, 2020

DrahtBot commented at 6:43 am on March 6, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
#17331 (Use effective values throughout coin selection by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors commented at 5:27 pm on March 6, 2020: member

Can you add a test with createfundedpsbt’s feeRate argument?

in src/wallet/wallet.cpp:2665 in 7ac14a5a86 outdated

2658@@ -2659,6 +2659,12 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
2659 
2660             // Get the fee rate to use effective values in coin selection
2661             CFeeRate nFeeRateNeeded = GetMinimumFeeRate(*this, coin_control, &feeCalc);
2662+            // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly
2663+            // provided one
2664+            if (coin_control.m_feerate && nFeeRateNeeded > *coin_control.m_feerate) {
2665+                strFailReason = strprintf("Fee rate is too low for the current mempool. Increase it to %s or modify -maxmempool", nFeeRateNeeded.ToString());

MarcoFalke commented at 7:04 pm on March 6, 2020:

The mempool fee is not considered when the user picks the fee rate. See also https://doxygen.bitcoincore.org/wallet_2fees_8cpp_source.html#l00028

How did you trigger this bug? It would be nice to include a test vector or steps to reproduce, otherwise this will just be dead code.

Sjors commented at 7:14 pm on March 6, 2020:

@MarcoFalke the error is triggered by setting a fee rate below 1 sat / byte in the updated sendtoaddress RPCs in #11413. See #11413 (comment) for discussion and some alternative ideas.

MarcoFalke commented at 7:18 pm on March 6, 2020:

If it is not possible to hit this case with Bitcoin Core master, it probably makes sense to only add it when it can be hit. And also add a test to hit it.

kallewoof commented at 1:21 am on March 7, 2020:

I triggered it with no problem in bitcoin-qt (set fee to 0.00000001 btc/kb; note that the GUI has changed it back to the minimum in the screenshot after I clicked “Send”)…

Without this patch, it goes to confirm screen, silently changing my fee rate:

kallewoof commented at 1:38 am on March 7, 2020:

The mempool fee is not considered when the user picks the fee rate. See also https://doxygen.bitcoincore.org/wallet_2fees_8cpp_source.html#l00028

How did you trigger this bug? It would be nice to include a test vector or steps to reproduce, otherwise this will just be dead code.

That code calls mempoolMinFee() though.

MarcoFalke commented at 11:05 am on April 17, 2020:

That code calls mempoolMinFee() though.

No? It doesn’t.

The code is

0         // Obey mempool min fee when using smart fee estimation
1         CFeeRate min_mempool_feerate = wallet.chain().mempoolMinFee();

Setting a fee manually in the gui is not “smart fee”. What am I missing?

kallewoof commented at 5:00 am on April 24, 2020:

You’re not missing anything, because you’re absolutely right. Should’ve read the code more carefully…

I’ve fixed the message.

kallewoof commented at 1:30 am on March 7, 2020: member

Can you add a test with createfundedpsbt’s feeRate argument?

I tried and that takes me all the way to sendrawtransaction before it hits min relay fee not met, 1 < 141. The PSBT stuff doesn’t use the same method, unfortunately.

kallewoof force-pushed on Mar 7, 2020

Sjors commented at 9:12 am on March 7, 2020: member

I was hoping this was only an RPC issue. The error message is not very GUI friendly, but this should be bellt-and-suspenders check. The GUI is supposed to prevent you from entering a too low amount.

Your screenshot is a bit confusing, because the UI automatically corrects the input form when the value is below the minimum relay fee. But that correction only happens when you click somewhere else, like another field or the send button.

You can also see the issue when you launch with -minrelayfee=0.001 and then try 0.00001 as the fee. It will switch the amount back to 0.001, but when you click on Send it will still try the low fee rate.

tACK 7ac14a5a86d6bc8f15cf2c336a1bf560babf0c84

On a related note: I don’t like that the GUI changes the fee amount from under me. It should just color the field red and show an error message below the field. But that’s a separate PR imo, this check is useful regardless.

fjahr commented at 5:12 pm on March 13, 2020: member

tACK 7ac14a5a86d6bc8f15cf2c336a1bf560babf0c84

meshcollider commented at 10:54 am on April 17, 2020: contributor

Concept ACK

kallewoof force-pushed on Apr 24, 2020

kallewoof commented at 5:01 am on April 24, 2020: member

As @MarcoFalke pointed out, this actually does not rely on the current mempool limitations. I’ve updated the error message accordingly. It now reads

“Fee rate is lower than the minimum fee rate setting. Increase it to %s or modify -minrelaytxfee”, nFeeRateNeeded.ToString()

in src/wallet/wallet.cpp:2805 in f9b85c1832 outdated

2798@@ -2799,6 +2799,12 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
2799 
2800             // Get the fee rate to use effective values in coin selection
2801             CFeeRate nFeeRateNeeded = GetMinimumFeeRate(*this, coin_control, &feeCalc);
2802+            // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly
2803+            // provided one
2804+            if (coin_control.m_feerate && nFeeRateNeeded > *coin_control.m_feerate) {
2805+                strFailReason = strprintf("Fee rate is lower than the minimum fee rate setting. Increase it to %s or modify -minrelaytxfee", nFeeRateNeeded.ToString());

MarcoFalke commented at 12:04 pm on April 24, 2020:

I don’t think we should encourage user to modify the min relay fee in reply to issues the wallet or rpc encounters. Lowering your min fee won’t change transaction broadcast, as all your peers will still send you the same feefilter.

kallewoof commented at 1:57 am on April 25, 2020:

Yeah, I’m not sure what to say here, though. That is the only way to do what they’re attempting to do.

MarcoFalke commented at 12:22 pm on April 25, 2020:

0                strFailReason = strprintf("Fee rate (%s) is lower than the minimum fee rate setting (%s)", coin_control.m_feerate->ToString(), nFeeRateNeeded.ToString());

kallewoof commented at 2:19 pm on April 25, 2020:

Applied.

kallewoof force-pushed on Apr 25, 2020

fjahr commented at 3:49 pm on April 30, 2020: member

re-ACK b8711d07c85271b31f035686f77d44ee1c7a4365

Only changes were rebasing and improvement of the error message.

MarcoFalke commented at 1:31 pm on May 1, 2020: member

I can not reproduce with the steps given here: #18275 (review)

MarcoFalke commented at 1:43 pm on May 1, 2020: member

Couldn’t reproduce either with feeRate in fundrawtransaction. Still Concept ACK as a belt and suspenders, but this doesn’t fix a bug I can reproduce.

kallewoof commented at 5:52 am on May 2, 2020: member

I can not reproduce with the steps given here: #18275 (comment)

Are you trying to reproduce on this branch or on master? On master, this is what it ends up like (note that it silently resets my desired fee rate when I defocus and/or click send): https://youtu.be/I0JZpmJ-Gls

MarcoFalke commented at 8:51 pm on May 2, 2020: member

I mean I couldn’t get the newly introduced error to execute. Not in the GUI, and not in the RPC.

Just tried again with recompile. Maybe this is a mac-specific issue? Or I have something in my gui settings that prevents it.

Sjors commented at 10:14 am on May 4, 2020: member

tACK b8711d07c85271b31f035686f77d44ee1c7a4365. Just a rebase and text improvement since my last review.

kallewoof commented at 12:49 pm on May 4, 2020: member

@MarcoFalke Sorry about that [previous now-deleted comment showing ‘settxfee’ error], I got the messages mixed up. You’re right that it doesn’t seem possible to trigger this error from RPC/CLI. To trigger in QT, you need to change the fee rate and then click send without defocusing the window. This is probably platform dependent, so it may not be reproducable on non-macOS.

Sjors commented at 1:50 pm on May 4, 2020: member

Just double checked: I can still produce this error in the GUI.

MarcoFalke commented at 3:12 pm on May 4, 2020: member

code-review-only ACK b8711d07c85271b31f035686f77d44ee1c7a4365, seems this can only be hit on macos

meshcollider commented at 3:02 am on May 5, 2020: contributor

utACK b8711d07c85271b31f035686f77d44ee1c7a4365

meshcollider commented at 3:30 am on May 5, 2020: contributor

There is a silent merge conflict, strFailReason no longer exists on master (it has been replaced by a bilingual_str called error)

kallewoof force-pushed on May 5, 2020

kallewoof commented at 5:07 am on May 5, 2020: member

@meshcollider ~~Weird. Git happily automerged everything when I rebased on latest master. Pushed that rebase though.~~

Edit: Yeah, nevermind. Fixed.

wallet: error if an explicit fee rate was given but the needed fee rate differed

This avoids cases where a user requests a fee rate below the minimum and is silently overruled by the wallet.

44cc75f80e

kallewoof force-pushed on May 5, 2020

Sjors commented at 5:53 pm on May 5, 2020: member

utACK 44cc75f80ee7805a117e9298a182af1a44bcbff4 (rebased)

fjahr commented at 8:54 pm on May 5, 2020: member

re-ACK 44cc75f80ee7805a117e9298a182af1a44bcbff4

luke-jr referenced this in commit 6a8bc644b5 on Jun 9, 2020

Sjors commented at 10:07 am on June 16, 2020: member

@meshcollider?

MarcoFalke merged this on Jun 16, 2020

MarcoFalke closed this on Jun 16, 2020

domob1812 referenced this in commit 34e99427a4 on Jun 24, 2020

sidhujag referenced this in commit 437a0f8e4c on Jul 7, 2020

deadalnix referenced this in commit 21214d0ab8 on Aug 26, 2021

DrahtBot locked this on Feb 15, 2022

wallet: error if an explicit fee rate was given but the needed fee rate differed #18275

Conflicts