RPC: Introduce getblockstats to plot things#1913
Conversation
Cherry-picker notes: The following stats have been removed, as we don't have the added complexities of segwit: swtotal_size, swtotal_weight, swtxs, total_weight In addition, fee rate is calculated as sat/bytes and not "virtual bytes".
Includes commit from Anthony Towns @ajtowns: Tests: Save and load block and corresponding expected statistics
Generated with `rpc_getblockstats.py --gen-test-data`
| } | ||
|
|
||
| LOCK(cs_main); | ||
|
|
There was a problem hiding this comment.
I don't believe we need cs_main here. Getting the tip height does not need it in BU because it's atomic, and neither chainActive[height] nor chainActive.contains() need it because the data is immutable.
There was a problem hiding this comment.
you are right about that, but if cs_main is not taken, the chain could be reorged between 1677 and 1689 causing an out of bounds error. Its basically impossible IRL because the user would have to call InvalidateBlock to reorg to a shorter chain. But let's merge and then worry about narrowing cs_main.
There was a problem hiding this comment.
I don't see where an out of bounds could be caused but I don't want to hold up the PR for this either.
There was a problem hiding this comment.
There a 80x speedup PR for getblockstats in the pipeline, we could postpone discussing this for that one
| } | ||
| } | ||
|
|
||
| DbgAssert(pindex != nullptr, throw std::runtime_error(__func__)); |
There was a problem hiding this comment.
why not throw JSONRPCError? What is the behavior of throwing runtime_error here?
There was a problem hiding this comment.
Mostly for brevity, these are converted into RPC_MISC_ERROR here
|
|
||
| #include <univalue.h> | ||
|
|
||
| #include <boost/algorithm/string.hpp> |
There was a problem hiding this comment.
Hey -- I was just browsing around the extant PRs -- quick question: why include this header? I don't see any new lines using it.. does it fix a compile error that was already there or something?
There was a problem hiding this comment.
Looks like you're right, it is not needed. It was in the original PR. Maybe the original author used it in an earlier revision and forgot to take it out.
I will remove it if I do a revision on this PR.
There was a problem hiding this comment.
Yeah this happens to me a lot -- include some header for something -- take that something out -- leave header in. So I am vigilant always to detect that since it's a bane.. :)
bitcoin/bitcoin#10757