wallet, rpc: Settxfeerate

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

This PR aims to solve #31088

New RPC call settxfeerate is added. It does the same as the actual settxfee but takes the feerate in sat/vB instead of B/kvB.

settxfee still available but hidden so we avoid incompatibility problems with applications that use that RPC call. When deprecation is desired, the process should be as simple as just delete settxfee RPC call function and correct the tests where it is called.

Some simple tests are added to check that all changes work correctly. This has also been tested manually on regtest and the behavior is the expected one.

Update: settxfee is deprecated

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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

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

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

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.

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:1160 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.

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

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

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

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

settxfeerate sets tx feerate as sat/vB unit - settxfee is hidden from help - settxfee deprecated - tests added f2ef52d1ae

polespinasa force-pushed on Nov 26, 2024

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.

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.

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!

typo: Update test/functional/rpc_settxfeerate.py

Co-authored-by: Probot <94048855+Prabhat1308@users.noreply.github.com>

ddcef3f18d

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

See also #20391 for previous work in this area

wallet, rpc: Settxfeerate #31278

Code Coverage & Benchmarks

Reviews

Conflicts