Skip to content
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

[REST] add a rest endpoint for estimatesmartfee, docs, and test #11770

Open
wants to merge 1 commit into
base: master
from

Conversation

@joemphilips
Copy link
Contributor

joemphilips commented Nov 27, 2017

This could be useful if other clients want to use the core's fee estimation logic via REST.

@joemphilips joemphilips force-pushed the joemphilips:rest_fee branch Nov 27, 2017

test/functional/test_framework/rest.py Outdated


#allows simple http get calls
def http_get_call(host, port, path, response_object = 0):

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 27, 2017

Member

nit: Instead of moving them to a separate file, you could add convenience wrappers to the test_node class? Thus, it would not be required to pass in host and port all the time.

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

👍

@joemphilips joemphilips force-pushed the joemphilips:rest_fee branch Nov 27, 2017

@promag
Copy link
Member

promag left a comment

Light concept ACK. Maybe too much for REST?

At least the the URI could be improved. This looks weird:

  • /rest/fee/economical/6.json
  • /rest/fee/unset/3.json
src/rest.cpp Outdated
@@ -2,7 +2,6 @@
// Copyright (c) 2009-2016 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

This comment has been minimized.

@promag

promag Nov 27, 2017

Member

Unrelated change, please revert.

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

done.

src/rest.cpp Outdated
@@ -283,6 +283,7 @@ static bool rest_chaininfo(HTTPRequest* req, const std::string& strURIPart)
case RF_JSON: {
JSONRPCRequest jsonRequest;
jsonRequest.params = UniValue(UniValue::VARR);

This comment has been minimized.

@promag

promag Nov 27, 2017

Member

Unrelated change, please revert.

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

Thanks, done. I will make sure will not to include these kinds of changes from next time.

src/rest.cpp Outdated

switch (rf) {
case RF_JSON: {

This comment has been minimized.

@promag

promag Nov 27, 2017

Member

Remove empty line.

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

done

src/rest.cpp Outdated
return RESTERR(req, HTTP_BAD_REQUEST, "<MODE> must be one of <unset|economical|conservative>");
};


This comment has been minimized.

@promag

promag Nov 27, 2017

Member

Remove empty line.

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

done

src/rest.cpp Outdated
jsonRequest.params = params;

UniValue estimatedfee;
estimatedfee = estimatesmartfee(jsonRequest);

This comment has been minimized.

@promag

promag Nov 27, 2017

Member

Sounds weird to "redirect" to RPC handler (redo the mode parse etc). Maybe factor out what is needed from there?

Maybe just call ::feeEstimator.estimateSmartFee() here?

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

👍

doc/REST-interface.md Outdated
Returns proper fee estimated by bitcoind. `<MODE>` should be one of `<unset|conservative|economical>`.
`<TARGET>` is the desired confirmation time (in block height).


This comment has been minimized.

@promag

promag Nov 27, 2017

Member

Remove 2nd empty line.

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

done.

src/rest.cpp Outdated
boost::split(path, param, boost::is_any_of("/"));
path.erase(path.begin());

if (path.size() != 2)

This comment has been minimized.

@promag

promag Nov 27, 2017

Member

Missing {}.

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

done.

src/rest.cpp Outdated
UniValue estimatesmartfee(const JSONRPCRequest& request);

static bool rest_getfee(HTTPRequest* req, const std::string& strURIPart) {
if (!CheckWarmup(req))

This comment has been minimized.

@promag

promag Nov 27, 2017

Member

Missing {}.

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

done.

doc/REST-interface.md Outdated
#### Fees
`GET /rest/fee/<MODE>/<TARGET>.json`

Returns proper fee estimated by bitcoind. `<MODE>` should be one of `<unset|conservative|economical>`.

This comment has been minimized.

@promag

promag Nov 27, 2017

Member

Returns proper fee estimated by bitcoind, this is bitcoind. 😄

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

Thanks, done.

@joemphilips

This comment has been minimized.

Copy link
Contributor Author

joemphilips commented Nov 27, 2017

Thanks for reviewing.
I will update according to the review soon.

Maybe too much for REST?

Rationale for this PR is that some light client(or other services) wants to use information about block or mempool for estimating fee. This is pretty close to the motivation of getutxo in terms of it places some trust on the full node. core's fee estimation logic is likely to update to a more sophisticated scheme in the near future so many clients may want to use core's logic directly.

At least the the URI could be improved. This looks weird:

You are right. Can I use &foo=bar style query parameter here? I didn't because other endpoint didn't. in that case, what about
/rest/fee.json?target=<TARGET>&mode=<MODE> and make mode optional? Though I'm not sure how easy this is to implement.

Otherwise all I can think is enabling to omit <MODE> . and query in the form like
/rest/fee/5.json

@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 27, 2017

My vote goes to GET /rest/fee?target=<TARGET>&mode=<MODE>, with query parameters and without extension. IMO all endpoints should return JSON, unless Accept: headers requests other format.

src/rest.cpp Outdated
@@ -570,6 +571,55 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
}
}

// A bit of a hack - dependency on a function defined in rpc/mining.cpp
UniValue estimatesmartfee(const JSONRPCRequest& request);

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 27, 2017

Member

I think it's not worth passing through the RPC call estimatesmartfee.
Better directly use feeEstimator.estimateFee(nBlocks);.

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

you mean feeEstimator::estimateSmartFee ?

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Nov 27, 2017

Concept ACK.
Please no query string.
The in this PR proposed URL scheme seems correct and in align with other calls: https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md#query-utxo-set

test/functional/smartfees.py Outdated
rpcfeerate = node.estimatesmartfee((i+1)//2)["feerate"]
assert(rpcfeerate > float(e) - delta)

json_string = http_get_call(url.hostname, url.port, '/rest/fee/' + 'unset/' + "{}".format((i+1)//2) + '.json')

This comment has been minimized.

@jb55

jb55 Nov 28, 2017

Contributor

nit. why not:

"/rest/fee/unset/{}.json".format((i+1)//2)

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

👍

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 28, 2017

Please no query string.
The in this PR proposed URL scheme seems correct and in align with other calls:

Yep, agree.
Concept ACK.

@joemphilips joemphilips force-pushed the joemphilips:rest_fee branch 2 times, most recently Nov 29, 2017

@joemphilips
Copy link
Contributor Author

joemphilips left a comment

updated according to review by @promag , @jb55 and @MarcoFalke . Ready for next review.

doc/REST-interface.md Outdated
#### Fees
`GET /rest/fee/<MODE>/<TARGET>.json`

Returns proper fee estimated by bitcoind. `<MODE>` should be one of `<unset|conservative|economical>`.

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

Thanks, done.

doc/REST-interface.md Outdated
Returns proper fee estimated by bitcoind. `<MODE>` should be one of `<unset|conservative|economical>`.
`<TARGET>` is the desired confirmation time (in block height).


This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

done.

src/rest.cpp Outdated
@@ -283,6 +283,7 @@ static bool rest_chaininfo(HTTPRequest* req, const std::string& strURIPart)
case RF_JSON: {
JSONRPCRequest jsonRequest;
jsonRequest.params = UniValue(UniValue::VARR);

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

Thanks, done. I will make sure will not to include these kinds of changes from next time.

src/rest.cpp Outdated
@@ -2,7 +2,6 @@
// Copyright (c) 2009-2016 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

done.

src/rest.cpp Outdated
UniValue estimatesmartfee(const JSONRPCRequest& request);

static bool rest_getfee(HTTPRequest* req, const std::string& strURIPart) {
if (!CheckWarmup(req))

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

done.

src/rest.cpp Outdated

switch (rf) {
case RF_JSON: {

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

done

src/rest.cpp Outdated
return RESTERR(req, HTTP_BAD_REQUEST, "<MODE> must be one of <unset|economical|conservative>");
};


This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

done

src/rest.cpp Outdated
jsonRequest.params = params;

UniValue estimatedfee;
estimatedfee = estimatesmartfee(jsonRequest);

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

👍

src/rest.cpp Outdated
@@ -570,6 +571,55 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
}
}

// A bit of a hack - dependency on a function defined in rpc/mining.cpp
UniValue estimatesmartfee(const JSONRPCRequest& request);

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

you mean feeEstimator::estimateSmartFee ?

test/functional/test_framework/rest.py Outdated


#allows simple http get calls
def http_get_call(host, port, path, response_object = 0):

This comment has been minimized.

@joemphilips

joemphilips Nov 30, 2017

Author Contributor

👍

@promag

This comment has been minimized.

Copy link
Member

promag commented Feb 3, 2018

Needs rebase.

@joemphilips joemphilips force-pushed the joemphilips:rest_fee branch Feb 5, 2018

@joemphilips

This comment has been minimized.

Copy link
Contributor Author

joemphilips commented Feb 5, 2018

rebased.

@joemphilips

This comment has been minimized.

Copy link
Contributor Author

joemphilips commented Feb 15, 2018

umm... sorry, does "Needs rebase" imply needs for squashing? or just rebasing onto master?
I only did rebasing onto master but it needs squashing before merge.
(Sorry for silly question. Forgive me, this is my first PR.) @promag

@bitcoin bitcoin deleted a comment from WorkShop-Office Feb 15, 2018

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Feb 15, 2018

@joemphilips You'll need to rebase onto master, as this currently has merge conflicts in test/functional/feature_fee_estimation.py.

At the same time, you can also squash your changes into a single commit. See Squashing Commits. Use a descriptive commit message i.e "rest: add an endpoint for estimatesmartfee".

@joemphilips joemphilips force-pushed the joemphilips:rest_fee branch 5 times, most recently Feb 18, 2018

@joemphilips

This comment has been minimized.

Copy link
Contributor Author

joemphilips commented Feb 18, 2018

rebased (and squashed)

test/functional/test_framework/test_node.py Outdated
@@ -13,6 +13,8 @@
import re
import subprocess
import time
import urllib.parse
import http.client

This comment has been minimized.

@fanquake

fanquake Feb 19, 2018

Member

http.client is already imported above.

This comment has been minimized.

@joemphilips

joemphilips Feb 19, 2018

Author Contributor

Thanks, done.

@bitcoin bitcoin deleted a comment from Vasyo Feb 19, 2018

@joemphilips joemphilips force-pushed the joemphilips:rest_fee branch Feb 19, 2018

@joemphilips joemphilips force-pushed the joemphilips:rest_fee branch Mar 9, 2018

@jgarzik

jgarzik approved these changes Nov 9, 2018

Copy link
Contributor

jgarzik left a comment

concept ACK

@joemphilips joemphilips force-pushed the joemphilips:rest_fee branch Nov 9, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 9, 2018

Still needs rebase.

Could do something like this:

git checkout rest_fee
git fetch bitcoin
git merge bitcoin/master
git reset --soft bitcoin/master
git commit -m '[REST] add a rest endpoint for estimatesmartfee, docs, and test'
git push origin rest_fee -f

@joemphilips joemphilips force-pushed the joemphilips:rest_fee branch 7 times, most recently Nov 9, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 9, 2018

@joemphilips

This comment has been minimized.

Copy link
Contributor Author

joemphilips commented Nov 9, 2018

rebased

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 9, 2018

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

Conflicts

No conflicts as of last run.

src/rest.cpp Outdated
int64_t conf_target;
try {
conf_target = atoi64(path[1]);
} catch (std::invalid_argument& e) {

This comment has been minimized.

@practicalswift

practicalswift Nov 10, 2018

Member

Remove e since unused :-)

src/rest.cpp Outdated
conservative = false;
} else {
conservative = true;
}

This comment has been minimized.

@practicalswift

practicalswift Nov 10, 2018

Member

Nit: bool conservative = mode != FeeEstimateMode::ECONOMICAL; instead? :-)

FeeEstimateMode mode;
if (!FeeModeFromString(modestr, mode)){
return RESTERR(req, HTTP_BAD_REQUEST, "<MODE> must be one of <unset|economical|conservative>");
};

This comment has been minimized.

@practicalswift

practicalswift Nov 10, 2018

Member

Drop ; :-)

conf_target = atoi64(path[1]);
} catch (std::invalid_argument& e) {
return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Uneable to parse confirmation target to int"));
};

This comment has been minimized.

@practicalswift

practicalswift Nov 10, 2018

Member

Drop ; :-)

unsigned int max_target = ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE);
if (conf_target < 1 || (unsigned int)conf_target > max_target) {
return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Invalid confirmation target, must be in between %u - %u", 1, max_target));
};

This comment has been minimized.

@practicalswift

practicalswift Nov 10, 2018

Member

Drop ; :-)

test/functional/interface_rest.py Outdated
@@ -299,5 +299,17 @@ def run_test(self):
json_obj = self.test_rest_request("/chaininfo")
assert_equal(json_obj['bestblockhash'], bb_hash)

# Prepate for Fee estimation

This comment has been minimized.

@practicalswift

practicalswift Nov 10, 2018

Member

"Prepare" :-)

src/rest.cpp Outdated
try {
conf_target = atoi64(path[1]);
} catch (std::invalid_argument& e) {
return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Uneable to parse confirmation target to int"));

This comment has been minimized.

@practicalswift

practicalswift Nov 10, 2018

Member

"Unable" :-)

@joemphilips joemphilips force-pushed the joemphilips:rest_fee branch Nov 10, 2018

rest: add endpoint for estimatesmartfee
* write REST interface for getting estimated fee
* update docs about REST interface for fee estimation
* add test

@joemphilips joemphilips force-pushed the joemphilips:rest_fee branch to eff1b3e Jan 23, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.