ElementsProject / lightning

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

Remove the UNUSUAL warning about plugin bcli #3553

Closed ghost closed 4 years ago

ghost commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

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 commented 4 years ago

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 commented 4 years ago

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:

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 commented 4 years ago

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