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

maflcko · 2020-07-17T15:17:15Z

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

Some more context: #11413 (comment)

maflcko · 2020-07-17T15:18:48Z

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

jonatack · 2020-07-17T15:20:24Z

This is on my to-do list this month.

kallewoof · 2020-07-20T08:53:34Z

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

jonatack · 2020-10-08T19:23:51Z

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.

murchandamus · 2020-10-08T21:59:25Z

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 · 2020-10-09T03:55:41Z

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

jonatack · 2020-10-09T07:49:50Z

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 · 2020-10-19T13:00:44Z

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

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

desired replacement (no more overloading of conf_target)...

./src/bitcoin-cli -signet -named sendtoaddress address="tb1qga36nkpuy3f3qswy0ltrvcayj7wfrk48tfdrd9" \
amount=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)

maflcko · 2020-10-19T13:09:14Z

Yes, I think that makes sense and is an improvement

murchandamus · 2020-10-19T20:52:53Z

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.

maflcko · 2020-10-20T06:36:15Z

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 · 2020-10-20T07:45:53Z

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:

22:28 aren't the two spaces of the two allowed units non-overlapping anyway?
22:30 While sat/vB would goes from 1-10,000, BTC per kvB would go from 0.00001-0.1
22:31 So, would having a value smaller than 1 not generally only conform to BTC/kvB and
      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.

murchandamus · 2020-10-20T18:49:37Z

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 · 2020-10-20T19:00:04Z

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 · 2020-10-21T08:41:37Z

Concept ACK

laanwj · 2020-10-21T09:35:38Z

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 · 2020-10-22T18:36:53Z

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

0be2900 rpc: update conf_target helps for correctness/consistency (Jon Atack) 778b9be wallet, rpc: fix send subtract_fee_from_outputs help (Jon Atack) 603c005 wallet: add rpc send explicit fee rate coverage (Jon Atack) dd341e6 wallet: add sendtoaddress/sendmany explicit fee rate coverage (Jon Atack) 44e7bfa wallet: add walletcreatefundedpsbt explicit fee rate coverage (Jon Atack) 6e1ea42 test: refactor for walletcreatefundedpsbt fee rate coverage (Jon Atack) 3ac7b0c wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() (Jon Atack) 2d8eba8 wallet: combine redundant bumpfee invalid params and args tests (Jon Atack) 1697a40 wallet: improve bumpfee error/help, add explicit fee rate coverage (Jon Atack) fc57217 wallet: fix SetFeeEstimateMode() error message (Jon Atack) 052427e wallet, bugfix: fix bumpfee with explicit fee rate modes (Jon Atack) Pull request description: Follow-up to #11413 providing a base to build on for #19543: - bugfix for `bumpfee` raising a JSON error with explicit feerates, fixes issue #20219 - adds explicit feerate test coverage for `bumpfee`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendtoaddress`, and `sendmany` - improves a few related RPC error messages and `ParseConfirmTarget()` / error message - fixes/improves the explicit fee rate information in the 6 RPC helps, of which 2 were also missing `conf_target` sat/B units This provides a spec and regression coverage for the potential next step of a universal `sat/vB` feerate argument (see #19543), as well as immediate coverage and minimum fixes for 0.21. ACKs for top commit: kallewoof: Concept/Tested ACK 0be2900 meshcollider: Code review + functional test run ACK 0be2900 Tree-SHA512: efd965003e991cba51d4504e2940f06ab3d742e34022e96a673606b44fad85596aa03a8c1809f06df7ebcf21a38e18a891e54392fe3d6fb4d120bbe4ea0cf5e0

jonatack · 2020-11-05T06:28:44Z

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.

05e82d8 wallet: override minfee checks (fOverrideFeeRate) for fee_rate (Jon Atack) 9a670b4 wallet: update sendtoaddress, send RPC examples with fee_rate (Jon Atack) be481b7 wallet: use MIN_RELAY_TX_FEE in bumpfee help (Jon Atack) 449b730 wallet: provide valid values if invalid estimate mode passed (Jon Atack) 6da3afb wallet: update remaining rpcwallet fee rate units to BTC/kvB (Jon Atack) 173b5b5 wallet: update fee rate units, use sat/vB for fee_rate error messages (Jon Atack) 7f9835a wallet: remove fee rates from conf_target helps (Jon Atack) b7994c0 wallet: add fee_rate unit warnings to bumpfee (Jon Atack) 410e471 wallet: remove redundant bumpfee fee_rate checks (Jon Atack) a0d4957 wallet: introduce fee_rate (sat/vB) param/option (Jon Atack) e21212f wallet: remove unneeded WALLET_BTC_KB_TO_SAT_B constant (Jon Atack) 6112cf2 wallet: add CFeeRate ctor doxygen documentation (Jon Atack) 3f72791 wallet: fix bug in RPC send options (Jon Atack) Pull request description: This PR builds on #11413 and #20220 to address #19543. - replace overloading the conf_target and estimate_mode params with `fee_rate` in sat/vB in the sendtoaddress, sendmany, send, fundrawtransaction, walletcreatefundedpsbt, and bumpfee RPCs - allow non-actionable conf_target value of `0` and estimate_mode value of `""` to be passed to use `fee_rate` as a positional argument, in addition to as a named argument - fix a bug in the experimental send RPC described in #20220 (comment) where args were not being passed correctly into the options values - update the feerate error message units for these RPCs from BTC/kB to sat/vB - update the test coverage, help docs, doxygen docs, and some of the RPC examples - other changes to address the excellent review feedback See this wallet meeting log for more context: http://www.erisian.com.au/bitcoin-core-dev/log-2020-11-06.html#l-309 ACKs for top commit: achow101: re-ACK 05e82d8 MarcoFalke: review ACK 05e82d8 did not test and found a few style nits, which can be fixed later 🍯 Xekyo: tACK 05e82d8 Sjors: utACK 05e82d8 Tree-SHA512: a4ee5f184ada53f1840b2923d25873bda88c5a2ae48e67eeea2417a0b35154798cfdb3c147b05dd56bd6608a784e1b91623bb985ee2ab9ef2baaec22206d0a9c

maflcko added the Feature label Jul 17, 2020

maflcko added this to the 0.21.0 milestone Jul 17, 2020

maflcko added RPC/REST/ZMQ Wallet and removed Wallet labels Jul 17, 2020

bitcoin deleted a comment from amirofski Jul 26, 2020

maflcko mentioned this issue Aug 5, 2020

Add explicit feeRate option for sendmany RPC method #19662

Closed

maflcko added the Up for grabs label Aug 25, 2020

maflcko mentioned this issue Sep 7, 2020

RPC: Return transaction fee from testmempoolaccept #19093

Closed

maflcko mentioned this issue Sep 29, 2020

Followup from #19969 #20032

Closed

This was referenced Oct 22, 2020

wallet, rpc: explicit fee rate follow-ups/fixes for 0.21 #20220

Merged

wallet, rpc: universal feerate (sat/vB) param/option #20231

Closed

jonatack mentioned this issue Nov 5, 2020

wallet: introduce fee_rate sat/vB param/option #20305

Merged

fanquake removed the Up for grabs label Nov 5, 2020

jonatack mentioned this issue Nov 16, 2020

wallet: introduce setfeerate (an improved settxfee, in sat/vB) #20391

Closed

maflcko closed this as completed Nov 17, 2020

jonatack mentioned this issue Nov 24, 2020

wallet: deprecate feeRate in fundrawtransaction/walletcreatefundedpsbt #20483

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maflcko commented Jul 17, 2020

maflcko commented Jul 17, 2020

jonatack commented Jul 17, 2020

kallewoof commented Jul 20, 2020 •

edited

Loading

jonatack commented Oct 8, 2020

murchandamus commented Oct 8, 2020 •

edited

Loading

kallewoof commented Oct 9, 2020

jonatack commented Oct 9, 2020 •

edited

Loading

jonatack commented Oct 19, 2020

maflcko commented Oct 19, 2020

murchandamus commented Oct 19, 2020

maflcko commented Oct 20, 2020

jonatack commented Oct 20, 2020

murchandamus commented Oct 20, 2020 •

edited

Loading

jonatack commented Oct 20, 2020 •

edited

Loading

kallewoof commented Oct 21, 2020

laanwj commented Oct 21, 2020

jonatack commented Oct 22, 2020 •

edited

Loading

jonatack commented Nov 5, 2020 •

edited

Loading

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

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

Comments

maflcko commented Jul 17, 2020

maflcko commented Jul 17, 2020

jonatack commented Jul 17, 2020

kallewoof commented Jul 20, 2020 • edited Loading

jonatack commented Oct 8, 2020

murchandamus commented Oct 8, 2020 • edited Loading

kallewoof commented Oct 9, 2020

jonatack commented Oct 9, 2020 • edited Loading

jonatack commented Oct 19, 2020

maflcko commented Oct 19, 2020

murchandamus commented Oct 19, 2020

maflcko commented Oct 20, 2020

jonatack commented Oct 20, 2020

murchandamus commented Oct 20, 2020 • edited Loading

jonatack commented Oct 20, 2020 • edited Loading

kallewoof commented Oct 21, 2020

laanwj commented Oct 21, 2020

jonatack commented Oct 22, 2020 • edited Loading

jonatack commented Nov 5, 2020 • edited Loading

kallewoof commented Jul 20, 2020 •

edited

Loading

murchandamus commented Oct 8, 2020 •

edited

Loading

jonatack commented Oct 9, 2020 •

edited

Loading

murchandamus commented Oct 20, 2020 •

edited

Loading

jonatack commented Oct 20, 2020 •

edited

Loading

jonatack commented Oct 22, 2020 •

edited

Loading

jonatack commented Nov 5, 2020 •

edited

Loading