Change API to estimaterawfee #10543

pull morcos wants to merge 3 commits into bitcoin:master from morcos:estimaterawapi changing 5 files +112 −53
  1. morcos commented at 5:13 pm on June 6, 2017: member
    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?
  2. fanquake added the label RPC/REST/ZMQ on Jun 6, 2017
  3. fanquake added this to the milestone 0.15.0 on Jun 6, 2017
  4. RHavar commented at 5:59 pm on June 6, 2017: contributor
    utACK I prefer this API. I’ll compile it and play with it a bit later today. Thanks for doing this 🥇
  5. sipa commented at 6:35 pm on June 6, 2017: member
    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.
  6. RHavar commented at 6:47 pm on June 6, 2017: contributor

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

     0bitcoin-cli estimaterawfee 50 0.5
 1{
 2  "short": {
 3    "feerate": -1
 4  },
 5  "medium": {
 6    "feerate": -1
 7  },
 8  "long": {
 9    "feerate": 0.00056300,
10    "decay": 0.99931,
11    "scale": 24,
12    "pass": {
13      "startrange": 54641,
14      "endrange": 57374,
15      "withintarget": 878.66,
16      "totalconfirmed": 1495.65,
17      "inmempool": 0,
18      "leftmempool": 141.38
19    },
20    "fail": {
21      "startrange": 52040,
22      "endrange": 54641,
23      "withintarget": 959.04,
24      "totalconfirmed": 4799.12,
25      "inmempool": 0,
26      "leftmempool": 1430.93
27    }
28  }
29}
  7. laanwj commented at 1:00 pm on June 7, 2017: member
    Concept ACK
  8. instagibbs commented at 1:02 pm on June 7, 2017: member
    concept ACK as well.
  9. jlopp commented at 1:39 pm on June 7, 2017: contributor
    Concept ACK; this will reduce the number of RPC calls we need to make to get a full picture of the fee market.
  10. morcos force-pushed on Jun 14, 2017
  11. morcos commented at 7:27 pm on June 14, 2017: member
    rebased due to adjacent line change in src/rpc/client.cpp
  12. RHavar commented at 7:31 pm on June 14, 2017: contributor
    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
  13. laanwj commented at 11:02 am on June 26, 2017: member 
    0  "short": {
1    "feerate": -1
2  },

    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)

  14. sipa commented at 2:31 am on June 27, 2017: member

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

    ACK

  15. TheBlueMatt commented at 11:34 pm on June 27, 2017: member
    utACK 7780de8a0f333f9f168eeea2f2227597392034ff irrespective of if the “information absent” response is changed.
  16. morcos commented at 3:35 pm on June 28, 2017: member

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

    0morcos@queen:~/Projects/bitcoin2$ src/bitcoin-cli -regtest estimaterawfee 1009 
1error code: -8
2error message:
3Invalid 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):

     0morcos@queen:~/Projects/bitcoin2$ src/bitcoin-cli -regtest estimaterawfee 1 1
 1{
 2  "short": {
 3    "decay": 0.962,
 4    "scale": 1,
 5    "fail": {
 6      "startrange": 881683,
 7      "endrange": 1e+99,
 8      "withintarget": 13.45,
 9      "totalconfirmed": 14.96,
10      "inmempool": 0,
11      "leftmempool": 0
12    },
13    "errors": [
14      "Insufficient data or no feerate found which meets threshold"
15    ]
16  },
17  "medium": {
18    "feerate": 0.02210603,
19    "decay": 0.9952,
20    "scale": 2,
21    "pass": {
22      "startrange": 2121876,
23      "endrange": 3456312,
24      "withintarget": 40.6,
25      "totalconfirmed": 40.6,
26      "inmempool": 0,
27      "leftmempool": 0
28    },
29    "fail": {
30      "startrange": 1367780,
31      "endrange": 2121876,
32      "withintarget": 23.48,
33      "totalconfirmed": 23.48,
34      "inmempool": 0,
35      "leftmempool": 0
36    }
37  },
38  "long": {
39    "feerate": 0.00128368,
40    "decay": 0.99931,
41    "scale": 24,
42    "pass": {
43      "startrange": 125239,
44      "endrange": 131501,
45      "withintarget": 26652.56,
46      "totalconfirmed": 26652.56,
47      "inmempool": 0,
48      "leftmempool": 0
49    },
50    "fail": {
51      "startrange": 119276,
52      "endrange": 125239,
53      "withintarget": 76474.09,
54      "totalconfirmed": 76474.52,
55      "inmempool": 0,
56      "leftmempool": 0
57    }
58  }
59}

    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:

     0morcos@queen:~/Projects/bitcoin2$ src/bitcoin-cli -regtest estimaterawfee 122 0.0001
 1{
 2  "long": {
 3    "feerate": 0.00001000,
 4    "decay": 0.99931,
 5    "scale": 24,
 6    "pass": {
 7      "startrange": 0,
 8      "endrange": 1000,
 9      "withintarget": 168.05,
10      "totalconfirmed": 427.14,
11      "inmempool": 0,
12      "leftmempool": 21.99
13    }
14  }
15}

    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

     0morcos@queen:~/Projects/bitcoin2$ src/bitcoin-cli -regtest estimaterawfee 12
 1{
 2  "short": {
 3    "feerate": 0.00068495,
 4    "decay": 0.962,
 5    "scale": 1,
 6    "pass": {
 7      "startrange": 66417,
 8      "endrange": 69738,
 9      "withintarget": 346.39,
10      "totalconfirmed": 357.4,
11      "inmempool": 0,
12      "leftmempool": 0
13    },
14    "fail": {
15      "startrange": 63254,
16      "endrange": 66417,
17      "withintarget": 4365.32,
18      "totalconfirmed": 6062.21,
19      "inmempool": 0,
20      "leftmempool": 0
21    }
22  },
23  "medium": {
24    "decay": 0.9952,
25    "scale": 2,
26    "fail": {
27      "startrange": 0,
28      "endrange": 1e+99,
29      "withintarget": 262932.09,
30      "totalconfirmed": 301367.03,
31      "inmempool": 0,
32      "leftmempool": 3477.64
33    },
34    "errors": [
35      "Insufficient data or no feerate found which meets threshold"
36    ]
37  },
38  "long": {
39    "decay": 0.99931,
40    "scale": 24,
41    "fail": {
42      "startrange": 0,
43      "endrange": 1e+99,
44      "withintarget": 1852032.93,
45      "totalconfirmed": 1943598.4,
46      "inmempool": 0,
47      "leftmempool": 3236.77
48    },
49    "errors": [
50      "Insufficient data or no feerate found which meets threshold"
51    ]
52  }
53}
  17. laanwj added this to the "Blockers" column in a project
  18. in src/rpc/mining.cpp:879 in b63688146e outdated 
    875@@ -876,7 +876,7 @@ UniValue estimatesmartfee(const JSONRPCRequest& request)
876 
877 UniValue estimaterawfee(const JSONRPCRequest& request)
878 {
879-    if (request.fHelp || request.params.size() < 1|| request.params.size() > 3)
880+    if (request.fHelp || request.params.size() < 1|| request.params.size() > 2)
    promag commented at 9:49 pm on July 6, 2017:
    Missing space before second ||.
  19. in src/rpc/mining.cpp:959 in b63688146e outdated 
    1001+            failbucket.push_back(Pair("endrange", round(buckets.fail.end)));
1002+            failbucket.push_back(Pair("withintarget", round(buckets.fail.withinTarget * 100.0) / 100.0));
1003+            failbucket.push_back(Pair("totalconfirmed", round(buckets.fail.totalConfirmed * 100.0) / 100.0));
1004+            failbucket.push_back(Pair("inmempool", round(buckets.fail.inMempool * 100.0) / 100.0));
1005+            failbucket.push_back(Pair("leftmempool", round(buckets.fail.leftMempool * 100.0) / 100.0));
1006+            if (!(feeRate == CFeeRate(0))) {
    promag commented at 9:52 pm on July 6, 2017:
    Invert condition and switch blocks.
    morcos commented at 2:05 am on July 7, 2017:
    I like having the success condition be the first thing you read, but I’ll add a comment
  20. in src/rpc/mining.cpp:941 in b63688146e outdated 
    983+    const char* horizonNames[] = {"short", "medium", "long"};
984+
985+    for (FeeEstimateHorizon horizon : {FeeEstimateHorizon::SHORT_HALFLIFE, FeeEstimateHorizon::MED_HALFLIFE, FeeEstimateHorizon::LONG_HALFLIFE}) {
986+        CFeeRate feeRate;
987+        EstimationResult buckets;
988+        if ((unsigned int)nBlocks <= ::feeEstimator.HighestTargetTracked(horizon)) {
    promag commented at 9:56 pm on July 6, 2017:

    What about early continue; to avoid large indentation:

    0if ((unsigned int)nBlocks > ::feeEstimator.HighestTargetTracked(horizon)) continue;
  21. promag commented at 10:08 pm on July 6, 2017: member
    Partial review. Testing.
  22. morcos force-pushed on Jul 7, 2017
  23. morcos force-pushed on Jul 7, 2017
  24. morcos commented at 2:36 am on July 7, 2017: member
    Addressed nits and squashed (rawpi.ver2) -> 0220261 (rawapi.ver2.squash)
  25. in src/rpc/mining.cpp:907 in 022026122f outdated 
    915+            "          \"endrange\" : x.x,       (numeric) end of feerate range\n"
916+            "          \"withintarget\" : x.x,   (numeric) number of txs over history horizon in the feerate range that were confirmed within target\n"
917+            "          \"totalconfirmed\" : x.x, (numeric) number of txs over history horizon in the feerate range that were confirmed at any point\n"
918+            "          \"inmempool\" : x.x,      (numeric) current number of txs in mempool in the feerate range unconfirmed for at least target blocks\n"
919+            "          \"leftmempool\" : x.x,    (numeric) number of txs over history horizon in the feerate range that left mempool unconfirmed after target\n"
920+            "      }\n"
    promag commented at 9:09 am on July 7, 2017:
    Nit, missing comma here and in the following lines.
  26. in src/rpc/mining.cpp:881 in 022026122f outdated 
    875@@ -876,7 +876,7 @@ UniValue estimatesmartfee(const JSONRPCRequest& request)
876 
877 UniValue estimaterawfee(const JSONRPCRequest& request)
878 {
879-    if (request.fHelp || request.params.size() < 1|| request.params.size() > 3)
880+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
881         throw std::runtime_error(
882             "estimaterawfee nblocks (threshold horizon)\n"
    promag commented at 9:12 am on July 7, 2017:
    Nit, remove horizon.
  27. in src/rpc/mining.cpp:936 in 022026122f outdated 
    978-    failbucket.push_back(Pair("withintarget", round(buckets.fail.withinTarget * 100.0) / 100.0));
979-    failbucket.push_back(Pair("totalconfirmed", round(buckets.fail.totalConfirmed * 100.0) / 100.0));
980-    failbucket.push_back(Pair("inmempool", round(buckets.fail.inMempool * 100.0) / 100.0));
981-    failbucket.push_back(Pair("leftmempool", round(buckets.fail.leftMempool * 100.0) / 100.0));
982-    result.push_back(Pair("fail", failbucket));
983+    const char* horizonNames[] = {"short", "medium", "long"};
    promag commented at 9:15 am on July 7, 2017:
    Nit, rename horizon_names.
  28. in src/rpc/mining.cpp:964 in 022026122f outdated 
    1006+        failbucket.push_back(Pair("totalconfirmed", round(buckets.fail.totalConfirmed * 100.0) / 100.0));
1007+        failbucket.push_back(Pair("inmempool", round(buckets.fail.inMempool * 100.0) / 100.0));
1008+        failbucket.push_back(Pair("leftmempool", round(buckets.fail.leftMempool * 100.0) / 100.0));
1009+
1010+        // CFeeRate(0) is used to indicate error as a return value from estimateRawFee
1011+        if (!(feeRate == CFeeRate(0))) {
    promag commented at 9:18 am on July 7, 2017:
    Maybe for another PR, but IMO operator!= would be more expressive, no?
    morcos commented at 1:13 pm on July 7, 2017:
    Yeah I should have just added that operator before, since this has annoyed me a couple of times.
  29. in src/rpc/mining.cpp:946 in 022026122f outdated 
    988+
989+        // Only output results for horizons which track the target
990+        if ((unsigned int)nBlocks > ::feeEstimator.HighestTargetTracked(horizon)) continue;
991+
992+        feeRate = ::feeEstimator.estimateRawFee(nBlocks, threshold, horizon, &buckets);
993+        UniValue horizonresult(UniValue::VOBJ);
    promag commented at 9:19 am on July 7, 2017:
    Nit, snake case these multi word variables?
    morcos commented at 12:41 pm on July 7, 2017:
    i stuck with fixing the new ones
  30. in src/rpc/mining.cpp:911 in 022026122f outdated 
    993+        UniValue horizonresult(UniValue::VOBJ);
994+        UniValue errors(UniValue::VARR);
995+        UniValue passbucket(UniValue::VOBJ);
996+        passbucket.push_back(Pair("startrange", round(buckets.pass.start)));
997+        passbucket.push_back(Pair("endrange", round(buckets.pass.end)));
998+        passbucket.push_back(Pair("withintarget", round(buckets.pass.withinTarget * 100.0) / 100.0));
    promag commented at 9:26 am on July 7, 2017:
    Should we make these round with 2 decimal places as strings? See https://stackoverflow.com/a/1343925.
    morcos commented at 12:36 pm on July 7, 2017:
    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.
  31. morcos force-pushed on Jul 7, 2017
  32. morcos commented at 1:17 pm on July 7, 2017: member

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

    Thanks for review @promag !

  33. in src/policy/feerate.h:43 in 6dc14106a1 outdated 
    39@@ -40,6 +40,7 @@ class CFeeRate
40     friend bool operator==(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK == b.nSatoshisPerK; }
41     friend bool operator<=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK <= b.nSatoshisPerK; }
42     friend bool operator>=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK >= b.nSatoshisPerK; }
43+    friend bool operator!=(const CFeeRate& a, const CFeeRate& b) { return a.nSatoshisPerK != b.nSatoshisPerK; }
    promag commented at 2:17 pm on July 7, 2017:
    Nit nit, place after operator==.
    morcos commented at 4:05 pm on July 7, 2017:
    meh
  34. in src/policy/fees.h:220 in 6dc14106a1 outdated 
    189@@ -190,6 +190,9 @@ class CBlockPolicyEstimator
190     /** Empty mempool transactions on shutdown to record failure to confirm for txs still in mempool */
191     void FlushUnconfirmed(CTxMemPool& pool);
192 
193+    /** Calculation of highest target that estimates are tracked for */
194+    unsigned int HighestTargetTracked(FeeEstimateHorizon horizon) const;
    promag commented at 2:24 pm on July 7, 2017:
    Since nBlocks and also CBlockIndex::nHeight are int, why not return int here too?
    morcos commented at 4:02 pm on July 7, 2017:
    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 commented at 4:04 pm on July 7, 2017:
    No problem, uint32_t then?
    morcos commented at 5:02 pm on July 7, 2017:
    any reason?
    promag commented at 5:15 pm on July 7, 2017:
    Best practice? Not network code but still. See https://stackoverflow.com/a/21621533.
  35. in src/rpc/mining.cpp:908 in 6dc14106a1 outdated 
    991+        if ((unsigned int)nBlocks > ::feeEstimator.HighestTargetTracked(horizon)) continue;
992+
993+        feeRate = ::feeEstimator.estimateRawFee(nBlocks, threshold, horizon, &buckets);
994+        UniValue horizon_result(UniValue::VOBJ);
995+        UniValue errors(UniValue::VARR);
996+        UniValue passbucket(UniValue::VOBJ);
    promag commented at 2:28 pm on July 7, 2017:
    My suggestion was to snake case feeRate, passbucket and failbucket.
    morcos commented at 4:04 pm on July 7, 2017:
    yeah i just fixed variables introduced in this PR, instead of changing preexisting variables
    promag commented at 4:05 pm on July 7, 2017:
    Touched code should be fixed too?
    morcos commented at 5:02 pm on July 7, 2017:
    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.
  36. in src/rpc/mining.cpp:937 in 6dc14106a1 outdated 
    1020+            // Output only information that is still meaningful in the event of error
1021+            horizon_result.push_back(Pair("decay", buckets.decay));
1022+            horizon_result.push_back(Pair("scale", (int)buckets.scale));
1023+            horizon_result.push_back(Pair("fail", failbucket));
1024+            errors.push_back("Insufficient data or no feerate found which meets threshold");
1025+            horizon_result.push_back(Pair("errors",errors));
    promag commented at 2:28 pm on July 7, 2017:
    Nit, missing space before errors.
    morcos commented at 4:05 pm on July 7, 2017:
    will fix if there are any more changes, but leave for now to avoid churn.
  37. in src/rpc/mining.cpp:903 in 6dc14106a1 outdated 
    986+    for (FeeEstimateHorizon horizon : {FeeEstimateHorizon::SHORT_HALFLIFE, FeeEstimateHorizon::MED_HALFLIFE, FeeEstimateHorizon::LONG_HALFLIFE}) {
987+        CFeeRate feeRate;
988+        EstimationResult buckets;
989+
990+        // Only output results for horizons which track the target
991+        if ((unsigned int)nBlocks > ::feeEstimator.HighestTargetTracked(horizon)) continue;
    promag commented at 2:38 pm on July 7, 2017:

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

    0[{ "horizon": "short", "feerate": 0.00068495, ... }]
    morcos commented at 4:04 pm on July 7, 2017:
    Yes it’s omitted because it is not meaningful to return an answer for a target higher than the horizon tracks.
    promag commented at 4:07 pm on July 7, 2017:
    Hence the suggestion to return the array, a client iterates each element (if any) and uses the horizon value.
    morcos commented at 5:01 pm on July 7, 2017:
    Oh sorry, I misunderstood at first. I suppose I’m not familiar enough with JSON practices to know what would be preferred.
  39. TheBlueMatt commented at 9:56 pm on July 7, 2017: member
    re-utACK 6dc14106a14ebd91a740db925e0bef59ef6233f4
  40. in src/rpc/mining.cpp:958 in 069f575331 outdated 
     999+            failbucket.push_back(Pair("totalconfirmed", round(buckets.fail.totalConfirmed * 100.0) / 100.0));
1000+            failbucket.push_back(Pair("inmempool", round(buckets.fail.inMempool * 100.0) / 100.0));
1001+            failbucket.push_back(Pair("leftmempool", round(buckets.fail.leftMempool * 100.0) / 100.0));
1002+            horizon_result.push_back(Pair("fail", failbucket));
1003+        }
1004+        result.push_back(Pair(horizon_names[horizon], horizon_result));
    ryanofsky commented at 8:13 pm on July 10, 2017:

    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.

  41. in src/policy/fees.cpp:686 in b0d0c83e3f outdated 
    681+    }
682+    case FeeEstimateHorizon::LONG_HALFLIFE: {
683+        return longStats->GetMaxConfirms();
684+    }
685+    default: {
686+        return 0;
    ryanofsky commented at 8:21 pm on July 10, 2017:

    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.

  42. ryanofsky commented at 8:31 pm on July 10, 2017: member
    utACK 6dc14106a14ebd91a740db925e0bef59ef6233f4. Left comments but please ignore any suggestions you think are not worth implementing here.
  43. morcos force-pushed on Jul 11, 2017
  44. Change API to estimaterawfee 
    Report results for all 3 possible time horizons instead of specifying time horizon as an argument.
    9c85b91dc1
  45. Add function to report highest estimate target tracked per horizon 1fafd704da
  46. Improve error reporting for estimaterawfee 5e3b7b5686
  47. morcos force-pushed on Jul 11, 2017
  48. morcos commented at 0:10 am on July 11, 2017: member

    Addressed comments

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

  49. laanwj merged this on Jul 11, 2017
  50. laanwj closed this on Jul 11, 2017
  51. laanwj referenced this in commit b27b004532 on Jul 11, 2017
  52. ryanofsky commented at 3:30 pm on July 11, 2017: member
    utACK 5e3b7b5686d326814c40e5b70da6697971d27092. Same as previous review with the two suggested changes.
  53. laanwj removed this from the "Blockers" column in a project
  54. sipa referenced this in commit 75b5643c47 on Jul 17, 2017
  55. PastaPastaPasta referenced this in commit efc0afe04e on Aug 6, 2019
  56. PastaPastaPasta referenced this in commit 658dea55a1 on Aug 6, 2019
  57. PastaPastaPasta referenced this in commit 62a9405dfb on Aug 6, 2019
  58. PastaPastaPasta referenced this in commit 70c9ffc0c1 on Aug 7, 2019
  59. PastaPastaPasta referenced this in commit f5db1d9cfc on Aug 8, 2019
  60. PastaPastaPasta referenced this in commit e789608728 on Aug 12, 2019
  61. PastaPastaPasta referenced this in commit ee4f4d343b on Sep 16, 2019
  62. PastaPastaPasta referenced this in commit d04633d28c on Sep 18, 2019
  63. barrystyle referenced this in commit a3334d0f13 on Jan 22, 2020
  64. barrystyle referenced this in commit 0c317684a0 on Jan 22, 2020
  65. DrahtBot locked this on Sep 8, 2021


morcos RHavar sipa laanwj instagibbs jlopp TheBlueMatt promag ryanofsky

Labels
RPC/REST/ZMQ

Milestone
0.15.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me