[rpc] fix getchaintxstats() #11021

AkioNak commented at 9:41 AM on August 10, 2017: contributor

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.

AkioNak renamed this:
~~fix getchaintxstats()~~
[rpc] fix getchaintxstats()
on Aug 10, 2017

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

in src/rpc/blockchain.cpp:1515 in e73f8bc37b outdated

1511 | @@ -1516,8 +1512,14 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
1512 |          }
1513 |      }
1514 |  
1515 | +    blockcount = std::min(blockcount, pindex->nHeight - 1);

promag commented at 12:31 AM on August 12, 2017:

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

AkioNak commented at 5:40 AM on August 12, 2017:

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

promag commented at 12:33 AM on August 12, 2017: member

utACK e73f8bc.

in src/rpc/blockchain.cpp:1516 in 87815f2903 outdated

1511 | @@ -1512,6 +1512,9 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
1512 |          }
1513 |      }
1514 |  
1515 | +    if (pindex->nHeight < 2) {
1516 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "Block's height should be 2 or more.");

promag commented at 8:47 AM on August 12, 2017:

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.

promag commented at 8:47 AM on August 12, 2017:

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

AkioNak commented at 5:59 AM on August 13, 2017:

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 commented at 8:49 AM on August 12, 2017: member

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

AkioNak commented at 2:56 PM on August 13, 2017: contributor

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

AkioNak commented at 7:56 AM on August 14, 2017: contributor

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

promag commented at 10:57 PM on August 14, 2017: member

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 commented at 1:43 AM on August 15, 2017: contributor

@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?

in test/functional/blockchain.py:61 in 783d353d7f outdated

  55 | @@ -56,6 +56,22 @@ def _test_getchaintxstats(self):
  56 |          # we have to round because of binary math
  57 |          assert_equal(round(chaintxstats['txrate'] * 600, 10), Decimal(1))
  58 |  
  59 | +        b1 = self.nodes[0].getblock(self.nodes[0].getblockhash(1))
  60 | +        b200 = self.nodes[0].getblock(self.nodes[0].getblockhash(200))
  61 | +        ntimediff = b200['mediantime'] - b1['mediantime']

jnewbery commented at 8:28 PM on August 15, 2017:

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 commented at 8:32 PM on August 15, 2017: member

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 commented at 1:28 PM on August 16, 2017: member

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 commented at 2:25 PM on August 16, 2017: contributor

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

promag commented at 3:15 PM on August 16, 2017: member

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 commented at 3:32 PM on August 16, 2017: contributor

@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 commented at 3:46 PM on August 16, 2017: member

IMO the response could have both values txratecount and txrateinterval.

AkioNak commented at 4:19 PM on August 16, 2017: contributor

@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 force-pushed on Aug 16, 2017

AkioNak commented at 5:24 PM on August 16, 2017: contributor

@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.

in src/rpc/blockchain.cpp:1483 in 5eb13099ec outdated

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

sipa commented at 6:16 PM on August 16, 2017:

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.

jnewbery commented at 9:25 PM on August 16, 2017:

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

AkioNak commented at 12:25 AM on August 17, 2017:

@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 commented at 4:48 PM on August 18, 2017: contributor

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

AkioNak commented at 2:00 AM on August 22, 2017: contributor

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

AkioNak force-pushed on Aug 23, 2017

AkioNak commented at 11:10 AM on August 23, 2017: contributor

I resolved the conflicts.

in src/rpc/blockchain.cpp:1484 in 02755d0dc2 outdated

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

jnewbery commented at 2:27 PM on August 23, 2017:

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.

jnewbery commented at 2:43 PM on August 23, 2017:

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

AkioNak commented at 2:34 AM on August 24, 2017:

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

in src/rpc/blockchain.cpp:1526 in 02755d0dc2 outdated

1523 | +    }
1524 | +
1525 | +    if (!request.params[0].isNull()) {
1526 | +        blockcount = request.params[0].get_int();
1527 | +
1528 | +        if (blockcount < 1 || blockcount >= pindex->nHeight) {

jnewbery commented at 2:37 PM on August 23, 2017:

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?

AkioNak commented at 5:58 AM on August 24, 2017:

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.

in src/rpc/blockchain.cpp:1543 in 02755d0dc2 outdated

1539 | +    if (blockcount > 0) {
1540 | +        ret.push_back(Pair("block_count", blockcount));
1541 | +        ret.push_back(Pair("txratecount", nTxDiff));
1542 | +        ret.push_back(Pair("txrateinterval", nTimeDiff));
1543 | +    }
1544 | +    if (nTimeDiff > 0) {

jnewbery commented at 2:41 PM on August 23, 2017:

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

AkioNak commented at 8:57 AM on August 24, 2017:

I agree to move it.

in src/rpc/blockchain.cpp:1480 in 02755d0dc2 outdated

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

jnewbery commented at 2:41 PM on August 23, 2017:

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

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

AkioNak commented at 2:08 AM on August 24, 2017:

I agree to improve that help message.

in src/rpc/blockchain.cpp:1539 in 02755d0dc2 outdated

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

jnewbery commented at 2:44 PM on August 23, 2017:

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

AkioNak commented at 6:01 AM on August 24, 2017:

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

jnewbery commented at 2:47 PM on August 23, 2017: member

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.

AkioNak force-pushed on Aug 24, 2017

AkioNak commented at 12:03 PM on August 24, 2017: contributor

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

in src/rpc/blockchain.cpp:1483 in 0e30d6231a outdated

1481 | -            "  \"txcount\": xxxxx,     (numeric) The total number of transactions in the chain up to that point.\n"
1482 | -            "  \"txrate\": x.xx,       (numeric) The average rate of transactions per second in the window.\n"
1483 | +            "  \"time\": xxxxx,                (numeric) The timestamp for the final block in the window in UNIX format.\n"
1484 | +            "  \"txcount\": xxxxx,             (numeric) The total number of transactions in the chain up to that point.\n"
1485 | +            "  \"window_block_count\": xxxxx,  (numeric) Size of the window in number of blocks.\n"
1486 | +            "  \"window_tx_count\": xxxxx,     (numeric) The number of transactions in the window. Only returned if \"window_block_count\" is > 0.\n"

promag commented at 12:49 PM on August 24, 2017:

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

jnewbery commented at 1:32 PM on August 24, 2017:

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

AkioNak commented at 2:48 PM on August 24, 2017:

I agree to @jnewbery ,too.

in src/rpc/blockchain.cpp:1484 in 0e30d6231a outdated

1482 | -            "  \"txrate\": x.xx,       (numeric) The average rate of transactions per second in the window.\n"
1483 | +            "  \"time\": xxxxx,                (numeric) The timestamp for the final block in the window in UNIX format.\n"
1484 | +            "  \"txcount\": xxxxx,             (numeric) The total number of transactions in the chain up to that point.\n"
1485 | +            "  \"window_block_count\": xxxxx,  (numeric) Size of the window in number of blocks.\n"
1486 | +            "  \"window_tx_count\": xxxxx,     (numeric) The number of transactions in the window. Only returned if \"window_block_count\" is > 0.\n"
1487 | +            "  \"window_interval\": xxxxx,     (numeric) The elapsed time in the window. Only returned if \"window_block_count\" is > 0.\n"

promag commented at 12:51 PM on August 24, 2017:

Include unit (seconds)?

AkioNak commented at 3:36 PM on August 24, 2017:

I agree to append time unit(seconds)

in src/rpc/blockchain.cpp:1522 in 0e30d6231a outdated

1523 | +    }
1524 | +
1525 | +    if (!request.params[0].isNull()) {
1526 | +        blockcount = request.params[0].get_int();
1527 | +
1528 | +        if (blockcount < 0 || (blockcount > 0 && blockcount >= pindex->nHeight)) {

promag commented at 12:52 PM on August 24, 2017:

Remove blockcount > 0?

jnewbery commented at 2:04 PM on August 24, 2017:

Agree

AkioNak commented at 3:04 PM on August 24, 2017:

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

kallewoof commented at 8:37 AM on August 25, 2017:

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.

in src/rpc/blockchain.cpp:1517 in 0e30d6231a outdated

1513 | @@ -1515,8 +1514,18 @@ UniValue getchaintxstats(const JSONRPCRequest& request)
1514 |          }
1515 |      }
1516 |  
1517 | -    if (blockcount < 1 || blockcount >= pindex->nHeight) {
1518 | -        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid block count: should be between 1 and the block's height");
1519 | +    if (pindex->nHeight < 1) {

promag commented at 12:58 PM on August 24, 2017:

Move to else like:

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

jnewbery commented at 2:04 PM on August 24, 2017:

I agree that this is clearer

AkioNak commented at 2:58 PM on August 24, 2017:

@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 {
    ...
}

jnewbery commented at 4:26 PM on August 24, 2017:

No preference either way!

jnewbery commented at 2:05 PM on August 24, 2017: member

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

Fix getchaintxstats()

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.

33366768af

Add some tests for getchaintxstats

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.

07704c1b37

AkioNak force-pushed on Aug 25, 2017

AkioNak commented at 10:32 AM on August 25, 2017: contributor

@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 commented at 6:11 PM on August 25, 2017: member

Looks good to me! tested ACK 07704c1b3768d6c290046c783063644fc7b7d1da.

laanwj merged this on Oct 2, 2017

laanwj closed this on Oct 2, 2017

laanwj referenced this in commit 90926db238 on Oct 2, 2017

TheBlueMatt commented at 10:48 PM on October 3, 2017: member

Postumous utACK non-test parts of 07704c1b3768d6c290046c783063644fc7b7d1da

AkioNak deleted the branch on Oct 4, 2017

PastaPastaPasta referenced this in commit 5eb209661a on Dec 22, 2019

PastaPastaPasta referenced this in commit b7993bdd32 on Jan 2, 2020

PastaPastaPasta referenced this in commit db41b4c9d8 on Jan 4, 2020

PastaPastaPasta referenced this in commit 3f76b913f1 on Jan 12, 2020

PastaPastaPasta referenced this in commit 62ae437860 on Jan 12, 2020

PastaPastaPasta referenced this in commit b2e51391a0 on Jan 12, 2020

PastaPastaPasta referenced this in commit 0a71c27bac on Jan 12, 2020

PastaPastaPasta referenced this in commit 1f4a3f6395 on Jan 12, 2020

PastaPastaPasta referenced this in commit 5329aba10d on Jan 12, 2020

ckti referenced this in commit 5c9fc940dc on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021