[rpc] fix getchaintxstats() #11021

AkioNak · 2017-08-10T09:41:39Z

calculate nblocks more adaptive.
-> set default nblocks to min (blocks for 1 month, target block's height - 1)
-> before PR: if not specify nblocks-parameter, illegal parameter error will happen when target block height is below nblocks.
correct error message.
-> nblocks accepts [1 .. block's height -1] . so add a word "-1".
add check 0-divide.
-> if nTimeDiff = 0 then use UniValue(UniValue::VNULL) and returns {... "txrate": null} .
-> before PR: if nTimeDiff = 0 then returns {... "txrate":} and bitcoin-cli cannot handle the response.

promag

utACK e73f8bc.

promag · 2017-08-12T00:31:40Z

src/rpc/blockchain.cpp

@@ -1516,8 +1512,14 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
        }
    }

+    blockcount = std::min(blockcount, pindex->nHeight - 1);


Edge case: chain has 1 block only - blockcount will be 0 - should be a different error?

@promag Thank you for you review.
I added a commit that throws a new RPC_INVALID_PARAMETER if block's height below 2.

promag · 2017-08-12T08:47:29Z

src/rpc/blockchain.cpp

@@ -1512,6 +1512,9 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
        }
    }

+    if (pindex->nHeight < 2) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Block's height should be 2 or more.");


IMO the error should be RPC_MISC_ERROR since it's not a parameter. The message should be something like Chain height should be greater than 1.

I wonder if the error could be avoided by making the algorithm compatible with 1 block chain.

I think tx-rate for 1st block (and genesis block) can not calculate (or meaningless).

be avoided by making the algorithm compatible with 1 block chain.

So, I think we can treat this situation equal to nTimeDiff = 0.

promag · 2017-08-12T08:49:21Z

Can you improve the test in bitcoin/test/functional/blockchain.py?

AkioNak · 2017-08-13T14:56:23Z

Yes, I will add some tests in bitcoin/test/functional/blockchain.py.

AkioNak · 2017-08-14T07:56:40Z

@promag I avoided the error about low blockheight and added some tests.

promag

IMO throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid block count: should be between 1 and the block's height - 1") should be only for < 1case. nblocks is the size of the window, not really necessary to satisfy. Even the default value will throw for a chain with less than a month.

Also, CBlockIndex::GetAncestor return nullptr for out of bounds, just check that result.

AkioNak · 2017-08-15T01:43:17Z

@promag
I think these convenient commands is more useful that if they avoid errors.

IMO throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid block count: should be between 1 and the block's height - 1") should be only for < 1case.

When blockheight = 1, nblocks = 1 does not satisfy this condition. (blockheight> = 2 is required)
So, if we need throw JSONRPCError, I think it would be better to throw different error depending on whether the block(hash) is specified by the parameter or not.(same as your previous suggestion)

Also, CBlockIndex::GetAncestor return nullptr for out of bounds, just check that result.

Of course I can add to check it , but I think that the value of the parameter is always within the range. Am I missing something?

jnewbery

Thanks for the PR @AkioNak . Looks great. A couple of suggestions:

perhaps add a "block_count" element to the return object, so it's obvious over how many blocks the "txrate" is being calculated.
I'd prefer to not return a txrate at all if nTimeDiff is zero, ie:

if (nTimeDiff > 0) {
    ret.push_back(Pair("txrate", ((double)nTxDiff) / nTimeDiff);
}

That's just personal preference though. Tested ACK even without those changes.

One small style nit inline.

jnewbery · 2017-08-15T20:28:32Z

test/functional/blockchain.py

@@ -56,6 +56,22 @@ def _test_getchaintxstats(self):
        # we have to round because of binary math
        assert_equal(round(chaintxstats['txrate'] * 600, 10), Decimal(1))

+        b1 = self.nodes[0].getblock(self.nodes[0].getblockhash(1))
+        b200 = self.nodes[0].getblock(self.nodes[0].getblockhash(200))
+        ntimediff = b200['mediantime'] - b1['mediantime']


nit: name the variable time_diff. (the n prefix in the C++ code is no longer the style we use, and shouldn't be carried over to the python code)

jnewbery · 2017-08-16T13:28:43Z

I also recommend that you update the commit messages to be a bit more meaningful. I don't think this project has commit message style rules, but there are some general tips here: https://chris.beams.io/posts/git-commit/

AkioNak · 2017-08-16T14:25:31Z

@jnewbery Thank you for your suggestion. I agree with them.

promag · 2017-08-16T15:15:56Z

BTW, if nTimeDiff is zero then pindexPast and pindex are the same? If so nTxDiff is also zero then txrate should be zero too?

AkioNak · 2017-08-16T15:32:41Z

@promag No. Even if time_diff is zero, pindexPast and pindex may be different.
(For example, generates 100 on regtest)
Because GetMedianTimePast() returns mediantime of last 11 block's time (except the block itself),

promag · 2017-08-16T15:46:30Z

IMO the response could have both values txratecount and txrateinterval.

AkioNak · 2017-08-16T16:19:38Z

@promag In the edge case (block height <2), since there is no comparison target, its values are both zero. If that is OK then I will add it.

AkioNak · 2017-08-16T17:24:50Z

@jnewbery I have just done followings.

Add "block_count" element to return object.
Not to return "txrate" if nTimeDiff is zero.
Rename variable time_diff.
Fix commit comment.

sipa · 2017-08-16T18:16:09Z

src/rpc/blockchain.cpp

@@ -1480,6 +1480,7 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
            "{\n"
            "  \"time\": xxxxx,        (numeric) The timestamp for the statistics in UNIX format.\n"
            "  \"txcount\": xxxxx,     (numeric) The total number of transactions in the chain up to that point.\n"
+            "  \"block_count\": xxxxx, (numeric) The block height that ends the window.\n"


This seems to be a confusing name; I would associate block_count with the size of the window over which txrate is computed. To denote the block height that ends the window, I would use height.

I agree. My earlier suggestion was for block_count to be the size of the window.

@sipa Thank you for your review. I made a mistake.
@jnewbery I misunderstood. Same as your suggestion, I think the size of the window is better than block height as result of this command.

AkioNak · 2017-08-18T16:48:12Z

@sipa @jnewbery I fixed what "block_count" indicates and its description.

AkioNak · 2017-08-22T02:00:14Z

@promag I added both txratecount and txrateinterval to the return object.

AkioNak · 2017-08-23T11:10:27Z

I resolved the conflicts.

jnewbery

Looks really good now. I have a few nits inline.

You'll need to squash your commits down. Use git rebase -i and feel free to message me on IRC if you want some pointers on how to do that.

jnewbery · 2017-08-23T14:27:48Z

src/rpc/blockchain.cpp

@@ -1479,6 +1479,9 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
            "{\n"
            "  \"time\": xxxxx,        (numeric) The timestamp for the statistics in UNIX format.\n"
            "  \"txcount\": xxxxx,     (numeric) The total number of transactions in the chain up to that point.\n"
+            "  \"block_count\": xxxxx, (numeric) Size of the window in number of blocks.\n"
+            "  \"txratecount\": xxxxx, (numeric) The number of transactions in the window.\n"
+            "  \"txrateinterval\": xxxxx, (numeric) The elapsed time in the window.\n"


I don't love the txratecount and txrateinterval names (rate and count are fundamentally different things, and txrateinterval mentions tx but not time, but the value returned is time).

I suggest:

window_block_count

window_tx_count

window_interval

Note that new RPC arguments and return values should be snake case.

Also perhaps add a comment to the help text that these values are only returned if block_count is > 0

I agree to rename those names and to add condition to the help text.

jnewbery · 2017-08-23T14:37:44Z

src/rpc/blockchain.cpp

+    if (!request.params[0].isNull()) {
+        blockcount = request.params[0].get_int();
+
+        if (blockcount < 1 || blockcount >= pindex->nHeight) {


Any reason to disallow 0? blockcount can be 0 if we run this on a node with just the genesis block, so why not allow it to be set to 0 when running with a longer chain?

I think the reason to disallow 0 is that this RPC was originally intended to be able to calculate txrate.
Therefore if user specifies 0 by parameter, PRC should returns illegal parameter error.
So, for only genesis block and 1st block, I think it can allow 0 internally when user is not specify block count in 1st parameter, and can avoid error to be occured.

But now, it is easier to understand if we allow 0.

jnewbery · 2017-08-23T14:41:13Z

src/rpc/blockchain.cpp

+        ret.push_back(Pair("txratecount", nTxDiff));
+        ret.push_back(Pair("txrateinterval", nTimeDiff));
+    }
+    if (nTimeDiff > 0) {


Perhaps move this up into the if (blockcount > 0) code block (since nTimeDiff can only be non-zero when blockcount > 0)? Up to you.

I agree to move it.

jnewbery · 2017-08-23T14:41:24Z

src/rpc/blockchain.cpp

@@ -1479,6 +1479,9 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
            "{\n"
            "  \"time\": xxxxx,        (numeric) The timestamp for the statistics in UNIX format.\n"


Suggestion: improve this help message as part of this PR. Perhaps:

The timestamp for the final block in the window in UNIX format.

I agree to improve that help message.

jnewbery · 2017-08-23T14:44:00Z

src/rpc/blockchain.cpp

@@ -1526,7 +1535,14 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
    UniValue ret(UniValue::VOBJ);
    ret.push_back(Pair("time", (int64_t)pindex->nTime));
    ret.push_back(Pair("txcount", (int64_t)pindex->nChainTx));
-    ret.push_back(Pair("txrate", ((double)nTxDiff) / nTimeDiff));
+    if (blockcount > 0) {
+        ret.push_back(Pair("block_count", blockcount));


Perhaps display block_count if it is equal to zero? (but not the other two values since they're meaningless if block_count == 0

I agree to display block_count even if it is equal to zero.

AkioNak · 2017-08-24T12:03:23Z

@jnewbery Thank you for your various suggestion. I accepted with them and squashed commits for file by file.

promag · 2017-08-24T12:49:24Z

src/rpc/blockchain.cpp

+            "  \"time\": xxxxx,                (numeric) The timestamp for the final block in the window in UNIX format.\n"
+            "  \"txcount\": xxxxx,             (numeric) The total number of transactions in the chain up to that point.\n"
+            "  \"window_block_count\": xxxxx,  (numeric) Size of the window in number of blocks.\n"
+            "  \"window_tx_count\": xxxxx,     (numeric) The number of transactions in the window. Only returned if \"window_block_count\" is > 0.\n"


Nit, above it's txcount so here should be window_txcount?

I think this is fine. New arguments and return values should use snake_case.

I agree to @jnewbery ,too.

promag · 2017-08-24T12:51:44Z

src/rpc/blockchain.cpp

+            "  \"txcount\": xxxxx,             (numeric) The total number of transactions in the chain up to that point.\n"
+            "  \"window_block_count\": xxxxx,  (numeric) Size of the window in number of blocks.\n"
+            "  \"window_tx_count\": xxxxx,     (numeric) The number of transactions in the window. Only returned if \"window_block_count\" is > 0.\n"
+            "  \"window_interval\": xxxxx,     (numeric) The elapsed time in the window. Only returned if \"window_block_count\" is > 0.\n"


Include unit (seconds)?

I agree to append time unit(seconds)

promag · 2017-08-24T12:52:32Z

src/rpc/blockchain.cpp

+    if (!request.params[0].isNull()) {
+        blockcount = request.params[0].get_int();
+
+        if (blockcount < 0 || (blockcount > 0 && blockcount >= pindex->nHeight)) {


Remove blockcount > 0?

@promag @jnewbery I think that this condition is necessary if pindex points to genesis block.

blockcount == 0 is not an error so blockcount > 0 has to remain, I believe, for the case where pindex points at genesis block which means pindex->nHeight == 0.

promag · 2017-08-24T12:58:40Z

src/rpc/blockchain.cpp

@@ -1515,8 +1514,18 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
        }
    }

-    if (blockcount < 1 || blockcount >= pindex->nHeight) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid block count: should be between 1 and the block's height");
+    if (pindex->nHeight < 1) {


Move to else like:

if (!request.params[0].isNull()) { ... } else { blockcount = std::max(0, std::min(blockcount, pindex->nHeight - 1)); }

I agree that this is clearer

@promag Thanks for a good suggestion.
@promag @jnewbery Is it better to reverse the conditions?

if (request.params[0].isNull()) { blockcount = std::max(0, std::min(blockcount, pindex->nHeight - 1)); } else { ... }

No preference either way!

jnewbery · 2017-08-24T14:05:39Z

@AkioNak thanks for sticking with this. Looks really good now. Just a few more nits.

1. Calculate nblocks more adaptive. If not specify nblocks-parameter, illegal parameter error will happen when target block height is below blocks for 1 month. To avoid this error, set default nblocks to min(blocks for 1 month, target block's height - 1) And allowing 0 so that this RPC works good even if target block is genesis block or 1st block. 2. Correct error message. nblocks accepts [0 .. block's height -1] . so fix as following: "Invalid block count: should be between 0 and the block's height - 1" 3. Add check 0-divide. If nTimeDiff = 0 then returns {... "txrate":} and bitcoin-cli cannot handle the response. To avoid this error, do not return "txrate" if nTimeDiff = 0. 4. Add following 3 elements to the return object. 1) 'window_block_count' : Size of the window in number of blocks. 2) 'window_tx_count' : The number of transactions in the window. 3) 'window_interval' : The elapsed time in the window. They clarify how 'txrate' is calculated. 2) and 3) are returned only if 'window_block_count' is a positive value. 5. Improve help text for 'time' as following. 'The timestamp for the final block in the window in UNIX format.

1. Add a test for no parameters. 2. Add a test for the block's height = 1. 3. Add a test for nblocks is out of range.

AkioNak · 2017-08-25T10:32:04Z

@promag @jnewbery @kallewoof I fixed followings.

add unit(seconds) to the help text of window_interval.
calculate blockcount using std::max() rather than if-clause.

jnewbery · 2017-08-25T18:11:39Z

Looks good to me! tested ACK 07704c1.

07704c1 Add some tests for getchaintxstats (Akio Nakamura) 3336676 Fix getchaintxstats() (Akio Nakamura) Pull request description: 1. calculate nblocks more adaptive. -> set default nblocks to min (blocks for 1 month, target block's height - 1) -> before PR: if not specify nblocks-parameter, illegal parameter error will happen when target block height is below nblocks. 2. correct error message. -> nblocks accepts [1 .. block's height -1] . so add a word "-1". 3. add check 0-divide. -> if nTimeDiff = 0 then use UniValue(UniValue::VNULL) and returns {... "txrate": null} . -> before PR: if nTimeDiff = 0 then returns {... "txrate":} and bitcoin-cli cannot handle the response. Tree-SHA512: e1962ce7bb05a5bc7dec03eb04a8e7578f50fdb68927fcfc0a2232905ef4d679293eee148ebe0866682d209a8c458d21fbe71715e7311adb81f37089aae1ed93

TheBlueMatt · 2017-10-03T22:48:52Z

Postumous utACK non-test parts of 07704c1

07704c1 Add some tests for getchaintxstats (Akio Nakamura) 3336676 Fix getchaintxstats() (Akio Nakamura) Pull request description: 1. calculate nblocks more adaptive. -> set default nblocks to min (blocks for 1 month, target block's height - 1) -> before PR: if not specify nblocks-parameter, illegal parameter error will happen when target block height is below nblocks. 2. correct error message. -> nblocks accepts [1 .. block's height -1] . so add a word "-1". 3. add check 0-divide. -> if nTimeDiff = 0 then use UniValue(UniValue::VNULL) and returns {... "txrate": null} . -> before PR: if nTimeDiff = 0 then returns {... "txrate":} and bitcoin-cli cannot handle the response. Tree-SHA512: e1962ce7bb05a5bc7dec03eb04a8e7578f50fdb68927fcfc0a2232905ef4d679293eee148ebe0866682d209a8c458d21fbe71715e7311adb81f37089aae1ed93

AkioNak changed the title ~~fix getchaintxstats()~~ [rpc] fix getchaintxstats() Aug 10, 2017

fanquake added the RPC/REST/ZMQ label Aug 10, 2017

promag reviewed Aug 12, 2017

View reviewed changes

promag reviewed Aug 14, 2017

View reviewed changes

jnewbery reviewed Aug 15, 2017

View reviewed changes

AkioNak force-pushed the fix_getchaintxstats branch from 783d353 to 5eb1309 Compare August 16, 2017 16:41

sipa reviewed Aug 16, 2017

View reviewed changes

AkioNak force-pushed the fix_getchaintxstats branch from 3ddbee7 to 02755d0 Compare August 23, 2017 10:21

jnewbery reviewed Aug 23, 2017

View reviewed changes

AkioNak force-pushed the fix_getchaintxstats branch from 261bd2b to 0e30d62 Compare August 24, 2017 11:42

promag reviewed Aug 24, 2017

View reviewed changes

AkioNak added 2 commits August 25, 2017 18:32

Add some tests for getchaintxstats

07704c1

1. Add a test for no parameters. 2. Add a test for the block's height = 1. 3. Add a test for nblocks is out of range.

AkioNak force-pushed the fix_getchaintxstats branch from 0e30d62 to 07704c1 Compare August 25, 2017 09:33

laanwj merged commit 07704c1 into bitcoin:master Oct 2, 2017

AkioNak deleted the fix_getchaintxstats branch October 4, 2017 05:12

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

		@@ -1479,6 +1479,9 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
		"{\n"
		" \"time\": xxxxx, (numeric) The timestamp for the statistics in UNIX format.\n"

[rpc] fix getchaintxstats() #11021

[rpc] fix getchaintxstats() #11021

Conversation

AkioNak commented Aug 10, 2017

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkioNak Aug 13, 2017 • edited

Choose a reason for hiding this comment

promag commented Aug 12, 2017

AkioNak commented Aug 13, 2017

AkioNak commented Aug 14, 2017

promag left a comment • edited

Choose a reason for hiding this comment

AkioNak commented Aug 15, 2017 • edited

jnewbery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Aug 16, 2017

AkioNak commented Aug 16, 2017

promag commented Aug 16, 2017 • edited

AkioNak commented Aug 16, 2017

promag commented Aug 16, 2017

AkioNak commented Aug 16, 2017

AkioNak commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkioNak commented Aug 18, 2017

AkioNak commented Aug 22, 2017

AkioNak commented Aug 23, 2017

jnewbery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkioNak Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkioNak commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kallewoof Aug 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Aug 24, 2017

AkioNak commented Aug 25, 2017

jnewbery commented Aug 25, 2017

TheBlueMatt commented Oct 3, 2017

AkioNak Aug 13, 2017 •

edited

promag left a comment •

edited

AkioNak commented Aug 15, 2017 •

edited

promag commented Aug 16, 2017 •

edited

AkioNak Aug 24, 2017 •

edited

kallewoof Aug 25, 2017 •

edited