Merge #20220: wallet, rpc: explicit fee rate follow-ups/fixes for 0.21

0be29000c011dec0722481dbebb159873da6fa54 rpc: update conf_target helps for correctness/consistency (Jon Atack)
778b9be40667876c705e586849ea9c9e44cf451c wallet, rpc: fix send subtract_fee_from_outputs help (Jon Atack)
603c0050837ec65765208dd54dde354145fbe098 wallet: add rpc send explicit fee rate coverage (Jon Atack)
dd341e602d5160fc621c0299179b91403756b61d wallet: add sendtoaddress/sendmany explicit fee rate coverage (Jon Atack)
44e7bfa60313e4ae67da49e5ba4535038b71b453 wallet: add walletcreatefundedpsbt explicit fee rate coverage (Jon Atack)
6e1ea4273e52fdcd86c87628aa595c03a071ca8c test: refactor for walletcreatefundedpsbt fee rate coverage (Jon Atack)
3ac7b0c6f1c68e74a84d868a454f508bada6b09d wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() (Jon Atack)
2d8eba8f8425a2515022d51f1f5b4911329fbf55 wallet: combine redundant bumpfee invalid params and args tests (Jon Atack)
1697a40b6f841a54ee0d9744ed7fd09034b0ddad wallet: improve bumpfee error/help, add explicit fee rate coverage (Jon Atack)
fc5721723d34f76f9e1ffd2e31f274ea6b22f894 wallet: fix SetFeeEstimateMode() error message (Jon Atack)
052427eef1c9da84c474c5161b1910d3328ef0da wallet, bugfix: fix bumpfee with explicit fee rate modes (Jon Atack)

Pull request description:

  Follow-up to #11413 providing a base to build on for #19543:

  - bugfix for `bumpfee` raising a JSON error with explicit feerates, fixes issue #20219
  - adds explicit feerate test coverage for `bumpfee`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendtoaddress`, and `sendmany`
  - improves a few related RPC error messages and `ParseConfirmTarget()` / error message
  - fixes/improves the explicit fee rate information in the 6 RPC helps, of which 2 were also missing `conf_target` sat/B units

  This provides a spec and regression coverage for the potential next step of a universal `sat/vB` feerate argument (see #19543), as well as immediate coverage and minimum fixes for 0.21.

ACKs for top commit:
  kallewoof:
    Concept/Tested ACK 0be29000c011dec0722481dbebb159873da6fa54
  meshcollider:
    Code review + functional test run ACK 0be29000c011dec0722481dbebb159873da6fa54

Tree-SHA512: efd965003e991cba51d4504e2940f06ab3d742e34022e96a673606b44fad85596aa03a8c1809f06df7ebcf21a38e18a891e54392fe3d6fb4d120bbe4ea0cf5e0

1 files changed, 63 insertions, 47 deletions

diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py
index 9877a9cbd7..88a7778e19 100755
--- a/test/functional/wallet_bumpfee.py
+++ b/test/functional/wallet_bumpfee.py
@@ -17,7 +17,7 @@ from decimal import Decimal
 import io
 
 from test_framework.blocktools import add_witness_commitment, create_block, create_coinbase, send_to_witness
-from test_framework.messages import BIP125_SEQUENCE_NUMBER, CTransaction
+from test_framework.messages import BIP125_SEQUENCE_NUMBER, COIN, CTransaction
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
@@ -36,6 +36,8 @@ NORMAL       = 0.00100000
 HIGH         = 0.00500000
 TOO_HIGH     = 1.00000000
 
+BTC_MODE = "BTC/kB"
+SAT_MODE = "sat/B"
 
 class BumpFeeTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -76,10 +78,9 @@ class BumpFeeTest(BitcoinTestFramework):
 
         self.log.info("Running tests")
         dest_address = peer_node.getnewaddress()
-        self.test_invalid_parameters(rbf_node, dest_address)
-        test_simple_bumpfee_succeeds(self, "default", rbf_node, peer_node, dest_address)
-        test_simple_bumpfee_succeeds(self, "fee_rate", rbf_node, peer_node, dest_address)
-        test_feerate_args(self, rbf_node, peer_node, dest_address)
+        for mode in ["default", "fee_rate", BTC_MODE, SAT_MODE]:
+            test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address)
+        self.test_invalid_parameters(rbf_node, peer_node, dest_address)
         test_segwit_bumpfee_succeeds(self, rbf_node, dest_address)
         test_nonrbf_bumpfee_fails(self, peer_node, dest_address)
         test_notmine_bumpfee_fails(self, rbf_node, peer_node, dest_address)
@@ -98,28 +99,56 @@ class BumpFeeTest(BitcoinTestFramework):
         test_small_output_with_feerate_succeeds(self, rbf_node, dest_address)
         test_no_more_inputs_fails(self, rbf_node, dest_address)
 
-    def test_invalid_parameters(self, node, dest_address):
-        txid = spend_one_input(node, dest_address)
-        # invalid estimate mode
-        assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", node.bumpfee, txid, {
-            "estimate_mode": "moo",
-        })
-        assert_raises_rpc_error(-3, "Expected type string", node.bumpfee, txid, {
-            "estimate_mode": 38,
-        })
-        assert_raises_rpc_error(-3, "Expected type string", node.bumpfee, txid, {
-            "estimate_mode": {
-                "foo": "bar",
-            },
-        })
-        assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", node.bumpfee, txid, {
-            "estimate_mode": Decimal("3.141592"),
-        })
-        # confTarget and conf_target
-        assert_raises_rpc_error(-8, "confTarget and conf_target options should not both be set", node.bumpfee, txid, {
-            "confTarget": 123,
-            "conf_target": 456,
-        })
+    def test_invalid_parameters(self, rbf_node, peer_node, dest_address):
+        self.log.info('Test invalid parameters')
+        rbfid = spend_one_input(rbf_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(-3, "Unexpected key totalFee", rbf_node.bumpfee, rbfid, {"totalFee": NORMAL})
+        assert_raises_rpc_error(-4, "is too high (cannot be higher than", rbf_node.bumpfee, rbfid, {"fee_rate": TOO_HIGH})
+
+        # 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)
+
+        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": 100, "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")
+        assert_raises_rpc_error(-8, "confTarget and conf_target options should not both be set",
+                                rbf_node.bumpfee, rbfid, {"confTarget": 123, "conf_target": 456})
+        for field in ["confTarget", "conf_target"]:
+            assert_raises_rpc_error(-4, "is too high (cannot be higher than -maxtxfee",
+                                    lambda: rbf_node.bumpfee(rbfid, {"estimate_mode": BTC_MODE, "conf_target": 2009}))
+            assert_raises_rpc_error(-4, "is too high (cannot be higher than -maxtxfee",
+                                    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()
 
 
@@ -132,6 +161,13 @@ def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address):
     if mode == "fee_rate":
         bumped_psbt = rbf_node.psbtbumpfee(rbfid, {"fee_rate": NORMAL})
         bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate": NORMAL})
+    elif mode == BTC_MODE:
+        bumped_psbt = rbf_node.psbtbumpfee(rbfid, {"conf_target": NORMAL, "estimate_mode": BTC_MODE})
+        bumped_tx = rbf_node.bumpfee(rbfid, {"conf_target": NORMAL, "estimate_mode": BTC_MODE})
+    elif mode == SAT_MODE:
+        sat_fee = NORMAL * COIN / 1000  # convert NORMAL from BTC/kB to sat/B
+        bumped_psbt = rbf_node.psbtbumpfee(rbfid, {"conf_target": sat_fee, "estimate_mode": SAT_MODE})
+        bumped_tx = rbf_node.bumpfee(rbfid, {"conf_target": sat_fee, "estimate_mode": SAT_MODE})
     else:
         bumped_psbt = rbf_node.psbtbumpfee(rbfid)
         bumped_tx = rbf_node.bumpfee(rbfid)
@@ -158,26 +194,6 @@ def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address):
     self.clear_mempool()
 
 
-def test_feerate_args(self, rbf_node, peer_node, dest_address):
-    self.log.info('Test fee_rate args')
-    rbfid = spend_one_input(rbf_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})
-
-    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()
-
-
 def test_segwit_bumpfee_succeeds(self, rbf_node, dest_address):
     self.log.info('Test that segwit-sourcing bumpfee works')
     # Create a transaction with segwit output, then create an RBF transaction