/bitcoin

Change API to estimaterawfee #10543

Merged
merged 3 commits into from Jul 11, 2017

Conversation

Reviewers

@promag promag

@ryanofsky ryanofsky

Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
  0.15.0
10 participants
@morcos @RHavar @sipa @laanwj @instagibbs @jlopp @TheBlueMatt @promag @ryanofsky @fanquake
@morcos
Contributor

morcos commented Jun 6, 2017

Report results for all 3 possible time horizons instead of specifying time horizon as an argument.

@sipa requested this. I'm indifferent, but we should merge this for 0.15 if it's considered a better way to present the information, before the old api is released.

@instagibbs @RHavar both found the old interface a bit unintuitive

@jlopp any thoughts?

fanquake added the RPC/REST/ZMQ label Jun 6, 2017

fanquake added this to the 0.15.0 milestone Jun 6, 2017

@RHavar
Contributor

RHavar commented Jun 6, 2017 • edited

utACK I prefer this API. I'll compile it and play with it a bit later today. Thanks for doing this 🥇
@sipa
Owner

sipa commented Jun 6, 2017

My reason for asking this is that it would allow someone to gather information using this RPC, without needing implementation-specific information. Sure, they'll need to understand the details to process the output anyway, but not the just collect.
@RHavar
Contributor

RHavar commented Jun 6, 2017

ACK. Seems to work well, and easier to log/use

bitcoin-cli estimaterawfee 50 0.5
{
  "short": {
    "feerate": -1
  },
  "medium": {
    "feerate": -1
  },
  "long": {
    "feerate": 0.00056300,
    "decay": 0.99931,
    "scale": 24,
    "pass": {
      "startrange": 54641,
      "endrange": 57374,
      "withintarget": 878.66,
      "totalconfirmed": 1495.65,
      "inmempool": 0,
      "leftmempool": 141.38
    },
    "fail": {
      "startrange": 52040,
      "endrange": 54641,
      "withintarget": 959.04,
      "totalconfirmed": 4799.12,
      "inmempool": 0,
      "leftmempool": 1430.93
    }
  }
}
@laanwj
Owner

laanwj commented Jun 7, 2017

Concept ACK
@instagibbs
Member

instagibbs commented Jun 7, 2017

concept ACK as well.
@jlopp
Contributor

jlopp commented Jun 7, 2017

Concept ACK; this will reduce the number of RPC calls we need to make to get a full picture of the fee market.
@morcos
Contributor

morcos commented Jun 14, 2017

rebased due to adjacent line change in src/rpc/client.cpp
@RHavar
Contributor

RHavar commented Jun 14, 2017 • edited

I've been running this patch for the last week on https://www.estimatefee.com to come up with estimates for how long a specific transaction will take to confirm. Haven't had any problems, and it works well and offers a nicer API. So ACK'ing again
@laanwj
Owner

laanwj commented Jun 26, 2017 • edited 

  "short": {
    "feerate": -1
  },

While we're changing the interface anyway, would it make sense to change the "information absent" response to something else than -1? (according to discussion in #8758)
@sipa
Owner

sipa commented Jun 27, 2017

While we're changing the interface anyway, would it make sense to change the "information absent" response to something else than -1

ACK
@TheBlueMatt
Contributor

TheBlueMatt commented Jun 27, 2017

utACK 7780de8 irrespective of if the "information absent" response is changed.
@morcos
Contributor

morcos commented Jun 28, 2017

OK Updated to change error reporting and information absent
For a target outside the range:

morcos@queen:~/Projects/bitcoin2$ src/bitcoin-cli -regtest estimaterawfee 1009 
error code: -8
error message:
Invalid nblocks

When an answer can't be returned due to no fee rate meeting the threshold at the target (pass bucket is omitted but fail bucket still contains useful information):

morcos@queen:~/Projects/bitcoin2$ src/bitcoin-cli -regtest estimaterawfee 1 1
{
  "short": {
    "decay": 0.962,
    "scale": 1,
    "fail": {
      "startrange": 881683,
      "endrange": 1e+99,
      "withintarget": 13.45,
      "totalconfirmed": 14.96,
      "inmempool": 0,
      "leftmempool": 0
    },
    "errors": [
      "Insufficient data or no feerate found which meets threshold"
    ]
  },
  "medium": {
    "feerate": 0.02210603,
    "decay": 0.9952,
    "scale": 2,
    "pass": {
      "startrange": 2121876,
      "endrange": 3456312,
      "withintarget": 40.6,
      "totalconfirmed": 40.6,
      "inmempool": 0,
      "leftmempool": 0
    },
    "fail": {
      "startrange": 1367780,
      "endrange": 2121876,
      "withintarget": 23.48,
      "totalconfirmed": 23.48,
      "inmempool": 0,
      "leftmempool": 0
    }
  },
  "long": {
    "feerate": 0.00128368,
    "decay": 0.99931,
    "scale": 24,
    "pass": {
      "startrange": 125239,
      "endrange": 131501,
      "withintarget": 26652.56,
      "totalconfirmed": 26652.56,
      "inmempool": 0,
      "leftmempool": 0
    },
    "fail": {
      "startrange": 119276,
      "endrange": 125239,
      "withintarget": 76474.09,
      "totalconfirmed": 76474.52,
      "inmempool": 0,
      "leftmempool": 0
    }
  }
}

When the target is not supported at a given horizon, the result is just omitted. Also if all feerates pass, the fail bucket is omitted:

morcos@queen:~/Projects/bitcoin2$ src/bitcoin-cli -regtest estimaterawfee 122 0.0001
{
  "long": {
    "feerate": 0.00001000,
    "decay": 0.99931,
    "scale": 24,
    "pass": {
      "startrange": 0,
      "endrange": 1000,
      "withintarget": 168.05,
      "totalconfirmed": 427.14,
      "inmempool": 0,
      "leftmempool": 21.99
    }
  }
}

When there is insufficient data to even calculate whether the target is met (pass bucket is omitted but fail bucket still contains useful information): *hacked to require many more data points for med and long horizons

morcos@queen:~/Projects/bitcoin2$ src/bitcoin-cli -regtest estimaterawfee 12
{
  "short": {
    "feerate": 0.00068495,
    "decay": 0.962,
    "scale": 1,
    "pass": {
      "startrange": 66417,
      "endrange": 69738,
      "withintarget": 346.39,
      "totalconfirmed": 357.4,
      "inmempool": 0,
      "leftmempool": 0
    },
    "fail": {
      "startrange": 63254,
      "endrange": 66417,
      "withintarget": 4365.32,
      "totalconfirmed": 6062.21,
      "inmempool": 0,
      "leftmempool": 0
    }
  },
  "medium": {
    "decay": 0.9952,
    "scale": 2,
    "fail": {
      "startrange": 0,
      "endrange": 1e+99,
      "withintarget": 262932.09,
      "totalconfirmed": 301367.03,
      "inmempool": 0,
      "leftmempool": 3477.64
    },
    "errors": [
      "Insufficient data or no feerate found which meets threshold"
    ]
  },
  "long": {
    "decay": 0.99931,
    "scale": 24,
    "fail": {
      "startrange": 0,
      "endrange": 1e+99,
      "withintarget": 1852032.93,
      "totalconfirmed": 1943598.4,
      "inmempool": 0,
      "leftmempool": 3236.77
    },
    "errors": [
      "Insufficient data or no feerate found which meets threshold"
    ]
  }
}

morcos referenced this pull request Jun 29, 2017

Merged

Better API for estimatesmartfee RPC #10707

laanwj added to Blockers in High-priority for review Jun 29, 2017

@promag

promag reviewed Jul 6, 2017
View changes

Partial review. Testing.

src/rpc/mining.cpp
@@ -876,7 +876,7 @@ 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)
@promag

promag Jul 6, 2017

Contributor

Missing space before second ||.

src/rpc/mining.cpp
+ for (FeeEstimateHorizon horizon : {FeeEstimateHorizon::SHORT_HALFLIFE, FeeEstimateHorizon::MED_HALFLIFE, FeeEstimateHorizon::LONG_HALFLIFE}) {
+ CFeeRate feeRate;
+ EstimationResult buckets;
+ if ((unsigned int)nBlocks <= ::feeEstimator.HighestTargetTracked(horizon)) {
@promag

promag Jul 6, 2017

Contributor

What about early continue; to avoid large indentation:

if ((unsigned int)nBlocks > ::feeEstimator.HighestTargetTracked(horizon)) continue;
src/rpc/mining.cpp
+ 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));
+ if (!(feeRate == CFeeRate(0))) {
@promag

promag Jul 6, 2017

Contributor

Invert condition and switch blocks.

@morcos

morcos Jul 7, 2017

Contributor

I like having the success condition be the first thing you read, but I'll add a comment

@morcos
Contributor

morcos commented Jul 7, 2017

Addressed nits and squashed
(rawpi.ver2) -> 0220261 (rawapi.ver2.squash)
src/rpc/mining.cpp
@@ -876,7 +876,7 @@ 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"
@promag

promag Jul 7, 2017

Contributor

Nit, remove horizon.

src/rpc/mining.cpp
+ " \"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"
@promag

promag Jul 7, 2017

Contributor

Nit, missing comma here and in the following lines.

src/rpc/mining.cpp
- 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));
+ const char* horizonNames[] = {"short", "medium", "long"};
@promag

promag Jul 7, 2017

Contributor

Nit, rename horizon_names.

src/rpc/mining.cpp
+ if ((unsigned int)nBlocks > ::feeEstimator.HighestTargetTracked(horizon)) continue;
+
+ feeRate = ::feeEstimator.estimateRawFee(nBlocks, threshold, horizon, &buckets);
+ UniValue horizonresult(UniValue::VOBJ);
@promag

promag Jul 7, 2017

Contributor

Nit, snake case these multi word variables?

@morcos

morcos Jul 7, 2017

Contributor

i stuck with fixing the new ones

src/rpc/mining.cpp
+ 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));
@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.

src/rpc/mining.cpp
+ 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))) {
@promag

promag Jul 7, 2017

Contributor

Maybe for another PR, but IMO operator!= would be more expressive, no?

@morcos

morcos Jul 7, 2017

Contributor

Yeah I should have just added that operator before, since this has annoyed me a couple of times.

@morcos
Contributor

morcos commented Jul 7, 2017

Addressed nits and squashed
(rawpi.ver3) -> 6dc1410 (rawapi.ver3.squash)

Thanks for review @promag !
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

src/policy/fees.h
@@ -190,6 +190,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.

src/rpc/mining.cpp
+ 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.

src/rpc/mining.cpp
+ 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.

src/rpc/mining.cpp
+ 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.

@instagibbs
Member

instagibbs commented Jul 7, 2017

tACK 6dc1410
@TheBlueMatt
Contributor

TheBlueMatt commented Jul 7, 2017

re-utACK 6dc1410
@ryanofsky

ryanofsky reviewed Jul 10, 2017
View changes

utACK 6dc1410. Left comments but please ignore any suggestions you think are not worth implementing here.

src/rpc/mining.cpp
+ failbucket.push_back(Pair("leftmempool", round(buckets.fail.leftMempool * 100.0) / 100.0));
+ horizon_result.push_back(Pair("fail", failbucket));
+ }
+ result.push_back(Pair(horizon_names[horizon], horizon_result));
@ryanofsky

ryanofsky Jul 10, 2017

Contributor

In commit "Change API to estimaterawfee"

This is pretty fragile. Consider replacing horizon_names array with FeeEstimateHorizon -> std::string function, or at adding a comment to FeeEstimateHorizon saying the enum values are used as array indices and need to be stable.

src/policy/fees.cpp
+ return longStats->GetMaxConfirms();
+ }
+ default: {
+ return 0;
@ryanofsky

ryanofsky Jul 10, 2017

Contributor

In commit "Add function to report highest estimate target tracked per horizon"

Seems like it would be safer to throw an error than to rely on code handling this to do something with this 0.

@morcos
Contributor

morcos commented Jul 11, 2017

Addressed comments

(rawpi.ver4) -> (rawapi.ver4.squash) -> 5e3b7b5 (rawapi.ver4.rebase)

@laanwj laanwj merged commit 5e3b7b5 into bitcoin:master Jul 11, 2017
1 check passed

1 check passed

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

@laanwj laanwj added a commit that referenced this pull request Jul 11, 2017

@laanwj laanwj Merge #10543: Change API to estimaterawfee 
5e3b7b5 Improve error reporting for estimaterawfee (Alex Morcos)
1fafd70 Add function to report highest estimate target tracked per horizon (Alex Morcos)
9c85b91 Change API to estimaterawfee (Alex Morcos)

Tree-SHA512: e624c6e7967e9e48abe49f5818bd674e5710e571cc093029d2f90d39fdfba3c1f30e83bf89f6dce97052b59a7d9636a64642ccfb26effd149c417d0afbed0c0b
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
b27b004
@ryanofsky

ryanofsky reviewed Jul 11, 2017
View changes

utACK 5e3b7b5. Same as previous review with the two suggested changes.

laanwj removed from Blockers in High-priority for review Jul 11, 2017

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

@sipa sipa 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
This commit was signed with a verified signature.
@sipa sipa Pieter Wuille
GPG key ID: A636E97631F767E0 Learn about signing commits
75b5643
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment