Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow plugin hook chaining #2796

Closed
ZmnSCPxj opened this issue Jul 6, 2019 · 6 comments
Closed

Allow plugin hook chaining #2796

ZmnSCPxj opened this issue Jul 6, 2019 · 6 comments

Comments

@ZmnSCPxj
Copy link
Contributor

ZmnSCPxj commented Jul 6, 2019

Currently, only one plugin at a time can register for each hook. If two plugins want to register the same hook, then they have to be merged into a single plugin. This gets worse, if multiple plugins want to register the same hook.

I propose to add to each hook, the ability to chain hooks.

This is useful for many things.

Motivating Examples

Multiple backup strategies.

Currently, it is possible to connect only a single "backup" plugin to your node.

However, consider that you might want to have not just a single backup policy, but multiple:

  • A plugin that replays the db queries on an sqlite database on the same machine, but on a different physical persistent storage.
  • A plugin that sends the db queries unencrypted over an encrypted connection, to a receiver on a remote computer you control,
  • A plugin that encrypts the db queries, then saves it on some cloud-rented storage.

We could write a single plugin that does all the above and maintain a complicated plugin whose many functionalities are controlled by multiple options.

Or we could instead write one plugin for each backup policy and retain simplicity, and simply connect multiple plugins to the db_hook.

In principle db_hook is a "broadcast"-type hook, with the added wrinkle that all backup plugins should be in lockstep.

Multiple uses of htlc_accepted

Currently we could use htlc_accepted for many usecases:

Combining the disparate intended uses into a single plugin would be ridiculously unwieldy.

Proposal

When we define a hook in C-Lightning, we should add to the hook data also the below:

  • a combiner_function
  • a default_result

Then, hook callbacks no longer have to check if there is a result or not --- semantically, there is always a result.

Each plugin_hook then maintains a list of plugins connected to that hook. When a plugin is killed, it is removed from the list of plugins (care must be taken in the implementation of the list and list traversal in that a plugin may die either before, during, or after it is called on a hook).

If there is no plugin connected to the hook, the default_result is parsed as JSON and hook callbacks respond to that.

If there is at least one plugin connected, then after one plugin successfully returns in some result, we always invoke the combiner_function. It is an async function, and is give a callback (combined_result_cb) that will be invoked if it can provide the result now. Generally its choices are:

  • plugin_hook_combine_next(combined_result_cb, combined_result_cb_arg) - Call the next plugin; if there is a next plugin, call the combiner_function on it and do this processing again. If there is no next plugin, return default_result.
  • combined_result_cb(buffer, toks, combined_result_cb_arg) - Return the given result immediately and do not call other plugins.

Formally, the combiner function is a function you would pass to a foldr, and the default result is the zero you pass to foldr. You then invoke the list, i.e. foldr combiner_function default_result $ map (\f -> f hook_payload) registered_plugins_list.

Then, for existing hooks:

  • openchannel - default_result = {'result': 'continue'}, combiner_function - if result is continue then call plugin_hook_combine_next(), else call combined_result_cb with current result.
  • peer_connected - default_result = {'result': 'continue'}, combiner_function - if result is continue then call plugin_hook_combine_next(), else call combined_result_cb with current result.
  • htlc_accepted - default_result = {'result': 'continue'}, combiner_function - if result is continue then call plugin_hook_combine_next(), else call combined_result_cb with current result.
  • invoice_payment - default_result = {}, combiner_function - if failure_code field does not exist, then call plugin_hook_combine_next(), else call combined_result_cb with current result.
  • db_hook - default_result = {'result': True}, combiner_function - if result is True then call plugin_hook_combine_next(), else create a new result callback and call plugin_hook_combine_next, but return this result to the current callback when the new result callback is later invoked. This causes the db_hook to always be propagated to all plugins, but if any fail, then the hook as a whole fails and we fatal (we want to have as many backups as we can with the latest data, but if any backup fails, there is now a risk of backups getting descynched, so we have to fail: operator then has to pick the backups with most recent data and overwrite other backups manually).
@ZmnSCPxj
Copy link
Contributor Author

ZmnSCPxj commented Jul 6, 2019

What do you think @rustyrussell @cdecker @niftynei ?

@ZmnSCPxj ZmnSCPxj mentioned this issue Jul 6, 2019
@ZmnSCPxj
Copy link
Contributor Author

ZmnSCPxj commented Jul 8, 2019

PING.

Fixes #2666

It would be better if self-paying plugins just use htlc_accepted (as it would not leave any traces in C-Lightning memory at all) instead of paycodes but that requires that we support chaining (because otherwise you will be unable to deploy multiple plugins that need htlc_accepted).

@cdecker suggests in #2794 (comment) to make plugins within plugins, but please. In any case the result of hooks is mostly a Monoid type and the combiner_function is the associative binary operation and the default_result is the identity element. So we could just as well implement the Monoid operation in C-Lightning and dispense with the plugin-within-plugin structure.

@SimonVrouwe
Copy link
Collaborator

This is useful for many things.

This sounds like the swiss-pocket-knife :-). Can you name one concrete example or user that currently need this?

TBH I find it hard to keep up with all the changes, but that's probably because I am not a computer scientist. What is the risk-reward ratio (i.e. risk for bugs/confusion)?

@cdecker
Copy link
Member

cdecker commented Nov 29, 2019

I'm coming around to this idea, I must've been too short-sighted, sorry @ZmnSCPxj.

My concrete case is that I'm starting to pile up a bunch of plugins that are using htlc_accepted (our most prominent hook so far), and I'm tempted to have a cascade of hooks. Other hooks might require different semantics, so I think this is a per-hook type:

  • Chaining: ask each plugin in the order they were registered, if a plugin responds with {"result": "continue"} call the next one, until one responds with a concrete action, or we reach the end of the chain at which point we always call the default internal action
  • Single: for hooks that really can't be chained and we allow only one plugin to register this (today's semantics).
  • Each: for plugin hooks that should be called independently of their response (and this is the one that I was having trouble with since we need a combiner function that combines the results)

I think we should be able to get the first ones working rather quickly, but the interface for the latter needs some careful consideration.

@ZmnSCPxj
Copy link
Contributor Author

ZmnSCPxj commented Dec 17, 2019

The first and the last can be subsumed into the same mechanism. The combiner simply has to do lazy evaluation of the next, thus the internal interface I described. The interface I described is continuation-passing-style, which can be used to implement laziness semantics a la Haskell.

@ZmnSCPxj
Copy link
Contributor Author

A number of hooks are now chainable, and while I would prefer every hook to be chainable, the existing set is fine and we can move the non-chained hooks slowly towards chained-ness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants