/bitcoin

Better API for estimatesmartfee RPC #10707

Merged
merged 2 commits into from Jul 17, 2017
Commits
Jump to file or symbol
Failed to load files and symbols.
+52 −42
Split
View
19 src/policy/fees.cpp
@@ -839,20 +839,20 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, FeeCalculation
EstimationResult tempResult;
// Return failure if trying to analyze a target we're not tracking
- if (confTarget <= 0 || (unsigned int)confTarget > longStats->GetMaxConfirms())
- return CFeeRate(0);
+ if (confTarget <= 0 || (unsigned int)confTarget > longStats->GetMaxConfirms()) {
+ return CFeeRate(0); // error conditon
+ }
// It's not possible to get reasonable estimates for confTarget of 1
- if (confTarget == 1)
- confTarget = 2;
+ if (confTarget == 1) confTarget = 2;
unsigned int maxUsableEstimate = MaxUsableEstimate();
- if (maxUsableEstimate <= 1)
- return CFeeRate(0);
-
if ((unsigned int)confTarget > maxUsableEstimate) {
confTarget = maxUsableEstimate;
}
+ if (feeCalc) feeCalc->returnedTarget = confTarget;
+
+ if (confTarget <= 1) return CFeeRate(0); // error conditon
assert(confTarget > 0); //estimateCombinedFee and estimateConservativeFee take unsigned ints
/** true is passed to estimateCombined fee for target/2 and target so
@@ -899,10 +899,7 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, FeeCalculation
}
}
- if (feeCalc) feeCalc->returnedTarget = confTarget;
-
- if (median < 0)
- return CFeeRate(0);
+ if (median < 0) return CFeeRate(0); // error conditon
return CFeeRate(median);
}
View
1 src/rpc/client.cpp
@@ -114,7 +114,6 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "getrawmempool", 0, "verbose" },
{ "estimatefee", 0, "nblocks" },
{ "estimatesmartfee", 0, "nblocks" },
- { "estimatesmartfee", 1, "conservative" },
{ "estimaterawfee", 0, "nblocks" },
{ "estimaterawfee", 1, "threshold" },
{ "prioritisetransaction", 1, "dummy" },
View
74 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"
@promag

promag Jul 17, 2017

Contributor

Nit, missing space inside (),

@TheBlueMatt

TheBlueMatt Jul 17, 2017

Contributor

Since when do we add a space inside ()s?
"\nEstimates the approximate fee per kilobyte needed for a transaction to begin\n"
- "confirmation within nblocks blocks if possible and return the number of blocks\n"
+ "confirmation within conf_target blocks if possible and return the number of blocks\n"
"for which the estimate is valid. Uses virtual transaction size as defined\n"
"in BIP 141 (witness data is discounted).\n"
"\nArguments:\n"
- "1. nblocks (numeric)\n"
- "2. conservative (bool, optional, default=true) Whether to return a more conservative estimate which\n"
- " also satisfies a longer history. A conservative estimate potentially returns a higher\n"
- " 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. conf_target (numeric) Confirmation target in blocks (1 - 1008)\n"
+ "2. \"estimate_mode\" (string, optional, default=CONSERVATIVE) The fee estimate mode.\n"
+ " 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 as responsive to short term drops in the\n"
+ " prevailing fee market. Must be one of:\n"
+ " \"UNSET\" (defaults to CONSERVATIVE)\n"
+ " \"ECONOMICAL\"\n"
+ " \"CONSERVATIVE\"\n"
"\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"
@sipa

sipa Jul 15, 2017

Owner

Why are these not RPC errors?

@morcos

morcos Jul 15, 2017

Contributor

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

sipa Jul 16, 2017

Owner

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

morcos Jul 16, 2017

Contributor

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

promag Jul 16, 2017

Contributor

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

sipa Jul 17, 2017

Owner

We have RPC error codes, but those are usually for exceptional situations, rather than to communicate a totally inevitable not-enough-data situation.
" \"blocks\" : n (numeric) block number where estimate was found\n"
"}\n"
"\n"
- "A negative value is returned if not enough transactions and blocks\n"
+ "The request target will be clamped between 2 and the highest target\n"
+ "fee estimation is able to return based on how long it has been running.\n"
+ "An error is returned if not enough transactions and blocks\n"
"have been observed to make an estimate for any number of blocks.\n"
"\nExample:\n"
+ HelpExampleCli("estimatesmartfee", "6")
);
- RPCTypeCheck(request.params, {UniValue::VNUM});
-
- int nBlocks = request.params[0].get_int();
+ RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VSTR});
+ RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
+ unsigned int conf_target = ParseConfirmTarget(request.params[0]);
bool conservative = true;
if (request.params.size() > 1 && !request.params[1].isNull()) {
- RPCTypeCheckArgument(request.params[1], UniValue::VBOOL);
- conservative = request.params[1].get_bool();
+ FeeEstimateMode fee_mode;
+ if (!FeeModeFromString(request.params[1].get_str(), fee_mode)) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter");
+ }
+ if (fee_mode == FeeEstimateMode::ECONOMICAL) conservative = false;
}
UniValue result(UniValue::VOBJ);
+ UniValue errors(UniValue::VARR);
@promag

promag Jul 17, 2017

Contributor

Nit, move to else block below.
FeeCalculation feeCalc;
- 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)) {
@promag

promag Jul 17, 2017

Contributor

Suggestion for future PR, CFeeRate constructor argument could have default value = 0 so CFeeRate() sounds more like null fee rate.
+ result.push_back(Pair("feerate", ValueFromAmount(feeRate.GetFeePerK())));
+ } else {
+ errors.push_back("Insufficient data or no feerate found");
+ result.push_back(Pair("errors", errors));
+ }
result.push_back(Pair("blocks", feeCalc.returnedTarget));
return result;
}
@@ -850,18 +867,18 @@ UniValue estimaterawfee(const JSONRPCRequest& request)
{
if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
throw std::runtime_error(
- "estimaterawfee nblocks (threshold)\n"
+ "estimaterawfee conf_target (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"
" and the results it returns will change if the internal implementation changes.\n"
"\nEstimates the approximate fee per kilobyte needed for a transaction to begin\n"
- "confirmation within nblocks blocks if possible. Uses virtual transaction size as defined\n"
- "in BIP 141 (witness data is discounted).\n"
+ "confirmation within conf_target blocks if possible. Uses virtual transaction size as\n"
+ "defined in BIP 141 (witness data is discounted).\n"
"\nArguments:\n"
- "1. nblocks (numeric) Confirmation target in blocks (1 - 1008)\n"
+ "1. conf_target (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"
+ " confirmed within conf_target in order to consider those feerates as high enough and proceed to check\n"
" lower buckets. Default: 0.95\n"
"\nResult:\n"
"{\n"
@@ -889,12 +906,9 @@ UniValue estimaterawfee(const JSONRPCRequest& request)
+ HelpExampleCli("estimaterawfee", "6 0.9")
);
- RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VNUM, UniValue::VNUM}, true);
+ RPCTypeCheck(request.params, {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");
- }
+ unsigned int conf_target = ParseConfirmTarget(request.params[0]);
double threshold = 0.95;
if (!request.params[1].isNull()) {
threshold = request.params[1].get_real();
@@ -910,9 +924,9 @@ UniValue estimaterawfee(const JSONRPCRequest& request)
EstimationResult buckets;
// Only output results for horizons which track the target
- if ((unsigned int)nBlocks > ::feeEstimator.HighestTargetTracked(horizon)) continue;
+ if (conf_target > ::feeEstimator.HighestTargetTracked(horizon)) continue;
- feeRate = ::feeEstimator.estimateRawFee(nBlocks, threshold, horizon, &buckets);
+ feeRate = ::feeEstimator.estimateRawFee(conf_target, threshold, horizon, &buckets);
UniValue horizon_result(UniValue::VOBJ);
UniValue errors(UniValue::VARR);
UniValue passbucket(UniValue::VOBJ);
@@ -963,9 +977,9 @@ static const CRPCCommand commands[] =
{ "generating", "generatetoaddress", &generatetoaddress, true, {"nblocks","address","maxtries"} },
{ "util", "estimatefee", &estimatefee, true, {"nblocks"} },
- { "util", "estimatesmartfee", &estimatesmartfee, true, {"nblocks", "conservative"} },
+ { "util", "estimatesmartfee", &estimatesmartfee, true, {"conf_target", "estimate_mode"} },
- { "hidden", "estimaterawfee", &estimaterawfee, true, {"nblocks", "threshold"} },
+ { "hidden", "estimaterawfee", &estimaterawfee, true, {"conf_target", "threshold"} },
};
void RegisterMiningRPCCommands(CRPCTable &t)