wallet: improve bumpfee error/help, add explicit fee rate coverage

Author: jonatack

src/wallet/rpcwallet.cpp

@@ -3039,7 +3039,7 @@ static RPCHelpMan listunspent()
     };
 }
 
-void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options, CCoinControl& coinControl)
+void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl)
 {
     // Make sure the results are valid at least up to the most recent block
     // the user could have gotten from another RPC command prior to now
@@ -3373,7 +3373,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
         "The command will pay the additional fee by reducing change outputs or adding inputs when necessary. It may add a new change output if one does not already exist.\n"
         "All inputs in the original transaction will be included in the replacement transaction.\n"
         "The command will fail if the wallet or mempool contains a transaction that spends one of T's outputs.\n"
-        "By default, the new fee will be calculated automatically using estimatesmartfee.\n"
+        "By default, the new fee will be calculated automatically using the estimatesmartfee RPC.\n"
         "The user can specify a confirmation target for estimatesmartfee.\n"
         "Alternatively, the user can specify a fee_rate (" + CURRENCY_UNIT + " per kB) for the new transaction.\n"
         "At a minimum, the new fee rate must be high enough to pay an additional new relay fee (incrementalfee\n"
@@ -3459,7 +3459,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
 
         if (!conf_target.isNull()) {
             if (options.exists("fee_rate")) {
-                throw JSONRPCError(RPC_INVALID_PARAMETER, "conf_target can't be set with fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.");
+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both conf_target and fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.");
             }
         } else if (options.exists("fee_rate")) {
             CFeeRate fee_rate(AmountFromValue(options["fee_rate"]));
@@ -4115,7 +4115,7 @@ static RPCHelpMan send()
             CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf);
             CCoinControl coin_control;
             // Automatically select coins, unless at least one is manually selected. Can
-            // be overriden by options.add_inputs.
+            // be overridden by options.add_inputs.
             coin_control.m_add_inputs = rawTx.vin.size() == 0;
             FundTransaction(pwallet, rawTx, fee, change_position, options, coin_control);
 

test/functional/wallet_bumpfee.py

@@ -173,19 +173,50 @@ def test_feerate_args(self, rbf_node, peer_node, dest_address):
     self.sync_mempools((rbf_node, peer_node))
     assert rbfid in rbf_node.getrawmempool() and rbfid in peer_node.getrawmempool()
 
-    assert_raises_rpc_error(-8, "conf_target can't be set with fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.", rbf_node.bumpfee, rbfid, {"fee_rate": NORMAL, "confTarget": 1})
-
     assert_raises_rpc_error(-3, "Unexpected key totalFee", rbf_node.bumpfee, rbfid, {"totalFee": NORMAL})
-    assert_raises_rpc_error(-8, "conf_target can't be set with fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.", rbf_node.bumpfee, rbfid, {"fee_rate":0.00001, "confTarget": 1})
-
-    # Bumping to just above minrelay should fail to increase total fee enough, at least
-    assert_raises_rpc_error(-8, "Insufficient total fee", rbf_node.bumpfee, rbfid, {"fee_rate": INSUFFICIENT})
 
+    # For each fee mode, bumping to just above minrelay should fail to increase the total fee enough.
+    for options in [{"fee_rate": INSUFFICIENT}, {"conf_target": INSUFFICIENT, "estimate_mode": BTC_MODE}, {"conf_target": 1, "estimate_mode": SAT_MODE}]:
+        assert_raises_rpc_error(-8, "Insufficient total fee", rbf_node.bumpfee, rbfid, options)
     assert_raises_rpc_error(-3, "Amount out of range", rbf_node.bumpfee, rbfid, {"fee_rate": -1})
-
     assert_raises_rpc_error(-4, "is too high (cannot be higher than", rbf_node.bumpfee, rbfid, {"fee_rate": TOO_HIGH})
-    self.clear_mempool()
 
+    self.log.info("Test explicit fee rate raises RPC error if estimate_mode is passed without a conf_target")
+    for unit, fee_rate in {"SAT/B": 10, "BTC/KB": NORMAL}.items():
+        assert_raises_rpc_error(-8, "Selected estimate_mode {} requires a fee rate to be specified in conf_target".format(unit),
+                                rbf_node.bumpfee, rbfid, {"fee_rate": fee_rate, "estimate_mode": unit})
+
+    self.log.info("Test explicit fee rate raises RPC error if both fee_rate and conf_target are passed")
+    error_both = "Cannot specify both conf_target and fee_rate. Please provide either a confirmation " \
+                 "target in blocks for automatic fee estimation, or an explicit fee rate."
+    assert_raises_rpc_error(-8, error_both, rbf_node.bumpfee, rbfid, {"conf_target": NORMAL, "fee_rate": NORMAL})
+
+    self.log.info("Test invalid conf_target settings")
+    for field in ["confTarget", "conf_target"]:
+        assert_raises_rpc_error(-8, error_both, rbf_node.bumpfee, rbfid, {field: 1, "fee_rate": NORMAL})
+    too_high = "is too high (cannot be higher than -maxtxfee"
+    assert_raises_rpc_error(-4, too_high, lambda: rbf_node.bumpfee(rbfid, {"estimate_mode": BTC_MODE, "conf_target": 2009}))
+    assert_raises_rpc_error(-4, too_high, lambda: rbf_node.bumpfee(rbfid, {"estimate_mode": SAT_MODE, "conf_target": 2009 * 10000}))
+
+    self.log.info("Test invalid estimate_mode settings")
+    for k, v in {"number": 42, "object": {"foo": "bar"}}.items():
+        assert_raises_rpc_error(-3, "Expected type string for estimate_mode, got {}".format(k),
+                                lambda: rbf_node.bumpfee(rbfid, {"estimate_mode": v, "fee_rate": NORMAL}))
+    for mode in ["foo", Decimal("3.141592")]:
+        assert_raises_rpc_error(-8, "Invalid estimate_mode parameter",
+                                lambda: rbf_node.bumpfee(rbfid, {"estimate_mode": mode, "fee_rate": NORMAL}))
+
+    self.log.info("Test invalid fee_rate settings")
+    for mode in ["unset", "economical", "conservative", BTC_MODE, SAT_MODE]:
+        self.log.debug("{}".format(mode))
+        for k, v in {"string": "", "object": {"foo": "bar"}}.items():
+            assert_raises_rpc_error(-3, "Expected type number for fee_rate, got {}".format(k),
+                                    lambda: rbf_node.bumpfee(rbfid, {"estimate_mode": mode, "fee_rate": v}))
+        assert_raises_rpc_error(-3, "Amount out of range",
+                                lambda: rbf_node.bumpfee(rbfid, {"estimate_mode": mode, "fee_rate": -1}))
+        assert_raises_rpc_error(-8, "Invalid fee_rate 0.00000000 BTC/kB (must be greater than 0)",
+                                lambda: rbf_node.bumpfee(rbfid, {"estimate_mode": mode, "fee_rate": 0}))
+    self.clear_mempool()
 
 def test_segwit_bumpfee_succeeds(self, rbf_node, dest_address):
     self.log.info('Test that segwit-sourcing bumpfee works')