Skip to content

Commit

Permalink
PR19521 suggestions, commit 6aedd7b "gettxoutsetinfo can be requested…
Browse files Browse the repository at this point in the history
… for specific blockheights"
  • Loading branch information
jonatack committed Mar 27, 2021
1 parent d4fb34d commit 452a4fb
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 12 deletions.
22 changes: 11 additions & 11 deletions src/rpc/blockchain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -128,18 +128,18 @@ static int ComputeNextBlockAndDepth(const CBlockIndex* tip, const CBlockIndex* b

CBlockIndex* ParseHashOrHeight(const UniValue& param) {
if (param.isNum()) {
const int height = param.get_int();
const int current_tip = ::ChainActive().Height();
const int height{param.get_int()};
if (height < 0) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d is negative", height));
}
const int current_tip{::ChainActive().Height()};

This comment has been minimized.

Copy link
@jonatack

jonatack Mar 27, 2021

Author Owner
  • initialize local variables as close as possible to first use
  • Throughout this PR, I think we want to avoid calling the global scope of ::ChainActive() now (cf Carl Dong's ongoing/mostly merged consensus separation work) but I didn't check further if this is the right approach (it seems to work)
if (height > current_tip) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d after current tip %d", height, current_tip));
}

return ::ChainActive()[height];
} else {
const uint256 hash(ParseHashV(param, "hash_or_height"));
const uint256 hash{ParseHashV(param, "hash_or_height")};

This comment has been minimized.

Copy link
@jonatack

jonatack Mar 29, 2021

Author Owner

braced initialization for type safety

CBlockIndex* pindex;
{
LOCK(cs_main);
Expand Down Expand Up @@ -1072,9 +1072,9 @@ static RPCHelpMan gettxoutsetinfo()
"\nReturns statistics about the unspent transaction output set.\n"
"Note this call may take some time if you are not using coinstatsindex.\n",
{
{"hash_type", RPCArg::Type::STR, /* default */ "hash_serialized_2", "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'."},
{"hash_type", RPCArg::Type::STR, /* default */ "hash_serialized_2", "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'"},
{"hash_or_height", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The block hash or height of the target height (only available with coinstatsindex)", "", {"", "string or numeric"}},
{"use_index", RPCArg::Type::BOOL, /* default */ "true", "Use Coinstatsindex even when it is available."},
{"use_index", RPCArg::Type::BOOL, /* default */ "true", "Use coinstatsindex, if available"},

This comment has been minimized.

Copy link
@jonatack

jonatack Mar 27, 2021

Author Owner

"Use Coinstatsindex even when it is available." doesn't make sense?

s/Coinstatsindex/coinstatsindex/ for consistency

drop full stops for consistency

},
RPCResult{
RPCResult::Type::OBJ, "", "",
Expand Down Expand Up @@ -1108,7 +1108,7 @@ static RPCHelpMan gettxoutsetinfo()
HelpExampleCli("gettxoutsetinfo", "") +
HelpExampleCli("gettxoutsetinfo", R"("none")") +
HelpExampleCli("gettxoutsetinfo", R"("none" 1000)") +
HelpExampleCli("gettxoutsetinfo", R"("none" "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09")") +
HelpExampleCli("gettxoutsetinfo", R"("none" '"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"')") +

This comment has been minimized.

Copy link
@jonatack

jonatack Mar 27, 2021

Author Owner

Bug: missing quotes here for the example to work

HelpExampleRpc("gettxoutsetinfo", "") +
HelpExampleRpc("gettxoutsetinfo", R"("none")") +
HelpExampleRpc("gettxoutsetinfo", R"("none", 1000)") +
Expand All @@ -1120,7 +1120,7 @@ static RPCHelpMan gettxoutsetinfo()

::ChainstateActive().ForceFlushStateToDisk();
CBlockIndex* pindex{nullptr};
CCoinsView* coins_view = WITH_LOCK(cs_main, return &ChainstateActive().CoinsDB());
CCoinsView* coins_view{WITH_LOCK(::cs_main, return &ChainstateActive().CoinsDB())};

This comment has been minimized.

Copy link
@jonatack

jonatack Mar 27, 2021

Author Owner

::cs_main or cs_main? other calls use the former but I didn't look into it further

braced init


const CoinStatsHashType hash_type{request.params[0].isNull() ? CoinStatsHashType::HASH_SERIALIZED : ParseHashType(request.params[0].get_str())};
CCoinsStats stats{hash_type};
Expand All @@ -1129,7 +1129,7 @@ static RPCHelpMan gettxoutsetinfo()

if (!request.params[1].isNull()) {
if (!g_coin_stats_index) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Querying specific block heights requires CoinStatsIndex");
throw JSONRPCError(RPC_INVALID_PARAMETER, "Querying specific block heights requires coinstatsindex");

This comment has been minimized.

Copy link
@jonatack

jonatack Mar 27, 2021

Author Owner

refer to coinstatsindex in a consistent manner

}

if (stats.m_hash_type == CoinStatsHashType::HASH_SERIALIZED) {
Expand Down Expand Up @@ -1158,7 +1158,7 @@ static RPCHelpMan gettxoutsetinfo()
}
ret.pushKV("total_amount", ValueFromAmount(stats.nTotalAmount));
if (!stats.from_index) {
ret.pushKV("transactions", (int64_t)stats.nTransactions);
ret.pushKV("transactions", static_cast<int64_t>(stats.nTransactions));

This comment has been minimized.

Copy link
@jonatack

jonatack Mar 27, 2021

Author Owner
ret.pushKV("disk_size", stats.nDiskSize);

This comment has been minimized.

Copy link
@jonatack

jonatack Mar 27, 2021

Author Owner

the "disk_size" help should be updated in this commit 6aedd7b "gettxoutsetinfo can be requested for specific blockheights"

- {RPCResult::Type::NUM, "disk_size", "The estimated size of the chainstate on disk"},
+ {RPCResult::Type::NUM, "disk_size", "The estimated size of the chainstate on disk (not available when coinstatsindex is used)"},
} else {
ret.pushKV("total_unspendable_amount", ValueFromAmount(stats.block_unspendable_amount));
Expand Down Expand Up @@ -1888,7 +1888,7 @@ static constexpr size_t PER_UTXO_OVERHEAD = sizeof(COutPoint) + sizeof(uint32_t)
static RPCHelpMan getblockstats()
{
return RPCHelpMan{"getblockstats",
"\nCompute per block statistics for a given window. All amounts are in satoshis.\n"
"\nCompute statistics per block for a given window. All amounts are in satoshis.\n"
"It won't work for some heights with pruning.\n",
{
{"hash_or_height", RPCArg::Type::NUM, RPCArg::Optional::NO, "The block hash or height of the target block", "", {"", "string or numeric"}},
Expand Down Expand Up @@ -1948,7 +1948,7 @@ static RPCHelpMan getblockstats()
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
{
LOCK(cs_main);
CBlockIndex* pindex = ParseHashOrHeight(request.params[0]);
CBlockIndex* pindex{ParseHashOrHeight(request.params[0])};
CHECK_NONFATAL(pindex != nullptr);

std::set<std::string> stats;
Expand Down
2 changes: 1 addition & 1 deletion test/functional/feature_coinstatsindex.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ def _test_coin_stats_index(self):
assert_equal(res0, res3)

# It does not work without coinstatsindex
assert_raises_rpc_error(-8, "Querying specific block heights requires CoinStatsIndex", node.gettxoutsetinfo, hash_option, 102)
assert_raises_rpc_error(-8, "Querying specific block heights requires coinstatsindex", node.gettxoutsetinfo, hash_option, 102)

This comment has been minimized.

Copy link
@jonatack

jonatack Mar 29, 2021

Author Owner

consistency


self.log.info("Test gettxoutsetinfo() with index and verbose flag")

Expand Down

0 comments on commit 452a4fb

Please sign in to comment.