RPC: Introduce getblockstats to plot things

jtimon commented at 11:21 pm on July 6, 2017: contributor

It returns per block statistics about several things. It should be easy to add more if people think of other things to add or remove some if I went too far (but once written, why not keep it? EDIT: answer: not to test or maintain them).

The currently available options are: minfee,maxfee,totalfee,minfeerate,maxfeerate,avgfee,avgfeerate,txs,ins,outs (EDIT: see updated list in the rpc call documentation)

For the x axis, one can use height or block.nTime (I guess I could add mediantime if there’s interest [EDIT: nobody showed interest but I implemented mediantime nonetheless, in fact there’s no distinction between x or y axis anymore, that’s for the caller to judge]).

To calculate fees, -txindex is required.

jtimon force-pushed on Jul 7, 2017

fanquake added the label RPC/REST/ZMQ on Jul 7, 2017

clarkmoody commented at 3:03 pm on July 7, 2017: none

Some ideas for additions:

Non-fee total output amount
Coinbase reward
Money supply including this block
Transaction weight txweight (it can be derived from existing fields, however)

I would prefer to see both time and mediantime returned, since they are available.

Should we return non-independent fields, such as avgfee when also including totalfee and txs?

I find that for bitcoin-related data, the median is often more useful than the average of a distribution. Including medianweight, medianfee, medianfeerate, medianoutput etc would expose these useful quantities to the user.

jnewbery commented at 3:28 pm on July 7, 2017: member

but once written, why not keep it?

Because more code => more bugs and more maintenance effort. I prefer:

If it’s not really needed, why add it?

This is perhaps a nice-to-have, but since #8704, getblock can return all transactions in a block (without requiring txindex). Those can then be parsed and analysed offline.

Is there a compelling use-case I’m missing here? This seems like a feature only a small subset of users would be interested in, in which case an offline tools seems more appropriate.

Sorry - not meaning to be negative, but my default reaction to new RPCs/arguments tends towards NACK unless I can see a compelling and widespread use-case.

clarkmoody commented at 3:50 pm on July 7, 2017: none

This is perhaps a nice-to-have, but since #8704, getblock can return all transactions in a block (without requiring txindex). Those can then be parsed and analysed offline.

This code pulls each transaction input’s previous outpoint in order to compute transaction fees. Replicating that in RPC would require thousands of calls for most blocks.

jnewbery commented at 3:53 pm on July 7, 2017: member

This code pulls each transaction input’s previous outpoint

Ah yes, of course. ~Concept ACK~ in that case. Doing this with getblock / getrawtransaction is infeasible.

EDIT: I’m going to reverse myself again: I don’t think +700 lines is worthwhile for something with limited usage for most users. I’m -0 on this.

jtimon commented at 4:29 pm on July 7, 2017: contributor

Because more code => more bugs and more maintenance effort. I prefer:

Sure, but I mean, removing for example the avgfee or avgfeerate won’t safe much code or testing code, just a few lines. Forget I said this, if there’s specific functions to remove because nobody will want them, let’s remove those and focus on the ones people want. Adding specific things only a few people want can also happen in their own branches, so it’s no big deal.

The only use case is gather statistics, presumably to plot things, create charts. That is, at least, compelling to me, but I don’t think that will have widespread usage. I also don’t think all rpc calls have it. Is getchaintxstats, for example, a widespread use case?

If that’s enough reason not to merge this, it’s fine, I can maintain it as a separate branch that I periodically rebase, it is simple enough, so that won’t be a big deal. On the other hand, if I can get it reviewed and merged it’ll be less work for me in the long run and I also get the review.

Non-fee total output amount Coinbase reward

Sounds good.

Money supply including this block

Mhmm, it would be simpler to calculate here from start to end here than from genesis. But it’s pretty trivial to write a function in any language that returns the total supply for a given height without access to any historic data. Unless you are talking about discounting op_return outputs or something like that. I don’t think this is very interesting here. Perhaps that can be done in getchaintxstats ?

Transaction weight txweight (it can be derived from existing fields, however)

In fact I’m using weight for everything. I should s/size/weight/ and probably also show size separately. Maybe separate feerates in by weight and serialize size? I don’t know…

I would prefer to see both time and mediantime returned, since they are available.

Yeah, the mediantime takes a little bit longer to be calculated but not much and one can always disable anything. In fact, the height and time shouldn’t be treated in any special way for being “the x axis” and should be allowed to be disabled like the rest.

Should we return non-independent fields, such as avgfee when also including totalfee and txs?

This is a good question. This is mostly what I meant by “why not if it’s this easy?”. But yeah, I guess non-independent are good candidates to be removed.

re median: yeah, that sounds interesting too, good idea!

clarkmoody commented at 4:37 pm on July 7, 2017: none

Mhmm, it would be simpler to calculate here from start to end here than from genesis. But it’s pretty trivial to write a function in any language that returns the total supply for a given height without access to any historic data. Unless you are talking about discounting op_return outputs or something like that. I don’t think this is very interesting here. Perhaps that can be done in getchaintxstats ?

I was thinking of the more trivial version, rather than the supply - provably_unspendable version, so keeping that as external code makes more sense. Maintaining the sum of spendable outputs against block height is a much more ambitious idea, and it may make sense in the future. However, it is probably out of scope of this PR.

in src/rpc/blockchain.cpp:1637 in 53ea4dea6f outdated

1632+    return false;
1633+}
1634+
1635+UniValue getperblockstats(const JSONRPCRequest& request)
1636+{
1637+    std::string str_allowed_plot_values = "minfee,maxfee,totalfee,minfeerate,maxfeerate,avgfee,avgfeerate,txs,ins,outs";

promag commented at 10:37 pm on July 7, 2017:

Remove.

in src/rpc/blockchain.cpp:1638 in 53ea4dea6f outdated

1633+}
1634+
1635+UniValue getperblockstats(const JSONRPCRequest& request)
1636+{
1637+    std::string str_allowed_plot_values = "minfee,maxfee,totalfee,minfeerate,maxfeerate,avgfee,avgfeerate,txs,ins,outs";
1638+    std::vector<std::string> allowed_plot_values;

promag commented at 10:52 pm on July 7, 2017:

0std::set<std::string> allowed_plot_values = {"minfee", "maxfee", "..."};

in src/rpc/blockchain.cpp:1639 in 53ea4dea6f outdated

1634+
1635+UniValue getperblockstats(const JSONRPCRequest& request)
1636+{
1637+    std::string str_allowed_plot_values = "minfee,maxfee,totalfee,minfeerate,maxfeerate,avgfee,avgfeerate,txs,ins,outs";
1638+    std::vector<std::string> allowed_plot_values;
1639+    boost::split(allowed_plot_values, str_allowed_plot_values, boost::is_any_of(","));

promag commented at 10:53 pm on July 7, 2017:

Remove.

in src/rpc/blockchain.cpp:1648 in 53ea4dea6f outdated

1643+            "getperblockstats ( nStart nEnd plotValues )\n"
1644+            "\nCompute per block statistics for a given window.\n"
1645+            "\nArguments:\n"
1646+            "1. \"start\"      (numeric, required) The height of the block that starts the window.\n"
1647+            "2. \"end\"        (numeric, optional) The height of the block that ends the window (default: current tip).\n"
1648+            "3. \"plotvalues\"  (string,  optional) Values to plot (comma separated), default(all): " + str_allowed_plot_values +

promag commented at 10:54 pm on July 7, 2017:

0"..." + boost::join(allowed_plot_values, ",")

in src/rpc/blockchain.cpp:1869 in 53ea4dea6f outdated

1646+            "1. \"start\"      (numeric, required) The height of the block that starts the window.\n"
1647+            "2. \"end\"        (numeric, optional) The height of the block that ends the window (default: current tip).\n"
1648+            "3. \"plotvalues\"  (string,  optional) Values to plot (comma separated), default(all): " + str_allowed_plot_values +
1649+            "\nResult:\n"
1650+            "{\n"
1651+            "}\n"

promag commented at 10:55 pm on July 7, 2017:

Missing result example.

in src/rpc/blockchain.cpp:1654 in 53ea4dea6f outdated

1649+            "\nResult:\n"
1650+            "{\n"
1651+            "}\n"
1652+            "\nExamples:\n"
1653+            + HelpExampleCli("getperblockstats", "1000 1000 \"minfeerate,avgfeerate\"")
1654+            + HelpExampleRpc("getperblockstats", "1000 1000 \"maxfeerate,avgfeerate\"")

promag commented at 10:55 pm on July 7, 2017:

Duplicate.

jtimon commented at 0:08 am on July 8, 2017:

One is HelpExampleCli and the other is HelpExampleRpc

promag commented at 0:16 am on July 8, 2017:

Ops sorry, overlooked it.

in src/rpc/blockchain.cpp:1678 in 53ea4dea6f outdated

1673+
1674+    std::string str_plot_values = str_allowed_plot_values;
1675+    if (request.params.size() > 2) {
1676+        str_plot_values = request.params[2].get_str();
1677+    }
1678+    std::vector<std::string> plot_values;

promag commented at 11:00 pm on July 7, 2017:

0std::set<std::string> plot_values;
1if (request.params.size() > 2) {
2  boost::split(plot_values, request.params[2].get_str(), boost::is_any_of(","));
3
4  // only validate in this case
5  // ... 
6} else {
7  plot_values = allowed_plot_values;
8}

in src/rpc/blockchain.cpp:1625 in 53ea4dea6f outdated

1620+            map_stats[plot_value].push_back(outputs);
1621+        }
1622+    }
1623+}
1624+
1625+static bool IsAllowedPlotValue(const std::string& plot_value, std::vector<std::string>& allowed_plot_values)

promag commented at 11:03 pm on July 7, 2017:

Remove.

in src/rpc/blockchain.cpp:1681 in 53ea4dea6f outdated

1676+        str_plot_values = request.params[2].get_str();
1677+    }
1678+    std::vector<std::string> plot_values;
1679+    boost::split(plot_values, str_plot_values, boost::is_any_of(","));
1680+    for (const std::string plot_value : plot_values) {
1681+        if (!IsAllowedPlotValue(plot_value, allowed_plot_values)) {

promag commented at 11:07 pm on July 7, 2017:

0if (allowed_plot_values.count(plot_value) == 0) {

jtimon commented at 0:57 am on July 8, 2017:

duh, I was so much over-complicating things so much for no good reason…thank you!

promag commented at 11:07 pm on July 7, 2017: member

Partial review, suggestion to use std::set.

Nit, rename allowed_plot_values to valid_plot_values. Nit, rename getperblockstats to just getblockstats?

jtimon force-pushed on Jul 8, 2017

jtimon commented at 7:27 am on July 8, 2017: contributor

Thanks again for the great feedback! @promag I think I solved all your nits except for #10757 (review) @clarkmoody I think I added most of your suggestions, explicitly excluding anything that involved accumulations neither from height=1 nor from height=start. The former potentially implies a world of complexity and the latter can be trivially calculated on the visual side: I would completely discard any accumulator redundancy in this rpc beforehand.

And for the rest of the redundancies, @jnewbery and @clarkmoody - thanks again for pointing it out -, it’s never too late to remove them before merging like a trivial squash and it’s never too soon to start saying which ones you would bikesay* out first. Also bikesay the names for the curves and even the order in the list (duplicated for c++ and python).

In the meantime, I embraced redundancy since, as said, it will be trivial for me to remove later. And also the pertinent optimizations to skip calculations when plot_values.count(“minfee”) == 0 or actually only when the extra calculation is more expensive than the searching in plot_values which is a set of strings.

For example, we have blockfees, reward, subsidy, complying with consensus rule reward == blockfees + subsidy. Only 2 of the 3 are necessary, at least one is redundant. My personal preference is removing either subsidy or reward or subsidy, but not blockfees. But at said once written there’s no problem with me in just making sure their tests don’t surprise me until we decide which ones didn’t deserve it.

Which one seems bikesaying in principle. But not in this case. blockfees/total_fees serves for other calculations like avgfeerate. Let’s not remove that one, just rename it.

But it is more interesting to propose new ones than to rename or vote for removal IMO. I believe the most interesting addition to this point was utxo_size_inc, which would welcomed some review from people who measures sizes more carefully like @sipa , since this doesn’t use GetSerializeSize for Coin intentionally, independently of the optimization to read Coin if available in the utxo before calling RpcGetTx. I’m still not sure what to do with pre/post segwit feerates, does anybody care about the pre ones? which one needs the scale factor? none?

REM CalculateTruncatedMedian doesn’t need to be a template at this point, but there’s no harm being static IMO

EDIT: still some TODOs, mostly documentation and pending decisions

promag commented at 7:36 am on July 8, 2017: member

@jtimon no problem. There are some nits to fix but I’ll review more in depth later.

in src/policy/feerate.cpp:23 in c316a9f069 outdated

19@@ -20,20 +20,22 @@ CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nBytes_)
20         nSatoshisPerK = 0;
21 }
22 
23-CAmount CFeeRate::GetFee(size_t nBytes_) const
24+CAmount CFeeRate::GetTruncatedFee(size_t nBytes_) const

promag commented at 0:11 am on July 9, 2017:

Drop _? Same below.

jtimon commented at 0:28 am on July 11, 2017:

Well, yes, conserving the old name only saves 1 line of extra disruption. But I guess if we’re touching the variable name we should use the new style. just bytes?

in src/rpc/blockchain.cpp:689 in c316a9f069 outdated

685@@ -685,6 +686,22 @@ UniValue getblockheader(const JSONRPCRequest& request)
686     return blockheaderToJSON(pblockindex);
687 }
688 
689+static void ReadBlockCheckPruned(const CBlockIndex* pblockindex, CBlock& block)

promag commented at 0:13 am on July 9, 2017:

Keep argument order as ReadBlockFromDisk? Is there a convention for where the output arguments should be?

jtimon commented at 5:56 pm on July 10, 2017:

Not that I know of, but your proposed change sounds good to me.

in src/rpc/blockchain.cpp:1556 in c316a9f069 outdated

1551+template<typename T>
1552+static T CalculateTruncatedMedian(std::vector<T>& scores)
1553+{
1554+    T median;
1555+    size_t size = scores.size();
1556+    std::sort(scores.begin(), scores.end());

promag commented at 0:19 am on July 9, 2017:

Nit, could sort only after size == 1 case.

in src/rpc/blockchain.cpp:1641 in c316a9f069 outdated

1565+    }
1566+    return median;
1567+}
1568+
1569+// outpoint (needed for the utxo index) + nHeight + fCoinBase
1570+static const size_t PER_UTXO_OVERHEAD = sizeof(COutPoint) + sizeof(uint32_t) + sizeof(bool);

promag commented at 3:34 pm on July 10, 2017:

static constexpr ...

jtimon commented at 5:58 pm on July 10, 2017:

what’s the gain? https://stackoverflow.com/a/41132221/935325 says it’s the same…

ryanofsky commented at 3:26 pm on October 31, 2017:

what’s the gain? https://stackoverflow.com/a/41132221/935325 says it’s the same…

Gain is just that constexpr is more descriptive, and that stackoverflow answer isn’t really correct. const and constexpr aren’t identical even for integers, for example const is valid here:

0static const int X = rand();

where constexpr would not be:

0static constexpr int X = rand();

Also that stackoverflow answer is relying on special case treatment for integers that doesn’t apply to other types like floats. constexpr is just better for declaring compile time constants that const, so we should prefer it.

in src/rpc/blockchain.cpp:1688 in c316a9f069 outdated

1590+
1591+    CBlock block;
1592+    ReadBlockCheckPruned(pindex, block);
1593+
1594+    for (const auto& tx : block.vtx) {
1595+

promag commented at 3:34 pm on July 10, 2017:

Remove empty line.

in src/rpc/blockchain.cpp:1784 in c316a9f069 outdated

1779+    std::map<std::string, UniValue> map_stats;
1780+    for (const std::string& plot_value : plot_values) {
1781+        map_stats[plot_value] = UniValue(UniValue::VARR);
1782+    }
1783+
1784+    CBlockIndex* pindex = chainActive[end];

promag commented at 3:51 pm on July 10, 2017:

Remove.

in src/rpc/blockchain.cpp:1786 in c316a9f069 outdated

1781+        map_stats[plot_value] = UniValue(UniValue::VARR);
1782+    }
1783+
1784+    CBlockIndex* pindex = chainActive[end];
1785+    for (int i = end; i >= start; i--) {
1786+        UpdateBlockStats(pindex, plot_values, map_stats);

promag commented at 3:52 pm on July 10, 2017:

0UpdateBlockStats(chainActive[i], ...);

jtimon commented at 6:00 pm on July 10, 2017:

This will be slightly less efficient, no?

promag commented at 10:48 am on July 11, 2017:

I guess it takes few more cycles but non critical code should be cleaner?

in src/rpc/blockchain.cpp:1737 in c316a9f069 outdated

1734+            "getblockstats ( nStart nEnd plotValues )\n"
1735+            "\nCompute per block statistics for a given window.\n"
1736+            "\nArguments:\n"
1737+            "1. \"start\"      (numeric, required) The height of the block that starts the window.\n"
1738+            "2. \"end\"        (numeric, optional) The height of the block that ends the window (default: current tip).\n"
1739+            "3. \"plotvalues\"  (string,  optional) Values to plot (comma separated), default(all): " + boost::join(valid_plot_values, ",") +

promag commented at 3:54 pm on July 10, 2017:

Replace plotvalues with stats? Also, 3rd argument could be object options?

jtimon commented at 6:03 pm on July 10, 2017:

Isn’t the string simpler?

promag commented at 10:46 am on July 11, 2017:

Ignore options suggestion.

in src/rpc/blockchain.cpp:1759 in c316a9f069 outdated

1754+    }
1755+
1756+    int end;
1757+    if (request.params.size() > 1) {
1758+        end = request.params[1].get_int();
1759+        if (end < 0 || end > chainActive.Height()) {

promag commented at 3:56 pm on July 10, 2017:

Nit, negative block could mean end = height - end to avoid early blocks (not new concept here I believe)?

jtimon commented at 6:03 pm on July 10, 2017:

Mhmm, interesting. To be clear you mean start=-10 end=200 would be equivalent to start=190 end=200, right?

promag commented at 10:45 am on July 11, 2017:

No, I meant negative values are relative to the tip. To get the stats for the last 10 blocks you would pass start = -10 without querying the current block height.

jtimon commented at 9:47 pm on July 11, 2017: contributor

Here are some images generated using this branch in combination with (WIP): https://github.com/jtimon/rpc-explorer

GUI detail:

screenshot_plotter

GUI detail zoom:

screenshot_plotter_zoom

Hide some:

plotter1

Hide more:

plotter2

Fees:

plotterfees

Utxo size increase:

plotter_utxo

jtimon renamed this:
~~RPC: Introduce getperblockstats to plot things~~
RPC: Introduce getblockstats to plot things
on Jul 11, 2017

jtimon force-pushed on Jul 12, 2017

jtimon commented at 5:54 am on July 12, 2017: contributor

Without the documentation for the result it was impossible to distinguish a weird choice to spring discussion from an implementation mistake. Removed the other TODO comments. Coded more pending suggestions by @promag (hopefully all pending ones? if not, please insist) with some extra bikeshedding derived from s/plotvalues/stats/ and adapt tests to start and end being allowed to be negative.

More cleanups can be done, specially in the tests if we go further with #10757 (review) and not calculate in inverse order (there’s no point if we don’t get the slight optimization).

jtimon force-pushed on Aug 24, 2017

jtimon commented at 8:27 pm on August 24, 2017: contributor

Needed rebase. If somebody made a web for it, it may be interesting to show number of segwit txs too http://segwit.5gbfree.com/countsegwit

forklol commented at 8:35 am on August 29, 2017: none

Just wanted to say that this would be massively helpful to track statistics. I hope this finds it’s way into a release soon.

jtimon force-pushed on Aug 31, 2017

jtimon commented at 6:57 am on August 31, 2017: contributor

Reversed the order of the values to the natural one, since as discussed the optimization of doing fetching the blocks in reverse order is not worth the loss in clarity of the code. Added segwit tx counter stat, and also the total size and weight for those txs (txs that at least have one sw input, txs sending to sw outputs don’t count).

trippysalmon commented at 3:29 pm on September 1, 2017: none

Perhaps a better name for <stat>_old is <stat>_virtual, _virt or _v. Or perhaps prepend it with “v” just like the tx size in the output of getrawtransaction (vsize).

For example:

avgfeerate_old becomes vavgfeerate maxfeerate_old becomes vmaxfeerate medianfeerate_old becomes vmedianfeerate

etc.

trippysalmon commented at 5:34 pm on September 3, 2017: none

I just finished calling getblockstats on every block in the chain and saving it into a database. I didn’t encounter any issues and the performance is quite good (100-1000ms per “full” block on an i7 6900k /w 32gb ram + nvme ssd).

Btw, if anyone is interested in the dataset I can share it. Just convo me at freenode irc (nick: “trippysalmon”). It includes some other stats as well, like rolling average hashrates.

in src/rpc/blockchain.cpp:1642 in b2d93e4dfa outdated

1631+        fee_array.push_back(txfee);
1632+        totalfee += txfee;
1633+        minfee = std::min(minfee, txfee);
1634+        maxfee = std::max(maxfee, txfee);
1635+
1636+        CAmount feerate_old = CFeeRate(txfee, tx_size).GetTruncatedFee(1);

sipa commented at 9:19 pm on September 3, 2017:

For feerate you should use the virtual size, not total size.

jtimon commented at 1:06 am on September 7, 2017:

feerate is using vsize, feerate_old is using old size. Perhaps we just want to remove the whole *_old family.

jlopp commented at 10:09 pm on September 3, 2017: contributor

Just noticed this PR; I’ll definitely be incorporating it into Statoshi once it’s merged! :+1:

jtimon commented at 1:19 am on September 7, 2017: contributor

@trippysalmon I think you mean replacing s/feerate/vfeerate/ and s/feerate_old/feerate/. As discussed with @sipa the *feerate_old are using old size and the *feerate ones are using vsize.

How much interest there is in maintaining the old ones? Pre-segwit both are identical and post segwit the old ones mean the how high the feerate would have been for mempool and mining purposes if you weren’t using segwit. For example, the same tx can have feerate 4 sat/vbyte (new) and 2 sat/byte (old), meaning for the same fee, your tx gets propagated/mined as if you had paid twice as much thanks to segwit’s discount.

If there’s not much interest, perhaps it’s just better to just remove all the old ones. By the way, I said before that we hold on removing redundant or uninteresting stats. I would start with reward, which the caller can calculate by simply adding subsidy and totalfees.

Needs rebase.

jtimon force-pushed on Sep 7, 2017

jtimon commented at 6:22 am on September 7, 2017: contributor

Rebased

trippysalmon commented at 3:10 pm on September 7, 2017: none

@jtimon ah yes, I got it now. I misinterpreted the meaning of the _old statistics.

In that case the _old statistics are indeed not that interesting. I would however like to see a pre segwit total_size statistic. That one is currently missing and it’s kind of a hassle to calculate it through RPC calls.

I will update my pre segwit total_vsize PR soon.

jtimon commented at 1:53 pm on September 8, 2017: contributor

@trippysalmon not sure what you mean by “pre segwit total size”. Total size is included, but size it’s size post and pre segwit. You mean you want a stat for total vsize ? Not sure that’s very interesting…

trippysalmon commented at 3:54 pm on September 8, 2017: none

@jtimon yes I’m talking about total_vsize. Perhaps it depends on my particular use-case (comparing and graphing total_size vs total_vsize). My reasoning is that it is easy to add and compute inside core but not so much through RPC calls.

If you think total_vsize is not useful/interesting to others I don’t mind maintaining it in a custom patch. I’m actually already using it in a project atm.

edit: never mind, total_vsize can be calculated by total_weight / 4

jtimon commented at 11:52 pm on September 8, 2017: contributor

Removed reward and *feerate_old as discussed, but didn’t squashed just in case. Discussing with @trippysalmon we thought could show vsize instead of weight, but the former is just the latter / 4 (ie WITNESS_SCALE_FACTOR) and presenting the weight we can completely forget about rounding concerns (plus that’s what getblock presents too, perhaps that’s a reason to exclude it here).

jtimon commented at 3:23 pm on October 5, 2017: contributor

Using this branch more, at first a range of heights was convenient but I was ready to wait even if I had to call this rpc block by block. Right now, that’s what I’m doing since once you start caching, calling this function is never a success but a cache failure. It is completely understandable that one doesn’t want to manage failure ranges when failures can be discovered individually and thus processed in parallel. That’s probably opinionated, but perhaps others trying to use this got many errors forgetting “[0]” in some places.

Supporting ranges may be an optimization for users, but since I have been lucky enough to attract some potential users of this rpc call, I can ask: will anybody miss the height ranges?

Since I’m indexing my cache by block height and that’s inherently “reorg unfriendly” I mostly see 2 options going forward:

Subscribe to the zmq interface, detect reorgs and remove block stats above the reorg height (while at it, one can put new blocks in the cache preemtively even if nobody asked for them yet)
Stop indexing the block stat cache by height, do it by block hash. This is compatible with reorgs (more space but less disk writing and it’s also required for advanced features like plotting abandoned/orphan chains).

I am inclined to trying both as a user since I want to subscribe to blocks but I also don’t want to erase blocks I have seen and don’t want to lose the option to chart reorged chains.

A second question is simply, assuming the height ranges are removed, would anyone else apart from me use the option to search by single block hash instead of single height?

Anyway, that would be my preference: replace height ranges with single height or single block hash (both options). Please let me know what you think.

trippysalmon commented at 8:00 pm on October 13, 2017: none

I use this RPC call extensively (and persist the results in a DB) and never used the block height range, only single blocks.

Anyway, that would be my preference: replace height ranges with single height or single block height (both options).

I think you meant block hash as the second option? For my use-case it doesn’t really matter if I have to use the block height or block hash. Either option works for me.

–

Also, something that I needed today was the median transaction size. I don’t think it can be calculated using the existing statistics or am I wrong?

It’s only a minor change and could be interesting to track over time to see it change when more complex scripts are made available.

jtimon force-pushed on Oct 17, 2017

jtimon commented at 12:15 pm on October 17, 2017: contributor

Rebased, squashed, added “mintxsize”, “maxtxsize”, “mediantxsize” and “avgtxsize” stats.

Regarding the ranges, thanks for the feedback, I’ll wait to hear from other people.

EDIT: btw, some of the tests are commented because tx sizes don’t seem to be deterministic, I suspect due to coin selection. Not sure what to do about that.

in src/rpc/blockchain.cpp:1621 in caeaf86de4 outdated

1570@@ -1564,6 +1571,309 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
1571     return ret;
1572 }
1573 
1574+static void RpcGetTx(const uint256& hash, CTransactionRef& tx_out)
1575+{
1576+    uint256 hashBlock;
1577+    if (!GetTransaction(hash, tx_out, Params().GetConsensus(), hashBlock, true)) {
1578+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string(fTxIndex ? "No such mempool or blockchain transaction"

ajtowns commented at 7:51 am on October 19, 2017:

Shouldn’t this be GetTransaction(…, false) ?

in src/rpc/blockchain.cpp:1689 in 2497afa411 outdated

1684+        } else if (stat == "time") {
1685+            map_stats[stat].push_back(pindex->GetBlockTime());
1686+        } else if (stat == "mediantime") {
1687+            map_stats[stat].push_back(pindex->GetMedianTimePast());
1688+        } else if (stat == "subsidy") {
1689+            map_stats[stat].push_back(GetBlockSubsidy(pindex->nHeight, Params().GetConsensus()));

ajtowns commented at 10:21 am on October 19, 2017:

I think the values, fees and fee rates should be wrapped with ValueFromAmount, so outputs are decimal bitcoins rather than integer satoshis, for consistency with other RPC calls.

jtimon commented at 2:33 pm on October 21, 2017:

No, we want to allow the slow fetch too. If the data is calculable we want to provide it no matter if more slowly. This rpc call shouldn’t be expected to be fast anyway.

ajtowns commented at 12:24 pm on October 24, 2017:

I’m presuming this reply should be to the “GetTransaction(…, false)” comment. Getting fee info without txindex seems like a weird thing to do to me, but if it’s supported then presumably the getblockstats RPC help text doesn’t need to still say “It won’t work .. without -txindex.” ?

jtimon commented at 1:04 pm on October 24, 2017:

Yeah, sorry. Was meant for the other comment. Actually looking at the code again the slow search will only find txs in the utxo, so using false should be fine. I’ll test it with false.

Regarding using BTC instead of satoshis, I think we want to move everything to satoshis but we don’t do it because it would be too disruptive, so we only do it with new calls. The caller can trivially divide by 100000000 to get BTC if they want t show that.

in test/functional/getblockstats.py:84 in caeaf86de4 outdated

79+
80+        assert_equal(stats['height'][0], start_height)
81+        assert_equal(stats['height'][max_stat_pos], start_height + max_stat_pos)
82+
83+        assert_equal(stats['txs'][0], 1)
84+        assert_equal(stats['swtxs'][0], 0)

ajtowns commented at 10:24 am on October 19, 2017:

None of the transactions have witness data, so this isn’t actually checking whether swtxs, swtotal_size, swtotal_weight ever output anything other than 0, or whether weight is ever anything other than size*4.

jtimon commented at 2:37 pm on October 21, 2017:

This is true. When I wrote this segwit wasn’t activated, but now it would be nice to add segwit txs to the test because otherwise this functionality isn’t really being tested. Good call.

ajtowns commented at 10:30 am on October 19, 2017: member

Suggestions at https://github.com/jtimon/bitcoin/pull/10

I think requesting by height/blockhash makes more sense than a height range. (I was expecting a range to give me aggregate statistics over the entire range before looking into it)

jtimon force-pushed on Oct 27, 2017

jtimon force-pushed on Oct 28, 2017

in src/policy/feerate.cpp:23 in ced73054c5 outdated

19@@ -20,20 +20,22 @@ CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nBytes_)
20         nSatoshisPerK = 0;
21 }
22 
23-CAmount CFeeRate::GetFee(size_t nBytes_) const
24+CAmount CFeeRate::GetTruncatedFee(size_t bytes) const

ryanofsky commented at 2:56 pm on October 31, 2017:

In commit “Avoid special case for truncated zeros with new CFeeRate::GetTruncatedFee”:

I found this commit message hard to understand. Would suggest something more like:

[refactoring] Add new CFeeRate::GetTruncatedFee method

Add new truncated fee method that unlike the CFeeRate::GetFee will round fees between 1 and -1 satoshi to zero instead of 1 or -1. This does not change the behavior of CFeeRate::GetFee.

in src/rpc/blockchain.cpp:691 in d8b8582496 outdated

687@@ -688,6 +688,22 @@ UniValue getblockheader(const JSONRPCRequest& request)
688     return blockheaderToJSON(pblockindex);
689 }
690 
691+static void ReadBlockCheckPruned(CBlock& block, const CBlockIndex* pblockindex)

ryanofsky commented at 3:05 pm on October 31, 2017:

In commit “RPC: Separate ReadBlockCheckPruned() from getblock()”

Since this is throwing on error it seems like it would be friendlier to just return the CBlock instead of taking it as output parameter.

0CBlock ReadBlockChecked(const BlockIndex*)

Also it would be good if commit message mentioned this is a refactoring and not a change in behavior.

in src/rpc/blockchain.cpp:1574 in f55badc57a outdated

1570@@ -1570,6 +1571,286 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
1571     return ret;
1572 }
1573 
1574+static void RpcGetTx(const uint256& hash, CTransactionRef& tx_out)

ryanofsky commented at 3:12 pm on October 31, 2017:

In commit “RPC: Introduce getblockstats to plot things”

Maybe call this “GetTransactionChecked” instead of “RpcGetTx” to be consistent with “ReadBlockChecked” in previous commit.

Also again since this is throwing on error this would be simpler to use if it just returned CTransactionRef instead of using an output parameter.

in src/rpc/blockchain.cpp:1589 in f55badc57a outdated

1584+static T CalculateTruncatedMedian(std::vector<T>& scores)
1585+{
1586+    size_t size = scores.size();
1587+    if (size == 0) {
1588+        return 0;
1589+    } if (size == 1) {

ryanofsky commented at 3:13 pm on October 31, 2017:

In commit “RPC: Introduce getblockstats”

If statement should be on new line, or preceded by else, or just dropped since the code below will handle this case anyway.

in src/rpc/blockchain.cpp:1895 in f55badc57a outdated

1831+    if (request.params.size() > 1) {
1832+        boost::split(stats, request.params[1].get_str(), boost::is_any_of(","));
1833+
1834+        for (const std::string& stat : stats) {
1835+            if (valid_stats.count(stat) == 0) {
1836+                throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid selected statistic %s", stat));

ryanofsky commented at 3:36 pm on October 31, 2017:

In commit “RPC: Introduce getblockstats”

Maybe s/selected/requested/ since “selected” sounds more like something the RPC is computing.

in src/rpc/blockchain.cpp:1604 in f55badc57a outdated

1599+}
1600+
1601+// outpoint (needed for the utxo index) + nHeight + fCoinBase
1602+static const size_t PER_UTXO_OVERHEAD = sizeof(COutPoint) + sizeof(uint32_t) + sizeof(bool);
1603+
1604+static void UpdateBlockStats(const CBlockIndex* pindex, std::set<std::string>& stats, std::map<std::string, UniValue>& map_stats)

ryanofsky commented at 3:38 pm on October 31, 2017:

In commit “RPC: Introduce getblockstats”

stats reference should be const, and again I think map_stats should be a return value instead of an output parameter so calling this function is simpler and less error prone.

Also ‘UpdateBlockStats’ should probably be called ‘GetBlockStats’ because ‘UpdateBlockStats’ sounds like something that would be called repeatedly to update existing statistics, where this function is just computing and returning wholly new statistics.

jtimon commented at 9:47 pm on January 15, 2018:

Yeah, it made sense to be update when the call did a loop from start height to end height, but not now that it calculates stats for a single block.

in src/rpc/blockchain.cpp:1794 in f55badc57a outdated

1739+    }
1740+}
1741+
1742+UniValue getblockstats(const JSONRPCRequest& request)
1743+{
1744+    std::set<std::string> valid_stats = {

ryanofsky commented at 3:49 pm on October 31, 2017:

In commit “RPC: Introduce getblockstats”

It seems clumsy and unnecessary to have to list these entries three separate places (here, in UpdateBlockStats, and in the getblockstats help string). It seems like it would be better to drop this listing and simplify the code by just building a fixed map:

0std::map<std::string, UniValue> GetBlockStats(CBlockIndex* block)
1{
2    ...
3    return {
4        {"height", (int64_t)pindex->nHeight},
5        {"time", pindex->GetBlockTime()},
6        ...
7    }
8}

If in the future there are statistics that are expensive to compute, it would be easy to add options to UpdateBlockStats to skip them.

jtimon commented at 9:42 pm on January 15, 2018:

Some statistics are already more expensive to compute than others, it seems like a good feature to allow the user to optionally skip some from the beginning.

jtimon commented at 0:40 am on January 16, 2018:

Oh, but it is currently calculating everything regardless of the stats requested…it wasn’t like that before, I should re-introduce the optimzations for the option to really make sense. Even if not all of them are expensive, perhaps one caller is only interested in one or a few stats and if the caller is going to make many calls for many heights, the performance hit of any undesired stat adds up.

jtimon commented at 7:46 pm on April 20, 2018:

I ended up taking the code suggestion, even though I’m not eliminating the “stats” argument. It is still much closer to what you wanted now.

in src/rpc/blockchain.cpp:1837 in f55badc57a outdated

1777+            "getblockstats ( nStart nEnd stats )\n"
1778+            "\nCompute per block statistics for a given window. All amounts are in satoshis.\n"
1779+            "\nIt won't work in some cases with pruning or without -txindex.\n"
1780+            "\nArguments:\n"
1781+            "1. \"height\"     (numeric, required) The height of the target block.Negative values count back from the current tip.\n"
1782+            "2. \"stats\"      (string,  optional) Values to plot (comma separated), default(all): " + boost::join(valid_stats, ",") +

ryanofsky commented at 3:55 pm on October 31, 2017:

In commit “RPC: Introduce getblockstats”

It seems weird for a JSON api to be taking a comma separated string instead of a list of strings. Do we have other RPCs that use comma separated strings?

jtimon commented at 9:39 pm on January 15, 2018:

I don’t know. I don’t mind moving to a list of strings if that’s preferred. Makes sense.

jimpo commented at 2:49 am on April 13, 2018:

Seems to be updated to an array, but comment is not updated.

jtimon commented at 6:17 pm on April 20, 2018:

Oops, github hid this reminder about the comment from me, fixing.

in src/rpc/blockchain.cpp:1824 in f55badc57a outdated

1819+    LOCK(cs_main);
1820+
1821+    int height = request.params[0].get_int();
1822+    int current_tip = chainActive.Height();
1823+    if (height < 0) {
1824+        height = current_tip + height;

ryanofsky commented at 4:21 pm on October 31, 2017:

In commit “RPC: Introduce getblockstats”:

Bug: This should be current_tip + height + 1 so height -1 will return tip.

in test/functional/getblockstats.py:17 in 44dcea038b outdated

12+    assert_raises_rpc_error,
13+)
14+
15+def assert_contains(data, values, check_cointains=True):
16+    for val in values:
17+        if (check_cointains):

ryanofsky commented at 4:26 pm on October 31, 2017:

In commit “QA: Test new getblockstats RPC”

Unneeded parentheses

in test/functional/getblockstats.py:18 in 44dcea038b outdated

13+)
14+
15+def assert_contains(data, values, check_cointains=True):
16+    for val in values:
17+        if (check_cointains):
18+            assert(val in data)

ryanofsky commented at 4:26 pm on October 31, 2017:

In commit “QA: Test new getblockstats RPC”

Unneeded parentheses (assert is not a function)

in test/functional/getblockstats.py:80 in 44dcea038b outdated

75+            "mediantxsize",
76+            "avgtxsize",
77+        ]
78+        assert_contains(stats[0], all_values)
79+        # Make sure all valid statistics are included
80+        assert_contains(all_values, stats[0].keys())

ryanofsky commented at 4:42 pm on October 31, 2017:

In commit “QA: Test new getblockstats RPC”

Seems like it would be simpler to combine the two asserts

0assert_equal(set(all_values), set(stats[0]))

in test/functional/getblockstats.py:44 in 44dcea038b outdated

39+        self.sync_all()
40+        node.generate(1)
41+
42+        start_height = 101
43+        max_stat_pos = 2
44+        stats = []

ryanofsky commented at 4:44 pm on October 31, 2017:

In commit “QA: Test new getblockstats RPC”

Could simplify with list comprehension:

0stats = [node.getblockstats(height=start_height + i) for i in range(max_stat_pos+1)]

in test/functional/getblockstats.py:176 in 44dcea038b outdated

171+        # Make sure we aren't always returning inv_sel_stat as the culprit stat
172+        assert_raises_rpc_error(-8, 'Invalid selected statistic aaa%s' % inv_sel_stat, node.getblockstats, height=1, stats='minfee,aaa%s' % inv_sel_stat)
173+
174+        # Make sure only the selected statistics are included
175+        stats = node.getblockstats(height=1, stats='minfee,maxfee')
176+        some_values = [

ryanofsky commented at 4:48 pm on October 31, 2017:

In commit “QA: Test new getblockstats RPC”

Again, seems like it would be simpler to just check for equality:

0stats = node.getblockstats(height=1, stats='minfee,maxfee')
1assert_equal(set(stats), {"minfee", "maxfee"})

ryanofsky commented at 5:00 pm on October 31, 2017: member

utACK 44dcea038b44890e5f49c71ff26cfcb4ecf0da15. Left some comments but all are minor (feel free to ignore them) except the height + 1 bug.

jtimon force-pushed on Jan 16, 2018

jtimon commented at 4:37 am on January 16, 2018: contributor

Fixed all nits by @ryanofsky except for #10757 (review) . Instead of solving it, I added a few new commits for people to discuss on.

The tests still can’t test size or feerates while passing/failing in a deterministic fashion, but just by rebasing now “swtxs” seems to be actually tested without me doing anything (probably by #11403 being merged), as requested in #10757 (review)

I thought of a simpler way to deterministacally get sizes and feerates: simply force the desired/expected size for each of the few txs this test creates. Using something like:

0while not desired_size(tx, desired_size)
1     tx = try_creating_tx_again()

or something of the sort.

I’ll try that next, but please comment on the latest commits.

jtimon force-pushed on Jan 16, 2018

jtimon commented at 9:59 pm on January 31, 2018: contributor

This needs rebase, but some feedback on the latest things and potential squashes would be nice before doing so.

ajtowns commented at 5:13 am on February 1, 2018: member

What do you think about using a static blockchain so that testing is deterministic? I added some code to save the generated chain, and to reuse it if it’s present and it doesn’t seem too bad – just using getblock(__,0) to get hex encoded blocks, and dumping them gives a 56kB file (or 10kB gzipped). That seems within the ballpark of something that could easily just be added as a static data file to the test suite?

I don’t think while tx_isnt_what_i_want: try_again() is a good approach – it seems a bit too easy to accidentally become while i_trigger_a_bug: try_again() which kind-of defeats the point of testing.

I’m not a huge fan of the for(stat:stats) { if (stat == "x") m[stat] = x(); else if ...; } approach. What about something along the lines of:

0    auto set_stat = [&](const char* stat, const UniValue val)
1                    { if (stats.count(stat) != 0) map_stats[stat] = val; };
2
3    set_stat("height", (int64_t)pindex->nHeight);
4    set_stat("time", pindex->GetBlockTime());
5    set_stat("mediantime", pindex->GetMedianTimePast());
6    ...

? I don’t think any of the calculations at that point are particularly heavy, so it shouldn’t make things much slower (and any that are could probably just be put in an if (is_loop_outputs_required) { .. } block or similar).

I think the optimisation commits make sense. Maybe add bools for medianfeerate, medianfee, mediantxsize so you’re not calling .count() inside the loops. The naming of the bools is annoyingly ungrammatical (“is_loop_inputs_required” should be “are_loop_inputs_required” in english), but I don’t have a better suggestion.

Might be good to explicitly say which stats want -txindex enabled and won’t work with pruning in the help (fee related and utxo size I think?) Could make the default be “whatever stats we can output efficiently” rather than “everything”, though not sure that’s a good idea.

jtimon commented at 6:29 pm on February 4, 2018: contributor

What do you think about using a static blockchain so that testing is deterministic? I added some code to save the generated chain, and to reuse it if it’s present and it doesn’t seem too bad – just using getblock(__,0) to get hex encoded blocks, and dumping them gives a 56kB file (or 10kB gzipped). That seems within the ballpark of something that could easily just be added as a static data file to the test suite?

I didn’t found the time to try that but if you can share that code that would be great. What you’re saying seems reasonable to me, I don’t know if other people would have a problem with that. And I cannot think of a simpler way to make this test deterministic.

I will look into the other suggestions, thanks.

jtimon commented at 9:35 am on March 22, 2018: contributor

@MarcoFalke what is the status of this in terms of testing?

I think @ajtowns is right that *size, and *feerate fields can only be deterministically tested, but I don’t have the time to write that at this point. I would be happy to review and test if anybody wants to take over though.

In the meantime, I would be happy to include the new call even if it’s without any stat that depends on signature size or coin selection for deterministic testing and those fields and their tests are already written.

jtimon force-pushed on Mar 30, 2018

jtimon commented at 4:07 am on March 30, 2018: contributor

Needed rebase. And still needs squashing pending on requested feedback.

Also, since testing some of the stats deterministically is not trivial, I think the new rpc call should be introduced only with the stats that can be deterministacally tested (even though we will miss some of the most interesting ones like sizes, fees and feerates) and then make the tests deterministic and add the missing stats. Thoughts?

jtimon force-pushed on Mar 30, 2018

ajtowns commented at 7:45 am on March 30, 2018: member

Proposed patch that makes testing of the stats kind of deterministic at https://github.com/jtimon/bitcoin/pull/11/commits

Idea is you do ./feature_rpc_getblockstats --gen-test-data to generate a test blockchain and the expected statistics for that blockchain, and will write it to data/rpc_getblockstats.json file (about 50kB uncompressed text). You then check the stats look reasonable, and commit that file to git, and then when you/travis runs the test case without –gen-test-data it will load the chain from the file and check that the calculated stats still match what was recorded as being expected.

If additional stats are added, you’ll need to edit the test case to update the EXPECTED_STATS variable; and you’ll need to add the expected values to the json file (by hand I guess), both of which should show up nicely in git diff.

jtimon force-pushed on Apr 7, 2018

jtimon commented at 0:41 am on April 7, 2018: contributor

Thanks a lot, incorporated those changes. Fixed some older nits too. Please re-review.

jtimon force-pushed on Apr 7, 2018

jtimon force-pushed on Apr 8, 2018

laanwj added this to the "Blockers" column in a project

in src/rpc/blockchain.cpp:1890 in cf6585a6ec outdated

1885+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d after current tip %d", height, current_tip));
1886+    }
1887+
1888+    std::set<std::string> stats;
1889+    if (request.params.size() > 1 && !request.params[1].isNull()) {
1890+        UniValue stats_univalue(UniValue::VARR);

jimpo commented at 2:47 am on April 13, 2018:

commit: RPC: Introduce getblockstats to plot things

nit: Copy initialization works here. UniValue stats_univalue = request.params[1].get_array();

in src/rpc/blockchain.cpp:1831 in cf6585a6ec outdated

1826+        "avgtxsize",
1827+    };
1828+
1829+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 4)
1830+        throw std::runtime_error(
1831+            "getblockstats ( nStart nEnd stats )\n"

jimpo commented at 2:54 am on April 13, 2018:

commit: RPC: Introduce getblockstats to plot things

The nStart and nEnd seem wrong. Should just be height now, no?

in src/rpc/blockchain.cpp:1909 in cf6585a6ec outdated

1904+    const CBlock block = GetBlockChecked(pblockindex);
1905+    std::map<std::string, UniValue> map_stats = GetBlockStatsMap(block, pblockindex, stats);
1906+
1907+    UniValue ret(UniValue::VOBJ);
1908+    for (const std::string stat : stats) {
1909+        ret.push_back(Pair(stat, map_stats[stat]));

jimpo commented at 2:57 am on April 13, 2018:

commit: RPC: Introduce getblockstats

I got confused by the push_back(Pair(...)) syntax. Can you just use pushKV(stat, map_stats[stat]) which seems to be more standard?

in src/rpc/blockchain.cpp:1707 in cf6585a6ec outdated

1702+
1703+        if (tx->IsCoinBase()) {
1704+            continue;
1705+        }
1706+
1707+        total_out += tx_total_out;

jimpo commented at 3:07 am on April 13, 2018:

commit: RPC: Introduce getblockstats

I had to check the RPC help doc to see if this was a mistake. Would be helpful to leave an explicit comment: // Don't count coinbase reward.

in src/rpc/blockchain.cpp:1621 in cf6585a6ec outdated

1612@@ -1612,6 +1613,304 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
1613     return ret;
1614 }
1615 
1616+static CTransactionRef GetTransactionChecked(const uint256& hash)
1617+{
1618+    CTransactionRef tx_out;
1619+    uint256 hashBlock;
1620+    if (!GetTransaction(hash, tx_out, Params().GetConsensus(), hashBlock, false)) {
1621+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string(fTxIndex ? "No such mempool or blockchain transaction"

jimpo commented at 3:45 am on April 16, 2018:

Surfacing this error seems like the the wrong error handling strategy. In the context this is used, the transaction should never be in the mempool and it should always be found if the txindex is present.

I think it would be better for the RPC to raise an RPC error that the txindex is required if it is not present, and an RPC error for an unexpected internal error (txindex is corrupt) otherwise.

in test/functional/rpc_getblockstats.py:19 in 69dd52d839 outdated

10+from test_framework.util import (
11+    assert_equal,
12+    assert_raises_rpc_error,
13+)
14+
15+def assert_contains(data, values, check_cointains=True):

jimpo commented at 3:53 am on April 16, 2018:

commit: Tests: Test new getblockstats RPC

typo: check_cointains

jimpo commented at 3:55 am on April 16, 2018:

commit: Tests: Test new getblockstats RPC

The assert_contains function appears unused.

promag commented at 5:58 pm on April 24, 2018:

Ping.

in test/functional/rpc_getblockstats.py:48 in 69dd52d839 outdated

51+        "avgtxsize",
52+    ]
53+
54+    def set_test_params(self):
55+        self.num_nodes = 2
56+        self.extra_args = [['-txindex'], ['-paytxfee=0.003']]

jimpo commented at 3:53 am on April 16, 2018:

commit: Tests: Test new getblockstats RPC

I think it’s worth testing the case of no txindex.

in test/functional/rpc_getblockstats.py:80 in bb4c8f9635 outdated

134+        stats = self.get_stats()
135 
136         # Make sure all valid statistics are included but nothing else is
137         assert_equal(set(stats[0].keys()), set(self.EXPECTED_STATS))
138 
139-        print(stats)

jimpo commented at 3:58 am on April 16, 2018:

commit: Tests: Save and load block and corresponding expected statistics

If all these test cases are getting removed anyway, I’d just drop them from the previous commit.

in test/functional/rpc_getblockstats.py:101 in bb4c8f9635 outdated

 99+            blkhash = node.getblockhash(height)
100+            blocks.append(node.getblock(blkhash, 0))
101+            height += 1
102+
103+        with open(filename, "w") as f:
104+            f.write("""{\n  "stats": """)

jimpo commented at 4:03 am on April 16, 2018:

commit: Tests: Save and load block and corresponding expected statistics

Instead of hand-crafting the JSON, I think it’s better to just create a dict with stats and blocks keys and json.dump that. It seems you might be doing this for better control over the indentation, but the data file added in the next commit has each stat on its own line.

ajtowns commented at 5:08 am on April 16, 2018:

It puts the stats on separate lines, but all the blocks on a single line. Seemed like a good idea at the time (multiline stats hand editable, block data as a minimal blob), but doesn’t make much difference.

jtimon force-pushed on Apr 19, 2018

jtimon commented at 10:22 am on April 19, 2018: contributor

Fixed @jimpo ’s nits, thanks.

in src/rpc/blockchain.cpp:1682 in 3699947a9e outdated

1677+
1678+    const bool do_calculate_size = do_mediantxsize || stats.count("total_size") != 0 || stats.count("avgtxsize") != 0 ||
1679+        stats.count("mintxsize") != 0 || stats.count("maxtxsize") != 0 || stats.count("swtotal_size") != 0;
1680+
1681+    const bool do_calculate_weight = stats.count("total_weight") != 0 || stats.count("avgfeerate") != 0 ||
1682+        stats.count("swtotal_weight") != 0 || stats.count("medianfeerate_weight") != 0;

ajtowns commented at 4:10 am on April 20, 2018:

Should be medianfeerate, not _weight.

in src/rpc/blockchain.cpp:1689 in 3699947a9e outdated

1684+    const bool do_calculate_sw = stats.count("swtxs") != 0 || stats.count("swtotal_size") != 0 ||
1685+        stats.count("swtotal_weight") != 0;
1686+
1687+    const bool loop_inputs = stats.count("totalfee") != 0 || stats.count("avgfee") != 0 || stats.count("avgfeerate") != 0 ||
1688+        do_medianfee || stats.count("minfee") != 0 || stats.count("maxfee") != 0 ||
1689+        do_medianfeerate || stats.count("minfeerate") != 0 || stats.count("maxfeerate") != 0;

ajtowns commented at 4:12 am on April 20, 2018:

loop_inputs is also needed when utxo_size_inc is requested. Setting loop_outputs = loop_inputs || stats.count("total_out") might simplify things a little.

in src/rpc/blockchain.cpp:1902 in 3699947a9e outdated

1904+    std::map<std::string, UniValue> map_stats = GetBlockStatsMap(block, pblockindex, stats);
1905+
1906+    UniValue ret(UniValue::VOBJ);
1907+    for (const std::string stat : stats) {
1908+        ret.pushKV(stat, map_stats[stat]);
1909+    }

ajtowns commented at 4:19 am on April 20, 2018:

Could move the “Invalid requested statistic” check to here, ie:

0    for (auto stat : stats) {
1        if (map_stats.count(stat) == 0) {
2            throw JSONRPCError(...);
3        }
4        ret.pushKV(stat, map_stats[stat]);
5   }

Then you wouldn’t need valid_stats. Downside is the error response would only come after the valid stats were corrected.

in src/rpc/blockchain.cpp:1677 in 3699947a9e outdated

1672+
1673+    const bool loop_outputs = stats.count("total_out") != 0 || stats.count("utxo_size_inc") != 0 ||
1674+        stats.count("totalfee") != 0 || stats.count("avgfee") != 0 || stats.count("avgfeerate") != 0 ||
1675+        do_medianfee || stats.count("minfee") != 0 || stats.count("maxfee") != 0 ||
1676+        do_medianfeerate || stats.count("minfeerate") != 0 || stats.count("maxfeerate") != 0;
1677+

ajtowns commented at 4:22 am on April 20, 2018:

These initialisations are a bit hard to follow. What do you think of:

0template<typename T> static inline bool SetHasKeys(const std::set<T>& set) {return false;}
1template<typename T, typename Tk, typename... Args> static inline bool SetHasKeys(const std::set<T>& set, const Tk& key, const Args&... args)
2{
3    return (set.count(key) != 0) || SetHasKeys(set, args...);
4}
5
6    const bool loop_inputs = do_medianfee || do_medianfeerate ||
7       SetHasKeys(stats, "totalfee", "avgfee", "avgfeerate", "minfee", "maxfee", "minfeerate", "maxfeerate", "utxo_size_inc");

instead? Seems easier to understand to me, should be equally efficient at runtime, and the recursive template thing is in use elsewhere already.

jimpo commented at 7:13 pm on April 20, 2018:

Do you know if fancy templating stuff like this will slow down compilation times noticeably? I’d kind of prefer a normal iterative runtime approach for simplicity, especially given that it would be far from the most expensive part of this API call.

in test/functional/rpc_getblockstats.py:174 in 05eddd0361 outdated

173+        # Make sure only the selected statistics are included
174+        some_stats = {'minfee', 'maxfee'}
175+        stats = node.getblockstats(height=1, stats=list(some_stats))
176+        # Make sure only valid stats that have been requested appear
177+        assert_equal(set(stats.keys()), some_stats)
178+

ajtowns commented at 4:27 am on April 20, 2018:

Suggest adding:

0       # Make sure each stat can be queried on its own
1       for stat in self.EXPECTED_STATS:
2           for i in range(self.max_stat_pos+1):
3               result = node.getblockstats(height=self.start_height + i, stats=[stat])
4               assert_equal(list(result.keys()), [stat])
5               if result[stat] != self.expected_stats[i][stat]:
6                   self.log.info("result[%s] (%d) failed, %r != %r" % (stat, i, result[stat], self.expected_stats[i][stat]))
7               assert_equal(result[stat], self.expected_stats[i][stat])

to ensure that each statistic can be queried on its own (in particular ensuring the GetBlockStatsMap() doesn’t optimise out collecting the data needed for each stat). The above should automatically catch problems if new stats are introduced since it’s just re-using the results from the all-the-stats check.

ajtowns commented at 4:42 am on April 20, 2018: member

I’m a bit confused by the bit about the x-axis in the PR description – isn’t plotting stuff in a different PR?

Seems a bit strange to exclude the coinbase from total_out; there’s no way to tell from the output you’ve got if the coinbase failed to claim all its reward.

jtimon force-pushed on Apr 20, 2018

jtimon commented at 7:48 pm on April 20, 2018: contributor

Fixed nits. The latest commit I did not squash because I’m not convinced it is an improvement even though is +32-48 (see #10757 (review) )

jtimon commented at 8:21 pm on April 20, 2018: contributor

I’m a bit confused by the bit about the x-axis in the PR description – isn’t plotting stuff in a different PR?

Sorry about the confusing edits. At first, height, time and mediantime were treated separately as I was using them as potential x-axis with my caller. Now they’re just treated as like any other stat.

Seems a bit strange to exclude the coinbase from total_out; there’s no way to tell from the output you’ve got if the coinbase failed to claim all its reward.

If the coinbase tx doesn’t claim subsidy + totalfee then it failed to claim all its reward. For seeing if miners claimed all the reward, perhaps we can add a new stat coinbase_total_out or better, unclaimed_reward, since subsidy + totalfee = coinbase_total_out + unclaimed_reward.

I don’t see how adding the claimed reward to the total_out helps with this. The idea of discarding the coinbase is that, even if we can’t discard change outputs or pay to yourself txs or similar to approximate real throughput, we definitely can clearly discard the subsidy and the fees, but if people feel strongly about this it is easy to change. Perhaps we can have this as “total_moved” or “total_paid” And another redundant one total_out that’s total_moved + coinbase_total_out (redundant only assuming we add coinbase_total_out or unclaimed_reward).

Btw, the only person who answered the question about selecting by height or block hash was @trippysalmon who said he didn’t care. Should I move from height to block hash to be more consistent with getblock ? Should both options be allowed?

in src/rpc/blockchain.cpp:1685 in f80bb3cf6d outdated

1690+    bool loop_outputs = true;
1691+    bool do_calculate_size = true;
1692+    bool do_calculate_weight = true;
1693+    bool do_calculate_sw = true;
1694+    // Calculate everything if nothing selected (default)
1695+    if (stats.size() > 0) {

ajtowns commented at 7:45 am on April 21, 2018:

Could do:

0    const bool do_all = stats.size() > 0;
1    const bool do_mediantxsize = do_all || SetHasKeys(stats, "mediantxsize);
2    const bool do_medianfee = do_all || ...;
3    ...

in src/rpc/blockchain.cpp:1862 in 3402b0dc83 outdated

1857+    int current_tip = chainActive.Height();
1858+    if (height < 0) {
1859+        height = current_tip + height + 1;
1860+    }
1861+    if (height < 0) {
1862+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d is negative", height));

ajtowns commented at 7:55 am on April 21, 2018:

There’s two tests for (height < 0) here, the first of which should be setting height to a positive value. Seems like a rebasing error?

jtimon commented at 3:06 pm on April 21, 2018:

I think it is a rest from when moving from a range to a single height, but yeah, thanks.

ajtowns commented at 8:03 am on April 21, 2018: member

I don’t really have a strong opinion on total_out, I guess I just found it a little weird it doesn’t include the coinbase outputs.

As far as height vs blockhash is concerned, I think specifying by height is probably easiest for people using the api, but maybe it would be good to include the blockhash in the output (so if you get different results for a given height due to a reorg, it’s easy to tell why), and/or allow either height/hash as the first param? I don’t feel strongly about this either way, either.

jtimon commented at 3:12 pm on April 21, 2018: contributor

Yeah, my concern is precisely that, asking for a given height and getting the results for a block it’s not what you were expecting, and yeah, both solutions (optionally accepting hash instead of height or always returning the block hash work) solve that problem. I guess I like more the first option because it’s less data sent, but I guess the “hash_or_height” param name is ugly. I will do that and ask for “squash or remove”?

By the way, thoughts on the “squash or remove” for “?? f’RPC: Introduce getblockstats’”? Is that what you were expecting? Oh, wait, you have a nit on that commit. I’ll solve them and then ask again (unless you say “yeah it’s not worth it” now and I skip solving that nit).

jtimon force-pushed on Apr 23, 2018

jtimon commented at 11:03 pm on April 23, 2018: contributor

Since all the stats are optional anyway, I ended up doing both adding an optional blockhash in the result and my preference, which was moving from height to hash_or_height. Also did some squashing and did further simplifications on both the rpc and the tests, including inling the function, since it didn’t sense anymore after moving away from height ranges and removing the hardcoded list of expected stats from the tests (since they’re already hardcoded in the data file).

Since the diff from last review was kind of big, I took the opportunity to rebase too.

jtimon force-pushed on Apr 24, 2018

jtimon commented at 0:07 am on April 24, 2018: contributor

Fixed the remaining nits: test when there’s no txindex and give a specific error when txindex is required but not set as suggested by @jimpo.

jtimon force-pushed on Apr 24, 2018

jtimon commented at 1:36 pm on April 24, 2018: contributor

Fixed the tests with the node 1 in travis (in my computer the sync_all wasn’t needed, oops) and did some bikeshedding.

jtimon force-pushed on Apr 24, 2018

in src/rpc/blockchain.cpp:1875 in e73e374b23 outdated

1870+    if (do_all) {
1871+        return ret_all;
1872+    }
1873+
1874+    UniValue ret(UniValue::VOBJ);
1875+    for (const std::string stat : stats) {

promag commented at 5:48 pm on April 24, 2018:

for (const std::string& stat : stats) {

in src/rpc/blockchain.cpp:1879 in e73e374b23 outdated

1874+    UniValue ret(UniValue::VOBJ);
1875+    for (const std::string stat : stats) {
1876+        if (ret_all[stat].isNull()) {
1877+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid selected statistic %s", stat));
1878+        }
1879+        ret.pushKV(stat, ret_all[stat]);

promag commented at 5:52 pm on April 24, 2018:

Could avoid 2nd lookup:

0const UniValue& value = ret_all[stat];
1if (value.isNull()) {
2    throw ...;
3}
4ret.pushKV(stat, value);

promag commented at 7:43 pm on April 24, 2018: member

Code looks good, some nits though. I’ll try it out so I can give a tested ACK.

in src/rpc/client.cpp:125 in ddfe0918b0 outdated

121@@ -122,6 +122,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
122     { "importmulti", 1, "options" },
123     { "verifychain", 0, "checklevel" },
124     { "verifychain", 1, "nblocks" },
125+    { "getblockstats", 0, "hash_or_height" },

jimpo commented at 7:59 pm on April 24, 2018:

Also need to add argument 1 for stats to be parsed as an array.

jtimon commented at 10:18 pm on April 24, 2018:

Mhmm, yeah. Why is this working without it?

promag commented at 10:21 pm on April 24, 2018:

Only relevant for bitcoin-cli client.

in src/rpc/blockchain.cpp:1692 in e73e374b23 outdated

1687+            "  \"txs\": xxxxx,             (numeric) The number of transactions (excluding coinbase)\n"
1688+            "  \"utxo_increase\": xxxxx,   (numeric) The increase/decrease in the number of unspent outputs\n"
1689+            "  \"utxo_size_inc\": xxxxx,   (numeric) The increase/decrease in size for the utxo index (not discounting op_return and similar)\n"
1690+            "}\n"
1691+            "\nExamples:\n"
1692+            + HelpExampleCli("getblockstats", "1000 \"minfeerate,avgfeerate\"")

jimpo commented at 8:03 pm on April 24, 2018:

CLI help doc is incorrect. It should be "1000 '[\"minfeerate\",\"avgfeerate\"]'" (once stats is added in client.cpp).

jimpo commented at 8:07 pm on April 24, 2018: contributor

Looks good aside from the CLI usage. I’d ACK with that change.

jtimon force-pushed on Apr 24, 2018

jtimon commented at 11:06 pm on April 24, 2018: contributor

Hopefully fixed all the newest nits, thanks again.

jimpo commented at 1:15 am on April 25, 2018: contributor

ACK ddfe091

ajtowns commented at 9:30 pm on April 25, 2018: member

Weird. I’m getting consistent failures with 2dffcdc2afd7ee98f170af933b94186c455425bc on the sync_all() after load_test_data() – it just hangs, because node 1 is never actually seeing the blocks. If I add a node.generate(1) beforehand, things work. The problem seems to be that the node doing the submitblock calls is never leaving InitialBlockDownload, and while it’s in IBD it’s not relaying blocks to its peer. Ah, it looks like this only shows up when it’s been more than 24 hours since --gen-test-data was run – so rerunning travis or running it locally should fail reliably now, I think.

Adding a call to node.generate(1) at the start of load_test_data should fix this:

0     def load_test_data(self, filename):
1         node = self.nodes[0]
2+        node.generate(1) # finish IBD; will get reorged out

jtimon force-pushed on Apr 26, 2018

jtimon force-pushed on Apr 27, 2018

jtimon commented at 1:36 am on April 27, 2018: contributor

@ajtowns I think I solved it with mocktime as discussed, but since the time is in the file generated again, I advice not to merge it after we test it again after 24 of the data file being created (last time it only started failing after that),

jtimon force-pushed on Apr 27, 2018

in src/rpc/blockchain.cpp:1842 in 866c1c8861 outdated

1837+            maxfee = std::max(maxfee, txfee);
1838+            minfee = std::min(minfee, txfee);
1839+            totalfee += txfee;
1840+
1841+            // New feerate uses satoshis per virtual byte instead of per serialized byte
1842+            CAmount feerate = CFeeRate(txfee, weight).GetTruncatedFee(WITNESS_SCALE_FACTOR);

TheBlueMatt commented at 1:49 am on April 28, 2018:

I find this incredibly hard to read. Why not just use floating point and calculate it all manually? Then you wouldn’t have to work around rounding issues (and wouldnt have the rounding issues you see here where things are rounded to the nearest 1).

in test/functional/rpc_getblockstats.py:96 in f8ad78ad4b outdated

93+        for b in blocks:
94+            self.nodes[0].submitblock(b)
95+
96+        # Set the timestamps from the file so that the nodes can get out of Initial Block Download
97+        self.nodes[0].setmocktime(mocktime)
98+        self.nodes[1].setmocktime(mocktime)

ajtowns commented at 7:06 am on April 30, 2018:

Think mocktime needs to be set prior to submitblock

jtimon force-pushed on Apr 30, 2018

jtimon force-pushed on May 1, 2018

jtimon commented at 7:20 pm on May 1, 2018: contributor

@ajtowns now it seems to work 24 hours after generating the data @TheBlueMatt yeah, it looks simpler now without using CFeeRate or CFeeRate::GetTruncatedFee. Thanks

Independently of that, if we want more precision for feerates (say, move from sat/vbyte to sat/vKB or whatever), now it’s the right time to decide so. I guess we could also support arbitrary precision with an optional feerates_precision parameter that defaults to 1 (ie sat/vbyte), or 1000 (ie sat/vKB) or 1024 or whatever. Or we can simply leave the latter for later if anybody asks, but it’s a simple change if people want more precision.

in src/rpc/blockchain.cpp:1729 in de63105609 outdated

1724+        const uint256 hash(uint256S(strHash));
1725+        pindex = LookupBlockIndex(hash);
1726+    }
1727+
1728+    std::set<std::string> stats;
1729+    if (request.params.size() > 1 && !request.params[1].isNull()) {

TheBlueMatt commented at 8:04 pm on May 1, 2018:

You dont need the size() check.

in src/rpc/blockchain.cpp:1726 in de63105609 outdated

1720+
1721+        pindex = chainActive[height];
1722+    } else {
1723+        const std::string strHash = request.params[0].get_str();
1724+        const uint256 hash(uint256S(strHash));
1725+        pindex = LookupBlockIndex(hash);

TheBlueMatt commented at 8:05 pm on May 1, 2018:

I believe if the hash is invalid GetBlockChecked will crash trying to deref the blockindex nullptr.

promag commented at 1:22 pm on May 15, 2018:

This is fixed.

TheBlueMatt commented at 8:11 pm on May 1, 2018: member

Matt was here.

jtimon force-pushed on May 7, 2018

Refactor: RPC: Separate GetBlockChecked() from getblock()

This does not change functionality

cda8e36f01

jtimon force-pushed on May 7, 2018

jtimon commented at 10:53 am on May 7, 2018: contributor

Fixed last 2 nits. Added a test for when blocks aren’t found. Also needed rebase.

in test/functional/rpc_getblockstats.py:179 in 4523b5d9c4 outdated

174+
175+        # Mainchain's genesis block shouldn't be found on regtest
176+        assert_raises_rpc_error(-5, 'Block not found', self.nodes[0].getblockstats,
177+                                hash_or_height='000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f')
178+
179+

promag commented at 1:04 pm on May 7, 2018:

Linter error:

0W293 blank line contains whitespace.

jtimon commented at 10:42 pm on May 8, 2018:

Fixed

jtimon force-pushed on May 8, 2018

jtimon force-pushed on May 9, 2018

promag commented at 1:21 pm on May 15, 2018: member

Lightly tested ACK 563eee9.

in src/rpc/blockchain.cpp:1672 in 563eee92c9 outdated

1667+            "      \"height\",   (string, optional) Selected statistic\n"
1668+            "      \"time\",     (string, optional) Selected statistic\n"
1669+            "      ,...\n"
1670+            "    ]\n"
1671+            "\nResult: (all values are in reverse order height-wise)\n"
1672+            "{                             (json object)\n"

kallewoof commented at 7:59 am on May 17, 2018:

Due to two escapes in below lines, this needs to remove two spaces for alignment.

in src/rpc/blockchain.cpp:1728 in 563eee92c9 outdated

1722+    } else {
1723+        const std::string strHash = request.params[0].get_str();
1724+        const uint256 hash(uint256S(strHash));
1725+        pindex = LookupBlockIndex(hash);
1726+        if (!pindex) {
1727+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");

kallewoof commented at 8:05 am on May 17, 2018:

Just a note here. In https://github.com/bitcoin/bitcoin/blob/4cfe17c3382ba750131cdc8703b2978132822070/src/rpc/blockchain.cpp#L809-L818 RPC_MISC_ERROR is used, while RPC_INVALID_ADDRESS_OR_KEY is used here. Not sure which is better, but I think they should both use the same error type. (This PR touches the other one so could easily tweak, I think.)

jimpo commented at 6:42 pm on May 17, 2018:

I disagree. If the header is not found, that means the request was invalid, hence RPC_INVALID ADDRESS_OR KEY (key loosely taken to mean request argument, I take it). Whereas if the header is known but the block is not known, I do consider that to be a different error.

In #13144, I introduce RPC_DATA_UNAVAILABLE, which may be more appropriate than RPC_MISC in the latter case.

in src/rpc/blockchain.cpp:1851 in 563eee92c9 outdated

1845+            maxfee = std::max(maxfee, txfee);
1846+            minfee = std::min(minfee, txfee);
1847+            totalfee += txfee;
1848+
1849+            // New feerate uses satoshis per virtual byte instead of per serialized byte
1850+            CAmount feerate = weight ? (txfee * WITNESS_SCALE_FACTOR) / weight : 0;

kallewoof commented at 8:13 am on May 17, 2018:

May be ok, but I think (txfee * WITNESS_SCALE_FACTOR) / (weight + WITNESS_SCALE_FACTOR - 1) is precisely accurate while the above will have truncation errors sometimes.

jimpo commented at 7:03 pm on May 17, 2018:

That equation has truncated errors. Consider a tx with weight 3 and fee 100. The vsize is 1, so the fee rate should be 1, but your equation would give (100 * 4) / 6 = 66.

You point out correctly though that vsize is computed incorrectly here by rounding down rather than up. This should use GetVirtualTransactionSize().

jtimon commented at 7:15 pm on May 17, 2018:

Sorry, I don’t follow. This is using GetTransactionWeight() which does ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * (WITNESS_SCALE_FACTOR - 1) + ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION)

GetVirtualTransactionSize does (std::max(nWeight, nSigOpCost * nBytesPerSigOp) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR

But I’m not sure why would we prefer that here.

kallewoof commented at 5:19 am on May 18, 2018:

Sorry, I think I got things mixed up.

ajtowns commented at 5:42 am on May 18, 2018:

The fee rate for a tx with weight 3 and fee 100 is the same as if there were 100 such tx’s. That would have weight 300 or vsize 75, and fee 10,000, for a fee rate of 10,000/75 or 133.3, not 1, 100, or 66. :) Formula looks fine to me, though I guess estimatefee uses fee/vkbyte rather than fee/byte, which would be (txfee * WITNESS_SCALE_FACTOR * 1000) / weight.

jtimon commented at 8:37 pm on May 22, 2018:

Well, we can move from sat/vbyte to sat/vkbyte, sure. I t should be a simple change if people prefer that.

kallewoof approved

kallewoof commented at 8:19 am on May 17, 2018: member

utACK 563eee92c9ddc5b537fca1ce08bce811d97aed97 w/ minor nits

in src/rpc/blockchain.cpp:1664 in 563eee92c9 outdated

1659+            "getblockstats ( hash_or_height stats )\n"
1660+            "\nCompute per block statistics for a given window. All amounts are in satoshis.\n"
1661+            "\nIt won't work for some heights with pruning.\n"
1662+            "\nIt won't work without -txindex for utxo_size_inc, *fee or *feerate stats.\n"
1663+            "\nArguments:\n"
1664+            "1. \"hash_or_height\"     (string or numeric, required) The block hash or height of the target block. If height, negative values count back from the current tip\n"

ajtowns commented at 6:55 am on May 18, 2018:

But, negative values don’t count back from the current tip, they give an error?

in src/rpc/blockchain.cpp:1657 in 563eee92c9 outdated

1652+// outpoint (needed for the utxo index) + nHeight + fCoinBase
1653+static constexpr size_t PER_UTXO_OVERHEAD = sizeof(COutPoint) + sizeof(uint32_t) + sizeof(bool);
1654+
1655+static UniValue getblockstats(const JSONRPCRequest& request)
1656+{
1657+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 4)

ajtowns commented at 6:55 am on May 18, 2018:

Multiline if statement should have braces.

in src/rpc/blockchain.cpp:1671 in 563eee92c9 outdated

1666+            "    ["
1667+            "      \"height\",   (string, optional) Selected statistic\n"
1668+            "      \"time\",     (string, optional) Selected statistic\n"
1669+            "      ,...\n"
1670+            "    ]\n"
1671+            "\nResult: (all values are in reverse order height-wise)\n"

ajtowns commented at 6:58 am on May 18, 2018:

“(all values are in reverse order height-wise)” is outdated now that this only does one block’s stats, I think? If it’s not, it doesn’t make sense to me…

ajtowns approved

ajtowns commented at 7:23 am on May 18, 2018: member

ACK 563eee92c9ddc5b537fca1ce08bce811d97aed97 modulo nits

RPC: Introduce getblockstats 35e77a0288

Tests: Test new getblockstats RPC

Includes commit from Anthony Towns @ajtowns:

Tests: Save and load block and corresponding expected statistics

4cbfb6aad9

Tests: Add data file 41d0476f62

jtimon force-pushed on May 22, 2018

jtimon commented at 9:29 pm on May 22, 2018: contributor

Fixed help nits (also made other improvements, some other things in the help were ugly).

laanwj merged this on May 23, 2018

laanwj closed this on May 23, 2018

laanwj referenced this in commit b9551d3663 on May 23, 2018

fanquake removed this from the "Blockers" column in a project

laanwj referenced this in commit 121cbaacc2 on Jun 8, 2018

UdjinM6 referenced this in commit 0aa34ea89c on Aug 15, 2019

UdjinM6 referenced this in commit 7918327c2a on Aug 20, 2019

UdjinM6 referenced this in commit f2dcac3a4f on Aug 28, 2019

barrystyle referenced this in commit b7353b4885 on Jan 22, 2020

PastaPastaPasta referenced this in commit b2c3237b1c on Jun 17, 2020

PastaPastaPasta referenced this in commit 000b2794a5 on Jul 2, 2020

MarcoFalke locked this on Dec 16, 2021

RPC: Introduce getblockstats to plot things #10757