wallet: rpc: `settxfee` sets the wallet feerate not fee

ismaelsadeeq commented at 9:03 pm on October 14, 2024: member

The wallet RPC settxfee sets the fee rate for a wallet.

Current help text:

0Set the transaction fee rate in BTC/kvB for this wallet. Overrides the global -paytxfee command line parameter.
1Can be deactivated by passing 0 as the fee. In that case automatic fee selection will be used by default.
2
3Arguments:
41. amount    (numeric or string, required) The transaction fee rate in BTC/kvB
5
6Result:
7true|false    (boolean) Returns true if successful

This is a misnomer, as stated here #29278 (review) so should instead be setfeerate @jonatack suggested a safer approach to avoid breaking things

(see: #20484 (comment)). I think this is better approach than just renaming the settxfee RPC to setfeerate?

Add setfeerate RPC which is a mirror of settxfee but in sat/vB.
Keep settxfee hidden, but prefer the setfeerate RPC in future use.
Eventually deprecate settxfee.

This issue is limited to fixing the ambiguity in settxfee.

maflcko commented at 9:38 am on October 15, 2024: member

In theory, it would be possible to register RPC argument name aliases, IIRC. They are denoted and split by |.

maflcko added the label Wallet on Oct 15, 2024

maflcko added the label RPC/REST/ZMQ on Oct 15, 2024

maflcko commented at 11:51 am on November 29, 2024: member

On a greater scope, I wonder if it is a good approach to one-by-one deprecate one unit and add another.

If the goal is to only support one unit, it would be better to adjust all places to return that unit, so that it can be used in round-trips. Otherwise, users will have to convert values they receive from the RPC to pass them to another RPC of the same program.

polespinasa commented at 11:31 am on March 18, 2025: contributor

If the goal is to only support one unit, it would be better to adjust all places to return that unit, so that it can be used in round-trips. Otherwise, users will have to convert values they receive from the RPC to pass them to another RPC of the same program.

I agree, @ismaelsadeeq maybe we could move this issue to a more “general” one where we migrate all units from BTC/kvB to sat/vB?

ismaelsadeeq commented at 1:08 pm on March 18, 2025: member

It will be verbose to update all instances to use sat/vB instead of BTC/kvB in one go. Moreover, I imagine this will be a breaking change.

I also understand @maflcko’s concern that having both units in the code makes things somewhat ambiguous for clients.

polespinasa commented at 1:18 pm on March 18, 2025: contributor

We can look at what changes are needed, update the ones that don’t break anything (e.g. rpc that simply give information but don’t take a feerate as an argument). And the others can be put together in a PR, if they are not too many or too critical we can warn during a couple of releases with “deprecated” and at some point update to change the units.

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

We can look at what changes are needed, update the ones that don’t break anything (e.g. rpc that simply give information but don’t take a feerate as an argument).

This makes sense, currently internally CFeeRate is represented as feerate in BTC/kvB, although the newer replacement to it FeeFrac is going to be in s/vb see https://github.com/bitcoin/bitcoin/pull/31363/commits/8d1bbafa84bbca0e412f939823fa1a30ef839951

It will makes to first analyse how many or too critical of a change this is.

ryanofsky referenced this in commit 5f3848c63b on Mar 24, 2025

maflcko closed this on Mar 25, 2025

maflcko commented at 2:56 pm on March 25, 2025: member

Closing for now, after deprecation

wallet: rpc: settxfee sets the wallet feerate not fee #31088

wallet: rpc: `settxfee` sets the wallet feerate not fee #31088