Skip to content

Commit

Permalink
blockstats: only take block hashes as input
Browse files Browse the repository at this point in the history 
Only take a block hash string as input to getblockstats, to retain
strong typing.

- Renames "hash_or_height" argument to "hash"
- Changes first param to be a string for dogecoin-cli
- Fixes tests to use hashes instead of heights
- Removes tests about bad heights given
- Adds test to ensure heights aren't supported
- Adjusts help text to support only hashes
  • Loading branch information
@patricklodder
patricklodder committed Dec 30, 2023
1 parent ec04014 commit 59dcc34
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 38 deletions.
32 changes: 18 additions & 14 deletions qa/rpc-tests/getblockstats.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,13 @@ def setup_network(self, split=False):
self.sync_all()

def get_stats(self):
return [self.nodes[0].getblockstats(hash_or_height=self.start_height + i) for i in range(self.max_stat_pos+1)]
return [self.get_stats_for_height(self.start_height + i) for i in range(self.max_stat_pos+1)]

def get_stats_for_height(self, height, stats=None):
blockhash = self.nodes[0].getblockhash(height)
if stats == None:
return self.nodes[0].getblockstats(hash=blockhash)
return self.nodes[0].getblockstats(hash=blockhash, stats=stats)

def generate_test_data(self, filename):
mocktime = 1525107225
Expand Down Expand Up @@ -121,13 +127,13 @@ def run_test(self):

# Check selecting block by hash too
blockhash = self.expected_stats[i]['blockhash']
stats_by_hash = self.nodes[0].getblockstats(hash_or_height=blockhash)
stats_by_hash = self.nodes[0].getblockstats(hash=blockhash)
assert_equal(stats_by_hash, self.expected_stats[i])

# Make sure each stat can be queried on its own
for stat in expected_keys:
for i in range(self.max_stat_pos+1):
result = self.nodes[0].getblockstats(hash_or_height=self.start_height + i, stats=[stat])
result = self.get_stats_for_height(self.start_height + i, [stat])
assert_equal(list(result.keys()), [stat])
if result[stat] != self.expected_stats[i][stat]:
self.log.info('result[%s] (%d) failed, %r != %r' % (
Expand All @@ -136,15 +142,10 @@ def run_test(self):

# Make sure only the selected statistics are included (more than one)
some_stats = {'minfee', 'maxfee'}
stats = self.nodes[0].getblockstats(hash_or_height=1, stats=list(some_stats))
stats = self.get_stats_for_height(1, list(some_stats))
assert_equal(set(stats.keys()), some_stats)

# Test invalid parameters raise the proper json exceptions
tip = self.start_height + self.max_stat_pos
assert_raises_jsonrpc(-8, 'Target block height %d after current tip %d' % (tip+1, tip),
self.nodes[0].getblockstats, hash_or_height=tip+1)
assert_raises_jsonrpc(-8, 'Target block height %d is negative' % (-1),
self.nodes[0].getblockstats, hash_or_height=-1)
blockhashone = self.nodes[0].getblockhash(1)

# Make sure not valid stats aren't allowed
inv_sel_stat = 'asdfghjkl'
Expand All @@ -156,17 +157,20 @@ def run_test(self):
]
for inv_stat in inv_stats:
assert_raises_jsonrpc(-8, 'Invalid selected statistic %s' % inv_sel_stat,
self.nodes[0].getblockstats, hash_or_height=1, stats=inv_stat)
self.nodes[0].getblockstats, hash=blockhashone, stats=inv_stat)

# Make sure we aren't always returning inv_sel_stat as the culprit stat
assert_raises_jsonrpc(-8, 'Invalid selected statistic aaa%s' % inv_sel_stat,
self.nodes[0].getblockstats, hash_or_height=1, stats=['minfee' , 'aaa%s' % inv_sel_stat])
self.nodes[0].getblockstats, hash=blockhashone, stats=['minfee' , 'aaa%s' % inv_sel_stat])
# Mainchain's genesis block shouldn't be found on regtest
assert_raises_jsonrpc(-5, 'Block not found', self.nodes[0].getblockstats,
hash_or_height='1a91e3dace36e2be3bf030a65679fe821aa1d6ef92e7c9902eb318182c355691')
hash='1a91e3dace36e2be3bf030a65679fe821aa1d6ef92e7c9902eb318182c355691')

# Invalid number of args
assert_raises_jsonrpc(-1, 'getblockstats hash_or_height ( stats )', self.nodes[0].getblockstats)
assert_raises_jsonrpc(-1, 'getblockstats hash ( stats )', self.nodes[0].getblockstats)

# Cannot pass a height
assert_raises_jsonrpc(-8, 'hash must be hexadecimal string', self.nodes[0].getblockstats, hash=1)


if __name__ == '__main__':
Expand Down
33 changes: 10 additions & 23 deletions src/rpc/blockchain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1591,11 +1591,11 @@ static UniValue getblockstats(const JSONRPCRequest& request)
{
if (request.fHelp || request.params.size() < 1 || request.params.size() > 2) {
throw std::runtime_error(
"getblockstats hash_or_height ( stats )\n"
"getblockstats hash ( stats )\n"
"\nCompute per block statistics for a given window. All amounts are in koinus.\n"
"It won't work for some heights with pruning.\n"
"\nArguments:\n"
"1. \"hash_or_height\" (string or numeric, required) The block hash or height of the target block\n"
"1. \"hash\" (string, required) The block hash of the target block\n"
"2. \"stats\" (array, optional) Values to plot, by default all values (see result below)\n"
" [\n"
" \"height\", (string, optional) Selected statistic\n"
Expand Down Expand Up @@ -1639,34 +1639,21 @@ static UniValue getblockstats(const JSONRPCRequest& request)
"}\n"
"\nExamples:\n"
+ HelpExampleCli("getblockstats", R"('"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"' '["minfeerate","avgfeerate"]')")
+ HelpExampleCli("getblockstats", R"(1000 '["minfeerate","avgfeerate"]')")
+ HelpExampleRpc("getblockstats", R"("00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09", ["minfeerate","avgfeerate"])")
+ HelpExampleRpc("getblockstats", R"(1000, ["minfeerate","avgfeerate"])")
);
}

LOCK(cs_main);

CBlockIndex* pindex;
if (request.params[0].isNum()) {
const int height = request.params[0].get_int();
const int current_tip = chainActive.Height();
if (height < 0) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d is negative", height));
}
if (height > current_tip) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d after current tip %d", height, current_tip));
}
const uint256 hash = ParseHashV(request.params[0], "hash");

pindex = chainActive[height];
} else {
const uint256 hash = ParseHashV(request.params[0], "hash_or_height");
if (mapBlockIndex.count(hash) == 0)
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
pindex = mapBlockIndex[hash];
if (!chainActive.Contains(pindex)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Block is not in chain %s", Params().NetworkIDString()));
}
if (mapBlockIndex.count(hash) == 0)
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
pindex = mapBlockIndex[hash];

if (!chainActive.Contains(pindex)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Block is not in chain %s", Params().NetworkIDString()));
}

assert(pindex != nullptr);
Expand Down Expand Up @@ -1837,7 +1824,7 @@ static const CRPCCommand commands[] =
{ // category name actor (function) okSafe argNames
// --------------------- ------------------------ ----------------------- ------ ----------
{ "blockchain", "getblockchaininfo", &getblockchaininfo, true, {} },
{ "blockchain", "getblockstats", &getblockstats, true, {"hash_or_height", "stats"} },
{ "blockchain", "getblockstats", &getblockstats, true, {"hash", "stats"} },
{ "blockchain", "getbestblockhash", &getbestblockhash, true, {} },
{ "blockchain", "getblockcount", &getblockcount, true, {} },
{ "blockchain", "getblock", &getblock, true, {"blockhash","verbosity"} },
Expand Down
1 change: 0 additions & 1 deletion src/rpc/client.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "importmulti", 1, "options" },
{ "verifychain", 0, "checklevel" },
{ "verifychain", 1, "nblocks" },
{ "getblockstats", 0, "hash_or_height" },
{ "getblockstats", 1, "stats" },
{ "pruneblockchain", 0, "height" },
{ "keypoolrefill", 0, "newsize" },
Expand Down

0 comments on commit 59dcc34

Please sign in to comment.