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 hook chaining for the `htlc_accepted` hook #3489

Merged
merged 9 commits into from Feb 11, 2020

Conversation

@cdecker
Copy link
Member

cdecker commented Feb 6, 2020

This pull request creates the infrastructure for registering multiple plugins
for a given hook and showcases this new capability with the htlc_accepted
hook. The htlc_accepted hook was chosen due to its relative simple semantics
and because it currently is the hook with the most plugins making.

Before you could only use a single plugin that registers the htlc_accepted
hook at a time, which was severely limiting its uses. With this you can
finally run multiple at the same time.

The htlc_accepted hook returns one of the following three outcomes:

  • {"result": "continue"}: the plugin signals that it could not handle the
    HTLC internally, so the chain continues. If there are more plugins that
    have registered for this hook they get called, otherwise we handle
    internally.
  • {"result": "fail", ...}: the plugin decided that the HTLC should be
    rejected, with the given parameters. This exits the call chain and fails
    the HTLC. If there are more plugins they will not get called.
  • {"result": "resolve", ...}: similar to the previous case, the plugin
    knows the preimage and choses to resolve this HTLC. The call chain is
    exited, and no further plugins are called.

Future steps

The following details are as of yet not addressed in this pull request, but I
plan to continue working on these in new PRs:

  • Allow plugins to provide a numeric priority which determines the order in
    which the plugins should be called
  • Allow more hooks to be chained. We currently only enable htlc_accepted to
    register multiple times, and we will enable more as the need arises and the
    semantics are clarified.
  • Potentially we could add a fanout type that calls all hooks in parallel,
    and collected the results again. One potential candidate is the db_write
    hook whose results can be combined into all continues or at least one
    fail.

Related issues

  • Allow plugin hook chaining (#2796)
  • Bitcoin backend plugins planning (#3354)
  • Add a sendcustommsg RPC call and a custommsg plugin hook for
    experimental protocol extensions (#3315)
@cdecker cdecker added this to the 0.8.1 milestone Feb 6, 2020
@cdecker cdecker requested review from rustyrussell, darosior and ZmnSCPxj Feb 6, 2020
@cdecker cdecker self-assigned this Feb 6, 2020
@cdecker cdecker force-pushed the cdecker:plugin-hook-chaining branch 2 times, most recently from 70e5355 to 7367597 Feb 6, 2020
Copy link
Collaborator

darosior left a comment

ACK 7367597

Tested with hacky jitrebalance.py + noise.py combo tests.

Unrelated Travis spurious failure (actually common on all PRs)

lightningd/plugin_hook.c Outdated Show resolved Hide resolved
tests/plugins/hook-chain-even.py Outdated Show resolved Hide resolved
lightningd/plugin_hook.h Outdated Show resolved Hide resolved
Copy link
Contributor

rustyrussell left a comment

Think we might now be buggy if we remove a plugin while it's supposed to be answering a hook? Request used to be freed previously, what now?

There are several possible fixes for plugins being removed while we're calling hooks.

The most specific would be to re-search hook.plugins for the plugin we were just on, and call the next one (O(n^2) but n is tiny). But the plugin could have been freed (unless we fix the above). We could attach a u64 for each hook.plugins[] entry (assigned off some global counter), and look for the next u64 instead.

lightningd/plugin_hook.c Show resolved Hide resolved
lightningd/plugin_hook.c Outdated Show resolved Hide resolved
lightningd/plugin_hook.c Outdated Show resolved Hide resolved
lightningd/plugin_hook.c Show resolved Hide resolved
lightningd/peer_htlcs.c Show resolved Hide resolved
@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

ZmnSCPxj commented Feb 10, 2020

Think we might now be buggy if we remove a plugin while it's supposed to be answering a hook? Request used to be freed previously, what now?

What I was planning for this was that invocation of a hook would create a new list of plugins currently registered on that hook. Each list element would have a pointer to the plugin and a pointer to the hook invocation; the list element would be tallocated off the hook invocation object, but would have a tal_destructor2 triggered by detsruction of the plugin object. If the plugin object is destroyed, we check if the hook invocation was currently calling into the plugin, and schedule the hook invocation to call into the next plugin; regardless we would then remove and tal_free the list element from the hook invocation list. When hook chaining completes --- if a plugin returns non-{'result':'continue'} or all plugins have been invoked --- then we unregister the detructors on the plugins and destroy the hook invocation object (could be a tal_destructor on the hook invocation object). This allows multiple parallel invocations of the hook (each invocation has its own list which it traverses independently of the other lists) and removal of plugins while our hook is calling into it as well.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Feb 10, 2020

Think we might now be buggy if we remove a plugin while it's supposed to be answering a hook? Request used to be freed previously, what now?

...

If the plugin object is destroyed, we check if the hook invocation was currently calling into the plugin, and schedule the hook invocation to call into the next plugin`

Even with the current scheme, we could walk all plugin_hook_request (we'd need a list somewhere), and if we're about to remove the hook->plugins[req->current_plugin] then we do req->current_plugin--.

We still need to check that we don't crap ourselves if the plugin gets removed while a hook is pending, though. Let me try that...

@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Feb 10, 2020

OK, testing answered my question. If a plugin is freed, the requests are abandoned.

This mean an HTLC is stuck, which is kinda nasty, but OK for now. I've filed a separate issue #3496 to track that.

So there's no issue for now with this PR, please cleanup and apply.

@cdecker

This comment has been minimized.

Copy link
Member Author

cdecker commented Feb 10, 2020

Yes, I had noticed that as well, and was planning to address this in a separate PR (using current_plugin-- as @rustyrussell suggested). I probably should have mentioned that in the OP to avoid having reviewers need to check.

I'll address the feedback asap and update the PR 👍 Thanks for the review @rustyrussell and @ZmnSCPxj

@cdecker cdecker force-pushed the cdecker:plugin-hook-chaining branch 3 times, most recently from e9ace4b to d0ae53b Feb 10, 2020
@cdecker

This comment has been minimized.

Copy link
Member Author

cdecker commented Feb 10, 2020

Added fixup!s to address the review, rebased and squashed.

You can see the changes between the version reviewed and the current version of the PR here

@cdecker

This comment has been minimized.

Copy link
Member Author

cdecker commented Feb 10, 2020

Now having a failing assertion:

lightningd: lightningd/plugin_hook.c:135: plugin_hook_call_next: Assertion `ph_req->current_plugin < tal_count(hook->plugins)' failed.

Need to look into this.

Edit: Nevermind, turns out I killed the wrong one of the two overlapping checks. Now it works :-)

@cdecker cdecker force-pushed the cdecker:plugin-hook-chaining branch from d0ae53b to 6cd0d59 Feb 10, 2020
cdecker added 9 commits Feb 4, 2020
The newly introduced type is used to determine what the call semantics of the
hook are. We have `single` corresponding to the old behavior, as well as
`chain` which allows multiple plugins to register for the hook, and they are
then called sequentially (if all plugins return `{"result": "continue"}`) or
exit the chain if the hook event was handled.
Switch from having a single plugin to a list of plugins. If the hook is of
type single we will enforce that constraint on the number of registered
plugins when attempting to add.
We are about to call multiple plugins, and we'll have to pass the payload into
each call. Sadly the serialized stream gets consumed during the call, so keep
the unserialized payload around.
We will be using `plugin_hook_call_next` as part of the loop to traverse all
plugins that registered the hook, so group initialization in the init function
and move per-plugin logic into `plugin_hook_call_next`
This used to be necessary because we allocated the `plugin_hook_request` off
of the plugin instance (only tal allocated object we could grab at that
time. Now the plugin was replaced by a list, which itself is tal-allocated,
making that workaround pointless, or even wrong once we have multiple plugins
registering for that hook.
Make the `htlc_accepted` hook the first chained hook in our repertoire. The
plugins are called one after the other in order until we have no more plugins
or the HTLC was handled by one of the plugins. If no plugins handles the HTLC
we continue to handle it internally like always.

Handling in this case means the plugin returns either `{"result": "resolve",
...}` or `{"result": "fail", ...}`.

Changelog-Changed: plugin: Multiple plugins can now register for the htlc_accepted hook.
Triple nesting seems a bit excessive, I can't even read the titles in the
sidebar of http://lightning.readthedocs.org anymore :-)
@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Feb 11, 2020

Trivial rebase...

@rustyrussell rustyrussell force-pushed the cdecker:plugin-hook-chaining branch from 6cd0d59 to d8c2a05 Feb 11, 2020
@rustyrussell rustyrussell merged commit c223932 into ElementsProject:master Feb 11, 2020
3 of 4 checks passed
3 of 4 checks passed
bitcoin-bot/acks Need more acks: 0 out of 1 required
bitcoin-bot/changelog This PR has at least one changelog entry
bitcoin-bot/fixups PR does not contain unsquashed fixups
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.