RPC: Migrate estimatesmartfee return value from "feerate" (BTC/kvB) to "fee

luke-jr commented at 1:15 am on November 25, 2020: member

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

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

core_io: Add ValueFromFeeRate helper 8798383475

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

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

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

fanquake requested review from jonatack on Nov 25, 2020

DrahtBot commented at 6:50 am on November 25, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20286 (rpc: deprecate addresses and reqSigs from rpc outputs by mjdietzx)
#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 commented at 0:57 am on November 26, 2020: contributor

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?

in src/rpc/mining.cpp:1045 in 50c941638e

1040@@ -1041,7 +1041,8 @@ static RPCHelpMan estimatesmartfee()
1041                 RPCResult{
1042                     RPCResult::Type::OBJ, "", "",
1043                     {
1044-                        {RPCResult::Type::NUM, "feerate", /* optional */ true, "estimate fee rate in " + CURRENCY_UNIT + "/kB (only present if no errors were encountered)"},
1045+                        {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)"},
1046+                        {RPCResult::Type::NUM, "fee_rate", /* optional */ true, "estimate fee rate in " + CURRENCY_ATOM + "/vB (only present if no errors were encountered)"},

jonatack commented at 5:45 pm on November 26, 2020:

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

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

jonatack commented at 5:49 pm on November 26, 2020: member

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

jonatack commented at 6:05 pm on November 26, 2020: member

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.

MarcoFalke commented at 6:14 pm on November 26, 2020: member

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

jonatack commented at 6:55 pm on November 26, 2020: member

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 commented at 7:10 pm on November 26, 2020: member

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 commented at 7:12 pm on November 26, 2020: contributor

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 commented at 8:24 pm on November 26, 2020: member

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 commented at 8:30 pm on November 26, 2020: contributor

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 commented at 9:26 pm on November 26, 2020: member

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

0{
1  "feerate": 0.00078464,
2  "fee_rate": 78.464,
3  "blocks": 6
4}

kristapsk commented at 9:34 pm on November 26, 2020: contributor

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

jonatack commented at 11:20 am on November 27, 2020: member

Thinking about this more, I propose we:

add sat/vB RPCs setfeerate (done in #20391), estimatefeerate, and maybe estimaterawfeerate
deprecate feeRate (done in #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 #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 commented at 11:33 am on November 27, 2020: contributor

@jonatack , sounds like a good plan to me!

jonatack commented at 11:35 am on November 27, 2020: member

@kristapsk great! I’ll do it today.

luke-jr commented at 3:33 pm on November 27, 2020: member

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

luke-jr closed this on Nov 27, 2020

DrahtBot locked this on Aug 16, 2022

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

Conflicts