Better API for estimatesmartfee RPC #10707

morcos · 2017-06-29T18:36:49Z

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

morcos · 2017-07-10T23:57:15Z

src/rpc/mining.cpp

-            "                 feerate and is more likely to be sufficient for the desired target, but is not as\n"
-            "                 responsive to short term drops in the prevailing fee market\n"
+            "1. nblocks         (numeric) Confirmation target in blocks (1 - 1008)\n"
+            "2. \"estimate_mode\" (string, optional, default=CONSERVATIVE) The fee estimate mode.\n"


s/conservative/estimate_mode/ 7 lines up..

sipa · 2017-07-11T17:19:14Z

Needs rebase.

morcos · 2017-07-11T18:53:07Z

Rebased correctly for #10589 merge

morcos · 2017-07-12T19:15:45Z

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 · 2017-07-13T14:47:30Z

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

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

morcos · 2017-07-14T15:20:09Z

rebased on new #10706, no changes

sipa · 2017-07-15T18:54:33Z

src/rpc/mining.cpp

+            "                   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"
+            "                   target, but is not asresponsive to short term drops in the\n"


Typo: asresponsive

sipa · 2017-07-15T18:55:04Z

src/rpc/mining.cpp

+    if (!(feeRate == CFeeRate(0))) {
+        result.push_back(Pair("feerate", ValueFromAmount(feeRate.GetFeePerK())));
+    }
+    else {


Nit: else on the same line as }

sipa · 2017-07-15T18:58:45Z

src/rpc/mining.cpp

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


Why are these not RPC errors?

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?

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?

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?

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.

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

promag · 2017-07-16T23:34:00Z

src/rpc/mining.cpp

-    CFeeRate feeRate = ::feeEstimator.estimateSmartFee(nBlocks, &feeCalc, ::mempool, conservative);
-    result.push_back(Pair("feerate", feeRate == CFeeRate(0) ? -1.0 : ValueFromAmount(feeRate.GetFeePerK())));
+    CFeeRate feeRate = ::feeEstimator.estimateSmartFee(conf_target, &feeCalc, conservative);
+    if (!(feeRate == CFeeRate(0))) {


operator!=.

promag · 2017-07-16T23:43:58Z

src/rpc/mining.cpp

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


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.

morcos · 2017-07-17T10:01:31Z

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

utACK

ryanofsky

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

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.

in estimatesmartfee and estimaterawfee. Also reuse existing bounds checking.

morcos · 2017-07-17T14:55:50Z

Added 2 more lines of RPC help.

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

TheBlueMatt · 2017-07-17T15:04:01Z

utACK 06bcdb8

ryanofsky

utACK 06bcdb8, only change is help text

gmaxwell

reACK

promag · 2017-07-17T19:37:52Z

src/rpc/mining.cpp

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


Nit, missing space inside (),

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

promag · 2017-07-17T19:40:32Z

src/rpc/mining.cpp

    }

    UniValue result(UniValue::VOBJ);
+    UniValue errors(UniValue::VARR);


Nit, move to else block below.

promag · 2017-07-17T19:46:42Z

src/rpc/mining.cpp

-    CFeeRate feeRate = ::feeEstimator.estimateSmartFee(nBlocks, &feeCalc, conservative);
-    result.push_back(Pair("feerate", feeRate == CFeeRate(0) ? -1.0 : ValueFromAmount(feeRate.GetFeePerK())));
+    CFeeRate feeRate = ::feeEstimator.estimateSmartFee(conf_target, &feeCalc, conservative);
+    if (feeRate != CFeeRate(0)) {


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

sipa · 2017-07-17T23:16:33Z

src/rpc/mining.cpp

-    RPCTypeCheck(request.params, {UniValue::VNUM});
-
+    RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VSTR});
+    RPCTypeCheckArgument(request.params[0], UniValue::VNUM);


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

sipa · 2017-07-17T23:33:07Z

utACK 06bcdb8

06bcdb8 Convert named argument from nblocks to conf_target (Alex Morcos) 439c4e8 Improve api to estimatesmartfee (Alex Morcos) Pull request description: 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 Tree-SHA512: 6d8ebee8bb410e2950ffd59663eebfed8d1611d995dc935bb91e430d9da7e2f306796f45631458376027d26341c660f09e825e61748103d2f2736ec6dc3df3ae

jnewbery · 2017-07-31T03:54:46Z

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 · 2017-08-01T14:30:47Z

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

jnewbery · 2017-08-07T17:55:01Z

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.

06bcdb8 Convert named argument from nblocks to conf_target (Alex Morcos) 439c4e8 Improve api to estimatesmartfee (Alex Morcos) Pull request description: 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 bitcoin/bitcoin#10707 (comment) for renaming of nblocks argument to conf_target. Will need to be included before string freeze. PR description edited for clarity Tree-SHA512: 6d8ebee8bb410e2950ffd59663eebfed8d1611d995dc935bb91e430d9da7e2f306796f45631458376027d26341c660f09e825e61748103d2f2736ec6dc3df3ae

06bcdb8 Convert named argument from nblocks to conf_target (Alex Morcos) 439c4e8 Improve api to estimatesmartfee (Alex Morcos) Pull request description: 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 bitcoin#10706 and bitcoin#10543~. See bitcoin#10707 (comment) for renaming of nblocks argument to conf_target. Will need to be included before string freeze. PR description edited for clarity Tree-SHA512: 6d8ebee8bb410e2950ffd59663eebfed8d1611d995dc935bb91e430d9da7e2f306796f45631458376027d26341c660f09e825e61748103d2f2736ec6dc3df3ae

06bcdb8da Convert named argument from nblocks to conf_target (Alex Morcos) 439c4e8ad Improve api to estimatesmartfee (Alex Morcos) Pull request description: 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 bitcoin/bitcoin#10707 (comment) for renaming of nblocks argument to conf_target. Will need to be included before string freeze. PR description edited for clarity Tree-SHA512: 6d8ebee8bb410e2950ffd59663eebfed8d1611d995dc935bb91e430d9da7e2f306796f45631458376027d26341c660f09e825e61748103d2f2736ec6dc3df3ae

06bcdb8 Convert named argument from nblocks to conf_target (Alex Morcos) 439c4e8 Improve api to estimatesmartfee (Alex Morcos) Pull request description: 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 bitcoin#10706 and bitcoin#10543~. See bitcoin#10707 (comment) for renaming of nblocks argument to conf_target. Will need to be included before string freeze. PR description edited for clarity Tree-SHA512: 6d8ebee8bb410e2950ffd59663eebfed8d1611d995dc935bb91e430d9da7e2f306796f45631458376027d26341c660f09e825e61748103d2f2736ec6dc3df3ae

morcos mentioned this pull request Jun 29, 2017

Add allowable range to estimatesmartfee RPC help #10654

Closed

fanquake added RPC/REST/ZMQ TX fees and policy labels Jun 30, 2017

jonasschnelli added this to the 0.15.0 milestone Jul 6, 2017

morcos commented Jul 10, 2017

View reviewed changes

morcos force-pushed the bettersmartfeeapi branch from d50ab6a to 4f28d1a Compare July 11, 2017 14:51

TheBlueMatt mentioned this pull request Jul 11, 2017

TODO for release notes 0.15.0 #9889

Closed

12 tasks

morcos force-pushed the bettersmartfeeapi branch from 4f28d1a to e7f2d25 Compare July 11, 2017 18:52

morcos force-pushed the bettersmartfeeapi branch from e7f2d25 to 7d70bb7 Compare July 12, 2017 19:10

ryanofsky reviewed Jul 13, 2017

View reviewed changes

morcos mentioned this pull request Jul 13, 2017

Improve wallet fee logic and fix GUI bugs #10706

Merged

morcos force-pushed the bettersmartfeeapi branch from 7d70bb7 to 3801dbb Compare July 14, 2017 15:19

morcos force-pushed the bettersmartfeeapi branch from 3801dbb to 7ebf6ac Compare July 15, 2017 03:50

sipa reviewed Jul 15, 2017

View reviewed changes

promag reviewed Jul 16, 2017

View reviewed changes

morcos force-pushed the bettersmartfeeapi branch from 7ebf6ac to a9ee31a Compare July 17, 2017 09:59

gmaxwell approved these changes Jul 17, 2017

View reviewed changes

ryanofsky reviewed Jul 17, 2017

View reviewed changes

morcos added 2 commits July 17, 2017 10:52

Improve api to estimatesmartfee

439c4e8

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.

Convert named argument from nblocks to conf_target

06bcdb8

in estimatesmartfee and estimaterawfee. Also reuse existing bounds checking.

morcos force-pushed the bettersmartfeeapi branch from a9ee31a to 06bcdb8 Compare July 17, 2017 14:52

ryanofsky reviewed Jul 17, 2017

View reviewed changes

gmaxwell approved these changes Jul 17, 2017

View reviewed changes

promag reviewed Jul 17, 2017

View reviewed changes

sipa reviewed Jul 17, 2017

View reviewed changes

sipa merged commit 06bcdb8 into bitcoin:master Jul 17, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Better API for estimatesmartfee RPC #10707

Better API for estimatesmartfee RPC #10707

Uh oh!

Conversation

morcos commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jul 11, 2017

Uh oh!

morcos commented Jul 11, 2017

Uh oh!

morcos commented Jul 12, 2017

Uh oh!

ryanofsky commented Jul 13, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 17, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 17, 2017

Uh oh!

TheBlueMatt commented Jul 17, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jul 17, 2017

Uh oh!

jnewbery commented Jul 31, 2017

Uh oh!

TheBlueMatt commented Aug 1, 2017

Uh oh!

morcos commented Jun 29, 2017 •

edited

Loading