Normalize fee units for RPC ("BTC/kB" and "sat/B)

MarcoFalke commented at 3:17 pm on July 17, 2020: member

This needs to happen before the next major release. Otherwise, it will be a breaking change.

Some more context: #11413 (comment)

MarcoFalke added the label Feature on Jul 17, 2020

MarcoFalke added this to the milestone 0.21.0 on Jul 17, 2020

MarcoFalke added the label RPC/REST/ZMQ on Jul 17, 2020

MarcoFalke added the label Wallet on Jul 17, 2020

MarcoFalke removed the label Wallet on Jul 17, 2020

MarcoFalke commented at 3:18 pm on July 17, 2020: member

cc @instagibbs @jonatack @kallewoof (from original discussion)

jonatack commented at 3:20 pm on July 17, 2020: member

This is on my to-do list this month.

kallewoof commented at 8:53 am on July 20, 2020: member

Ping me when there’s something reviewable (preferably in the form of a PR) and/or if help is needed.

fanquake deleted a comment on Jul 26, 2020

MarcoFalke added the label Up for grabs on Aug 25, 2020

jonatack commented at 7:23 pm on October 8, 2020: member

Am now targeting doing this after October 15 feature freeze, as ISTM it’s a bugfix, for 0.21, but if that is incorrect please let me know.

I just now noticed that has been labeled as Up For Grabs, so if anyone is working on it please advise here so that we don’t both work on it.

Xekyo commented at 9:59 pm on October 8, 2020: member

I’d be interested in helping with this (feerates are something I have an idea about), but I am not sure how I would start. @jonatack would you be interested in pairing on this? Otherwise, I’d also be happy to review.

Edit: Regarding the title, I would definitely suggest only a single unit for feerates, preferably BTC/kvB or sat/vB, i.e. per vbyte rather than byte.

kallewoof commented at 3:55 am on October 9, 2020: member

@jonatack I’m not working on it, but if you drop it, let us know.

jonatack commented at 7:49 am on October 9, 2020: member

I’d be interested in helping with this (feerates are something I have an idea about), but I am not sure how I would start. @jonatack would you be interested in pairing on this?

SGTM @Xekyo! My priorities for the next 6 days are reviewing PRs 19953 (BIPs 340-342), 19954 (BIP 155/Tor v3), and 19988 (tx relay logic). Then this on Oct 16 to propose it by Oct 20/21, will ping you and @kallewoof when starting. Have a look at the last week of comments on #11413, if you like; ISTM we need to (a) fill in the tests to have a regression spec, which I’ve more or less done (b) catch up with changes since that merge, (c) design the change, (d) implement and propose it.

jonatack commented at 1:00 pm on October 19, 2020: member

@kallewoof @MarcoFalke @Xekyo Moving forward on this. To be sure I’m understanding the desired result here, is this what you have in mind?

current master…

0./src/bitcoin-cli -signet -named sendtoaddress address="tb1qga36nkpuy3f3qswy0ltrvcayj7wfrk48tfdrd9" \
1amount=0.00001 conf_target=2 estimate_mode=sat/b

desired replacement (no more overloading of conf_target)…

0./src/bitcoin-cli -signet -named sendtoaddress address="tb1qga36nkpuy3f3qswy0ltrvcayj7wfrk48tfdrd9" \
1amount=0.00001 feerate="2 sat/vb"

e.g. the feerate arg takes a string with a numerical + optional space + “btc/kvb” or “sat/vb” (case insensitive)

(applied to the same RPCs as #11413 plus the new send rpc)

MarcoFalke commented at 1:09 pm on October 19, 2020: member

Yes, I think that makes sense and is an improvement

Xekyo commented at 8:52 pm on October 19, 2020: member

I have read the last week of #11413 that @jonatack linked above, but I don’t understand why overloading the parameter to take a string that includes the magnitude and unit is more desirable than having separate parameters for the two different inputs and only allow a single unit for either.

MarcoFalke commented at 6:36 am on October 20, 2020: member

Not sure if I understood you correctly. Are you asking why feerate="0.00 unit" is better than feerate_amount="0.00", feerate_unit="unit"? I think either is fine to use.

jonatack commented at 7:45 am on October 20, 2020: member

ISTM the main goal is to not overload conf_target and estimate_mode, and use feerate instead:

sendtoaddress/sendmany/send don’t yet have a feerate argument
fundrawtransaction and walletcreatefundedpsbt have feeRate
bumpfee has fee_rate

Murch made a good point in a message yesterday:

022:28 aren't the two spaces of the two allowed units non-overlapping anyway?
122:30 While sat/vB would goes from 1-10,000, BTC per kvB would go from 0.00001-0.1
222:31 So, would having a value smaller than 1 not generally only conform to BTC/kvB and
3      a value 1 or greater generally only make sense in the context of sat/vB?

We could indeed leave off the units, if users don’t find it too confusing or scary? e.g. feerate=1 for 1 sat/vb.

Xekyo commented at 6:49 pm on October 20, 2020: member

ISTM the main goal is to not overload conf_target and estimate_mode, and use feerate instead:

Oh great! Not sure why I got confused about that then.

We could indeed leave off the units, if users don’t find it too confusing or scary? e.g. feerate=1 for 1 sat/vb.

Adding the unit makes it much easier to input an incorrect value. In the comments here alone, I see sat/vb, sat/vB, sat/b, while the title is sat/B. Would we allow all of these? ;) I would also assume that parsing a string to magnitude and unit is more complicated than just parsing a float.

I just realized, though, if we make 1 sat/vB the cutoff, we may be in trouble when the minRelayTxFeerate gets lower under 1 sat/vB. My preference would actually be to just have one unit in the first place, and then I’d either go with sat/vB or sat/kvB.

jonatack commented at 7:00 pm on October 20, 2020: member

I just realized, though, if we make 1 sat/vB the cutoff, we may be in trouble when the minRelayTxFeerate gets lower under 1 sat/vB. My preference would actually be to just have one unit in the first place, and then I’d either go with sat/vB or sat/kvB.

SGTM, I’ll start the day tomorrow with running this by @kallewoof on IRC if he is around and otherwise move forward with proposing feerate=amount, standardized to sat/vB or sat/kvB, with no need to specify units.

kallewoof commented at 8:41 am on October 21, 2020: member

Concept ACK

laanwj commented at 9:35 am on October 21, 2020: member

Are you asking why feerate=“0.00 unit” is better than feerate_amount=“0.00”, feerate_unit=“unit”? I think either is fine to use.

Please, don’t encode the unit into the value. Definitely don’t vary the unit for the same parameter. I have seen this in c-lightning and I think this is very inconvenient for RPC clients, as they have to evaluate units every time at parsing time (making parsing a two-step process too). Either put it in the key (e.g. feerate_unit=x.xx) or standardize it over the entire program, but it might be too late for that.

jonatack commented at 6:36 pm on October 22, 2020: member

First follow-up is #20220 to fix bugs, error messages, and docs before the 0.21 release and have a test spec in place.

meshcollider referenced this in commit 5d32009f1a on Nov 4, 2020

fanquake removed the label Up for grabs on Nov 5, 2020

jonatack commented at 6:28 am on November 5, 2020: member

This needs to happen before the next major release. Otherwise, it will be a breaking change.

Some more context: #11413 (comment)

Proposals in #20305 and #20391.

MarcoFalke referenced this in commit 80e32e120e on Nov 17, 2020

MarcoFalke closed this on Nov 17, 2020

DrahtBot locked this on Feb 15, 2022

Normalize fee units for RPC (“BTC/kB” and “sat/B) #19543