Merge bitcoin/bitcoin#31278: wallet, rpc: deprecate settxfee and paytxfee

2f2ab47 Release notes (Pol Espinasa)
bf194c9 wallet, rpc: deprecate settxfee and paytxfee (Pol Espinasa)

Pull request description:

  **Summary**

  This PR deprecates the settxfee RPC and paytxfee setting, marking it for removal in Bitcoin Core 31.0.

  **Motivation**

  The PR was initially motivated by bitcoin/bitcoin#31088. The intention was to create a new function `settxfeerate` to allow users to set a static fee rate in `sat/vB` instead of `btc/kvB`.

  The `settxfee` RPC allows users to set a static fee rate for all transactions created by the wallet. However, in a dynamic fee environment, this can lead to poor fee choices, either overpaying when the mempool is empty or underpaying when congestion is high. The preferred approach is to rely on fee estimation, which is designed to adapt to network conditions, and is the one by default. Same argument apply for `paytxfee` setting.

  During discussion the consensus was that static fee settings are a footgun and that users should instead specify the fee rate per transaction if they don't want to rely on the fee estimation. Given this, rather than introducing a `settxfeerate` alternative, this PR goes towards removing `settxfee` and `paytxfee` entirely.

  **Key Changes**

  `settxfee` and `paytxfee` is now deprecated and will be removed in Bitcoin Core 31.0.
  Users should rely on fee estimation or explicitly specify a fee rate when constructing transactions.

  **Impact on Users**

  If users currently use settxfee or paytxfee, they should transition to specifying fees per transaction.
  No immediate breakage in 30.0 (must use `-deprecatedrpc=settxfee`), but `settxfee` and `paytxfee` will be removed in 31.0.

  **Alternative Approaches Considered**

  A settxfeerate alternative (using sat/vB) was initially proposed but ultimately rejected in favor of deprecating static fee setting entirely.

  **Notes for removal**
  - When removing paytxfee we should also update txconfirmtarget startup option help text.
  - Get back the comment from `rpc_deprecated.py` test. [+info](bitcoin/bitcoin#31278 (comment))

ACKs for top commit:
  fjahr:
    re-ACK 2f2ab47
  ismaelsadeeq:
    re-ACK 2f2ab47
  rkrux:
    Concept and utACK 2f2ab47

Tree-SHA512: 0272812cbe5a519737c5d0683acc2072e67559792b4a6472bca8b23426e5ce9e88a3a1eba987feda70a082b8b474b3126893848628d7bf11e1520357b18c8d3e

Author: ryanofsky

doc/release-notes-31278.md

@@ -0,0 +1,3 @@
+RPC and Startup Option
+---
+The `-paytxfee` startup option and the `settxfee` RPC are now deprecated and will be removed in Bitcoin Core 31.0. They allowed the user to set a static fee rate for wallet transactions, which could potentially lead to overpaying or underpaying. Users should instead rely on fee estimation or specify a fee rate per transaction using the `fee_rate` argument in RPCs such as `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, and `sendmany`. (#31278)

src/wallet/init.cpp

@@ -66,7 +66,7 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
         CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-mintxfee=<amt>", strprintf("Fee rates (in %s/kvB) smaller than this are considered zero fee for transaction creation (default: %s)",
                                                             CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MINFEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
-    argsman.AddArg("-paytxfee=<amt>", strprintf("Fee rate (in %s/kvB) to add to transactions you send (default: %s)",
+    argsman.AddArg("-paytxfee=<amt>", strprintf("(DEPRECATED) Fee rate (in %s/kvB) to add to transactions you send (default: %s)",
                                                             CURRENCY_UNIT, FormatMoney(CFeeRate{DEFAULT_PAY_TX_FEE}.GetFeePerK())), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
 #ifdef ENABLE_EXTERNAL_SIGNER
     argsman.AddArg("-signer=<cmd>", "External signing tool, see doc/external-signer.md", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);

src/wallet/rpc/spend.cpp

@@ -418,7 +418,7 @@ RPCHelpMan sendmany()
 RPCHelpMan settxfee()
 {
     return RPCHelpMan{"settxfee",
-                "\nSet the transaction fee rate in " + CURRENCY_UNIT + "/kvB for this wallet. Overrides the global -paytxfee command line parameter.\n"
+                "\n(DEPRECATED) Set the transaction fee rate in " + CURRENCY_UNIT + "/kvB for this wallet. Overrides the global -paytxfee command line parameter.\n"
                 "Can be deactivated by passing 0 as the fee. In that case automatic fee selection will be used by default.\n",
                 {
                     {"amount", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "The transaction fee rate in " + CURRENCY_UNIT + "/kvB"},
@@ -437,6 +437,11 @@ RPCHelpMan settxfee()
 
     LOCK(pwallet->cs_wallet);
 
+    if (!pwallet->chain().rpcEnableDeprecated("settxfee")) {
+        throw JSONRPCError(RPC_METHOD_DEPRECATED, "settxfee is deprecated and will be fully removed in v31.0."
+        "\nTo use settxfee restart bitcoind with -deprecatedrpc=settxfee.");
+    }
+
     CAmount nAmount = AmountFromValue(request.params[0]);
     CFeeRate tx_fee_rate(nAmount, 1000);
     CFeeRate max_tx_fee_rate(pwallet->m_default_max_tx_fee, 1000);

src/wallet/wallet.cpp

@@ -3181,6 +3181,8 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
     }
 
     if (args.IsArgSet("-paytxfee")) {
+        warnings.push_back(_("-paytxfee is deprecated and will be fully removed in v31.0."));
+
         std::optional<CAmount> pay_tx_fee = ParseMoney(args.GetArg("-paytxfee", ""));
         if (!pay_tx_fee) {
             error = AmountErrMsg("paytxfee", args.GetArg("-paytxfee", ""));

test/functional/rpc_deprecated.py

@@ -4,13 +4,21 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test deprecation of RPC calls."""
 from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_raises_rpc_error
 
 class DeprecatedRpcTest(BitcoinTestFramework):
+    def add_options(self, parser):
+        self.add_wallet_options(parser)
+
     def set_test_params(self):
         self.num_nodes = 1
         self.setup_clean_chain = True
         self.extra_args = [[]]
 
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
     def run_test(self):
         # This test should be used to verify the errors of the currently
         # deprecated RPC methods (without the -deprecatedrpc flag) until
@@ -23,7 +31,8 @@ def run_test(self):
         # at least one other functional test that still tests the RPCs
         # functionality using the respective -deprecatedrpc flag.
 
-        self.log.info("Currently no tests for deprecated RPC methods")
+        self.log.info("Test settxfee RPC")
+        assert_raises_rpc_error(-32, 'settxfee is deprecated and will be fully removed in v31.0.', self.nodes[0].rpc.settxfee, 0.01)
 
 if __name__ == '__main__':
     DeprecatedRpcTest(__file__).main()

test/functional/test_framework/test_framework.py

@@ -641,9 +641,9 @@ def stop_nodes(self, wait=0):
             # Wait for nodes to stop
             node.wait_until_stopped()
 
-    def restart_node(self, i, extra_args=None, clear_addrman=False):
+    def restart_node(self, i, extra_args=None, clear_addrman=False, *, expected_stderr=''):
         """Stop and start a test node"""
-        self.stop_node(i)
+        self.stop_node(i, expected_stderr=expected_stderr)
         if clear_addrman:
             peers_dat = self.nodes[i].chain_path / "peers.dat"
             os.remove(peers_dat)

test/functional/wallet_basic.py

@@ -35,7 +35,7 @@ def set_test_params(self):
         # whitelist peers to speed up tx relay / mempool sync
         self.noban_tx_relay = True
         self.extra_args = [[
-            "-dustrelayfee=0", "-walletrejectlongchains=0"
+            "-dustrelayfee=0", "-walletrejectlongchains=0", "-deprecatedrpc=settxfee"
         ]] * self.num_nodes
         self.setup_clean_chain = True
         self.supports_cli = False

test/functional/wallet_bumpfee.py

@@ -61,6 +61,7 @@ def set_test_params(self):
             "-walletrbf={}".format(i),
             "-mintxfee=0.00002",
             "-addresstype=bech32",
+            "-deprecatedrpc=settxfee"
         ] for i in range(self.num_nodes)]
 
     def skip_test_if_missing_module(self):

test/functional/wallet_create_tx.py

@@ -23,6 +23,7 @@ def add_options(self, parser):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
+        self.extra_args = [["-deprecatedrpc=settxfee"]]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
@@ -71,7 +72,7 @@ def test_tx_size_too_large(self):
             )
 
         self.log.info('Check maxtxfee in combination with settxfee')
-        self.restart_node(0)
+        self.restart_node(0, expected_stderr='Warning: -paytxfee is deprecated and will be fully removed in v31.0.')
         self.nodes[0].settxfee(0.01)
         assert_raises_rpc_error(
             -6,

test/functional/wallet_fundrawtransaction.py

@@ -44,6 +44,9 @@ def add_options(self, parser):
 
     def set_test_params(self):
         self.num_nodes = 4
+        self.extra_args = [[
+            "-deprecatedrpc=settxfee"
+        ] for i in range(self.num_nodes)]
         self.setup_clean_chain = True
         # whitelist peers to speed up tx relay / mempool sync
         self.noban_tx_relay = True

test/functional/wallet_multiwallet.py

@@ -46,7 +46,7 @@ def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 2
         self.rpc_timeout = 120
-        self.extra_args = [["-nowallet"], []]
+        self.extra_args = [["-nowallet", "-deprecatedrpc=settxfee"], []]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()

test/functional/wallet_txn_clone.py

@@ -18,6 +18,9 @@ class TxnMallTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 3
         self.supports_cli = False
+        self.extra_args = [[
+            "-deprecatedrpc=settxfee"
+        ] for i in range(self.num_nodes)]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()