From 439c4e8ad5871f59d87ae2ab77fe01aa6fe41414 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Thu, 29 Jun 2017 13:57:33 -0400 Subject: [PATCH 1/2] 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. --- src/policy/fees.cpp | 19 ++++++++--------- src/rpc/client.cpp | 1 - src/rpc/mining.cpp | 50 +++++++++++++++++++++++++++++++-------------- 3 files changed, 43 insertions(+), 27 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 0f186fa84..73cc0b4a5 100644 --- a/src/policy/fees.cpp +++ b/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); } diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 775ad4b6c..1ab195612 100644 --- a/src/rpc/client.cpp +++ b/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" }, diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index b8c94d32e..caad78859 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -806,42 +806,62 @@ 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 nblocks (\"estimate_mode\")\n" "\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" "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. nblocks (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" " \"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}); - + RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VSTR}); + 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"); + } 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); FeeCalculation feeCalc; CFeeRate feeRate = ::feeEstimator.estimateSmartFee(nBlocks, &feeCalc, conservative); - result.push_back(Pair("feerate", feeRate == CFeeRate(0) ? -1.0 : ValueFromAmount(feeRate.GetFeePerK()))); + if (feeRate != CFeeRate(0)) { + 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; } @@ -889,7 +909,7 @@ 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)) { @@ -963,7 +983,7 @@ 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, {"nblocks", "estimate_mode"} }, { "hidden", "estimaterawfee", &estimaterawfee, true, {"nblocks", "threshold"} }, }; From 06bcdb8da64502a64df03f3c89fbc6ccb72cd349 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Wed, 12 Jul 2017 15:10:16 -0400 Subject: [PATCH 2/2] Convert named argument from nblocks to conf_target in estimatesmartfee and estimaterawfee. Also reuse existing bounds checking. --- src/rpc/mining.cpp | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index caad78859..4fd632dfc 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -806,13 +806,13 @@ UniValue estimatesmartfee(const JSONRPCRequest& request) { if (request.fHelp || request.params.size() < 1 || request.params.size() > 2) throw std::runtime_error( - "estimatesmartfee nblocks (\"estimate_mode\")\n" + "estimatesmartfee conf_target (\"estimate_mode\")\n" "\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) Confirmation target in blocks (1 - 1008)\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" @@ -839,10 +839,7 @@ UniValue estimatesmartfee(const JSONRPCRequest& request) RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VSTR}); 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]); bool conservative = true; if (request.params.size() > 1 && !request.params[1].isNull()) { FeeEstimateMode fee_mode; @@ -855,7 +852,7 @@ UniValue estimatesmartfee(const JSONRPCRequest& request) UniValue result(UniValue::VOBJ); UniValue errors(UniValue::VARR); FeeCalculation feeCalc; - CFeeRate feeRate = ::feeEstimator.estimateSmartFee(nBlocks, &feeCalc, conservative); + CFeeRate feeRate = ::feeEstimator.estimateSmartFee(conf_target, &feeCalc, conservative); if (feeRate != CFeeRate(0)) { result.push_back(Pair("feerate", ValueFromAmount(feeRate.GetFeePerK()))); } else { @@ -870,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" @@ -911,10 +908,7 @@ UniValue estimaterawfee(const JSONRPCRequest& request) 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(); @@ -930,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); @@ -983,9 +977,9 @@ static const CRPCCommand commands[] = { "generating", "generatetoaddress", &generatetoaddress, true, {"nblocks","address","maxtries"} }, { "util", "estimatefee", &estimatefee, true, {"nblocks"} }, - { "util", "estimatesmartfee", &estimatesmartfee, true, {"nblocks", "estimate_mode"} }, + { "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)