[RPC][Refactoring] Meaningful error code when called with wrong number of arguments #12736

Any RPC command called with the wrong number of arguments currently raise a std::runtime_error exception which is then mapped to a RPC_MISC_ERROR when executed from a CRPCTable.

This error code doesn’t bring any value and should only be used when no other error codes make sense. However in this case, it could easily fall into RPC_INVALID_REQUEST case or maybe RPC_INVALID_PARAMETER? ...just found out that, for example, Ethereum choose to map this to RPC_INVALID_PARAMETER: https://github.com/ethereum/go-ethereum/blob/master/rpc/errors.go#L47-L52

The current RPC http implementation would internally map RPC_INVALID_REQUEST to a 400 http code whereas RPC_INVALID_PARAMETER would still be mapped to a 500 as it previously was with the RPC_MISC_ERROR code.

Furthermore, the current implementation also handle the help command by throwing and then catching a std::exception in std::string CRPCTable::help. The flaw in this implementation is that any JSONRPCError thrown are just ignored, because JSONRPCError isn’t an Exception. For example in the case of getinfo, which is deprecated, a call to help getinfo would lead to an error whereas the help command is just doing its job.

By switching to a JSONRPCError this proposition would catch each std::exception and JSONRPCError when executing the help command. This preserve the main help command behaviour: to present the usage/help to the user without any error code. A test case has been added to ensure every help can be reached without throwing an error.

A step ahead would be to remove the help command dependence to the error handling process. Each commands could just return the help message when called from the help command.

I'm on the fence about this one. RPC_INVALID_REQUEST does seem like a more sensible error for this, but is this just change for change's sake? The response already contains the help text which shows how many arguments are required and optional. Is receiving a RPC_MISC_ERROR causing any problems for users?

Changing this is an API change so would require release notes.

No it might not be a problem for users as is. The motivations behind this are:

• consistency on the Exception/Error type thrown by the RPC layer
• more meaning on the error code thrown by the RPC layer
• more consistency for the help command output

...to eventually raise discussion on the RPC Http layer.

My take on this might not be right. Maybe should I focus on one of the above first? What would be your take on this?

My take is that the error you suggest is slightly better, but it's not clear to me that it's so much better that it warrants and a code change and an API change.

No strong opinion here. I do like better error and parameter handling though.

However I would prefer to see something like this:

UniValue stop(const JSONRPCRequest& req, bool is_help)
{
    if (is_help) {
        return "stop\n"
        ...;
    }
    
    if (req.params.size() > 1) {
        throw JSONRPCError(RPC_INVALID_REQUEST, ...);
    }

This way the help text is distinguished from the parameter handling, and the server could then ask for the help.

the help text is distinguished from the parameter handling @promag I do agree this is the best solution, but it's even more changes to the code base, isn't this against the principle of keeping PR simple? About the implementation though, are you proposing to drop the usage of the fHelp flag on the JSONRPCRequest?

I was thinking about something like the following:

    std::string help = "...";
    if (request.fHelp) return help;
    if (request.params.size() > 1) throw JSONRPCError(RPC_INVALID_REQUEST, help);

Thinking even further, maybe the help message could be stored somewhere in the CRPCTable?

Needs rebase if still relevant. Otherwise, I think this can be closed.