/bitcoin

Change API to estimaterawfee #10543

Merged
merged 3 commits into from Jul 11, 2017
Commits
Jump to file or symbol
Failed to load files and symbols.
+112 −53
Split
View
1 src/policy/feerate.h
@@ -40,6 +40,7 @@ class CFeeRate
friend bool operator==(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK == b.nSatoshisPerK; }
friend bool operator<=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK <= b.nSatoshisPerK; }
friend bool operator>=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK >= b.nSatoshisPerK; }
+ friend bool operator!=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK != b.nSatoshisPerK; }
@promag

promag Jul 7, 2017

Contributor

Nit nit, place after operator==.

@morcos

morcos Jul 7, 2017

Contributor

meh
CFeeRate& operator+=(const CFeeRate& a) { nSatoshisPerK += a.nSatoshisPerK; return *this; }
std::string ToString() const;
View
33 src/policy/fees.cpp
@@ -16,6 +16,19 @@
static constexpr double INF_FEERATE = 1e99;
+std::string StringForFeeEstimateHorizon(FeeEstimateHorizon horizon) {
+ static const std::map<FeeEstimateHorizon, std::string> horizon_strings = {
+ {FeeEstimateHorizon::SHORT_HALFLIFE, "short"},
+ {FeeEstimateHorizon::MED_HALFLIFE, "medium"},
+ {FeeEstimateHorizon::LONG_HALFLIFE, "long"},
+ };
+ auto horizon_string = horizon_strings.find(horizon);
+
+ if (horizon_string == horizon_strings.end()) return "unknown";
+
+ return horizon_string->second;
+}
+
std::string StringForFeeReason(FeeReason reason) {
static const std::map<FeeReason, std::string> fee_reason_strings = {
{FeeReason::NONE, "None"},
@@ -671,7 +684,7 @@ CFeeRate CBlockPolicyEstimator::estimateRawFee(int confTarget, double successThr
break;
}
default: {
- return CFeeRate(0);
+ throw std::out_of_range("CBlockPoicyEstimator::estimateRawFee unknown FeeEstimateHorizon");
}
}
@@ -690,6 +703,24 @@ CFeeRate CBlockPolicyEstimator::estimateRawFee(int confTarget, double successThr
return CFeeRate(median);
}
+unsigned int CBlockPolicyEstimator::HighestTargetTracked(FeeEstimateHorizon horizon) const
+{
+ switch (horizon) {
+ case FeeEstimateHorizon::SHORT_HALFLIFE: {
+ return shortStats->GetMaxConfirms();
+ }
+ case FeeEstimateHorizon::MED_HALFLIFE: {
+ return feeStats->GetMaxConfirms();
+ }
+ case FeeEstimateHorizon::LONG_HALFLIFE: {
+ return longStats->GetMaxConfirms();
+ }
+ default: {
+ throw std::out_of_range("CBlockPoicyEstimator::HighestTargetTracked unknown FeeEstimateHorizon");
+ }
+ }
+}
+
unsigned int CBlockPolicyEstimator::BlockSpan() const
{
if (firstRecordedHeight == 0) return 0;
View
5 src/policy/fees.h
@@ -74,6 +74,8 @@ enum FeeEstimateHorizon {
LONG_HALFLIFE = 2
};
+std::string StringForFeeEstimateHorizon(FeeEstimateHorizon horizon);
+
/* Enumeration of reason for returned fee estimate */
enum class FeeReason {
NONE,
@@ -214,6 +216,9 @@ class CBlockPolicyEstimator
/** Empty mempool transactions on shutdown to record failure to confirm for txs still in mempool */
void FlushUnconfirmed(CTxMemPool& pool);
+ /** Calculation of highest target that estimates are tracked for */
+ unsigned int HighestTargetTracked(FeeEstimateHorizon horizon) const;
@promag

promag Jul 7, 2017

Contributor

Since nBlocks and also CBlockIndex::nHeight are int, why not return int here too?

@morcos

morcos Jul 7, 2017

Contributor

Yeah I thought about that, but I figure it should really be an unsigned int, and so why not start moving things slowly in right direction.

@promag

promag Jul 7, 2017

Contributor

No problem, uint32_t then?

@morcos

morcos Jul 7, 2017

Contributor

any reason?

@promag

promag Jul 7, 2017

Contributor

Best practice? Not network code but still. See https://stackoverflow.com/a/21621533.
+
private:
unsigned int nBestSeenHeight;
unsigned int firstRecordedHeight;
View
1 src/rpc/client.cpp
@@ -113,7 +113,6 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "estimatesmartfee", 1, "conservative" },
{ "estimaterawfee", 0, "nblocks" },
{ "estimaterawfee", 1, "threshold" },
- { "estimaterawfee", 2, "horizon" },
{ "prioritisetransaction", 1, "dummy" },
{ "prioritisetransaction", 2, "fee_delta" },
{ "setban", 2, "bantime" },
View
125 src/rpc/mining.cpp
@@ -838,9 +838,9 @@ UniValue estimatesmartfee(const JSONRPCRequest& request)
UniValue estimaterawfee(const JSONRPCRequest& request)
{
- if (request.fHelp || request.params.size() < 1|| request.params.size() > 3)
+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
throw std::runtime_error(
- "estimaterawfee nblocks (threshold horizon)\n"
+ "estimaterawfee nblocks (threshold)\n"
"\nWARNING: This interface is unstable and may disappear or change!\n"
"\nWARNING: This is an advanced API call that is tightly coupled to the specific\n"
" implementation of fee estimation. The parameters it can be called with\n"
@@ -849,72 +849,95 @@ UniValue estimaterawfee(const JSONRPCRequest& request)
"confirmation within nblocks blocks if possible. Uses virtual transaction size as defined\n"
"in BIP 141 (witness data is discounted).\n"
"\nArguments:\n"
- "1. nblocks (numeric)\n"
+ "1. nblocks (numeric) Confirmation target in blocks (1 - 1008)\n"
"2. threshold (numeric, optional) The proportion of transactions in a given feerate range that must have been\n"
" confirmed within nblocks in order to consider those feerates as high enough and proceed to check\n"
" lower buckets. Default: 0.95\n"
- "3. horizon (numeric, optional) How long a history of estimates to consider. 0=short, 1=medium, 2=long.\n"
- " Default: 1\n"
"\nResult:\n"
"{\n"
- " \"feerate\" : x.x, (numeric) estimate fee-per-kilobyte (in BTC)\n"
- " \"decay\" : x.x, (numeric) exponential decay (per block) for historical moving average of confirmation data\n"
- " \"scale\" : x, (numeric) The resolution of confirmation targets at this time horizon\n"
- " \"pass\" : { (json object) information about the lowest range of feerates to succeed in meeting the threshold\n"
- " \"startrange\" : x.x, (numeric) start of feerate range\n"
- " \"endrange\" : x.x, (numeric) end of feerate range\n"
- " \"withintarget\" : x.x, (numeric) number of txs over history horizon in the feerate range that were confirmed within target\n"
- " \"totalconfirmed\" : x.x, (numeric) number of txs over history horizon in the feerate range that were confirmed at any point\n"
- " \"inmempool\" : x.x, (numeric) current number of txs in mempool in the feerate range unconfirmed for at least target blocks\n"
- " \"leftmempool\" : x.x, (numeric) number of txs over history horizon in the feerate range that left mempool unconfirmed after target\n"
- " }\n"
- " \"fail\" : { ... } (json object) information about the highest range of feerates to fail to meet the threshold\n"
+ " \"short\" : { (json object, optional) estimate for short time horizon\n"
+ " \"feerate\" : x.x, (numeric, optional) estimate fee-per-kilobyte (in BTC)\n"
+ " \"decay\" : x.x, (numeric) exponential decay (per block) for historical moving average of confirmation data\n"
+ " \"scale\" : x, (numeric) The resolution of confirmation targets at this time horizon\n"
+ " \"pass\" : { (json object, optional) information about the lowest range of feerates to succeed in meeting the threshold\n"
+ " \"startrange\" : x.x, (numeric) start of feerate range\n"
+ " \"endrange\" : x.x, (numeric) end of feerate range\n"
+ " \"withintarget\" : x.x, (numeric) number of txs over history horizon in the feerate range that were confirmed within target\n"
+ " \"totalconfirmed\" : x.x, (numeric) number of txs over history horizon in the feerate range that were confirmed at any point\n"
+ " \"inmempool\" : x.x, (numeric) current number of txs in mempool in the feerate range unconfirmed for at least target blocks\n"
+ " \"leftmempool\" : x.x, (numeric) number of txs over history horizon in the feerate range that left mempool unconfirmed after target\n"
+ " },\n"
+ " \"fail\" : { ... }, (json object, optional) information about the highest range of feerates to fail to meet the threshold\n"
+ " \"errors\": [ str... ] (json array of strings, optional) Errors encountered during processing\n"
+ " },\n"
+ " \"medium\" : { ... }, (json object, optional) estimate for medium time horizon\n"
+ " \"long\" : { ... } (json object) estimate for long time horizon\n"
"}\n"
"\n"
- "A negative feerate is returned if no answer can be given.\n"
+ "Results are returned for any horizon which tracks blocks up to the confirmation target.\n"
"\nExample:\n"
- + HelpExampleCli("estimaterawfee", "6 0.9 1")
+ + HelpExampleCli("estimaterawfee", "6 0.9")
);
RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VNUM, UniValue::VNUM}, true);
RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
int nBlocks = request.params[0].get_int();
+ if (nBlocks < 1 || (unsigned int)nBlocks > ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE)) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid nblocks");
+ }
double threshold = 0.95;
- if (!request.params[1].isNull())
+ if (!request.params[1].isNull()) {
threshold = request.params[1].get_real();
- FeeEstimateHorizon horizon = FeeEstimateHorizon::MED_HALFLIFE;
- if (!request.params[2].isNull()) {
- int horizonInt = request.params[2].get_int();
- if (horizonInt < 0 || horizonInt > 2) {
- throw JSONRPCError(RPC_TYPE_ERROR, "Invalid horizon for fee estimates");
- } else {
- horizon = (FeeEstimateHorizon)horizonInt;
- }
}
+ if (threshold < 0 || threshold > 1) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid threshold");
+ }
+
UniValue result(UniValue::VOBJ);
- CFeeRate feeRate;
- EstimationResult buckets;
- feeRate = ::feeEstimator.estimateRawFee(nBlocks, threshold, horizon, &buckets);
- result.push_back(Pair("feerate", feeRate == CFeeRate(0) ? -1.0 : ValueFromAmount(feeRate.GetFeePerK())));
- result.push_back(Pair("decay", buckets.decay));
- result.push_back(Pair("scale", (int)buckets.scale));
- UniValue passbucket(UniValue::VOBJ);
- passbucket.push_back(Pair("startrange", round(buckets.pass.start)));
- passbucket.push_back(Pair("endrange", round(buckets.pass.end)));
- passbucket.push_back(Pair("withintarget", round(buckets.pass.withinTarget * 100.0) / 100.0));
- passbucket.push_back(Pair("totalconfirmed", round(buckets.pass.totalConfirmed * 100.0) / 100.0));
- passbucket.push_back(Pair("inmempool", round(buckets.pass.inMempool * 100.0) / 100.0));
- passbucket.push_back(Pair("leftmempool", round(buckets.pass.leftMempool * 100.0) / 100.0));
- result.push_back(Pair("pass", passbucket));
- UniValue failbucket(UniValue::VOBJ);
- failbucket.push_back(Pair("startrange", round(buckets.fail.start)));
- failbucket.push_back(Pair("endrange", round(buckets.fail.end)));
- failbucket.push_back(Pair("withintarget", round(buckets.fail.withinTarget * 100.0) / 100.0));
- failbucket.push_back(Pair("totalconfirmed", round(buckets.fail.totalConfirmed * 100.0) / 100.0));
- failbucket.push_back(Pair("inmempool", round(buckets.fail.inMempool * 100.0) / 100.0));
- failbucket.push_back(Pair("leftmempool", round(buckets.fail.leftMempool * 100.0) / 100.0));
- result.push_back(Pair("fail", failbucket));
+ for (FeeEstimateHorizon horizon : {FeeEstimateHorizon::SHORT_HALFLIFE, FeeEstimateHorizon::MED_HALFLIFE, FeeEstimateHorizon::LONG_HALFLIFE}) {
+ CFeeRate feeRate;
+ EstimationResult buckets;
+
+ // Only output results for horizons which track the target
+ if ((unsigned int)nBlocks > ::feeEstimator.HighestTargetTracked(horizon)) continue;
@promag

promag Jul 7, 2017

Contributor

This omits the given horizon key from the output. Is this considered best practice for an API? An alternative is to return an array:

[{ "horizon": "short", "feerate": 0.00068495, ... }]
@morcos

morcos Jul 7, 2017

Contributor

Yes it's omitted because it is not meaningful to return an answer for a target higher than the horizon tracks.

@promag

promag Jul 7, 2017

Contributor

Hence the suggestion to return the array, a client iterates each element (if any) and uses the horizon value.

@morcos

morcos Jul 7, 2017

Contributor

Oh sorry, I misunderstood at first. I suppose I'm not familiar enough with JSON practices to know what would be preferred.
+
+ feeRate = ::feeEstimator.estimateRawFee(nBlocks, threshold, horizon, &buckets);
+ UniValue horizon_result(UniValue::VOBJ);
+ UniValue errors(UniValue::VARR);
+ UniValue passbucket(UniValue::VOBJ);
@promag

promag Jul 7, 2017

Contributor

My suggestion was to snake case feeRate, passbucket and failbucket.

@morcos

morcos Jul 7, 2017

Contributor

yeah i just fixed variables introduced in this PR, instead of changing preexisting variables

@promag

promag Jul 7, 2017

Contributor

Touched code should be fixed too?

@morcos

morcos Jul 7, 2017

Contributor

Maybe in this case it could have been since I touched a lot of the code, but I think in general it's easier not to make more changes than necessary in order to facilitate the review.
+ passbucket.push_back(Pair("startrange", round(buckets.pass.start)));
+ passbucket.push_back(Pair("endrange", round(buckets.pass.end)));
+ passbucket.push_back(Pair("withintarget", round(buckets.pass.withinTarget * 100.0) / 100.0));
@promag

promag Jul 7, 2017

Contributor

Should we make these round with 2 decimal places as strings? See https://stackoverflow.com/a/1343925.

@morcos

morcos Jul 7, 2017

Contributor

This RPC is kind of a low level debugging function anyway, so I might just save that for some later PR if people want it.
+ passbucket.push_back(Pair("totalconfirmed", round(buckets.pass.totalConfirmed * 100.0) / 100.0));
+ passbucket.push_back(Pair("inmempool", round(buckets.pass.inMempool * 100.0) / 100.0));
+ passbucket.push_back(Pair("leftmempool", round(buckets.pass.leftMempool * 100.0) / 100.0));
+ UniValue failbucket(UniValue::VOBJ);
+ failbucket.push_back(Pair("startrange", round(buckets.fail.start)));
+ failbucket.push_back(Pair("endrange", round(buckets.fail.end)));
+ failbucket.push_back(Pair("withintarget", round(buckets.fail.withinTarget * 100.0) / 100.0));
+ failbucket.push_back(Pair("totalconfirmed", round(buckets.fail.totalConfirmed * 100.0) / 100.0));
+ failbucket.push_back(Pair("inmempool", round(buckets.fail.inMempool * 100.0) / 100.0));
+ failbucket.push_back(Pair("leftmempool", round(buckets.fail.leftMempool * 100.0) / 100.0));
+
+ // CFeeRate(0) is used to indicate error as a return value from estimateRawFee
+ if (feeRate != CFeeRate(0)) {
+ horizon_result.push_back(Pair("feerate", ValueFromAmount(feeRate.GetFeePerK())));
+ horizon_result.push_back(Pair("decay", buckets.decay));
+ horizon_result.push_back(Pair("scale", (int)buckets.scale));
+ horizon_result.push_back(Pair("pass", passbucket));
+ // buckets.fail.start == -1 indicates that all buckets passed, there is no fail bucket to output
+ if (buckets.fail.start != -1) horizon_result.push_back(Pair("fail", failbucket));
+ } else {
+ // Output only information that is still meaningful in the event of error
+ horizon_result.push_back(Pair("decay", buckets.decay));
+ horizon_result.push_back(Pair("scale", (int)buckets.scale));
+ horizon_result.push_back(Pair("fail", failbucket));
+ errors.push_back("Insufficient data or no feerate found which meets threshold");
+ horizon_result.push_back(Pair("errors",errors));
@promag

promag Jul 7, 2017

Contributor

Nit, missing space before errors.

@morcos

morcos Jul 7, 2017

Contributor

will fix if there are any more changes, but leave for now to avoid churn.
+ }
+ result.push_back(Pair(StringForFeeEstimateHorizon(horizon), horizon_result));
+ }
return result;
}
@@ -932,7 +955,7 @@ static const CRPCCommand commands[] =
{ "util", "estimatefee", &estimatefee, true, {"nblocks"} },
{ "util", "estimatesmartfee", &estimatesmartfee, true, {"nblocks", "conservative"} },
- { "hidden", "estimaterawfee", &estimaterawfee, true, {"nblocks", "threshold", "horizon"} },
+ { "hidden", "estimaterawfee", &estimaterawfee, true, {"nblocks", "threshold"} },
};
void RegisterMiningRPCCommands(CRPCTable &t)