Change API to estimaterawfee #10543

morcos · 2017-06-06T17:13:24Z

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?

RHavar · 2017-06-06T17:59:27Z

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

sipa · 2017-06-06T18:35:26Z

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 · 2017-06-06T18:47:20Z

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 · 2017-06-07T13:00:33Z

Concept ACK

instagibbs · 2017-06-07T13:02:18Z

concept ACK as well.

jlopp · 2017-06-07T13:39:22Z

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

morcos · 2017-06-14T19:27:58Z

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

RHavar · 2017-06-14T19:31:50Z

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 · 2017-06-26T11:02:40Z

  "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 · 2017-06-27T02:31:53Z

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

ACK

TheBlueMatt · 2017-06-27T23:34:51Z

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

morcos · 2017-06-28T15:35:44Z

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"
    ]
  }
}

promag

Partial review. Testing.

promag · 2017-07-06T21:49:30Z

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)


Missing space before second ||.

promag · 2017-07-06T21:52:19Z

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))) {


Invert condition and switch blocks.

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

promag · 2017-07-06T21:56:05Z

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)) {


What about early continue; to avoid large indentation:

if ((unsigned int)nBlocks > ::feeEstimator.HighestTargetTracked(horizon)) continue;

morcos · 2017-07-07T02:36:41Z

Addressed nits and squashed
(rawpi.ver2) -> 0220261 (rawapi.ver2.squash)

promag · 2017-07-07T09:09:21Z

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"


Nit, missing comma here and in the following lines.

promag · 2017-07-07T09:12:17Z

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"


Nit, remove horizon.

promag · 2017-07-07T09:15:55Z

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"};


Nit, rename horizon_names.

promag · 2017-07-07T09:18:40Z

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))) {


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

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

promag · 2017-07-07T09:19:50Z

src/rpc/mining.cpp

+        if ((unsigned int)nBlocks > ::feeEstimator.HighestTargetTracked(horizon)) continue;
+
+        feeRate = ::feeEstimator.estimateRawFee(nBlocks, threshold, horizon, &buckets);
+        UniValue horizonresult(UniValue::VOBJ);


Nit, snake case these multi word variables?

i stuck with fixing the new ones

promag · 2017-07-07T09:26:26Z

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));


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

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.

morcos · 2017-07-07T13:17:18Z

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

Thanks for review @promag !

promag · 2017-07-07T14:17:52Z

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; }


Nit nit, place after operator==.

promag · 2017-07-07T14:24:20Z

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;


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

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.

No problem, uint32_t then?

any reason?

Best practice? Not network code but still. See https://stackoverflow.com/a/21621533.

promag · 2017-07-07T14:28:03Z

src/rpc/mining.cpp

+        feeRate = ::feeEstimator.estimateRawFee(nBlocks, threshold, horizon, &buckets);
+        UniValue horizon_result(UniValue::VOBJ);
+        UniValue errors(UniValue::VARR);
+        UniValue passbucket(UniValue::VOBJ);


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

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

Touched code should be fixed too?

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.

promag · 2017-07-07T14:28:38Z

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));


Nit, missing space before errors.

will fix if there are any more changes, but leave for now to avoid churn.

promag · 2017-07-07T14:38:36Z

src/rpc/mining.cpp

+        EstimationResult buckets;
+
+        // Only output results for horizons which track the target
+        if ((unsigned int)nBlocks > ::feeEstimator.HighestTargetTracked(horizon)) continue;


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, ... }]

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

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

Oh sorry, I misunderstood at first. I suppose I'm not familiar enough with JSON practices to know what would be preferred.

instagibbs · 2017-07-07T14:59:30Z

tACK 6dc1410

TheBlueMatt · 2017-07-07T21:56:49Z

re-utACK 6dc1410

ryanofsky

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

ryanofsky · 2017-07-10T20:13:49Z

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));


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.

ryanofsky · 2017-07-10T20:21:44Z

src/policy/fees.cpp

+        return longStats->GetMaxConfirms();
+    }
+    default: {
+        return 0;


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.

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

morcos · 2017-07-11T00:10:14Z

Addressed comments

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

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

ryanofsky

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

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

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

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 bitcoin#10706 and bitcoin#10543~. See bitcoin#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

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

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 bitcoin#10706 and bitcoin#10543~. See bitcoin#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

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

fanquake added this to the 0.15.0 milestone Jun 6, 2017

morcos force-pushed the estimaterawapi branch from ae7d507 to 7780de8 Compare June 14, 2017 19:27

morcos mentioned this pull request Jun 29, 2017

Better API for estimatesmartfee RPC #10707

Merged

promag reviewed Jul 6, 2017

View reviewed changes

morcos force-pushed the estimaterawapi branch 2 times, most recently from 13c683f to 0220261 Compare July 7, 2017 02:33

promag reviewed Jul 7, 2017

View reviewed changes

morcos force-pushed the estimaterawapi branch from 0220261 to 6dc1410 Compare July 7, 2017 13:13

promag reviewed Jul 7, 2017

View reviewed changes

ryanofsky reviewed Jul 10, 2017

View reviewed changes

morcos force-pushed the estimaterawapi branch from 6dc1410 to a5f44c5 Compare July 11, 2017 00:03

morcos added 2 commits July 10, 2017 20:07

Change API to estimaterawfee

9c85b91

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

Add function to report highest estimate target tracked per horizon

1fafd70

Improve error reporting for estimaterawfee

5e3b7b5

morcos force-pushed the estimaterawapi branch from a5f44c5 to 5e3b7b5 Compare July 11, 2017 00:07

laanwj merged commit 5e3b7b5 into bitcoin:master Jul 11, 2017

ryanofsky reviewed Jul 11, 2017

View reviewed changes

jnewbery mentioned this pull request Jul 31, 2017

TODO for release notes 0.15.0 #9889

Closed

12 tasks

jnewbery mentioned this pull request Aug 17, 2017

AI for fee estimation and other parts of the system #11064

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Change API to estimaterawfee #10543

Change API to estimaterawfee #10543

Uh oh!

Conversation

morcos commented Jun 6, 2017

Uh oh!

RHavar commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jun 6, 2017

Uh oh!

RHavar commented Jun 6, 2017

Uh oh!

laanwj commented Jun 7, 2017

Uh oh!

instagibbs commented Jun 7, 2017

Uh oh!

jlopp commented Jun 7, 2017

Uh oh!

morcos commented Jun 14, 2017

Uh oh!

RHavar commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jun 27, 2017

Uh oh!

TheBlueMatt commented Jun 27, 2017

Uh oh!

morcos commented Jun 28, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RHavar commented Jun 6, 2017 •

edited

Loading

RHavar commented Jun 14, 2017 •

edited

Loading

laanwj commented Jun 26, 2017 •

edited

Loading