New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better API for estimatesmartfee RPC #10707

Merged
merged 2 commits into from Jul 17, 2017

Conversation

@morcos
Member

morcos commented Jun 29, 2017

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

Show outdated Hide outdated src/rpc/mining.cpp Outdated

@TheBlueMatt TheBlueMatt referenced this pull request Jul 11, 2017

Closed

TODO for release notes 0.15.0 #9889

12 of 12 tasks complete
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 11, 2017

Member

Needs rebase.

Member

sipa commented Jul 11, 2017

Needs rebase.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jul 11, 2017

Member

Rebased correctly for #10589 merge

Member

morcos commented Jul 11, 2017

Rebased correctly for #10589 merge

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jul 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.

Member

morcos commented Jul 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ryanofsky

ryanofsky Jul 13, 2017

Contributor

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.

Contributor

ryanofsky commented Jul 13, 2017

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 7d70bb7. Should update PR description to mention conf_target renames before this is merged.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jul 14, 2017

Member

rebased on new #10706, no changes

Member

morcos commented Jul 14, 2017

rebased on new #10706, no changes

Show outdated Hide outdated src/rpc/mining.cpp Outdated
Show outdated Hide outdated src/rpc/mining.cpp Outdated
"\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"

This comment has been minimized.

@sipa

sipa Jul 15, 2017

Member

Why are these not RPC errors?

@sipa

sipa Jul 15, 2017

Member

Why are these not RPC errors?

This comment has been minimized.

@morcos

morcos Jul 15, 2017

Member

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?

@morcos

morcos Jul 15, 2017

Member

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?

This comment has been minimized.

@sipa

sipa Jul 16, 2017

Member

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?

@sipa

sipa Jul 16, 2017

Member

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?

This comment has been minimized.

@morcos

morcos Jul 16, 2017

Member

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?

@morcos

morcos Jul 16, 2017

Member

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?

This comment has been minimized.

@promag

promag Jul 16, 2017

Member

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.

@promag

promag Jul 16, 2017

Member

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.

This comment has been minimized.

@sipa

sipa Jul 17, 2017

Member

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

@sipa

sipa Jul 17, 2017

Member

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

Show outdated Hide outdated src/rpc/mining.cpp Outdated
"\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"

This comment has been minimized.

@promag

promag Jul 16, 2017

Member

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.

@promag

promag Jul 16, 2017

Member

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

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jul 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)

Member

morcos commented Jul 17, 2017

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.

morcos added some commits Jun 29, 2017

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.
Convert named argument from nblocks to conf_target
in estimatesmartfee and estimaterawfee.  Also reuse existing bounds checking.
@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jul 17, 2017

Member

Added 2 more lines of RPC help.

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

Member

morcos commented Jul 17, 2017

Added 2 more lines of RPC help.

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

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 17, 2017

Contributor

utACK 06bcdb8

Contributor

TheBlueMatt commented Jul 17, 2017

utACK 06bcdb8

@ryanofsky

utACK 06bcdb8, only change is help text

@gmaxwell

reACK

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

This comment has been minimized.

@promag

promag Jul 17, 2017

Member

Nit, missing space inside (),

@promag

promag Jul 17, 2017

Member

Nit, missing space inside (),

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 17, 2017

Contributor

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

@TheBlueMatt

TheBlueMatt Jul 17, 2017

Contributor

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

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

This comment has been minimized.

@promag

promag Jul 17, 2017

Member

Nit, move to else block below.

@promag

promag Jul 17, 2017

Member

Nit, move to else block below.

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)) {

This comment has been minimized.

@promag

promag Jul 17, 2017

Member

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

@promag

promag Jul 17, 2017

Member

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

Show outdated Hide outdated src/rpc/mining.cpp Outdated
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 17, 2017

Member

utACK 06bcdb8

Member

sipa commented Jul 17, 2017

utACK 06bcdb8

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Jul 17, 2017

Merge #10707: Better API for estimatesmartfee RPC
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

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Jul 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?

Member

jnewbery commented Jul 31, 2017

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

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Aug 1, 2017

Contributor

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

Contributor

TheBlueMatt commented Aug 1, 2017

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

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Aug 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.

Member

jnewbery commented Aug 7, 2017

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 pushed a commit to meritlabs/merit that referenced this pull request Sep 28, 2017

Merge #10707: Better API for estimatesmartfee RPC
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment