This is an alternative approach to #15448 and also tries to eliminate “Error parsing JSON” errors. This came up in connection with getblockstats in 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.
isxdigit because the linter complained about locale dependency.
220+
221+static bool IsHashVal(const std::string& str) {
222+ return (str.length() == 64) && std::all_of(str.begin(), str.end(), IsHexDigit);
223+}
224+
225 /** Non-RFC4627 JSON parser, accepts internal values (such as numbers, true, false, null)
Maybe add that this method also accepts tx and blockhashes?
0/** Non-RFC4627 JSON parser, accepts internal values (such as numbers, true, false, null or tx and blockhashes)
src/rpc/client.h as well for consistency :)
226 * as well as objects and arrays.
227 */
228 UniValue ParseNonRFCJSONValue(const std::string& strVal)
229 {
230 UniValue jVal;
231 if (!jVal.read(std::string("[")+strVal+std::string("]")) ||
0 if ((!jVal.read(std::string("[")+strVal+std::string("]")) &&
1 !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 🤔
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.
213@@ -214,15 +214,25 @@ CRPCConvertTable::CRPCConvertTable()
214
215 static CRPCConvertTable rpcCvtTable;
216
217+static bool IsHexDigit(const char c) {
218+ return ('0' <= c && c <= '9') || ('a' <= c && c <= 'f') || ('A' <= c && c <= 'F');
219+}
220+
221+static bool IsHashVal(const std::string& str) {
222+ return (str.length() == 64) && std::all_of(str.begin(), str.end(), IsHexDigit);
bool IsHex(const std::string&) from src/util/strencodings.h.
ParseHashV from rpc/util.cpp?
IsHex.
.SetHex() is too large (not sure it is), I’d be in favour of just carving it out in core_io.h instead of redefining the logic here.
Does’t feel right to place the workaround in
ParseNonRFCJSONValue.I prefer #15448.
Not sure if that changes your mind but I pulled the check out of ParseNonRFCJSONValue which seems like a simpler change anyway.
@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.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | laanwj, theStack |
| Approach NACK | stickies-v |
| Stale ACK | n-thumann, aureleoules |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
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.:
0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
1index 04b9c6b1c..66e6b9af5 100644
2--- a/src/rpc/blockchain.cpp
3+++ b/src/rpc/blockchain.cpp
4@@ -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)"}, }},
5RPCExamples{
6- HelpExampleCli("getblockstats", R"('"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"' '["minfeerate","avgfeerate"]')") +
7+ HelpExampleCli("getblockstats", R"(00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09 '["minfeerate","avgfeerate"]')") +
8 HelpExampleCli("getblockstats", R"(1000 '["minfeerate","avgfeerate"]')") +
Are there currently any other RPCs than
getblockstatsthat 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.:
Yepp, it’s is also useful now in gettxoutsetinfo after some changes were merged there in the meantime. I don’t know of any others. I added a commit with these changes and rebased for good measures.
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.
0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
1index f3cab7483..6bd86a5d8 100644
2--- a/src/rpc/blockchain.cpp
3+++ b/src/rpc/blockchain.cpp
4@@ -109,17 +109,17 @@ static const CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateMan
5 LOCK(::cs_main);
6 CChain& active_chain = chainman.ActiveChain();
7
8- if (param.isNum()) {
9- const int height{param.getInt<int>()};
10- if (height < 0) {
11- throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d is negative", height));
12+ const auto height{ToIntegral<int>(param.get_str())};
13+ if (height) {
14+ if (*height < 0) {
15+ throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d is negative", *height));
16 }
17 const int current_tip{active_chain.Height()};
18- if (height > current_tip) {
19- throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d after current tip %d", height, current_tip));
20+ if (*height > current_tip) {
21+ throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Target block height %d after current tip %d", *height, current_tip));
22 }
23
24- return active_chain[height];
25+ return active_chain[*height];
26 } else {
27 const uint256 hash{ParseHashV(param, "hash_or_height")};
28 const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash);
29@@ -1715,7 +1715,7 @@ static RPCHelpMan getblockstats()
30 "\nCompute per block statistics for a given window. All amounts are in satoshis.\n"
31 "It won't work for some heights with pruning.\n",
32 {
33- {"hash_or_height", RPCArg::Type::NUM, RPCArg::Optional::NO, "The block hash or height of the target block", "", {"", "string or numeric"}},
34+ {"hash_or_height", RPCArg::Type::STR, RPCArg::Optional::NO, "The block hash or height of the target block", "", {"", "string or numeric"}},
35 {"stats", RPCArg::Type::ARR, RPCArg::DefaultHint{"all values"}, "Values to plot (see result below)",
36 {
37 {"height", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Selected statistic"},
38diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
39index 90c69db7d..be47c3b79 100644
40--- a/src/rpc/client.cpp
41+++ b/src/rpc/client.cpp
42@@ -158,7 +158,6 @@ static const CRPCConvertParam vRPCConvertParams[] =
43 { "listdescriptors", 0, "private" },
44 { "verifychain", 0, "checklevel" },
45 { "verifychain", 1, "nblocks" },
46- { "getblockstats", 0, "hash_or_height" },
47 { "getblockstats", 1, "stats" },
48 { "pruneblockchain", 0, "height" },
49 { "keypoolrefill", 0, "newsize" },