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

Remove the UNUSUAL warning about plugin bcli #3553

Closed
ghost opened this issue Feb 28, 2020 · 6 comments · Fixed by #3857
Closed

Remove the UNUSUAL warning about plugin bcli #3553

ghost opened this issue Feb 28, 2020 · 6 comments · Fixed by #3857

Comments

@ghost
Copy link

ghost commented Feb 28, 2020

The first c-lightning output a user sees is the following message:

UNUSUAL plugin-bcli: Could not connect to 'lightning-rpc': Connection refused

as kindly explained by darosior

It's the expected behavior: a new plugin (bcli) introduced in the last release needs to be initialized before we even start listening on the RPC socket, hence it fails the connect().

If it's expected behavior, c-lightning shouldn't output UNUSUAL. It got me on the wrong track when updating to version 0.8.1. This should be corrected in some way.

@darosior
Copy link
Collaborator

I made it an UNUSUAL because it was before a fatal error, but also because we use UNUSUAL log for some others startup things ("Bitcoin now synced", etc..). Maybe you got confused because you were used to the other UNUSUAL log lines but not this one ? :)

@ghost
Copy link
Author

ghost commented Feb 29, 2020

I don't know,. It looks unusual to me :-)

It's not a big thing of course. I created this issue as a reminder. You developers can decide to close it or keep it open.

@ZmnSCPxj
Copy link
Collaborator

ZmnSCPxj commented Mar 3, 2020

Perhaps bcli can be modified, or use a modified libplugin entry point, so that the libplugin does not attempt to create an RPC interface to the plugin.

So:

  • Classic libplugin plugins like pay etc still FATAL if RPC interface is not available, as before.
  • A libplugin plugin like bcli which does not need to send commands to the lightningd does not initialize RPC interface.

Choosing unusual here feels like prevaricating: classic plugins still want an RPC interface, but the rare plugin (like bcli) might not need it and would be better off not wasting time establishing it.

@darosior
Copy link
Collaborator

darosior commented Mar 3, 2020

I think libplugin should be as general as possible, and requiring the socket is the special case for a plugin (a lot of the plugins from the community curated list dont send command to lightningd, for example).
But maybe I'm over-generalizing ?

About the special entrypoint it means charging plugin_main() even more, i think that's the reason I prefered the minimal change of logging instead of dying.

@ZmnSCPxj
Copy link
Collaborator

ZmnSCPxj commented Mar 4, 2020

Complications exist, and will remain existent. While we strive to reduce complications, in the case where we cannot, we can at least strive to keep the complication contained.

So I suggest doing something like:

  • plugin_main - normal plugin_main with classic behavior.
  • plugin_main_core - Adds some more flags, including an RPC/noRPC flag.
  • plugin_main_no_rpc - Like plugin_main but cannot issue commands to lightningd afterwards.

Then plugin_main just calls plugin_main_core with the RPC/noRPC flag set to RPC (i.e. it is an adaptor wrapper). And plugin_main_no_rpc does the same but with the flag set to noRPC. The plugin_main_core can be kept static within libplugin.c, thus containing the complication to only within libplugin.c. Classic plugins can keep using plugin_main without source modification, just a recompile, and bcli can use the new plugin_main_no_rpc.

What is happening now is that the complication leaks all the way up to the user, who is now confused with an unusual message, leading to this issue. Since this is a complication (the original libplugin was a simple library that did not consider such complications, having been designed in older, more innocent days; this tends to be inevitable in code which has to ship someday), leaking this complication to the user instead of properly wrapping it in our code is less desirable.

@nanotube
Copy link

FWIW, I am here because I saw the same message on my first lightningd run, and was curious if it is normal. :)

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

Successfully merging a pull request may close this issue.

3 participants