Currently, the medianfeerate statistic is calculated from the feerate of the middle transaction of a list of transactions sorted by feerate.
This PR instead uses the value of the 50th percentile weight unit in the block, and also calculates the feerate at the 10th, 25th, 75th, and 90th percentiles. This more accurately corresponds with what is generally meant by median feerate.
Concept ACK. The feerate is a measure on weight unit, not discrete transaction, so the median feerate should me the middle weight unit of the block.
Conceptually, this gives a better representation of the fees paid in the block, since the measure is not skewed by small or large txs.
Will review tomorrow.
utACK 2d4bf95973bb5224238762ce94e3ecb835b77280
Imo this is a bugfix for 0.17, but not a blocker for rc1.
1658 | + }; 1659 | + int64_t next_percentile_index = 0; 1660 | + int64_t cumulative_weight = 0; 1661 | + for (const auto& element: scores) { 1662 | + cumulative_weight += element.second; 1663 | + while (cumulative_weight >= weights[next_percentile_index]) {
This should be while (next_percentile_index < NUM_GETBLOCKSTATS_PERCENTILS && cumulative_weight >= weights[next_percentile_index]) {, no?
Yes, thanks for catching that.
1709 | @@ -1679,7 +1710,13 @@ static UniValue getblockstats(const JSONRPCRequest& request) 1710 | " \"maxfeerate\": xxxxx, (numeric) Maximum feerate (in satoshis per virtual byte)\n" 1711 | " \"maxtxsize\": xxxxx, (numeric) Maximum transaction size\n" 1712 | " \"medianfee\": xxxxx, (numeric) Truncated median fee in the block\n" 1713 | - " \"medianfeerate\": xxxxx, (numeric) Truncated median feerate (in satoshis per virtual byte)\n" 1714 | + " \"feerate_percentiles\": [ (array of numeric) Feerates at the 10th, 25th, 50th, 75th, and 90th percentile weight unit (in satoshis per virtual byte)\n"
Nit: indentation is off for (array of numeric) [...] -- see above line.
nit: the other result keys are dictionary sorted in the help text. I suggest you follow the same convention here.
utACK 1fd565be86e3727e8dabcb961f9a24c18bddd2a8
1644 | + 1645 | +template<typename T> 1646 | +static void CalculatePercentilesByWeight(T result[NUM_GETBLOCKSTATS_PERCENTILES], std::vector<std::pair<T, int64_t>>& scores, int64_t total_weight) 1647 | +{ 1648 | + const size_t size = scores.size(); 1649 | + if (size == 0) {
You don't use size anywhere else below - so you could just do:
if (scores.empty()) {
return;
}:+1:
1656 | + const double weights[NUM_GETBLOCKSTATS_PERCENTILES] = { 1657 | + total_weight / 10.0, total_weight / 4.0, total_weight / 2.0, (total_weight * 3.0) / 4.0, (total_weight * 9.0) / 10.0 1658 | + }; 1659 | + int64_t next_percentile_index = 0; 1660 | + int64_t cumulative_weight = 0; 1661 | + for (const auto& element: scores) {
Nit: I believe there should be spaces on both sides of :.
1658 | + }; 1659 | + int64_t next_percentile_index = 0; 1660 | + int64_t cumulative_weight = 0; 1661 | + for (const auto& element: scores) { 1662 | + cumulative_weight += element.second; 1663 | + while (next_percentile_index < NUM_GETBLOCKSTATS_PERCENTILES && cumulative_weight >= weights[next_percentile_index]) {
This logic seems correct, but would be nice to have some test cases to ensure expected behavior for cases like:
Agree that getblockstats could do with some more thorough testing, but this PR doesn't make things worse, so I'd be inclined to say leave it for a future PR.
utACK 1fd565be86e3727e8dabcb961f9a24c18bddd2a8
1639 | @@ -1640,6 +1640,37 @@ static T CalculateTruncatedMedian(std::vector<T>& scores) 1640 | } 1641 | } 1642 | 1643 | +static constexpr int NUM_GETBLOCKSTATS_PERCENTILES = 5;
nit: I think it's better to place constexprs at the top of the file (especially since this is used in multiple functions in this file)
1639 | @@ -1640,6 +1640,37 @@ static T CalculateTruncatedMedian(std::vector<T>& scores) 1640 | } 1641 | } 1642 | 1643 | +static constexpr int NUM_GETBLOCKSTATS_PERCENTILES = 5; 1644 | + 1645 | +template<typename T>
nit: using a template here is over-engineering this. Even if you plan to re-use this a measure other than fee-rate, it would make more sense to define this just for fee-rate now and then change to a template in the next PR.
1664 | + result[next_percentile_index] = element.first; 1665 | + ++next_percentile_index; 1666 | + } 1667 | + } 1668 | + 1669 | + for (int64_t i = next_percentile_index; i < NUM_GETBLOCKSTATS_PERCENTILES; i++) {
nit: perhaps a comment here to say that you're filling in the remaining percentile results to the same as the last filled percentile result.
1906 | @@ -1867,7 +1907,13 @@ static UniValue getblockstats(const JSONRPCRequest& request) 1907 | ret_all.pushKV("maxfeerate", maxfeerate); 1908 | ret_all.pushKV("maxtxsize", maxtxsize); 1909 | ret_all.pushKV("medianfee", CalculateTruncatedMedian(fee_array)); 1910 | - ret_all.pushKV("medianfeerate", CalculateTruncatedMedian(feerate_array)); 1911 | + 1912 | + UniValue feerates_res(UniValue::VARR);
nit: I'd push this code up to immediately below the CalculatePercentilesByWeight() call so all the ret_all.pushKV() calls are on contiguous (alphabetized) lines.
185 | @@ -174,13 +186,19 @@ 186 | "avgfeerate": 109, 187 | "avgtxsize": 228, 188 | "blockhash": "22d9b8b9c2a37c81515f3fc84f7241f6c07dbcea85ef16b00bcc33ae400a030f", 189 | + "feerate_percentiles": [
nit: replace tab with spaces please :)
Code looks correct so utACK 1fd565be86e3727e8dabcb961f9a24c18bddd2a8
I've also got a bunch of nits for you :)
Thanks for the reviews. I fixed all the nits and added some unit tests for CalculatePercentilesByWeight
I'm not sure that I put the tests in the best location, and also this now fails on Travis because of linting
✗ bitcoin (change-feerates-rpc) ./test/lint/lint-includes.sh
The following files #include .cpp files:
src/test/rpc_tests.cpp:#include <rpc/blockchain.cpp>
15 | @@ -16,6 +16,8 @@ 16 | 17 | #include <univalue.h> 18 | 19 | +#include <rpc/blockchain.cpp>
Can't you just include the .h file?
If there are static methods that you need, remove the static in the cpp file and add the declaration here
Yup, that works thanks. I had to move some of the other includes but I think that should be okay.
1884 | @@ -1848,26 +1885,34 @@ static UniValue getblockstats(const JSONRPCRequest& request) 1885 | 1886 | // New feerate uses satoshis per virtual byte instead of per serialized byte 1887 | CAmount feerate = weight ? (txfee * WITNESS_SCALE_FACTOR) / weight : 0; 1888 | - if (do_medianfeerate) { 1889 | - feerate_array.push_back(feerate); 1890 | + if (do_feerate_percentiles) { 1891 | + feerate_array.emplace_back(std::make_pair(feerate, weight));
With emplace_back, I think you should be able to get rid of make_pair - std::pair has a constructor with first and second, so that should be called then.
So use std::pair<CAmount, int64_t>(feerate, weight) instead? Is there a reason the constructor is preferred?
Unless there is compelling reason to change the code, you should keep the it as is for now.
No, I meant to just use freerate_array.emplace_back(feerate, weight). I haven't tested it, but according to my understanding it should work - and just be simpler in the code. Whether that is a compelling reason or not, I don't know.
Agree with @domob1812 that
feerate_array.emplace_back(feerate, weight);
looks cleaner (and yes, it works).
utACK b4718d26ab785229ecfb7d89b593cb050d883fc3
<!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
utACK 64101856b4e9a110770e389d2a1cad6a5802321c
This breaks the RPC interface. Do we need to go through the rpc deprecation process, or should we should maintain a legacy medianfeerate field?
Ah, I didn't notice this was considered to be a bugfix.
Removes medianfeerate result from getblockstats.
Adds feerate_percentiles which give the feerate of the 10th, 25th, 50th,
75th, and 90th percentile weight unit in the block.
30 | @@ -32,7 +31,6 @@ 31 | #include <warnings.h> 32 | 33 | #include <assert.h> 34 | -#include <stdint.h>
nit: you should leave these includes
Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers. https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization
Fixed.
re-utACK 4b7091a842c2c3a76f4136cb0fdcf1c5904fd237
utACK 4b7091a842c2c3a76f4136cb0fdcf1c5904fd237
Cannot really test this without txindex enabled, utACK 4b7091a842c2c3a76f4136cb0fdcf1c5904fd237
concept ACK, I wish somebody had pinged me for review.
I think I would have liked more to keep them as separated fields (ie percentile_feerate10, percentile_feerate25, percentile_feerate50, percentile_feerate75, percentile_feerate90) instead of giving them as an array, but no big deal, I can just do that on the caller.