Remove Legacy Lightning-RPC Client #579
Conversation
nepet
force-pushed
the
2508-fix-missing-sync
branch
from
9e28a75 to
0233d00
Compare
libs/gl-plugin/src/node/mod.rs
Outdated
|
|
||
| // Move the lock into the closure so we can release it later. | ||
| let rpc = rrpc.lock().await; | ||
| let mut rpc = cln_rpc::ClnRpc::new(rpc_path.clone()).await.unwrap(); |
There was a problem hiding this comment.
We got a bit unlucky here: we use the Mutex to prevent incoming JSON-RPC calls, before the JSON-RPC socket is ready. This is done by acquiring the lock during startup and releasing it once the socket file is present. The internal use-cases are ok, since they by definition are invoked only after the node started and the socket is ready.
The change below however breaks this, by not locking in get_rpc it is now possible for incoming grpc calls to be directly forwarded (not waiting behind a lock) and so they will fail. We will most likely notice this as the startup commands (i.e., the ones that caused the node to start) will fail in a large number of cases.
Please leave the Mutex in get_rpc
There was a problem hiding this comment.
I moved the rpc client into a Mutex inside of a OnceCell for a clean one-time initialization.
cdecker
left a comment
cdecker
left a comment
There was a problem hiding this comment.
Just one regression: a lock is required to ensure that the node has its JSON-RPC socket available before we can try to talk to it.
libs/gl-plugin/src/node/mod.rs
Outdated
| let rpc = self.get_rpc().await; | ||
| let mut rpc = self.get_rpc().await.map_err(|e| { | ||
| tonic::Status::new( | ||
| tonic::Code::Internal, |
There was a problem hiding this comment.
And this is the error messages all "early calls" will run into. Not the best UX.
| @@ -1,336 +1,13 @@ | |||
| tonic::include_proto!("greenlight"); | |||
| use self::amount::Unit; | |||
There was a problem hiding this comment.
This deletion has been so long in the making, thank you ^^
nepet
force-pushed
the
2508-fix-missing-sync
branch
from
0233d00 to
0712d79
Compare
Before there was cln_rpc there was a custom LightningClient that served as the rpc client for cln. Since we now have cln_rpc we can use that instead. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
Syncing to the latest version of the protos. This needed some rework and left us with a bunch of unused code (also thanks to ripping out the cln rpc client LightningClient. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
We want to ensure that we acutally can connect to the lightning-rpc socket before we send any calls to core lightning. Using a OnceCell ensures that we initialize it (only once) but independent of the actuall call. We still need to keep it in a Mutex as `cln_rpc`'s `call_raw_request` does not seem to be async safe. Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
nepet
force-pushed
the
2508-fix-missing-sync
branch
from
0712d79 to
3126b1f
Compare
|
|
||
| static RPC_CLIENT: OnceCell<Arc<Mutex<cln_rpc::ClnRpc>>> = OnceCell::const_new(); | ||
|
|
||
| pub async fn get_rpc<P: AsRef<Path>>(path: P) -> Arc<Mutex<cln_rpc::ClnRpc>> { |
There was a problem hiding this comment.
Looking good, but I wonder if OnceCell is already sufficient to get the semantics we want: the init function is guaranteed to be called at most once. This means OnceCell is already enforcing locking behavior. So as long as we can ensure that the first init call returns only once the socket exists, that is already sufficient, and we do not need to nest the Mutex inside the OnceCell.
Let's discuss that during our next meeting, merging this one in the meantime.
The plugin still had some references to a custom lightning-rpc client here and there which made it dificult to keep the
greenlight.protoin sync. This PR removes this rpc client and syncs thegreenlight.protofor the plugin and the signerproxy.