ElementsProject / lightning

Core Lightning — Lightning Network implementation focusing on spec compliance and performance
Other
2.83k stars 897 forks source link

Dynamic plugins don't get any options. #3731

Open fiatjaf opened 4 years ago

fiatjaf commented 4 years ago

If a plugin is restarted with plugin stop then plugin start it gets empty "options" in the init method call, doesn't matter if the options exist or if they are required.

I tried to circumvent this by calling listconfigs and trying to pick up the options that should be on init from there, but they had vanished from that list too after plugin stop.

cdecker commented 4 years ago

Kind of hard to say that this isn't the intended behavior: we free all resources that were used by a plugin if the plugin exits or is stopped by us, this includes the options.

Command line options don't really mesh well with dynamic plugins at all, and I'm tempted to make non-dynamic the default, so developers need to opt-in to the more complex dynamic behavior. FWIW it's not that complicated to implement a shim that acts like a normal (non-dynamic) plugin towards lightningd but allows restarting the underlying plugin, while keeping the configuration including the options active (and more importantly buffering notifications and hook calls). You can find an example of how this works here: https://github.com/lightningd/plugins/tree/master/autoreload

Another trick that the autoreload plugin does is to look at the parent process' command line and extract the options from there. That's rather an extreme workaround, but it works.

darosior commented 4 years ago

I don't see this as an unexpected behaviour. It seems to me to be natural for a non startup-only plugin not to rely on a startup option.
In addition, we don't allow startup-only (static) plugins to be stop and we don't allow unrecognized startup options :

$ lightningd --aaaa
lightningd: --aaaa: unrecognized option

So I think it's (most of the time) bad design for a dynamic plugin to use startup options which are maybe here, maybe not.

I tried to circumvent this by calling listconfigs and trying to pick up the options that should be on init from there, but they had vanished from that list too after plugin stop.

We free the dangling option when non startup-only plugins (the default) are plugin stoped.

fiatjaf commented 4 years ago

Ok, this may not be unexpected, but it certainly is undesirable!

darosior commented 4 years ago

Why is it undesirable ? Why cannot you use a command line parameter in place of the option ?

I can see the autoclean plugin as a counter-example but it can probably be made a static plugin.

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ Le mercredi, juin 24, 2020 12:13 AM, fiatjaf notifications@github.com a écrit :

Ok, this may not be unexpected, but it certainly is undesirable!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

fiatjaf commented 4 years ago

So if you pass a parameter on the command line when starting lightningd it will be persisted and always accessible from dynamic plugins?

I thought the cli parameters and the config file were treated as exactly the same.

darosior commented 4 years ago

So if you pass a parameter on the command line when starting lightningd it will be persisted and always accessible from dynamic plugins?

No, that's kind of the point isn't it ?

I thought the cli parameters and the config file were treated as exactly the same.

No, lightningd's startup parameters only affect plugins known to lightningd at that time (i.e. plugins started at startup).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

fiatjaf commented 4 years ago

Ok, let's start again.

I don't understand why can't settings (either passed as cli params or in the config file) be persisted by lightningd and directed to each plugin as they're request it through getmanifest. Why?

Also, why do you think dynamic plugins are so different from non-static plugins that they can't get options from lightningd config?

ZmnSCPxj commented 4 years ago

I don't understand why can't settings (either passed as cli params or in the config file) be persisted by lightningd and directed to each plugin as they're request it through getmanifest. Why?

I think the root of the confusion here is that lightningd allows plugins to add their own command-line arguments to the lightningd command line / config file.

plugins/autoclean, for example, adds --autocleaninvoice-cycle and --autocleaninvoice-expired-by command line parameters. lightningd does not know them naturally, yet doing lightningd --help will list them (because even something as simple as --help will load plugins!).

On the other hand, command-line arguments that neither lightningd knows directly, nor were indicated by any plugins, are rejected at command-line parsing stage and error out immediately.

Thus, plugins which are not loaded at the start cannot add their own command-line arguments to lightningd before lightningd has to process command-line arguments. This implies dynamic plugins, which are not loaded at lightningd startup, cannot receive command-line arguments, because they cannot tell lightningd that those command line arguments are valid and should be passed to the plugin.

We need to reject command-line arguments we do not recognize at startup because if the user misspells a command-line argument, we do not want to silently accept it, assuming it is a command-line argument that a dynamic plugin will query for later; so we error out immediately on an unrecognized command-line argument. So only plugins that get loaded right from the start can feasibly provide command-line arguments, because they already exist at that point.

fiatjaf commented 4 years ago

Ok, thank you. That seems reasonable.

fiatjaf commented 4 years ago

Now imagine this:

I have a ~/.lightning/config file like this:

plugin-opt=123

And a plugin that declares that option.

  1. I start lightningd, it reads the config file, finds that option and passes it to the plugin. Fine.
  2. I do plugin stop. At this point the plugin-opt config value is forgotten. Ok.
  3. I do plugin start: now what if at this point ~/.lightning/config was read again by lightningd looking specifically for options declared by that plugin that is just being started and ignoring everything else?
ZmnSCPxj commented 4 years ago

Hmm. I am wary of re-reading the file. This gives a strange inconsistency: what happens if before plugin start the user edited the config file? If we re-read, then the changes to plugin-specific options are updated, but not other options. Hmmm.

fiatjaf commented 4 years ago

My issue here is: if you want an application with super Lightning powers you want to make that application a plugin so it can access notifications and hooks, that's what I did with https://etleneum.com/ and https://t.me/lntxbot. However you may also want options for that app, and you may want to deploy new versions of that app without having to stop lightningd entirely and start again, because that takes a long time, for example, it must sync with the chain again upon restart and will, for example, fail all payments during that time. It's a minor thing (for me) but very annoying. I imagine it would be a bigger issue for a larger application with more users.

ZmnSCPxj commented 4 years ago

Hmmm. For complex enough applications, the plugin may very well be a small part of the larger application that is running elsewhere, with the plugin being a very thin wrapper that just forwards to a registered application.

Still, lemme check, you do have good points.

fiatjaf commented 4 years ago

Maybe this: Save a checksum of the config file and if it changes you have to restart lightningd, no way around it. If however the config file doesn't change (which will be the more common thing considering my cases above) then it can be safely reread and the plugin options can be passed again to the plugin. This creates a problem: if the config file changes for some different reason and you try to plugin start that should fail, but in that situation some naïve automated deployment process won't know what to do.

Ok, not the brightest idea, but I'm posting anyway.

cdecker commented 4 years ago

I share @ZmnSCPxj's wariness about re-reading the config file, I'd rather keep the options in memory after the plugin died. The problem is that we don't differentiate between options from the command line or a config file (or many given we allow includes). So re-reading just the config may end up with an inconsistent mix, even though the file wasn't edited.

Maybe better add a restart option that stops, keeps the options in memory, and then allows the plugin to re-register, add the options back in, and then provides the in memory options. Though, adding a new special case is bad... Dunno 🤨

The lazy developer in me says that not passing options to plugins loaded after startup is the desired outcome, but I can see @fiatjaf's arguments as well.

cdecker commented 4 years ago

For now maybe we can work around these issues by inspecting our parent process' command line for options we're interested in, or alternatively for important things we can set environment variables, which get transparently inherited.

Edit: I am not saying that we shouldn't address this issue, but wanted to provide alternatives if you're blocked on this.

fiatjaf commented 4 years ago

Yes, I was thinking about using an external file with environment variables for everything, but wanted to open this issue anyway. Probably will postpone it until some day when I'm very upset. But don't worry about me.

rustyrussell commented 4 years ago

Options that we accepted once should be persistent. Anything else is non-obvious!

But we should also allow options via plugin start. Deferring this for next release though, sorry!