RPC: Migrate estimatesmartfee return value from "feerate" (BTC/kvB) to "fee_rate" (sat/vB) #20484

luke-jr · 2020-11-25T01:15:56Z

Not sure if it makes sense to do this or not...

DrahtBot · 2020-11-25T06:50:27Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: deprecate addresses and reqSigs from rpc outputs #20286 (rpc: deprecate addresses and reqSigs from rpc outputs by mjdietzx)
rpc: have raw transaction decoding infer output descriptors #16795 (rpc: have raw transaction decoding infer output descriptors by instagibbs)

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.

kristapsk · 2020-11-26T00:57:39Z

I don't think deprecating feerate here so fast is a good idea, as estimatesmartfee is widely used RPC by various software. Maybe just add fee_rate here and deprecate feerate at some point later in the future?

jonatack

Concept ACK, ISTM this is the direction we need to go in for #19543. The last two commits need updated tests/deprecation test.

jonatack · 2020-11-26T17:45:50Z

src/rpc/mining.cpp

                    {
-                        {RPCResult::Type::NUM, "feerate", /* optional */ true, "estimate fee rate in " + CURRENCY_UNIT + "/kB (only present if no errors were encountered)"},
+                        {RPCResult::Type::NUM, "feerate", /* optional */ true, "estimate fee rate in " + CURRENCY_UNIT + "/kB (only present if no errors were encountered; DEPRECATED, also returned only if the config option -deprecatedrpc=feerate is passed)"},
+                        {RPCResult::Type::NUM, "fee_rate", /* optional */ true, "estimate fee rate in " + CURRENCY_ATOM + "/vB (only present if no errors were encountered)"},


Would need to also update the help further up, e.g.

+++ b/src/rpc/mining.cpp @@ -1022,7 +1022,7 @@ static RPCHelpMan submitheader() static RPCHelpMan estimatesmartfee() { return RPCHelpMan{"estimatesmartfee", - "\nEstimates the approximate fee per kilobyte needed for a transaction to begin\n" + "\nEstimates the approximate fee rate in satoshis per vbyte (" + CURRENCY_ATOM + "/vB) needed for a transaction to begin\n" "confirmation within conf_target blocks if possible and return the number of blocks\n" "for which the estimate is valid. Uses virtual transaction size as defined\n" "in BIP 141 (witness data is discounted).\n", @@ -1031,7 +1031,7 @@ static RPCHelpMan estimatesmartfee() {"estimate_mode", RPCArg::Type::STR, /* default */ "CONSERVATIVE", "The fee estimate mode.\n" " Whether to return a more conservative estimate which also satisfies\n" " a longer history. A conservative estimate potentially returns a\n" - " higher feerate and is more likely to be sufficient for the desired\n" + " higher fee rate and is more likely to be sufficient for the desired\n" " target, but is not as responsive to short term drops in the\n" " prevailing fee market. Must be one of:\n"

jonatack · 2020-11-26T18:05:10Z

I don't think deprecating feerate here so fast is a good idea, as estimatesmartfee is widely used RPC by various software. Maybe just add fee_rate here and deprecate feerate at some point later in the future?

I agree, but here are two reasons why deprecating immediately may (a) be acceptable to reviewers and (b) a good idea:

(a) I made a similar argument a couple months ago against immediately deprecating the getpeerinfo "addnode" field at the same time as a new field was added to replace it ("addnode": true/false -> "connection_type": "manual"/other string values); reviewers disagreed and preferred to deprecate immediately and have it done with

(b) More importantly IMO, having both feerate and fee_rate with different units is not only a bit confusing but also a potential footgun, albeit a hopefully benign one with the 1E5 downward difference between the two units.

maflcko · 2020-11-26T18:14:07Z

What is the benefit of forcing every user to change their scripts when both units can be supported forever at no additional cost?

jonatack · 2020-11-26T18:55:08Z

What is the benefit of forcing every user to change their scripts when both units can be supported forever at no additional cost?

Maybe this is answered by (b) in my comment above: confusion, possible footgun.

An alternative would be to rename them and add the units in the names, but I think that's sub-optimal if we want to simplify and standardize the naming and units and it would be a breaking change as well.

I took another approach with #20391: as settxfee, like estimatesmartfee, is arguably slightly misnamed (they are really about a fee rate, not a fee), I proposed a setfeerate RPC in sat/vB (that also aims to provide an improved user experience to encourage its adoption). Could do the same for estimatefeerate.

sipa · 2020-11-26T19:10:04Z

If the goal is reducing user confusion rather than due to maintenance burden, we could just make the new fields preferred over the old ones, and e.g. stop listing the old ones in help output.

kristapsk · 2020-11-26T19:12:18Z

What is the benefit of forcing every user to change their scripts when both units can be supported forever at no additional cost?

That's the approach c-lightning is taking with various calls.

This isn't affecting just some scripts, I think this may affect most of the Bitcoin software in existence that relies on Bitcoin Core. And I believe not breaking userspace is very important.

jonatack · 2020-11-26T20:24:13Z

What is the benefit of forcing every user to change their scripts when both units can be supported forever at no additional cost?

Remember, the scripts don't have to change if the deprecation flag is passed on restart after updating to 0.21. They have to change only if the code is actually removed later on. The deprecation just hides the fields somewhat similarly to @sipa's suggestion.

kristapsk · 2020-11-26T20:30:43Z

scripts don't have to change if the deprecation flag is passed on restart after updating to 0.21.

That might only help if Bitcoin Core is bundled into other software, user uses his own scripts or is Bitcoin dev in general.

Ordinary user may use Bitcoin Core, c-lightning, JoinMarket and other software relying on Bitcoin Core on his computer, updating everything or parts with OS package manager. They might miss such breaking API changes if they happen so fast, you can't expect everybody to read all the release notes. When more time passes, more software might by updated to use new API and it would be safer to start deprecating old behaviour then.

jonatack · 2020-11-26T21:26:39Z

Ok, to not spend more time on this, please confirm if you think this would be safe and clear.

{
  "feerate": 0.00078464,
  "fee_rate": 78.464,
  "blocks": 6
}

kristapsk · 2020-11-26T21:34:24Z

@jonatack , yes that looks ok to me, should not break any properly written software.

jonatack · 2020-11-27T11:20:32Z

Thinking about this more, I propose we:

add sat/vB RPCs setfeerate (done in wallet: introduce setfeerate (an improved settxfee, in sat/vB) #20391), estimatefeerate, and maybe estimaterawfeerate
deprecate feeRate (done in wallet: deprecate feeRate in fundrawtransaction/walletcreatefundedpsbt #20483)
make settxfee and estimatesmartfee hidden RPCs to avoid confusion for new users while allowing current ones to keep using them (update: settxfee now made hidden in wallet: introduce setfeerate (an improved settxfee, in sat/vB) #20391)

This seems the best way to:

normalize and transition to sat/vB per user demand
protect users by not having feeRate/feerate in BTC/kvB and fee_rate in sat/vB in the same RPCs
minimize the amount of breaking API changes

kristapsk · 2020-11-27T11:33:14Z

@jonatack , sounds like a good plan to me!

jonatack · 2020-11-27T11:35:15Z

@kristapsk great! I'll do it today.

luke-jr · 2020-11-27T15:33:29Z

I like @jonatack 's plan/approach better, closing.

jonatack and others added 4 commits November 25, 2020 01:15

wallet: CFeeRate::ToString helper to print sats without the units

59aadaf

core_io: Add ValueFromFeeRate helper

8798383

RPC: estimatesmartfee: Return result as "fee_rate" in sat/vB

52e1ead

RPC: estimatesmartfee: Deprecate BTC/kB "feerate" return value

50c9416

fanquake added the RPC/REST/ZMQ label Nov 25, 2020

fanquake requested a review from jonatack November 25, 2020 01:56

This was referenced Nov 25, 2020

rpc: deprecate addresses and reqSigs from rpc outputs #20286

Merged

rpc: have raw transaction decoding infer output descriptors #16795

Merged

jonatack reviewed Nov 26, 2020

View reviewed changes

luke-jr closed this Nov 27, 2020

This was referenced Nov 27, 2020

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

Closed

wallet: deprecate feeRate in fundrawtransaction/walletcreatefundedpsbt #20483

Closed

jonatack mentioned this pull request Jan 16, 2021

Improve documentation of estimatesmartfee #18217

Closed

jonatack mentioned this pull request Apr 21, 2021

since v21 settxfee not working correctly #21737

Closed

jonatack mentioned this pull request Aug 10, 2021

rpc: Add function walletestimatefee #22673

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

RPC: Migrate estimatesmartfee return value from "feerate" (BTC/kvB) to "fee_rate" (sat/vB) #20484

RPC: Migrate estimatesmartfee return value from "feerate" (BTC/kvB) to "fee_rate" (sat/vB) #20484

Uh oh!

Conversation

luke-jr commented Nov 25, 2020

Uh oh!

DrahtBot commented Nov 25, 2020

Conflicts

Uh oh!

kristapsk commented Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented Nov 26, 2020

Uh oh!

maflcko commented Nov 26, 2020

Uh oh!

jonatack commented Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristapsk commented Nov 26, 2020

Uh oh!

jonatack commented Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristapsk commented Nov 26, 2020

Uh oh!

jonatack commented Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristapsk commented Nov 26, 2020

Uh oh!

jonatack commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristapsk commented Nov 27, 2020

Uh oh!

jonatack commented Nov 27, 2020

Uh oh!

luke-jr commented Nov 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kristapsk commented Nov 26, 2020 •

edited

Loading

jonatack Nov 26, 2020 •

edited

Loading

jonatack commented Nov 26, 2020 •

edited

Loading

sipa commented Nov 26, 2020 •

edited

Loading

jonatack commented Nov 26, 2020 •

edited

Loading

jonatack commented Nov 26, 2020 •

edited

Loading

jonatack commented Nov 27, 2020 •

edited

Loading