Better API for estimatesmartfee RPC

morcos commented at 6:36 pm on June 29, 2017: member

Through 0.14 branch, the estimatesmartfee API was tagged “WARNING: This interface is unstable and may disappear or change!” and this warning is removed for 0.15, so any wanted API updates should happen now.

The changes here are to make the additional parameter for conservative estimates a more general estimate_mode string , to omit the feerate and include an error string instead of returning -1 on error, and to do better parameter checking initially.

~~It is only the last 2 commits, but it’s built on #10706 and #10543~~.

See #10707 (comment) for renaming of nblocks argument to conf_target. Will need to be included before string freeze.

PR description edited for clarity

fanquake added the label RPC/REST/ZMQ on Jun 30, 2017

fanquake added the label TX fees and policy on Jun 30, 2017

jonasschnelli added this to the milestone 0.15.0 on Jul 6, 2017

in src/rpc/mining.cpp:816 in d50ab6a0c0 outdated

840-            "2. conservative  (bool, optional, default=true) Whether to return a more conservative estimate which\n"
841-            "                 also satisfies a longer history. A conservative estimate potentially returns a higher\n"
842-            "                 feerate and is more likely to be sufficient for the desired target, but is not as\n"
843-            "                 responsive to short term drops in the prevailing fee market\n"
844+            "1. nblocks         (numeric) Confirmation target in blocks (1 - 1008)\n"
845+            "2. \"estimate_mode\" (string, optional, default=CONSERVATIVE) The fee estimate mode.\n"

morcos commented at 11:57 pm on July 10, 2017:

s/conservative/estimate_mode/ 7 lines up..

morcos commented at 2:51 pm on July 11, 2017:

done

morcos force-pushed on Jul 11, 2017

sipa commented at 5:19 pm on July 11, 2017: member

Needs rebase.

morcos force-pushed on Jul 11, 2017

morcos commented at 6:53 pm on July 11, 2017: member

Rebased correctly for #10589 merge

morcos force-pushed on Jul 12, 2017

morcos commented at 7:15 pm on July 12, 2017: member

Clean rebase again for changes to #10707

Added a new optional commit which changes the named arguments in estimatesmartfee and estimaterawfee from nblocks to conf_target. estimaterawfee is new for 0.15 and estimatesmartfee was previously labeled unstable. I think it makes sense to align the argument names here with the ones used in fundrawtransaction, sendtoaddress, and sendmany. Those 3 wallet RPC calls only got this new argument for 0.15, but I think conf_target is far superior to nblocks. Note that bumpfee was already released with confTarget however. estimatefee is left unchanged as it is deprecated.

I don’t feel strongly about this change, but if we want to align the argument names, now is the time to do it.

ryanofsky commented at 2:47 pm on July 13, 2017: member

It is only the last commit, but it’s built on […]

Looks like it’s the last two commits now, for anyone else looking at this.

ryanofsky commented at 3:32 pm on July 13, 2017: member

utACK 7d70bb734c6ff31248a0457f47520d6e8c3fbf2f. Should update PR description to mention conf_target renames before this is merged.

morcos force-pushed on Jul 14, 2017

morcos commented at 3:20 pm on July 14, 2017: member

rebased on new #10706, no changes

morcos force-pushed on Jul 15, 2017

in src/rpc/mining.cpp:820 in 682ab70b5e outdated

821+            "1. nblocks         (numeric) Confirmation target in blocks (1 - 1008)\n"
822+            "2. \"estimate_mode\" (string, optional, default=CONSERVATIVE) The fee estimate mode.\n"
823+            "                   Whether to return a more conservative estimate which also satisfies\n"
824+            "                   a longer history. A conservative estimate potentially returns a\n"
825+            "                   higher feerate and is more likely to be sufficient for the desired\n"
826+            "                   target, but is not asresponsive to short term drops in the\n"

sipa commented at 6:54 pm on July 15, 2017:

Typo: asresponsive

in src/rpc/mining.cpp:860 in 682ab70b5e outdated

868     CFeeRate feeRate = ::feeEstimator.estimateSmartFee(nBlocks, &feeCalc, conservative);
869-    result.push_back(Pair("feerate", feeRate == CFeeRate(0) ? -1.0 : ValueFromAmount(feeRate.GetFeePerK())));
870+    if (!(feeRate == CFeeRate(0))) {
871+        result.push_back(Pair("feerate", ValueFromAmount(feeRate.GetFeePerK())));
872+    }
873+    else {

sipa commented at 6:55 pm on July 15, 2017:

Nit: else on the same line as }

in src/rpc/mining.cpp:828 in 682ab70b5e outdated

830+            "       \"CONSERVATIVE\"\n"
831             "\nResult:\n"
832             "{\n"
833-            "  \"feerate\" : x.x,     (numeric) estimate fee-per-kilobyte (in BTC)\n"
834+            "  \"feerate\" : x.x,     (numeric, optional) estimate fee-per-kilobyte (in BTC)\n"
835+            "  \"errors\": [ str... ] (json array of strings, optional) Errors encountered during processing\n"

sipa commented at 6:58 pm on July 15, 2017:

Why are these not RPC errors?

morcos commented at 11:58 pm on July 15, 2017:

The whole command is not necessarily an error. It’s just an error in returning a value for that particular horizon. It was my understanding that this was the preferred method rather than silently omitting the horizon. Also there is no error with the request.

Or am I misunderstanding your question?

sipa commented at 7:31 pm on July 16, 2017:

I see. It makes sense that no RPC error is to be returned for cases where it is not due to something the caller could know. It still seems overkill to create a whole new errors field for just a lack of available information, though. Do we expect more kinds of errors to be added later?

morcos commented at 11:18 pm on July 16, 2017:

I had originally thought I’d be able to distinguish between “insufficient data” and “enough data but no passing feerate” (to the extent those are really different things) but it wasn’t easy. However I do like the idea of setting the API now to be general enough that we don’t have to change the API in the future if suppose we want to give an answer but also give an errors indicating part of the calculation couldn’t work or something.

Also estimaterawfee now is merged to master with a similar errors field.

In short, I don’t know that there will be more kinds, but I also don’t see much harm in adding the errors field. If users don’t care they can just ignore it right?

promag commented at 11:43 pm on July 16, 2017:

In terms of REST, the status code can represent both situations. In this case it could be a 412 Precondition Failed with { "message": "Insufficient data or no feerate found" } as body instead of a 200 with an error body. Not sure if it can be like that with RPC.

sipa commented at 10:30 pm on July 17, 2017:

We have RPC error codes, but those are usually for exceptional situations, rather than to communicate a totally inevitable not-enough-data situation.

in src/rpc/mining.cpp:854 in 7ebf6ac747 outdated

866+    UniValue errors(UniValue::VARR);
867     FeeCalculation feeCalc;
868-    CFeeRate feeRate = ::feeEstimator.estimateSmartFee(nBlocks, &feeCalc, ::mempool, conservative);
869-    result.push_back(Pair("feerate", feeRate == CFeeRate(0) ? -1.0 : ValueFromAmount(feeRate.GetFeePerK())));
870+    CFeeRate feeRate = ::feeEstimator.estimateSmartFee(conf_target, &feeCalc, conservative);
871+    if (!(feeRate == CFeeRate(0))) {

promag commented at 11:34 pm on July 16, 2017:

operator!=.

morcos force-pushed on Jul 17, 2017

morcos commented at 10:01 am on July 17, 2017: member

Rebased now that #10706 is merged. Typo fixed in the comments, 2 white space changes and !(feerate == CFeeRate(0)) changed to feerate != CFeeRate(0)

gmaxwell approved

gmaxwell commented at 10:12 am on July 17, 2017: contributor

utACK

ryanofsky commented at 2:43 pm on July 17, 2017: member

utACK a9ee31af97a65232a992c5b81ce6f1115a94d117. Only changes since previous review were rebase, switching to operator != and fixing whitespace around else.

Improve api to estimatesmartfee

Change parameter for conservative estimates to be an estimate_mode string.
Change to never return a -1 for failure but to instead omit the feerate and
return an error string.  Throw JSONRPC error on invalid nblocks parameter.

439c4e8ad5

Convert named argument from nblocks to conf_target

in estimatesmartfee and estimaterawfee. Also reuse existing bounds checking.

06bcdb8da6

morcos force-pushed on Jul 17, 2017

morcos commented at 2:55 pm on July 17, 2017: member

Added 2 more lines of RPC help.

(smartapi.ver2) -> 06bcdb8 (smartapi.ver2.squash)

TheBlueMatt commented at 3:04 pm on July 17, 2017: member

utACK 06bcdb8da64502a64df03f3c89fbc6ccb72cd349

ryanofsky commented at 3:10 pm on July 17, 2017: member

utACK 06bcdb8da64502a64df03f3c89fbc6ccb72cd349, only change is help text

gmaxwell approved

gmaxwell commented at 3:52 pm on July 17, 2017: contributor

reACK

in src/rpc/mining.cpp:809 in 06bcdb8da6

805@@ -806,42 +806,59 @@ UniValue estimatesmartfee(const JSONRPCRequest& request)
806 {
807     if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
808         throw std::runtime_error(
809-            "estimatesmartfee nblocks (conservative)\n"
810+            "estimatesmartfee conf_target (\"estimate_mode\")\n"

promag commented at 7:37 pm on July 17, 2017:

Nit, missing space inside (),

TheBlueMatt commented at 7:52 pm on July 17, 2017:

Since when do we add a space inside ()s?

in src/rpc/mining.cpp:853 in 06bcdb8da6

862+        }
863+        if (fee_mode == FeeEstimateMode::ECONOMICAL) conservative = false;
864     }
865 
866     UniValue result(UniValue::VOBJ);
867+    UniValue errors(UniValue::VARR);

promag commented at 7:40 pm on July 17, 2017:

Nit, move to else block below.

in src/rpc/mining.cpp:856 in 06bcdb8da6

867+    UniValue errors(UniValue::VARR);
868     FeeCalculation feeCalc;
869-    CFeeRate feeRate = ::feeEstimator.estimateSmartFee(nBlocks, &feeCalc, conservative);
870-    result.push_back(Pair("feerate", feeRate == CFeeRate(0) ? -1.0 : ValueFromAmount(feeRate.GetFeePerK())));
871+    CFeeRate feeRate = ::feeEstimator.estimateSmartFee(conf_target, &feeCalc, conservative);
872+    if (feeRate != CFeeRate(0)) {

promag commented at 7:46 pm on July 17, 2017:

Suggestion for future PR, CFeeRate constructor argument could have default value = 0 so CFeeRate() sounds more like null fee rate.

in src/rpc/mining.cpp:841 in 439c4e8ad5 outdated

846             );
847 
848-    RPCTypeCheck(request.params, {UniValue::VNUM});
849-
850+    RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VSTR});
851+    RPCTypeCheckArgument(request.params[0], UniValue::VNUM);

sipa commented at 11:16 pm on July 17, 2017:

Isn’t this redudant, and already checked on the line above?

sipa commented at 11:22 pm on July 17, 2017:

Nevermind.

sipa commented at 11:33 pm on July 17, 2017: member

utACK 06bcdb8da64502a64df03f3c89fbc6ccb72cd349

sipa merged this on Jul 17, 2017

sipa closed this on Jul 17, 2017

sipa referenced this in commit 75b5643c47 on Jul 17, 2017

jnewbery commented at 3:54 am on July 31, 2017: member

Through 0.14 branch, the estimatesmartfee API was tagged “WARNING: This interface is unstable and may disappear or change!” and this warning is removed for 0.15

This PR does not remove the "WARNING: This interface is unstable and may disappear or change!" warning, and it’s still in master. Should the warning be removed?

TheBlueMatt commented at 2:30 pm on August 1, 2017: member

@jnewbery are you sure? looks like its not there on my node

jnewbery commented at 5:55 pm on August 7, 2017: member

are you sure? looks like its not there on my node

My mistake. I must have been looking at the same warning in estimaterawfee. This warning has definitely been removed in estimatesmartfee in master.

mempko referenced this in commit bdd90273e1 on Sep 28, 2017

PastaPastaPasta referenced this in commit ee4f4d343b on Sep 16, 2019

PastaPastaPasta referenced this in commit d04633d28c on Sep 18, 2019

charlesrocket referenced this in commit beabec7e30 on Dec 9, 2019

barrystyle referenced this in commit 0c317684a0 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

Better API for estimatesmartfee RPC #10707