wallet, rpc: deprecate settxfee and paytxfee

polespinasa commented at 2:19 pm on November 12, 2024: contributor

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 #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

DrahtBot commented at 2:19 pm on November 12, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31278.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr, ismaelsadeeq, rkrux
Concept ACK	murchandamus, glozow, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31896 (refactor: Remove redundant and confusing calls to IsArgSet by maflcko)

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.

polespinasa commented at 2:24 pm on November 12, 2024: contributor

Please note that the 2 commits are different approaches of the same solution.

I personally prefer 5132d82d216df2d17cfef679998bf7dc607c1891 as the two functions are independent and deprecating will be easier.

b96cfae733a8a8eb79ee894a7cbe903fa955bcec is based on HandleRequest to forward the call to the original settxfee function. This gave me problems with how to handle error codes and also functions aren’t independent, so I think it’s a worse approach.

polespinasa renamed this:
~~Wallet, RPC: Settxfeerate~~
wallet, rpc: Settxfeerate
on Nov 12, 2024

polespinasa commented at 2:36 pm on November 12, 2024: contributor

The functional test runs correctly on my local environment: 64/313 - rpc_settxfeerate.py passed, Duration: 1 s

And also is passed on multiprocess, i686, DEBUG, Win64, unit tests, no gui tests, no functional tests, etc. Tried with skip_if_no_wallet() but then other tests fail.

Help regarding this is greatly appreciated.

DrahtBot added the label CI failed on Nov 12, 2024

DrahtBot commented at 2:50 pm on November 12, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32867056192

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

polespinasa force-pushed on Nov 12, 2024

DrahtBot removed the label CI failed on Nov 12, 2024

polespinasa force-pushed on Nov 12, 2024

in test/functional/rpc_settxfeerate.py:2 in 426dac6a71 outdated

0@@ -0,0 +1,56 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2014-2022 The Bitcoin Core developers

fjahr commented at 8:33 pm on November 12, 2024:

Why 2014-2022?

polespinasa commented at 8:50 pm on November 12, 2024:

have to update, used a template from an old test

in src/wallet/rpc/spend.cpp:472 in 426dac6a71 outdated

467+                },
468+                RPCResult{
469+                    RPCResult::Type::BOOL, "", "Returns true if successful"
470+                },
471+                RPCExamples{
472+                    HelpExampleCli("settxfee", "1")

fjahr commented at 8:33 pm on November 12, 2024:

Examples copy+pasted but not updated

polespinasa commented at 8:50 pm on November 12, 2024:

thanks for the observation, will correct it

in src/wallet/rpc/spend.cpp:421 in 426dac6a71 outdated

417@@ -418,7 +418,8 @@ RPCHelpMan sendmany()
418 RPCHelpMan settxfee()
419 {
420     return RPCHelpMan{"settxfee",
421-                "\nSet the transaction fee rate in " + CURRENCY_UNIT + "/kvB for this wallet. Overrides the global -paytxfee command line parameter.\n"
422+                "\nWill be deprecated see 'settxfeerate'.\n"

fjahr commented at 8:42 pm on November 12, 2024:

Why “will be deprecated”? Why not actually deprecate it?

polespinasa commented at 8:51 pm on November 12, 2024:

Avoid incompatibility problems with programs actually using this RPC call.

fjahr commented at 0:26 am on November 13, 2024:

That’s what we have the -deprecatedrpc parameter for

polespinasa commented at 2:02 pm on November 13, 2024:

I will mark this as resolved as we’re discussing the same in the other comment thread

in src/wallet/rpc/wallet.cpp:1159 in 426dac6a71 outdated

1156@@ -1156,7 +1157,8 @@ Span<const CRPCCommand> GetWalletRPCCommands()
1157         {"wallet", &sendtoaddress},
1158         {"wallet", &sethdseed},
1159         {"wallet", &setlabel},
1160-        {"wallet", &settxfee},
1161+        {"hidden", &settxfee},

fjahr commented at 8:45 pm on November 12, 2024:

Do we set deprecated rpc methods to hidden? I’m not sure I have seen that before.

polespinasa commented at 8:52 pm on November 12, 2024:

it’s what it’s suggested on https://github.com/bitcoin/bitcoin/issues/31088

fjahr commented at 0:36 am on November 13, 2024:

Looking back it seems like we haven’t hidden them, just deprecated. That’s also what I would suggest here.

polespinasa commented at 0:57 am on November 13, 2024:

Do you have an example of deprecated RPC so I can compare on how it was done before?

fjahr commented at 1:11 am on November 13, 2024:

Checking a few of these should give you an overview: https://github.com/bitcoin/bitcoin/pulls?q=is%3Apr+Deprecate+in%3Atitle+rpc+in%3Atitle+is%3Aclosed+

polespinasa commented at 2:04 pm on November 13, 2024:

Thanks! Applied following some of the examples.

murchandamus commented at 7:17 pm on February 27, 2025:

It seems to me that you agreed with @fjahr on his comment here, but the RPC is still being hidden. Is that an oversight?

polespinasa commented at 2:49 pm on February 28, 2025:

@fjahr @ismaelsadeeq txfeerate is now deprecated, but I kept it hidden. IMO it makes sense to have a deprecated rpc call hidden as only users who were using it before should know about it and may need info, they can continue using help settxfee

I justified it in another comment below. If you consider that it should not be hidden for some reason or if it is the way it has always been done and you want to keep the form, I will change.

ismaelsadeeq commented at 2:20 pm on March 3, 2025:

Makes sense to me to be hidden but no strong opinion

fjahr commented at 5:30 pm on March 3, 2025:

I posted the link above with examples and I haven’t seen a case where the RPC was hidden in there, so I think “the way it has always been” was to not hide. We haven’t deprecated/removed a whole RPC in a while though. But we can look at the options and returned values that were deprecated too: In that case it would make sense to remove the documentation of these options at the same time as we are deprecating them if we are following the justification for hiding. But instead we only add information that the option is deprecated and keep the documentation in place. My vote would still to not hide since it’s a form of documentation that may still be valuable but I am not feeling so strongly about it that it’s a blocker. It’s an interesting question in general though, so I think I will as in IRC if others have an opinion on this. It would be good to have clear guidance on this.

IMO it is considered best practice to retain documentation, generally speaking, but it’s just a feeling from having seen thousands of documentation pages with deprecation notices at the top. There seem to be some projects going in the other direction as well thought, a quick search revealed this for example: https://discourse.gohugo.io/t/documentation-for-deprecated-functions-methods-etc/46919

jonatack commented at 5:41 pm on March 3, 2025:

I don’t recall deprecated RPCs being set to hidden in the past. Generally we do that for developer-only ones that could represent footguns for users, or sometimes for an experimental RPC that later on is made public. I also don’t recall a public RPC being subsequently made hidden (after the hidden feature addition). If this RPC is considered to be sufficiently dangerous (am unconvinced), that could perhaps justify hiding it, but not due only to being deprecated.

ismaelsadeeq commented at 6:23 pm on March 3, 2025:

My thinking of deprecating a feature is discouraging its use without outright disabling it, only allowing it when explicitly required.

Hiding it from the help text is a good step in this direction.

You can still access details about the RPC by running:

bitcoin-cli settxfee –help

You just won’t see the feature description in the overview of available RPCs.

As for paytxfee, it is still listed in the startup option help text.

AFAICT, it is not deprecated it’s just warnings that are are emitted when it is used. See: #31278 (review)

jonatack commented at 6:43 pm on March 3, 2025:

Software clients that use the deprecated RPC API will see breakage when they update and look for why. It may be more confusing to no longer see the RPC in the list and potentially think it has been removed outright (“core devs broke my app without warning”) rather than deprecated behind a flag. Hopefully, people read the release notes but 🤷.

ismaelsadeeq commented at 6:46 pm on March 3, 2025:

fair point cc @polespinasa

polespinasa commented at 6:56 pm on March 3, 2025:

It may be more confusing to no longer see the RPC in the list and potentially think it has been removed.

That’s a fair point and a good reason to not hide it. I will just revert that.

It would be good to have clear guidance on this.

Would be good to have a written procedure on the dev notes to follow always the same procedure in the future and avoid repeating the discussion.

ismaelsadeeq commented at 7:04 pm on March 3, 2025:

Would be good to have a written procedure on the dev notes to follow always the same procedure in the future and avoid repeating the discussion.

Good idea for a follow up PR, a section on removing a feature will be nice.

But we have different feature category.

RPC
Startup option
Rest
ZMQ interface
Wallet settings
In the future ( multiprocess interface)

We should document how we remove each.

Documenting for RPC is a good first step.

polespinasa commented at 10:37 am on March 4, 2025:

Left an open issue for it: #31980 I think @jonatack and @fjahr have more to say here than me.

in test/functional/rpc_settxfeerate.py:5 in 426dac6a71 outdated

0@@ -0,0 +1,56 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2014-2022 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test the wallet accounts properly when there are cloned transactions with malleated scriptsigs."""

fjahr commented at 8:47 pm on November 12, 2024:

I don’t think that’s the right description

polespinasa commented at 8:52 pm on November 12, 2024:

copy paste for a template, I will update it

fjahr commented at 8:48 pm on November 12, 2024: contributor

Did you try to follow the approach suggested here: #31088 (comment)? If not, can you say why it didn’t work?

polespinasa commented at 9:24 pm on November 12, 2024: contributor

Did you try to follow the approach suggested here: #31088 (comment)? If not, can you say why it didn’t work?

I would have to look deeper on how to do that. But I don’t see why it would be better than this approach. Deprecating here is easy and there will not be the need to modify code other than deleting the actual function.

in test/functional/rpc_settxfeerate.py:81 in b96cfae733 outdated

43+    	self.log.debug("Test that settxfee limit (0.1 B/kvB) can not be surpassed")
44+    	assert_raises_rpc_error(-8, "txfee cannot be more than wallet max tx fee (0.10000000 BTC/kvB)", node.settxfee, 1)
45+
46+    	self.log.debug("Test that settxfee limit (10000 sat/vB) can not be surpassed")
47+    	assert_raises_rpc_error(-8, "txfee cannot be more than wallet max tx fee (0.10000000 BTC/kvB)", node.settxfeerate, 10001)
48+

ismaelsadeeq commented at 9:51 pm on November 12, 2024:

Should probably add a test for the utility of this, not just edge cases; i.e., set a fee rate, create a transaction, and verify that the fee rate matches.

polespinasa commented at 0:19 am on November 13, 2024:

Good point, added!

in src/wallet/rpc/spend.cpp:424 in 426dac6a71 outdated

422+                "\nWill be deprecated see 'settxfeerate'.\n"
423+                "Set the transaction fee rate in " + CURRENCY_UNIT + "/kvB for this wallet. Overrides the global -paytxfee command line parameter.\n"
424                 "Can be deactivated by passing 0 as the fee. In that case automatic fee selection will be used by default.\n",
425                 {
426-                    {"amount", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "The transaction fee rate in " + CURRENCY_ATOM + "/vB"},
427+                    {"amount", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "The transaction fee rate in " + CURRENCY_UNIT + "/kvB"},

ismaelsadeeq commented at 9:54 pm on November 12, 2024:

Its weird that you change this line to s/vb in previous commit and then revert it back to s/kvB here.

polespinasa commented at 10:51 pm on November 12, 2024:

That was a mistake in the previous commit, as it’s the settxfee function and not settxfeerate.

in src/wallet/rpc/spend.cpp:479 in 642243fab6 outdated

473-            + HelpExampleRpc("settxfee", "1")
474+                    HelpExampleCli("settxfeerate", "1")
475+            + HelpExampleRpc("settxfeerate", "1")
476                 },
477         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
478 {

ismaelsadeeq commented at 9:56 pm on November 12, 2024:

fix-ups should be squashed according to contributing.md guidelines.

ismaelsadeeq commented at 10:04 pm on November 12, 2024: member

Nice fixing the ambiguity.

But generally, I’m Concept ~0 on setting static feerate for wallet transaction as a whole because of the current dynamic fee environment, it’s just a footgun. It will make a transaction either pay too high or too low.

Feerate estimate from fee estimation algorithm should be used in most cases, and when that algorithm is deem inaccurate by users they should pass the fee rate for each individual transaction.

We should lean towards deprecating this RPC instead.

polespinasa force-pushed on Nov 12, 2024

polespinasa commented at 11:31 pm on November 12, 2024: contributor

Thanks for reviewing!

Feerate estimate from fee estimation algorithm should be used in most cases, and when that algorithm is deem inaccurate by users should they should pass the fee rate for each individual transaction.

I agree that the algorithm should be used in most cases, and actually that’s how it’s working according to the RPC call definition. By default it uses the dynamic fee so this is only an extra option to set a static fee in case anyone needs too. https://github.com/bitcoin/bitcoin/blob/b96cfae733a8a8eb79ee894a7cbe903fa955bcec/src/wallet/rpc/spend.cpp#L422

polespinasa force-pushed on Nov 13, 2024

in test/functional/rpc_settxfeerate.py:2 in f77f15da75 outdated

0@@ -1,9 +1,10 @@
1 #!/usr/bin/env python3
2-# Copyright (c) 2014-2022 The Bitcoin Core developers
3+# Copyright (c) 2014-2024 The Bitcoin Core developers

fjahr commented at 0:27 am on November 13, 2024:

This file didn’t exist before 2024, so it should just say 2024

polespinasa force-pushed on Nov 13, 2024

polespinasa commented at 2:52 pm on November 13, 2024: contributor

@fjahr @ismaelsadeeq txfeerate is now deprecated, but I kept it hidden. IMO it makes sense to have a deprecated rpc call hidden as only users who were using it before should know about it and may need info, they can continue using help settxfee

polespinasa requested review from fjahr on Nov 13, 2024

polespinasa requested review from ismaelsadeeq on Nov 13, 2024

polespinasa force-pushed on Nov 13, 2024

glozow added the label Wallet on Nov 18, 2024

polespinasa force-pushed on Nov 22, 2024

polespinasa force-pushed on Nov 26, 2024

polespinasa commented at 10:24 am on November 29, 2024: contributor

I’m not sure why that CI check ,Win64 native, VS2022 (pull_request), fails. Help is appreciated here.

in src/wallet/rpc/spend.cpp:483 in f2ef52d1ae outdated

478+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
479+{
480+
481+    CAmount test = AmountFromValue(request.params[0]);
482+    double conv_ammount = (static_cast<double>(test) / COIN) / 100000;  // Convert to BTC/kvB
483+    CAmount nAmount = AmountFromValue(UniValue(conv_ammount));

maflcko commented at 11:50 am on November 29, 2024:

Parsing it twice and casting to double in-between seems highly fragile. It would be better to directly parse into a fraction.

murchandamus commented at 9:51 pm on January 24, 2025:

Also the variable names here are a bit odd: “test” and “conv_ammount” with two “m”?

polespinasa commented at 8:47 am on January 27, 2025:

Thanks for the reviews! As we are not going to introduce settxfeerate these corrections will not be necessary.

in test/functional/rpc_settxfeerate.py:41 in f2ef52d1ae outdated

36+
37+        self.log.debug("Test that settxfee set the feerate in B/kvB")
38+        assert_equal(node.settxfee(0.001), True)
39+        assert_equal(str(node.getwalletinfo()['paytxfee']), "0.00100000")
40+
41+        self.log.debug("Test that settxfeerate set the feerate in v/vB")

Prabhat1308 commented at 9:39 am on December 3, 2024:

0        self.log.debug("Test that settxfeerate set the feerate in sat/vB")

polespinasa commented at 1:15 pm on December 3, 2024:

Thanks!

luke-jr commented at 7:32 am on December 6, 2024: member

See also #20391 for previous work in this area

in src/wallet/rpc/spend.cpp:441 in f2ef52d1ae outdated

436@@ -437,6 +437,10 @@ RPCHelpMan settxfee()
437 
438     LOCK(pwallet->cs_wallet);
439 
440+    if (!pwallet->chain().rpcEnableDeprecated("settxfee")) {
441+        throw JSONRPCError(RPC_METHOD_DEPRECATED, "Using settxfee is deprecated. Use settxfeerate instead or restart bitcoind with -deprecatedrpc=settxfee. This functionality will be removed in x.xx");

ismaelsadeeq commented at 8:04 pm on January 23, 2025:

I think just saying it is deprecated is enough, because you are giving half baked information by not specifying when it will be removed!

fjahr commented at 8:42 pm on January 23, 2025:

How is that different from just saying it is deprecated without giving information when it will be removed? Either way the information is half-baked.

polespinasa commented at 5:58 pm on January 24, 2025:

Shall we maybe agree on when it can be deprecated? :)

ismaelsadeeq commented at 7:57 pm on January 24, 2025:

@polespinasa

Resurfacing an IRC discussion about this.

11:39 < abubakarsadiq> I’ve searched but did not see guidelines on how long will a deprecated feature (e.g) RPC will take for it to be removed! I’ve seen an example from d4cdbd6fb6ac3663d069307c4fd0078f4ecf0245 that the RPC will be removed in the next release. Is that the case? if so @tdb3 maybe document that as well in #30142 11:39 <@gribble> #30142 | doc: add guidance for RPC to developer notes by tdb3 · Pull Request #30142 · bitcoin/bitcoin · GitHub 11:40 -!- saturday7 [~saturday7@59.167.129.22] has quit [Ping timeout: 248 seconds] 11:44 < sipa> abubakarsadiq: generally, one major release (so marked deprecated in version N.0, removed in (N+1).0) 11:44 < sipa> but of course, because it requires actual work (a PR being written and reviewed) during the N+1 release cycle, that may not happen, either because it’s forgotten, or because people worry that too many users rely on it still 11:47 < abubakarsadiq> Thanks so it’s a case by case basis, I see the reason why having an estimate maybe an issue

I think you can say that it is deprecated in 29.0 and will be removed in 30.0

ismaelsadeeq commented at 8:10 pm on January 23, 2025: member

@murchandamus pinging you here , since this PR originates from our discussion here #29278 (review).

See my comment above #31278#pullrequestreview-2430770541.

I think it would be better to simply deprecatesettxfee and not introduce settxfeerate. In future releases, we should even consider removing support for settxfee entirely and solely relying on fee rate estimates from a fee rate forecaster.

in src/wallet/rpc/spend.cpp:500 in f2ef52d1ae outdated

495+    } else if (tx_fee_rate < pwallet->chain().relayMinFee()) {
496+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than min relay tx fee (%s)", pwallet->chain().relayMinFee().ToString(FeeEstimateMode::SAT_VB)));
497+    } else if (tx_fee_rate < pwallet->m_min_fee) {
498+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than wallet min fee (%s)", pwallet->m_min_fee.ToString(FeeEstimateMode::SAT_VB)));
499+    } else if (tx_fee_rate > max_tx_fee_rate) {
500+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be more than wallet max tx fee (%s)", max_tx_fee_rate.ToString(FeeEstimateMode::SAT_VB)));

murchandamus commented at 9:52 pm on January 24, 2025:

Let’s distinguish between fees and feerates: instead of “txfee” these errors should probably say “tx fee rate”

polespinasa commented at 8:55 am on January 27, 2025:

Thx for the review @murchandamus. This keyword (txfee) is also used in settxfee function. Do you think it should be changed there also? https://github.com/bitcoin/bitcoin/blob/0a931a9787b196d7a620863cc143d9319ffd356d/src/wallet/rpc/spend.cpp#L445-L451

murchandamus commented at 9:07 pm on February 5, 2025:

Unfortunately, there are feerates titled “…fee” all over the codebase. If you are touching the prospective lines already, you could fix the terminology, but if we are going to remove settxfee anyway, it’s not necessary to change it first and then remove it.

murchandamus commented at 9:54 pm on January 24, 2025: contributor

I concur with @ismaelsadeeq, do we think that anyone is still using settxfee? Should we even introduce settxfeerate?

polespinasa commented at 9:05 am on January 27, 2025: contributor

I concur with @ismaelsadeeq, do we think that anyone is still using settxfee? Should we even introduce settxfeerate?

I agree and it makes sense not to introduce settxfeerate dynamic fee environment make it obsolete.

Although core estimations are not really accurate right now to rely on them. I think @ismaelsadeeq proposed an improvement on Delving Bitcoin.

polespinasa force-pushed on Jan 27, 2025

polespinasa renamed this:
~~wallet, rpc: Settxfeerate~~
wallet, rpc: deprecate settxfee
on Jan 27, 2025

in test/functional/wallet_create_tx.py:28 in 5ea315d181 outdated

22@@ -23,6 +23,9 @@ def add_options(self, parser):
23     def set_test_params(self):
24         self.setup_clean_chain = True
25         self.num_nodes = 1
26+        self.extra_args = [[
27+            "-deprecatedrpc=settxfee"
28+        ] for i in range(self.num_nodes)]

fjahr commented at 10:31 am on January 27, 2025:

nit: There is just one node here, so no need for such a loop

in src/wallet/rpc/spend.cpp:421 in 5ea315d181 outdated

417@@ -418,7 +418,7 @@ RPCHelpMan sendmany()
418 RPCHelpMan settxfee()
419 {
420     return RPCHelpMan{"settxfee",
421-                "\nSet the transaction fee rate in " + CURRENCY_UNIT + "/kvB for this wallet. Overrides the global -paytxfee command line parameter.\n"
422+                "\n(DEPRECATED)Set the transaction fee rate in " + CURRENCY_UNIT + "/kvB for this wallet. Overrides the global -paytxfee command line parameter.\n"

fjahr commented at 10:31 am on January 27, 2025:

nit

0                "\n(DEPRECATED) Set the transaction fee rate in " + CURRENCY_UNIT + "/kvB for this wallet. Overrides the global -paytxfee command line parameter.\n"

fjahr commented at 10:32 am on January 27, 2025: contributor

You should squash the two commits so that settxfeerate is never created in the first place. That will make review much easier.

polespinasa force-pushed on Jan 27, 2025

polespinasa requested review from ismaelsadeeq on Jan 27, 2025

polespinasa requested review from fjahr on Jan 27, 2025

polespinasa requested review from murchandamus on Jan 27, 2025

polespinasa requested review from maflcko on Jan 27, 2025

in src/wallet/rpc/spend.cpp:442 in a0ad6ec5d7 outdated

436@@ -437,6 +437,11 @@ RPCHelpMan settxfee()
437 
438     LOCK(pwallet->cs_wallet);
439 
440+    if (!pwallet->chain().rpcEnableDeprecated("settxfee")) {
441+        throw JSONRPCError(RPC_METHOD_DEPRECATED, "settxfee is deprecated and will be fully removed in v30.0."
442+        "\n To use settxfee in v29.0, restart bitcoind with -deprecatedrpc=settxfee.");

fjahr commented at 12:57 pm on January 27, 2025:

nit: Space at the beginning of the line leads to a slight misalignment. Referencing the current version also isn’t needed and may be confusing if this ends up in any other version. This probably won’t be backported but if it was, this could also be running in v28.2 for example. But not super critical I guess.

0        "\nTo use settxfee restart bitcoind with -deprecatedrpc=settxfee.");

polespinasa commented at 1:34 pm on January 27, 2025:

It can be deleted yes. I added it as in the example mentioned before is used: https://github.com/bitcoin/bitcoin/commit/d4cdbd6fb6ac3663d069307c4fd0078f4ecf0245#diff-ccc24453c13307f815879738d3bf00eec351417537fbf10dde1468180cacd2f1R794

fjahr commented at 1:18 pm on January 27, 2025: contributor

tACK a0ad6ec5d7b3e0cd337c03a5c8a27b87b928d1c1

FYI, it’s common to use prefixes for the modules touched in the commit too, so you could add rpc: or wallet, rpc: to the commit title if you retouch.

polespinasa force-pushed on Jan 27, 2025

fjahr commented at 1:49 pm on January 27, 2025: contributor

reACK b9060de308fd2691cd8ac82821fe2cd5a20cd948

ismaelsadeeq commented at 4:52 pm on January 27, 2025: member

utACK b9060de308fd2691cd8ac82821fe2cd5a20cd948

ismaelsadeeq commented at 4:55 pm on January 27, 2025: member

@polespinasa nit: Update the PR description to reflect the recent changes

maflcko commented at 7:07 pm on February 5, 2025: member

I don’t understand the changes here. Instead of repeating what the changes are in the pull description, it would be good to explain why they are done and why the downsides are acceptable.

I guess this deprecates an RPC without replacement, without release notes, without an explanation or long-term plan?

If yes, then I tend toward nack.

edit: Ok, I now see the reason hidden in a review comment somewhere. Still, my nack holds and the reason should be clearly communicated to reviewers and documented for users.

maflcko added the label Needs release note on Feb 5, 2025

polespinasa force-pushed on Feb 5, 2025

polespinasa commented at 8:45 pm on February 5, 2025: contributor

the reason should be clearly communicated to reviewers and documented for users.

Thanks for the comments. Updated the PR description with all the changes and idea proposed during the revisions. Also added a release note.

polespinasa force-pushed on Feb 5, 2025

DrahtBot added the label CI failed on Feb 5, 2025

DrahtBot commented at 8:47 pm on February 5, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36746930999

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

in doc/release-notes-31278.md:5 in 113f0b4df2 outdated

0@@ -0,0 +1,5 @@
1+Updated RPCs
2+---
3+- The `settxfee` RPC is now **deprecated** and will be removed in Bitcoin Core 30.0. This RPC allowed users to set a static fee rate for wallet transactions, but in a dynamic fee environment, this can lead to overpaying or underpaying. Users should instead rely on fee estimation or specify a fee rate per transaction. (#31278)
4+
5+As a reminder, manual fee rate selection should be done per transaction rather than globally. Fee estimation provides a more adaptive approach, and users can override it on a per-transaction basis when necessary.

murchandamus commented at 9:00 pm on February 5, 2025:

The last line here mostly repeats the previous sentence. Perhaps this could be deduplicated.

in test/functional/wallet_multiwallet.py:49 in 113f0b4df2 outdated

45@@ -46,7 +46,7 @@ def set_test_params(self):
46         self.setup_clean_chain = True
47         self.num_nodes = 2
48         self.rpc_timeout = 120
49-        self.extra_args = [["-nowallet"], []]
50+        self.extra_args = [["-nowallet", "-deprecatedrpc=settxfee"], ["-deprecatedrpc=settxfee"]]

murchandamus commented at 9:10 pm on February 5, 2025:

Why does deprecatedrpc=settxfee need to be specified twice here? Especially on a node without a wallet, the setting doesn’t seem to make sense.

polespinasa commented at 9:24 pm on February 5, 2025:

Actually the -nowallet one needs it!! But the second one does not! Thanks for pointing this!

murchandamus commented at 9:11 pm on February 5, 2025: contributor

Concept ACK

polespinasa force-pushed on Feb 5, 2025

DrahtBot removed the label CI failed on Feb 5, 2025

maflcko removed the label Needs release note on Feb 6, 2025

maflcko commented at 7:55 am on February 6, 2025: member

Thinking more about the motivation, I agree with it, but I fail to see the connection to removing this RPC:

The RPC doesn’t rule out dynamic fee estimation. In fact, I expect some users not using Bitcoin Core’s fee estimation, but an external one. This means that the fee estimates will need to be fed to Bitcoin Core. One way to do it is via the global RPC, another one is via the per-transaction RPC(s).
If a user wants to shoot themselves into the foot by setting the feerate to a constant, they can also do so via the per-transaction RPCs.

So it seems like this is closing a valid use case without reducing risks or footguns. Am I missing something obvious? If not, I still tend toward nack.

polespinasa commented at 9:37 am on February 6, 2025: contributor

I expect some users not using Bitcoin Core’s fee estimation, but an external one. This means that the fee estimates will need to be fed to Bitcoin Core. One way to do it is via the global RPC, another one is via the per-transaction RPC(s).

If a user relies on an external fee estimation service, setting a static wallet-wide fee rate makes little sense. Fees are dynamic, and the best practice is to set them individually per transaction rather than globally regardless of what you use to estimate them.

If a user wants to shoot themselves into the foot by setting the feerate to a constant, they can also do so via the per-transaction RPCs.

While users can still set a static fee manually via per-transaction RPCs, removing settxfee forces them to make that choice explicitly for each transaction, rather than applying it across all transactions by default.

This helps prevent users from accidentally sticking with an outdated fee rate, which could lead to overpayment or underpayment, increasing the risk of delayed confirmations, or the need for RBF (which harms privacy).

So it seems like this is closing a valid use case without reducing risks or footguns.

If a user’s external software forces a static fee rate, the risk remains, but it is now a conscious decision outside of Bitcoin Core, rather than a built-in risk within the wallet itself. By removing this option at the wallet level, we reduce the likelihood of users making costly mistakes directly in Core.

Edit: Extra points are that the settxfee RPC uses BTC/kvB, which is not widely recognized in modern Bitcoin usage. While per-transaction RPCs (sendtoaddress, sendmany) already use sat/vB, which is the industry-standard unit for fee rates. Keeping an RPC that uses an outdated and confusing unit can lead to misinterpretation and mistakes when setting fee rates.

maflcko commented at 10:02 am on February 6, 2025: member

I expect some users not using Bitcoin Core’s fee estimation, but an external one. This means that the fee estimates will need to be fed to Bitcoin Core. One way to do it is via the global RPC, another one is via the per-transaction RPC(s).

If a user relies on an external fee estimation service, setting a static wallet-wide fee rate makes little sense. Fees are dynamic

I understand that fees are dynamic. But it seems reasonable to (let’s say) call settxfee every minute to update the fee estimate from the external dynamic estimator. If the fee estimator only updates the estimates every 10 minutes, forcing users to call it for every transaction (or alternatively forcing them to implement settxfee logic in their own application) seems just busy-work.

fanquake commented at 8:22 pm on February 20, 2025: member

@achow101 can you weigh in here? The changes seem to indicate that deprecation was targetted for 29.x, for removal in 30.x, but it doesn’t seem like this is going into 29.x.

maflcko commented at 8:27 pm on February 20, 2025: member

If people think that deprecation of settxfee is appropriate, then I fail to see why -paytxfee isn’t deprecated at the same time. If anything, any reason for deprecation should be even stronger for that setting, given that it isn’t dynamic and thus can’t be used with dynamic fee estimation at all, and even less without the rpc.

in doc/release-notes-31278.md:3 in ba2fda226e outdated

0@@ -0,0 +1,3 @@
1+Updated RPCs
2+---
3+- The `settxfee` RPC is now **deprecated** and will be removed in Bitcoin Core 30.0. This RPC allowed users to set a static fee rate for wallet transactions, but in a dynamic fee environment, this can lead to overpaying or underpaying. Users should instead rely on fee estimation or specify a fee rate per transaction. (#31278)

glozow commented at 8:27 pm on February 20, 2025:

RPC examples for “specify a feerate per transaction” would be helpful

glozow commented at 8:28 pm on February 20, 2025: member

concept ACK, I wish I saw this earlier

achow101 commented at 8:30 pm on February 20, 2025: member

Concept ACK

The startup option should also be deprecated.

This won’t make 29.0, but could for 30.0.

glozow added this to the milestone 30.0 on Feb 20, 2025

polespinasa commented at 10:04 pm on February 20, 2025: contributor

Thanks for reviewing. I agree, startup option should be also deprecated. Will work on that.

polespinasa requested review from glozow on Feb 27, 2025

polespinasa requested review from fjahr on Feb 27, 2025

polespinasa requested review from ismaelsadeeq on Feb 27, 2025

in test/functional/test_framework/test_framework.py:593 in bd5280ee1e outdated

589@@ -590,9 +590,9 @@ def stop_nodes(self, wait=0):
590             # Wait for nodes to stop
591             node.wait_until_stopped()
592 
593-    def restart_node(self, i, extra_args=None, clear_addrman=False):
594+    def restart_node(self, i, extra_args=None, clear_addrman=False, expected_stderr=''):

maflcko commented at 11:50 am on February 27, 2025:

0    def restart_node(self, i, extra_args=None, clear_addrman=False, *, expected_stderr=''):

could force named args for new named args?

maflcko commented at 11:50 am on February 27, 2025: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

polespinasa force-pushed on Feb 27, 2025

polespinasa renamed this:
~~wallet, rpc: deprecate settxfee~~
wallet, rpc: deprecate settxfee and paytxfee
on Feb 27, 2025

in doc/release-notes-31278.md:3 in 61215aa1ac outdated

0@@ -0,0 +1,7 @@
1+Updated RPCs
2+---
3+- The `settxfee` RPC is now **deprecated** and will be removed in Bitcoin Core 31.0. This RPC allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can 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 like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, and `send_many`. (#31278)

murchandamus commented at 7:15 pm on February 27, 2025:

The RPC is “sendmany” without the underscore.

0- The `settxfee` RPC is now **deprecated** and will be removed in Bitcoin Core 31.0. This RPC allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can 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 like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, and `sendmany`. (#31278)

polespinasa commented at 2:37 pm on February 28, 2025:

You’re right, muscle memory 😅 Thanks!

in doc/release-notes-31278.md:7 in 61215aa1ac outdated

0@@ -0,0 +1,7 @@
1+Updated RPCs
2+---
3+- The `settxfee` RPC is now **deprecated** and will be removed in Bitcoin Core 31.0. This RPC allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can 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 like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, and `send_many`. (#31278)
4+
5+Updated settings
6+------
7+- The `-paytxfee` setting is now **deprecated** and will be removed in Bitcoin Core 31.0. This option set a static fee rate for all wallet transactions when starting the Bitcoin node, which is discouraged in a dynamic fee environment as it can 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 like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, and `send_many`. (#31278)

murchandamus commented at 7:16 pm on February 27, 2025:

Same:

0- The `-paytxfee` setting is now **deprecated** and will be removed in Bitcoin Core 31.0. This option set a static fee rate for all wallet transactions when starting the Bitcoin node, which is discouraged in a dynamic fee environment as it can 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 like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, and `sendmany`. (#31278)

polespinasa force-pushed on Feb 28, 2025

polespinasa requested review from murchandamus on Feb 28, 2025

in doc/release-notes-31278.md:7 in ee080ce689 outdated

0@@ -0,0 +1,7 @@
1+Updated RPCs
2+---
3+- The `settxfee` RPC is now **deprecated** and will be removed in Bitcoin Core 31.0. This RPC allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can 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 like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, and `sendmany`. (#31278)
4+
5+Updated settings
6+------
7+- The `-paytxfee` setting is now **deprecated** and will be removed in Bitcoin Core 31.0. This option set a static fee rate for all wallet transactions when starting the Bitcoin node, which is discouraged in a dynamic fee environment as it can 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 like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, and `sendmany`. (#31278)

ismaelsadeeq commented at 1:50 pm on March 3, 2025:

In “wallet,rpc: paytxfee and settxfee is deprecated and hidden from help” ee080ce689bfc8b74438a02e1c62bbe644893ffa

I think you can consolidate this to be dry

0
1RPC and Startup Option
2---
3The -paytxfee startup option and the settxfee RPC are now deprecated and will be removed in Bitcoin Core 31.0. They allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can 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)

in src/wallet/init.cpp:66 in ee080ce689 outdated

65@@ -66,7 +66,7 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
66         CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);

ismaelsadeeq commented at 1:54 pm on March 3, 2025:

In “wallet,rpc: paytxfee and settxfee is deprecated and hidden from help” ee080ce689bfc8b74438a02e1c62bbe644893ffa

The startup option is not hidden from startup help, IIUC it is not deprecated we just throw a warning when loading a wallet.

polespinasa commented at 7:15 pm on March 3, 2025:

I think this is outdated as we agreed to not hide anything?

in doc/release-notes-31278.md:1 in ee080ce689 outdated

0@@ -0,0 +1,7 @@
1+Updated RPCs

ismaelsadeeq commented at 1:57 pm on March 3, 2025:

In “wallet,rpc: paytxfee and settxfee is deprecated and hidden from help” ee080ce689bfc8b74438a02e1c62bbe644893ffa

Note: when removing paytxfee in the future we should also update txconfirmtarget startup option help text

ismaelsadeeq commented at 2:17 pm on March 3, 2025:

In “wallet,rpc: paytxfee and settxfee is deprecated and hidden from help” ee080ce689bfc8b74438a02e1c62bbe644893ffa

nit: I think release notes is usually on it’s own commit

polespinasa commented at 7:14 pm on March 3, 2025:

added to the PR description to have that in mind

ismaelsadeeq commented at 7:58 pm on March 4, 2025:

Can also leave unresolved

in src/wallet/wallet.cpp:3185 in ee080ce689 outdated

3160@@ -3161,6 +3161,8 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
3161     }
3162 
3163     if (args.IsArgSet("-paytxfee")) {
3164+        warnings.push_back(_("-paytxfee is deprecated and will be fully removed in v31.0."));
3165+

ismaelsadeeq commented at 2:07 pm on March 3, 2025:

In “wallet,rpc: paytxfee and settxfee is deprecated and hidden from help” ee080ce689bfc8b74438a02e1c62bbe644893ffa

Shouldn’t this be a failure instead of a warning since an error is being thrown in the RPC? I think we should be consistent and fail as well.

The user should explicitly provide something like deprecatedstartupoption=paytxfee. Do we support that?

polespinasa commented at 7:30 pm on March 3, 2025:

I don’t think we have something like deprecatedstartupoption. I did it with a warn trying to follow the way it’s done with Legacy wallets. https://github.com/bitcoin/bitcoin/blob/79bbb381a1fd13ad456fde736b3c195a791d4e58/src/wallet/wallet.cpp#L484-L487

ismaelsadeeq commented at 2:18 pm on March 3, 2025: member

Looking good @polespinasa I’ve reviewed the code and tested the RPC’s and the startup option Just a couple of comments

in doc/release-notes-31278.md:1 in 477e5adc12 outdated

0@@ -1,7 +1,4 @@
1-Updated RPCs
2----
3-- The `settxfee` RPC is now **deprecated** and will be removed in Bitcoin Core 31.0. This RPC allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can 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 like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, and `sendmany`. (#31278)
4 
5-Updated settings
6-------
7-- The `-paytxfee` setting is now **deprecated** and will be removed in Bitcoin Core 31.0. This option set a static fee rate for all wallet transactions when starting the Bitcoin node, which is discouraged in a dynamic fee environment as it can 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 like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, and `sendmany`. (#31278)
8\ No newline at end of file
9+RPC and Startup Option

fjahr commented at 7:05 pm on March 3, 2025:

You should remove the old version of the release notes from the first commit too.

polespinasa commented at 7:14 pm on March 3, 2025:

Sorry, what do you mean? There’s only one commit.

fjahr commented at 7:18 pm on March 3, 2025:

Squashing is also fine! I just thought you intended to keep them in a separate commit like suggested by @ismaelsadeeq here: #31278 (review) when you pushed two commits

polespinasa commented at 7:41 pm on March 3, 2025:

Done :)

polespinasa force-pushed on Mar 3, 2025

in doc/release-notes-31278.md:3 in 05254eb4e7 outdated

0@@ -0,0 +1,3 @@
1+RPC and Startup Option
2+---
3+The -paytxfee startup option and the settxfee RPC are now deprecated and will be removed in Bitcoin Core 31.0. They allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can 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)

fjahr commented at 7:22 pm on March 3, 2025:

formatting nit (no need to fix unless you repush, can be done in editing of the release notes in the wiki)

0The `-paytxfee` startup option and the `settxfee` RPC are now deprecated and will be removed in Bitcoin Core 31.0. They allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can 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)

jonatack commented at 8:06 pm on March 3, 2025:

also s/fee_rate/“fee_rate”/

“discouraged in a dynamic fee environment” -> this leads to the question whether the fee environment is more “dynamic” than before, e.g. when this RPC was introduced, to justify removal. Would be shorter and possibly preferable to write: “They allowed the user to set a static fee rate for wallet transactions, which could potentially lead to overpaying or underpaying.”

in test/functional/wallet_create_tx.py:75 in 05254eb4e7 outdated

71@@ -71,7 +72,7 @@ def test_tx_size_too_large(self):
72             )
73 
74         self.log.info('Check maxtxfee in combination with settxfee')
75-        self.restart_node(0)
76+        self.restart_node(0, expected_stderr='Warning: -paytxfee is deprecated and will be fully removed in v31.0.')

fjahr commented at 7:33 pm on March 3, 2025:

Nit: I would have liked it a bit more if you had added that where -paytxfee= is tested directly (above) or here it could use a small comment that this doesn’t have anything to do with the check mention in the info log. But since this is temporary it’s fine to keep it as is.

polespinasa force-pushed on Mar 3, 2025

in src/wallet/rpc/spend.cpp:441 in c68f6f4303 outdated

436@@ -437,6 +437,11 @@ RPCHelpMan settxfee()
437 
438     LOCK(pwallet->cs_wallet);
439 
440+    if (!pwallet->chain().rpcEnableDeprecated("settxfee")) {
441+        throw JSONRPCError(RPC_METHOD_DEPRECATED, "settxfee is deprecated and will be fully removed in v31.0."

fjahr commented at 7:50 pm on March 3, 2025:

Please add a test for this in test/functional/rpc_deprecated.py

fjahr commented at 9:07 pm on March 3, 2025:

Note that I think it’s enough to test the deprecation error there, see #31977

fjahr commented at 7:51 pm on March 3, 2025: contributor

Looks good to me aside from the missing functional deprecation test.

polespinasa force-pushed on Mar 4, 2025

DrahtBot added the label CI failed on Mar 4, 2025

fjahr commented at 11:55 am on March 4, 2025: contributor

utACK f4344220d7195324f921dcf001c1a117008477fd

DrahtBot requested review from ismaelsadeeq on Mar 4, 2025

DrahtBot requested review from achow101 on Mar 4, 2025

DrahtBot removed the label CI failed on Mar 4, 2025

in test/functional/rpc_deprecated.py:5 in f4344220d7 outdated

3@@ -4,12 +4,19 @@
4 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
5 """Test deprecation of RPC calls."""

ismaelsadeeq commented at 7:37 pm on March 4, 2025:

nit: Copyright update

polespinasa commented at 8:15 pm on March 4, 2025:

you mean the year?

ismaelsadeeq commented at 11:57 am on March 12, 2025:

yes

ismaelsadeeq commented at 7:57 pm on March 4, 2025: member

Code review ACK f4344220d7195324f921dcf001c1a117008477fd

Nit:

No immediate breakage in 30.0, but settxfee and paytxfee will be removed in 30.0.

No immediate breakage in 30.0, but settxfee and paytxfee will be removed in 31.0.

The release usually come as the last commit.

polespinasa force-pushed on Mar 12, 2025

ismaelsadeeq commented at 1:42 pm on March 13, 2025: member

re-ACK 8ceaecc89e20129f9c11727e4c7795633cfcdd2e 08477fd..8ceaec

DrahtBot requested review from fjahr on Mar 13, 2025

fjahr commented at 7:57 pm on March 14, 2025: contributor

re-ACK 8ceaecc89e20129f9c11727e4c7795633cfcdd2e

DrahtBot added the label Needs rebase on Mar 16, 2025

wallet, rpc: deprecate settxfee and paytxfee bf194c920c

Release notes 2f2ab47bf7

polespinasa force-pushed on Mar 16, 2025

polespinasa commented at 10:21 am on March 16, 2025: contributor

Code rebased to avoid conflicts with https://github.com/bitcoin/bitcoin/pull/31977

fjahr commented at 11:27 am on March 16, 2025: contributor

re-ACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579

DrahtBot requested review from ismaelsadeeq on Mar 16, 2025

DrahtBot removed the label Needs rebase on Mar 16, 2025

ismaelsadeeq commented at 1:39 pm on March 17, 2025: member

re-ACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579

in test/functional/rpc_deprecated.py:26 in 2f2ab47bf7

30@@ -23,7 +31,8 @@ def run_test(self):
31         # at least one other functional test that still tests the RPCs
32         # functionality using the respective -deprecatedrpc flag.
33 
34-        self.log.info("Currently no tests for deprecated RPC methods")

rkrux commented at 2:33 pm on March 17, 2025:

Having reviewed this PR #31977 earlier, I feel ideally this log should not be removed but commented at this stage and the above comment should also generally mention to uncomment this log later when the RPCs tested below are fully removed from the codebase. But we can get this done in a separate PR now after this one is merged.

polespinasa commented at 2:41 pm on March 17, 2025:

I agree. If it is not done in a separate PR we can simply add it back into the PR where we remove the RPC call. We will have to leave the test to its default form again anyway. I will add it into the notes for removal.

rkrux commented at 2:34 pm on March 17, 2025: contributor

Concept and utACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579

I am in favour of having both a consistent fee rate unit to specify across RPCs (sat/vB over BTC/kvB), and relying on fee estimation most of the times while creating the transaction that the user can override by setting the fee_rate argument in the transaction creation RPCs (including PSBT ones), which is expressed in sat/vB.

As a user, deprecating this RPC and startup option streamlines the overall flows for me by not having to navigate through multiple arguments & defaults while making it easier for me to gauge the fee rates used in the transaction creation.

ryanofsky assigned ryanofsky on Mar 24, 2025

ryanofsky merged this on Mar 24, 2025

ryanofsky closed this on Mar 24, 2025

wallet, rpc: deprecate settxfee and paytxfee #31278

Code Coverage & Benchmarks

Reviews

Conflicts