ElementsProject / lightning

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

Add support for plugin-defined RPC methods to `cln-plugin` #6080

Open justinmoon opened 1 year ago

justinmoon commented 1 year ago

I created a plugin using cln_plugin and defined a custom rpcmethod. Now I would like to call this method using cln-rpc.

It seems this isn't possible currently without forking cln-rpc.

Relevant discord discussion

CC @vincenzopalazzo

wtogami commented 1 year ago

Is that grpc?

vincenzopalazzo commented 1 year ago

Is that grpc?

it is not possible in the grpc because you need the model somehow, instead in the rpc interface you can, you need just an object that can be encoded to json

wtogami commented 1 year ago

They don't have plans to enable dynamic plugins to add RPC commands available via grpc.

Have you considered commando? It works with dynamic plugin RPC's and I hear it's quite nice as an alternative way to interface CLN.

justinmoon commented 1 year ago

Have you considered commando?

I would rather just add one RPC command than another plugin.

wtogami commented 1 year ago

I heard it might be very difficult for grpc to support adding rpc's of dynamic plugins. It might require a restart. It might require the compiler to be on the deployed host.

justinmoon commented 1 year ago

I heard it might be very difficult for grpc to support adding rpc's of dynamic plugins. It might require a restart. It might require the compiler to be on the deployed host.

That sounds right. For me I'm fine with just using JSON-RPC for now.

vincenzopalazzo commented 1 year ago

I heard it might be very difficult for grpc to support adding rpc's of dynamic plugins. It might require a restart. It might require the compiler to be on the deployed host.

Yeah this is one of the corner case of a typed client, in fact all my API library are going to a free type form. I think the grpc is not the right one for this kind of dinamicity?

cdecker commented 1 year ago

There is a fundamental disagreement between GRPC and dynamic interfaces: grpc interfaces are statically typed and derived at compile time from the protobuf definitions, whereas dynamic interfaces by their very nature are discovered at runtime.

We can however extend the generic mechanism used in cln-rpc to call just about any JSON-RPC method if we are ok working with JSON, rather than the strongly typed bindings built on top of that generic functionality.

At the moment the conversion chain is as follows:

graph LR
native --call--> A[bindings] --protobuf--> cln-grpc --Rust request--> cln-rpc --JSON request--> lightningd
lightningd --JSON response-->cln-rpc --Rust response-->cln-grpc --protobuf--> A --return--> native

We could add a bypass to all those conversions, build the JSON-RPC request on the native side and then pass it through unchanged:

graph LR
client --JSON--> bindings
bindings --proto+JSON-->cln-grpc
cln-grpc --unwrap JSON-->cln-rpc
cln-rpc --JSON --> lightningd
lightningd --JSON--> cln-rpc
cln-rpc --wrap JSON--> cln-grpc
cln-grpc --proto+JSON-->bindings
bindings --JSON-->client

This allows us to call anything that is expressible as a JSON-RPC call through the bindings, by moving how things get converted between the various components. The downside is of course that there is no strongly typed layer in any of this, and the client needs to know how to create and interpret JSON-RPC requests and responses. It may also impact authorization systems built into any of these layers, since it can be used to call non-dynamic methods as well, potentially sidestepping any access control that'd use its knowledge of calls, but that is likely less of a problem.

What do you guys think?

fusion44 commented 1 year ago

I was just about to open an issue asking if the bookkeeper plugin will be added to Core Lightning. Is there a way to keep the statically typed interface for core APIs and plugins, but add a dynamic JSON method on top?

cdecker commented 1 year ago

Yep, that's the idea. For the bkpr we should likely just add the schema since it is being shipped with the CLN release (which we consider built-in for the release).

justinmoon commented 1 year ago

What do you guys think?

This would be useful to me

daywalker90 commented 2 months ago

The issue in the OP is now possible with call_raw right?

Jhoyola commented 2 months ago

The issue in the OP is now possible with call_raw right?

Can you expand on that? I don't see a call_raw on the grpc definition.

This would be useful for us too at Torq. But we can bypass it by creating a separate grpc server inside the plugin.

daywalker90 commented 2 months ago

The issue in the OP is now possible with call_raw right?

Can you expand on that? I don't see a call_raw on the grpc definition.

This would be useful for us too at Torq. But we can bypass it by creating a separate grpc server inside the plugin.

the issue in the OP is about cln-rpc not cln-grpc. With grpc in general you need to predefine all your functions before starting the server.