A couple things about the new hooks...

[jarret]

I experimented with writing a plugin that registered for all the hooks in v0.8.0rc1. Actually, an extended version of the ZeroMQ plugin under review here: lightningd/plugins#70

My experimental version that registers for hooks is here: https://github.com/jarret/plugins/blob/zmqhooks/zmq/cl-zmq.py

Here are a few nits that I encountered:

1. The way you signal "do nothing" via return type is inconsistent across the various types.

HOOK_NAMES_NOOP_RETURN = {'peer_connected':  {'result': "continue"},
                          'db_write':        True,
                          'invoice_payment': {},
                          'openchannel':     {"result": "continue"},
                          'htlc_accepted':   {"result": "continue"},
                          'rpc_command':     {"continue": True}}

it would be nicer if they had a consistent way to signal the "do nothing" case.

2. The invoice_payment hook name collides with the invoice_payment notification name. It makes naming variables and corresponding options tricky. (is the notification going to get deprecated in favor of the hook in the future, perhaps?).

3. I am not sure if (via the pyln-client framework) returning True is the right thing to do for the db_write hook. I also don't want to experiment with my mainnet node since it sounds like it will corrupt the database if I do it wrong. A clearer example for how it is to be correctly used would be helpful.

4. The openchannel hook doesn't have an underscore in its name. I might expect it to be named open_channel following convention of the others.

[cdecker]

Thanks @jarret for reporting this. Indeed the hooks have grown organically for a while now, so it might be time to define some conventions.

I quite like the more explicit version of the return:

{"result": "continue"}

this allows us to extend the result in future if we need to get more info from the hook. It also has the advantage of not being binary when compared to booleans. That means we can have more verbs to instruct the node. Depending on context we might have the following verbs as a response:

• channel:
  • continue: just do what you would do if there was no plugin subscribed to the hook
  • close: close a channel, try collaborativel
  • fail the counterparty is misbehaving and we can't continue talking to it
• global:
  • continue same as channel continue
  • exit do a clean shutdown, possibly returning a provided return code and string logged explaining why we had to exit
  • fail: exit immediately with an abort() printing a stack trace and a message.

The invoice_payment hook name collides with the invoice_payment notification name. It makes naming variables and corresponding options tricky. (is the notification going to get deprecated in favor of the hook in the future, perhaps?).

I don't thing we're replacing notifications with hooks, though they might get a lot more similar once we start chaining hooks and allow mulitple plugins to subscribe to a hook. Notifications are parallel, hooks sequential. The naming conflict is a bit unfortunate, but they are treated orthogonally to each other, so besides the confusion of whether it's a hook or a notification there isn't much that can go wrong. What would you suggest to make the distinction clearer?

I am not sure if (via the pyln-client framework) returning True is the right thing to do for the db_write hook. I also don't want to experiment with my mainnet node since it sounds like it will corrupt the database if I do it wrong. A clearer example for how it is to be correctly used would be helpful.

Definitely, the db_write might require an abort() (see above) in case it detected that it has some missing pieces and can't recover. In that case we should be able to prevent lightningd from doing anything that'd touch the state.

The openchannel hook doesn't have an underscore in its name. I might expect it to be named open_channel following convention of the others.

My rationale for chosing the names for hooks and notifications was <subsystem>_<verb/action>, maybe we could register an alias and slowly migrate over to this over time?

[jarret]

Yeah, I like being maximally explicit with JSON - the human-readable quality is the benefit anyway, so a continue being spelled out makes sense.

The only minor issue I can imagine with continue word is that I have seen API directives like this used to parallel the return; continue; and break; statements in many languages as how they would behave in a function/loop. If you are iterating a list of items via repeated calls continue would mean "give me the next one", break would mean "end this particular iteration" and return would mean "I am done altogether, now leave me alone". I have seen proceed being the "execute the next line of code normally" directive.
Just throwing that out there in case there are other more advanced interactive hook behavior imagined where we could get some benefit from a bit of design foresight like that. I suppose an abort and/or exit directive would fit into such a design, too.

What would you suggest to make the distinction clearer?

One thought I had was a design fix where each hook also becomes a notification by default, such that you are able to optionally register for all the hooks as a notification. If registered as such, it would then default to the 'do nothing' return behavior of the hook. It might be that a common use of many of the hooks is to just snoop on the info (i.e. just use them exactly like they are a notification).

My rationale for chosing the names for hooks and notifications was _<verb/action>, maybe we could register an alias and slowly migrate over to this over time?

That sounds alright now that I understand it. I am not particularly studied on internals of the implementation, so I am reading the docs with the internals being a bit of a black box to me. Not sure whether that is a good thing or a bad thing at this point. 😋

[ZmnSCPxj]

One thought I had was a design fix where each hook also becomes a notification by default, such that you are able to optionally register for all the hooks as a notification. If registered as such, it would then default to the 'do nothing' return behavior of the hook. It might be that a common use of many of the hooks is to just snoop on the info (i.e. just use them exactly like they are a notification).

This becomes possible with hook chaining, but there is a performance penalty involved. Hooks are synchronous and lightningd stops what it is doing until the plugin replies. Notifications are asynchronous and the plugin does not have to reply (and lightningd can send notifications to all plugins simultaneously, whereas with hook chaining we have to check with each hook if it wants to do something now or we can proceed with the next chained hook), lightningd continues. In terms of risk of crashing lightningd when a plugin crashes and stops responding, notifications are safer and should be preferred, but we still need the ability for plugins to modify lightningd behavior. Thus we have a separation between hooks and notifications.

[ZmnSCPxj]

Looking at the code, invoice_payment will happily accept a {'result':'continue'} as well: it will just change its behavior if you (1) return an object and (2) if the returned object has a 'failure_code' field. I am unsure if we need to change invoice_payment, maybe just change its documentation?

Proposal:

• invoice_payment can accept an object with either {'failure_code' : 42} or {'result':'continue'} or {'result':'reject'}. The last will be equivalent to using incorrect_or_unknown_payment_details failure code, which is the most generic failure possible.

[ZmnSCPxj]

And then there is the new custommsg hook. Documentation recommends returning null (current hook just ignores the response). I think documentation should instead recommend returning {'result':'continue'} for consistency.