cli: Parse and allow hash value #19949
Conversation
|
Removed usage of |
src/rpc/client.cpp
Outdated
| static bool IsHashVal(const std::string& str) { | ||
| return (str.length() == 64) && std::all_of(str.begin(), str.end(), IsHexDigit); | ||
| } | ||
|
|
||
| /** Non-RFC4627 JSON parser, accepts internal values (such as numbers, true, false, null) | ||
| * as well as objects and arrays. | ||
| */ | ||
| UniValue ParseNonRFCJSONValue(const std::string& strVal) | ||
| { | ||
| UniValue jVal; | ||
| if (!jVal.read(std::string("[")+strVal+std::string("]")) || |
There was a problem hiding this comment.
| if (!jVal.read(std::string("[")+strVal+std::string("]")) || | |
| if ((!jVal.read(std::string("[")+strVal+std::string("]")) && | |
| !jVal.read(std::string("[\"")+strVal+std::string("\"]"))) || |
Just as an idea, feel free to ignore :)
This would be shorter, requires no additional methods and also does the job (except for getblockstats "'<blockhash>'"). But - and that could be more important - it keeps the error message if the blockhash is e.g. too long (hash_or_height must be of length 64 (not 66, for ...), while your solution throws an JSON parsing error which could be misleading for a user 🤔
There was a problem hiding this comment.
If I see that correctly what this change will do is it will end up passing everything as a string to the server and never throw that error client-side. So basically the same as #15448 which removes ParseNonRFCJSONValue and only does validation server-side (the arguably better error message is coming from the server). The feedback to that was that the validation shouldn't be removed completely from the client-side which is the approach I am trying here.
|
tACK 5076f83: ran functional & unit tests, works as expected. Only nit are the slightly degraded error messages when entering malformed hex strings ✌️ |
|
Does't feel right to place the workaround in I prefer #15448. |
Not sure if that changes your mind but I pulled the check out of @laanwj would you mind taking a quick look? This is mainly based on your feedback to #15448. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
I still don't like this kind of special casing based on "how a value looks". It adds a degree of ambiguity that can lead to nasty issues, especially if the value recognition becomes more complex. This is similar to what I said here: #15448 (comment) Still, I prefer this specialization to #15448, so if we really have to do something like this, mild ACK from me. |
|
I like the idea of doing it on the client side. That alleviates my concerns which are mostly about programmatic use. Although #20273 would be the most general solution, I guess this is shorter to type. |
There was a problem hiding this comment.
Concept ACK
Are there currently any other RPCs than getblockstats that also benefit from this change? It may make sense to also update the RPC help examples of those to get rid of the quotation marks. E.g.:
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 04b9c6b1c..66e6b9af5 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1746,7 +1746,7 @@ static UniValue getblockstats(const JSONRPCRequest& request) {RPCResult::Type::NUM, "utxo_size_inc", "The increase/decrease in size for the utxo index (not discounting op_return and similar)"}, }},
RPCExamples{
- HelpExampleCli("getblockstats", R"('"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"' '["minfeerate","avgfeerate"]')") +
+ HelpExampleCli("getblockstats", R"(00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09 '["minfeerate","avgfeerate"]')") +
HelpExampleCli("getblockstats", R"(1000 '["minfeerate","avgfeerate"]')") +
Yepp, it's is also useful now in |
There was a problem hiding this comment.
ACK bf12f7c
I think this makes the interface less confusing to use.
There was a problem hiding this comment.
Approach NACK
It's an elegant approach, and I think we should strive for more intuitive interfaces and this PR definitely would achieve that for the cli. However I think adding these kinds of exception handling 1) increases the maintenance burden instead of fixing the problem at its source - good method/endpoint design and 2) encourage further suboptimally designed methods/endpoints because we now have the tooling to interface with them more conveniently, making it more difficult to fix in the future.
For example, in this case I'd prefer the approach of letting the methods deal with parameter polymorphism (if they have a strong enough reason to use it in the first place), and encourage developers to be conservative with complex parameter design. I've updated getblockheight (but similar approach should probably work for gettxoutsetinfo` even though I didn't look at it) in the below diff to showcase how that could work. It's a very quick patch so please let me know if I didn't consider some negative side effects of this approach.
git diff on top of bf12f7c
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index f3cab7483..6bd86a5d8 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -109,17 +109,17 @@ static const CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateMan
LOCK(::cs_main);
CChain& active_chain = chainman.ActiveChain();
- if (param.isNum()) {
- const int height{param.getInt<int>()};
- if (height < 0) {
- throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d is negative", height));
+ const auto height{ToIntegral<int>(param.get_str())};
+ if (height) {
+ if (*height < 0) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d is negative", *height));
}
const int current_tip{active_chain.Height()};
- if (height > current_tip) {
- throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d after current tip %d", height, current_tip));
+ if (*height > current_tip) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d after current tip %d", *height, current_tip));
}
- return active_chain[height];
+ return active_chain[*height];
} else {
const uint256 hash{ParseHashV(param, "hash_or_height")};
const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash);
@@ -1715,7 +1715,7 @@ static RPCHelpMan getblockstats()
"\nCompute per block statistics 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"}},
+ {"hash_or_height", RPCArg::Type::STR, RPCArg::Optional::NO, "The block hash or height of the target block", "", {"", "string or numeric"}},
{"stats", RPCArg::Type::ARR, RPCArg::DefaultHint{"all values"}, "Values to plot (see result below)",
{
{"height", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Selected statistic"},
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index 90c69db7d..be47c3b79 100644
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp
@@ -158,7 +158,6 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "listdescriptors", 0, "private" },
{ "verifychain", 0, "checklevel" },
{ "verifychain", 1, "nblocks" },
- { "getblockstats", 0, "hash_or_height" },
{ "getblockstats", 1, "stats" },
{ "pruneblockchain", 0, "height" },
{ "keypoolrefill", 0, "newsize" },|
I am not interested enough in this anymore to argue about implementation details. Could be marked as up-for-grabs if the issue remains unsolved. |
This is an alternative approach to #15448 and also tries to eliminate "Error parsing JSON" errors. This came up in connection with
getblockstatsin this comment again: #19885 (comment) and that lead me to look into it.Conceptually I am not completely against the approach of #15448 but it stalled and maybe this approach is favored by reviewers.