Giving the user a hint about what action to take when they encounter an error related to a feerate that is below the minimum. I encountered this myself not knowing about -mintxfee and the manpage was slightly less than clear,
-mintxfee=<amt>
Fee rates (in BTC/kvB) smaller than this are considered zero fee for transaction creation
(default: 0.00001)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ismaelsadeeq, w0xlt, achow101 |
| Concept ACK | rkrux, musaHaruna |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
1434 | @@ -1435,7 +1435,18 @@ RPCHelpMan sendall() 1435 | // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly 1436 | // provided one 1437 | if (coin_control.m_feerate && fee_rate > *coin_control.m_feerate) { 1438 | - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Fee rate (%s) is lower than the minimum fee rate setting (%s)", coin_control.m_feerate->ToString(FeeRateFormat::SAT_VB), fee_rate.ToString(FeeRateFormat::SAT_VB))); 1439 | + const auto f = FeeRateFormat::SAT_VB;
Sure its usage is limited and bounded but calling it just f doesn't help the reader much.
Thanks, changed to feerate_format
1443 | + if (fee_calc_out.reason == FeeReason::REQUIRED) { 1444 | + msg += strprintf("\nConsider modifying %s (%s) or %s (%s).", 1445 | + "-mintxfee", 1446 | + pwallet->m_min_fee.ToString(f), 1447 | + "-minrelaytxfee", 1448 | + pwallet->chain().relayMinFee().ToString(f));
I don't see a strong reason to add placeholders for hardcoded values. The following diff for succinctness?
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -1440,10 +1440,8 @@ RPCHelpMan sendall()
coin_control.m_feerate->ToString(f),
fee_rate.ToString(f))};
if (fee_calc_out.reason == FeeReason::REQUIRED) {
- msg += strprintf("\nConsider modifying %s (%s) or %s (%s).",
- "-mintxfee",
+ msg += strprintf("\nConsider modifying -mintxfee (%s) or -minrelaytxfee (%s).",
pwallet->m_min_fee.ToString(f),
- "-minrelaytxfee",
pwallet->chain().relayMinFee().ToString(f));
}
This was copied and pasted from https://github.com/bitcoin/bitcoin/blob/b77555c8fba1e8c1cc25124e62302684fabdb32f/src/wallet/spend.cpp#L1160-L1166
where using variables is necessary to avoid parameter names from being translated, hopefully in the future this logic will be consolidated and live exclusively in src/wallet/spend.cpp and so I tried to keep the two as similar as possible, but I'll include it in the string since it's not that important.
Concept ACK for the intent to add an actionable message in case of this error.
Concept ACK on improving error message for low feerates
Sure its usage is limited and bounded but calling it just
fdoesn't help the reader much.
Also agree with this comment. I think naming the variable format or fee_rate_format makes it more readable.
Pushed to address @rkrux's feedback:
f -> feerate_format (https://github.com/bitcoin/bitcoin/pull/34640#discussion_r2852726961)
Concept ACK
1160 | + coin_selection_params.m_effective_feerate.ToString(feerate_format))}; 1161 | + if (feeCalc.reason == FeeReason::REQUIRED) { 1162 | + msg += strprintf(_("\nConsider modifying %s (%s) or %s (%s)."), 1163 | + "-mintxfee", 1164 | + wallet.m_min_fee.ToString(feerate_format), 1165 | + "-minrelaytxfee",
nit: I like the approach in the src/wallet/rpc/spend.cpp where you just include mintxfee and minrelaytxfee in the msg string, it is less verbose than this.
Here it is necessary for the translation strings, we don't want to include parameter names which should not be translated in format strings.
1442 | + fee_rate.ToString(feerate_format))}; 1443 | + if (fee_calc_out.reason == FeeReason::REQUIRED) { 1444 | + msg += strprintf("\nConsider modifying -mintxfee (%s) or -minrelaytxfee (%s).", 1445 | + pwallet->m_min_fee.ToString(feerate_format), 1446 | + pwallet->chain().relayMinFee().ToString(feerate_format)); 1447 | + }
Modifying -minrelaytxfee is potentially not a good idea for the user. They may modify and have the transaction created, but not propagated.
So maybe clarify that?
The same is true of mintxfee, maybe a more general warning is in order about modifying default values that may cause your node to produce transactions which others might not relay?
ACK 98fcd7af23f3628514a499936720fd0885886b30
Should also test this e,g here and other places?
diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py
index ab8f74f20e..1674341b99 100755
--- a/test/functional/wallet_send.py
+++ b/test/functional/wallet_send.py
@@ -328,7 +328,7 @@ class WalletSendTest(BitcoinTestFramework):
# Test setting explicit fee rate just below the minimum of 1 sat/vB.
self.log.info("Explicit fee rate raises RPC error 'fee rate too low' if fee_rate of 0.99999999 is passed")
- msg = "Fee rate (0.999 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB)"
+ msg = "Fee rate (0.999 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB).\nConsider modifying -mintxfee (1.000 sat/vB) or -minrelaytxfee (0.100 sat/vB)."
self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=0.999, expect_error=(-4, msg))
self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=0.999, expect_error=(-4, msg))
ACK 98fcd7af23f3628514a499936720fd0885886b30
Maybe a common helper can de-duplicate the code.
ACK 98fcd7af23f3628514a499936720fd0885886b30