Proper JSON-RPC error codes

[ZmnSCPxj]

Currently almost all commands just give a -1 error code in all errors. However this is not really particularly useful programmatically: it would be better for progammatic access to be able to determine what the exact error is without having to parse the error message (which is in fact the intent of the error code).

So I want to propose that we change command_fail to accept a proper code argument and make an effort to provide error codes for all error types!

IF we decide to go this route (this issue is a question) then w should probably schedule this for 0.6 also, as we would like to present a reasonably stable JSON-RPC interface.

Inspired-by: @wythe #1463

[wythe]

Other parts of the code use command_failed_detailed() to report the error with a code. We could just incrementally convert command_fail over to that. I can get to work with withdraw and fundchannel if we want to do this.

[ZmnSCPxj]

command_fail_detailed includes a JSON data field, which is optional for JSON-RPC errors. In many cases the data field is not needed. However JSON-RPC has the error codes mandatory, so we really should require command_fail to accept the error code as argument instead of always using -1.

[ZmnSCPxj]

A lot of errors are also related to parameters being wrong, which we should really have error code -32602, also #defined in lightningd/jsonrpc_errors.h as JSONRPC2_INVALID_PARAMS. We should have a simple command_fail_param that accepts the message, gives JSONRPC2_INVALID_PARAMS standard error code, and outputs a message like "msatoshi": Must be a positive integer: "string"

[wythe]

Yes, and in that case, maybe we could enhance json_get_params to take an additional "token_type" parameter for each token, AND a place to store the value upon success.

Just looking through a few of the json handlers, here's some possible values for the enum:

enum token_type {
    bool_type,
    msatoshi,
    positive_integer,
    sha256,
    json_array,
    bolt11,
    short_channel_id,
};

And then json_get_params would call, for example, json_tok_bool in the case of bool_type for us
automatically.

Well that's just a thought. Easier said than done.

[ZmnSCPxj]

It would not be very general, though; we would have to keep extending this. Consider newaddr, for example, which has an argument that is the address type, and is the only command which has this kind of argument. json_get_params also returns the JSONRPC2_INVALID_PARAMS error already.

[wythe]

I made a new branch to play around with this idea: https://github.com/wythe/lightning/tree/parse-params

Here is the basic usage I used for fundchannel:

        struct json_param_spec p[] = {
                { "id", JSON_PUBKEY, &fc->peerid },
                { "satoshi", JSON_SAT_OR_ALL, &fc->wtx },
                { NULL, JSON_END, NULL }
        };
        if (!json_marshall_params(fc->cmd, buffer, params, p))
                return;

It does clean up the handlers nicely, no need to do parameter checking after json_marshall_params is called.

I also made it work for newaddr. You're point about it not being general is of course valid. For these one-off verifiers we could expand the struct to take a function pointer. Better yet, require a function pointer always and dispense with the enum altogether.

Note: json_marshall_params doesn't work with named parameters. It's just proof of concept.

Anyway, just some ideas. I'm just having a good time getting back to my C programming roots.

[ZmnSCPxj]

Better yet, require a function pointer always and dispense with the enum altogether.

This may be better. It is also close to what the lightningd/options does.

[wythe]

Yes, it's also closer to the json_command itself. I updated the above struct to use a function pointer, and improved error reporting for fundchannel. It's nice to have the parameter validation and capturing in one place, but the callback as it is written takes a void pointer which requires casting in the handler.

Let me know if you think this is worth pursuing.

I think the big payoff, if you want to call it that, is when we get to reuse common callbacks for parameters like {msatoshi} and {id}. This can guarantee consistent error reporting.

[ZmnSCPxj]

but the callback as it is written takes a void pointer which requires casting in the handler.

There will (hopefully) only be a small number of handlers, so this is probably not a significant loss.

Let me know if you think this is worth pursuing.

I think it is worth pursuing.

[wythe]

This is starting to come together nicely. I used @rustyrussell's ccan/typesafe_cb to make the callbacks typesafe. There are no longer any varargs or void pointers. This a basic example usage:

struct param_table t = param_table_init(cmd);
param_table_add(&t, "id", json_parse_pubkey, &fc->peerid);
param_table_add(&t, "satoshi", json_tok_wtx, &fc->wtx);
if (!param_table_parse(&t, buffer, params))
        return;

and the callback sig looks like:

bool json_tok_wtx(struct command * cmd,
                  const char * buffer,
                  const jsmntok_t *sattok,
                  struct wallet_tx * wtx)

This is all on my parse-params branch and CI is passing.

I also updated the error codes for fundchannel as well as the man page. Commands other than fundchannel, withdraw and newaddr still use the json_get_params method.

What next? I would like to I convert all the other commands over before submitting, but that may be too big a pull request for everyone's liking. Also, the current naming convention isn't great.

[ZmnSCPxj]

Prefer param_table_add_ instead of _param_table_add for the core function.

Returning a structure is relatively unusual in C. In some ABIs your struct param_table param_table_init(struct command *cmd) will get compiled to void param_table_init(struct param_table *, struct command *cmd) anyway, which is more typical in C. But not a major issue.

It might be better to tal-allocate a struct param_table, parented at struct command, and have the array of param_def parented at the struct param_table, but that is just my preference.

Inside param_table_init you could have used t.defs = tal_arr(cmd, struct param_def, 0) so that param_table_add_ can use tal_resize directly.

Is struct param_def intended to be useable outside of the lightningd/params.c? If not, you can just foreward-declare struct param_def; in lighthningd/params.h and fill it in inside lightningd/params.c.

The callback signature would be better without the struct command *; perhaps the params system can recognize particular callbacks (i.e. if a json_tok_u64 fails it can report the failure as "foo" should be a 64-bit unsigned integer, and have a catchall case for unrecognized callbacks). This will make it compatible with most of the common/json and lightningd/json parsers. Alternatively, you should probably at minimum indicate that if the callback returns false, it should have called command_fail before returning false (but I prefer more removing the struct command * argument).

Also, s/command_fail2/command_fail/, remove the old command_fail, and propagate the changes to all callers of command_fail. That could be done as a separate PR. Some of the command_fail_detailed calls that provide a NULL error object can be converted to the new command_fail, and probably most of the command_fail that is parameter related should give the JSONRPC2_INVALID_PARAMS error code instead of -1.

[wythe]

I will get to work on your suggested changes.

I too was thinking about making the callback sig be the same as the json_tok_X functions. param_table_add can use that signature, and param_table_add_custom can be a version that takes the struct command *.