wallet, rpc: deprecate settxfee #31278

pull polespinasa wants to merge 1 commits into bitcoin:master from polespinasa:settxfeerate changing 9 files +20 −5
  1. polespinasa commented at 2:19 pm on November 12, 2024: none

    Summary

    This PR deprecates the settxfee RPC, marking it for removal in Bitcoin Core 30.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.

    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 entirely.

    Key Changes

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

    Impact on Users

    If users currently use settxfee, they should transition to specifying fees per transaction. No immediate breakage in 29.0, but settxfee will be removed in 30.0.

    Alternative Approaches Considered

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

  2. 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
    Concept ACK murchandamus, glozow, achow101
    Stale ACK fjahr, ismaelsadeeq

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

    Conflicts

    No conflicts as of last run.

  3. polespinasa commented at 2:24 pm on November 12, 2024: none

    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.

  4. polespinasa renamed this:
    Wallet, RPC: Settxfeerate
    wallet, rpc: Settxfeerate
    on Nov 12, 2024
  5. polespinasa commented at 2:36 pm on November 12, 2024: none

    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.

  6. DrahtBot added the label CI failed on Nov 12, 2024
  7. 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.

  8. polespinasa force-pushed on Nov 12, 2024
  9. polespinasa force-pushed on Nov 12, 2024
  10. polespinasa force-pushed on Nov 12, 2024
  11. DrahtBot removed the label CI failed on Nov 12, 2024
  12. polespinasa force-pushed on Nov 12, 2024
  13. 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
  14. 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
  15. 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
  16. 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:

    polespinasa commented at 2:04 pm on November 13, 2024:
    Thanks! Applied following some of the examples.
  17. 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
  18. 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?
  19. polespinasa commented at 9:24 pm on November 12, 2024: none

    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.

  20. 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!
  21. 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.
  22. 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.
  23. 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.

  24. polespinasa force-pushed on Nov 12, 2024
  25. polespinasa commented at 11:31 pm on November 12, 2024: none

    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

  26. polespinasa force-pushed on Nov 13, 2024
  27. 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
  28. polespinasa force-pushed on Nov 13, 2024
  29. polespinasa commented at 2:52 pm on November 13, 2024: none
    @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
  30. polespinasa requested review from fjahr on Nov 13, 2024
  31. polespinasa requested review from ismaelsadeeq on Nov 13, 2024
  32. polespinasa force-pushed on Nov 13, 2024
  33. glozow added the label Wallet on Nov 18, 2024
  34. polespinasa force-pushed on Nov 22, 2024
  35. polespinasa force-pushed on Nov 26, 2024
  36. polespinasa commented at 10:24 am on November 29, 2024: none
    I’m not sure why that CI check ,Win64 native, VS2022 (pull_request), fails. Help is appreciated here.
  37. 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.
  38. 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!
  39. luke-jr commented at 7:32 am on December 6, 2024: member
    See also #20391 for previous work in this area
  40. 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

  41. 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.

  42. 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.
  43. 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?
  44. polespinasa commented at 9:05 am on January 27, 2025: none

    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.

  45. polespinasa force-pushed on Jan 27, 2025
  46. polespinasa renamed this:
    wallet, rpc: Settxfeerate
    wallet, rpc: deprecate settxfee
    on Jan 27, 2025
  47. 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
  48. 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"
    
  49. 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.
  50. polespinasa force-pushed on Jan 27, 2025
  51. polespinasa force-pushed on Jan 27, 2025
  52. polespinasa requested review from ismaelsadeeq on Jan 27, 2025
  53. polespinasa requested review from fjahr on Jan 27, 2025
  54. polespinasa requested review from murchandamus on Jan 27, 2025
  55. polespinasa requested review from maflcko on Jan 27, 2025
  56. 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:
  57. 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.

  58. polespinasa force-pushed on Jan 27, 2025
  59. fjahr commented at 1:49 pm on January 27, 2025: contributor
    reACK b9060de308fd2691cd8ac82821fe2cd5a20cd948
  60. ismaelsadeeq commented at 4:52 pm on January 27, 2025: member
    utACK b9060de308fd2691cd8ac82821fe2cd5a20cd948
  61. ismaelsadeeq commented at 4:55 pm on January 27, 2025: member
    @polespinasa nit: Update the PR description to reflect the recent changes
  62. 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.

  63. maflcko added the label Needs release note on Feb 5, 2025
  64. polespinasa force-pushed on Feb 5, 2025
  65. polespinasa commented at 8:45 pm on February 5, 2025: none

    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.

  66. polespinasa force-pushed on Feb 5, 2025
  67. DrahtBot added the label CI failed on Feb 5, 2025
  68. 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.

  69. 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.
  70. 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!
  71. murchandamus commented at 9:11 pm on February 5, 2025: contributor
    Concept ACK
  72. wallet,rpc: settxfee is deprecated and hidden from help ba2fda226e
  73. polespinasa force-pushed on Feb 5, 2025
  74. DrahtBot removed the label CI failed on Feb 5, 2025
  75. maflcko removed the label Needs release note on Feb 6, 2025
  76. 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.

  77. polespinasa commented at 9:37 am on February 6, 2025: none
    • 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.

  78. 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.

  79. 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.
  80. 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.
  81. in doc/release-notes-31278.md:3 in ba2fda226e
    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
  82. glozow commented at 8:28 pm on February 20, 2025: member
    concept ACK, I wish I saw this earlier
  83. 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.

  84. glozow added this to the milestone 30.0 on Feb 20, 2025
  85. polespinasa commented at 10:04 pm on February 20, 2025: none
    Thanks for reviewing. I agree, startup option should be also deprecated. Will work on that.

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-02-22 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me