-
Notifications
You must be signed in to change notification settings - Fork 896
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
Dynamic plugins are not passed lightningd config options? #5294
Comments
Sorry! I miss understanding the issue! The question is on the title of the issue and not in the body! I never check this but it should be receiving all the information, but we have no change between lightning 0.10.2 and 0.11, are we? What wrapper are you using? it is written in go right? |
https://github.com/elementsproject/glightning You mean dynamic plugins written in other languages are able to get config options from lightningd conf? |
I'm just asking because I don't understand why 0.10.2 works and the 0.11, no? from the docs (https://lightning.readthedocs.io/lightning-plugin.7.html) I'm not able to see a way to pass the plugin conf, but I need to check also the code! |
Oops. I meant I tested it with only v0.10.2 where conf options are passed to plugins only during lightningd startup, not during a dynamic load afterwards. I didn't test 0.11 but I had guessed it's the same there. In any case this is unexpected for it to behave differently and it might be the cause of confusion. Can we change it so plugins are always told conf options? |
Yes this is currently the case, also in latest version.
It sounds reasonable, but I am curious what use case is there (other then for plugin development) to need to restart a plugin that registered command line options? Anyway I hacked something together, with some assumptions:
|
Why these restrictions? It sounds a desire to protect lightningd from plugins being passed sensitive info? If so that's futile because of the wide access plugins have during runtime. It sounds like these restrictions would lead to further difficult to understand behavioral surprises where a lightning conf registered plugin would behave differently from a non-config dynamic plugin. |
The entire reason for this ticket was to ask for lightning config plugins to behave exactly the same as plugins started later. Passing any option a plugin asks for I would think is the best way to avoid behavioral surprises when you expect it to behave the same either way. |
To keep it is simple as possible and (belief it or not) to prevent behavioral surprises :) For example pluginA registers the option name Sharing options between plugins IMHO is also asking for trouble. So an option "name" should be unique and be associated with the plugin (identified via absolute path) that registered it.
Command line (config) parsing is only done at Note also that plugins don't need to use
Good example of above? |
Well I should have read manual more carefully myself.
You pass options to a dynamic plugin by adding it as parameter to Would this solve your issue? |
No, that is terribly inconvenient. If that is all we had in lightningd then I'd opt to instead move all plugin options into its own config file as that would be less burdensome for users to get expected behavior.
|
Good point, I think you are right. AFAIK plugins internal configs are not really relevant to
I came a long way, but the fact that parameters to the Whatever we choose effectively becomes the (new) default, i.e. what we get when we call |
…ne options, + extra checks Command line (config) options are parsed only when lightningd starst up. Before this commit a dying plugin would loose these parsed option's values. It is a little improvised because of ccan/opt limitations and adds additional checks on plugin's (incl. buildin plugins) options: - any option "name" is only be registered once, or aborts lightningd startup, or kills the (dynamic) plugin - an option "name" is _simple_ and consist of [a-zA-Z0-9_-] chars, so no aliases via '|' - an option is only reused (and taken) by the *original* plugin, i.e. options "name", "type" and absolute path should match Fixes ElementsProject#5294
Just to sum up my thoughts (and lessons):
Plugins can always make a copy (on disk) of options it received at startup (check the "startup" field "init" parameter) and reuse them with a There where some issues I came across during hacking (w.r.t. unique options names etc.) for which I will create a PR. |
This is problematic :( We parse the options straight into the plugin; if it goes away, we lose the memory of those options. If the plugin comes back and no longer accepts those same options, it would now fail; there's no clean way to clear options short of restarting. Maybe that's an OK failure mode? |
Requiring -k of options to
I am not concerned about this case. If the plugin's expected options change then it is fine to require restarting lightningd. |
It's not
What I mean here, when a user executes a command without arguments it expects (fixed) default behavior. |
... but commands that have a config file definitely remember those options! It's not impossible, it just requires some re-engineering. It means properly recalling options even after the plugin has gone. |
@wtogami If you really insist on this special behavior, you can engineer it in the plugin.
This works with current version, only reused values will not appear in the |
My Position:
|
I agree with Warren here. If I were to stop and restart a plugin that has options configured, I'd expect it to start with these options. Deviating from this behaviour could have grave consequences for the node operator (depending on the plugin's functionality and the options). |
This isn't going to get fixed for 0.12, sorry. I recommend you manually parse $HOME/.lightning/config and $HOME/.lightning//config if they exist, and parse any lines starting with Use those results as the defaults before overriding with any the init method hands you, so you will properly overwrite if they specify parameters manually. Then document the limitation for 0.12: that peerswap parameters on restart will only be found if they're directly in one of those two config files, not in any included files. That will be fixed in 0.13. |
I'm glad that you agree with the intent. Thank you! |
~/.lightning/conf contains:
Conf options are passed as expected only during initial startup of
lightningd
.You would expect the plugin to behave the same way but it is not passed the lightningd config options when started as a dynamic plugin.
My Position:
lightningd
config file I expect to behave identically whether started during init or later withstart
. Anything different from this expectation is terribly confusing and difficult to support. (New evidence today: I witnessed a well-meaning sysadmin trying to "help" a failed plugin by usingstart
manually upon it then wondering why it was wildly misbehaving.)lightningd
config passed options to putting everything into the plugin's own config file. In the case of PeerSwap this isn't a big deal because the LNDpeerswapd
already has its own config file so it would be simple to switch. It would be a shame though if we don't fix this because several other CLN plugins rely upon config options and I guarantee you those users had been confused when they don't behave the same upon a re-start
.The text was updated successfully, but these errors were encountered: