Maybe I’m wrong, but it looks like that the handling of the height/hash union for getblockstats first argument seems inconsistent or wrong.
bitcoin-cli getblockstats 10000 works
bitcoin-cli getblockstats '"<hash>"' works
bitcoin-cli getblockstats <hash> gives “Error parsing JSON”
Ignoring that a '"<hash>"' (simple string) is probably not a valid json text, it seems to be inconsistent with other calls like getblock <hash>
It looks like the error is because the first input param to getblockstats get passed to ParseNonRFCJSONValue regardless if its a string (which shouldn’t happen since that is valid in RFC4627 spec). This is also why the '"<hash>"' works, because it is considered invalid.
The issue lies in the check in client.cpp always returning true when searching vRPCConvertParams. AFAIK this table is only used by bitcoin-cli and the GUI debug console
I can think of solution whereby the call is changed from getblockstats <hash/height> to getblockstats <hash> <height> where height is the overriding input. However, that seems to be overkill since it’s specifically a problem when calling via bitcoin-cli and the GUI debug console but would break backward compatibility.
Happy to get some help in thinking of a better answer , but perhaps better parsing of the args is a preferred solution?
There is no solution to this. bitcoin-cli parses some arguments as JSON, and some as strings. As a numeric argument is permitted, it has to be JSON here. That also implies that if you want to pass a string, you need to write it as a JSON string. To make what you’re asking for work, bitcoin-cli would need to somehow recognize that specifically for getblockstats if the argument looks like a hash, it is in fact a string and not JSON.
It’s probably better to have a separate getblockstatsbyhash RPC.
getblockstats call code?
Can’t we just threat the argument as non-json string (no conversion) and parse the number/hash in the
getblockstatscall code?
Yes, but this seems like an inferior solution. My suggestion which would simplify bitcoin-cli logic (just eliminating the exception on UniValue::read failure), where this would complicate the server side logic. Also, my suggestion could be general and work for any argument, where this require new code to be written for individual arguments.
Also, you didn’t say what heuristic you would use to distinguish hash strings from height strings like "10000". Maybe it is just checking string length. But any heuristic you wanted to use server side wouldn’t be as good as the one I was suggesting client-side, because the server doesn’t know the context of the call, so will make things jankier for clients other than bitcoin-cli. For example, if you’re using python client right now, numbers are always treated as heights, strings are always treated as hashes. But with a server side check, some python strings would have to start being treated as heights and result in weird errors or return values.
RPCArg, this is hopefully the only inconsistency we will ever see.
@ryanofsky Mind to create a pull, so we can test this?
@ryanofsky Mind to create a pull, so we can test this?
Created #15448. This would also allow removing the ParseNonRFCJSONValue function.
