Plugin: Use autodata to register notifications

[trueptolemy]

I remember @rustyrussell said, we should avoid the extra line change in const char *notification_topics[] when we plan to add new notifications(in fact, I want to add sendpay_success and sendpay_failure notifications).

I guess using autodata to register notifications is an available way and I rewrite notification interfaces.

[trueptolemy]

Correct the comment and rebased to resolve the conflict.

[cdecker]

Looking very good, just some minor thoughts, and a more philosophical one: you encapsulate the notification payload in a struct, which I very much like, but IIRC @rustyrussell had strong feelings about masking unset fields this way, and preferring wide signatures (take all struct fields as arguments for function calls, see our wire implementation for example).

Maybe @rustyrussell can add to that point?

[cdecker]

Nit: It's likely better to changes this into if (noti == NULL) { fatal("Could not find notification topic %s", topic); } just to make it more obvious later.

[trueptolemy]

Thank you, it's better!

[trueptolemy]

Fixed.

[cdecker]

We can likely also generate a type-safe wrapper for each notification that wraps the association (topic, payload) and internally calls notification_call.

I'd suggest making this notification_##topic##_call or notify_##topic and then we can just pass in the payload, instead of having to match topic and payload type.

[trueptolemy]

Like the hook. Ok, I'll do it :-)

[cdecker]

Not sure this needs to be allocated on the heap at all, if we immediately call the notification-serialization and pass that to the subscribers (if there are any), we retain control of the struct and can leave it on the stack.

[cdecker]

Would save us one allocation :-)

[trueptolemy]

Thank you! I'll try to take all struct fields as arguments for function calls. :-)

[trueptolemy]

@cdecker
If "take all struct fields as arguments for function calls", it seems difficult to generate the unified format for notification_##topic##_call or notify_##topic, because we need use different parameters to replace a payload_type parameter.
From this point, it may be necessary to declare each notification_##topic##_call separately here.

[trueptolemy]

@cdecker
Now I declare each notification call functions separately (doesn't choose to generate a wrapper for internally calls notification_call), and take all struct fields as arguments for function calls

[cdecker]

I see the problem. How about creating a struct for each notification that holds the payload of the notification itself?

The struct for a forward would look like this:

struct forward_event {
    const struct htlc_in *in,
    const struct htlc_out *out,
    enum forward_status state,
    enum onion_type failcode,
    struct timeabs *resolved_time
};

We'd then just declare a function that knows how to serialize a single instance to the json-rpc notification:

void forward_event_serialize(struct jsonrpc_notification *n, struct forward_event *e) {...}

And then in the registration we ensure that the serialize's function signature matches the event type we declared:

#define REGISTER_NOTIFICATION(topic, payload_type, serialize)                  \
	struct notification topic##_notification_gen = {                       \
	    stringify(topic),                                                  \
	    typesafe_cb_cast(                                                  \
		void (*)(void *, struct jsonrpc_notification *),               \
		void (*)(struct node_id *, struct jsonrpc_notification *),     \
		serialize),                                                    \
	};                                                                     \
	AUTODATA(notifications, &topic##_notification_gen);

(this is free-hand code, and I'm not sure it works as is)

And then we can also autogenerate the dispatch. The whole thing then looks a bit like this:

struct notification_type {
	const char *topic;
	/* the serialization interface */
	void (*serialize)(struct jsonrpc_notification *, void *);
};

/* This actually dispatches the notification, using the configured settings from type */
void notify_(struct lightningd *ld, struct notification_type *type, void *event);

AUTODATA_TYPE(notifications, struct notification);

#define REGISTER_NOTIFICATION(topic, payload_type, serialize)                  \
	struct notification topic##_notification_gen = {                       \
	    stringify(topic),                                                  \
	    typesafe_cb_cast(                                                  \
		void (*)(void *, struct jsonrpc_notification *),               \
		void (*)(struct node_id *, struct jsonrpc_notification *),     \
		serialize),                                                    \
	};                                                                     \
	void notify_##topic(struct lightningd *ld, payload_type payload)       \
	{                                                                      \
		notify(ld, &topic##_notification_gen, payload);                \
	}                                                                      \
	AUTODATA(notifications, &topic##_notification_gen);

notify_ then just needs to call type.serialize(stream, payload) to get the serialized payload, and can then issue the notification.

The use of typesafe_cb_cast ensures the serialization matches the payload type, and we save the manual casting, converting and dispatching for each event type.

What do you think?

[trueptolemy]

@cdecker Thank you so much!! :-)

Your idea is like the original version(the original code for this PR is here, I backuped before change :-) ), and generates a wrapper (notification_##topic##_call) for each notification based on internally calls notification_call.

The original version implemented warpper, but it can't well implement "take all struct fields as arguments for function calls", and it also need to allocate the payload structure every time before notification call.

But concerned the comment, I present a new version now. This new version takes all struct fields as arguments, but can't well warp (because it discarded the payload structure wapper).

This is a contradiction. In my understanding, we need to choose between "takes all struct fields as arguments for notification call" and "well warp, like payload wapper". Do you prefer the old version or the new version?

Because of the backup, it is very convenient to switch between two versions for this PR.

[cdecker]

@trueptolemy you're right, we are contradicting ourselves there a bit (I personally prefer the wrap-everything-in-structs approach, while Rusty prefers wide function signatures that explicitly pass in the arguments forcing you to purposefully pass in NULL).

I think your PR is good as is, and we can bikeshed the details later a bit more 😉

ACK 9119c2a