Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 57 additions & 3 deletions libs/gl-plugin/src/node/mod.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use crate::config::Config;
use crate::pb::{self, node_server::Node};
use crate::storage::StateStore;
use crate::storage::{LspInvoiceMeta, StateStore};
use crate::{messages, Event};
use crate::{stager, tramp};
use anyhow::{Context, Error, Result};
use anyhow::{anyhow, Context, Error, Result};
use base64::{engine::general_purpose, Engine as _};
use bytes::BufMut;
use cln_rpc::Notification;
use cln_rpc::{ClnRpc, Notification};
use gl_client::metrics::{savings_percent, signer_state_request_wire_bytes};
use gl_client::persist::{State, StateSketch};
use governor::{
Expand Down Expand Up @@ -266,6 +266,16 @@ impl Node for PluginNodeServer {
.await
.map_err(|e| Status::new(Code::Internal, e.to_string()))?;

let meta = LspInvoiceMeta {
label: req.label.clone(),
payment_hash: res.payment_hash.to_string(),
requested_amount_msat: req.amount_msat,
bolt11: res.bolt11.clone(),
};

write_lsp_invoice_meta(rpc, meta).await
.map_err(|e| Status::new(Code::Internal, e.to_string()))?;

Comment on lines +269 to +278

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to store the invoice meta data for a regular invoice. It will just double what we already have. Storing it for a jit-channel invoice is enough to identify it as such: If it's in the datastore, it automatically is a jit-channel invoice.

return Ok(Response::new(pb::LspInvoiceResponse {
bolt11: res.bolt11,
created_index: res.created_index.unwrap_or(0) as u32,
Expand Down Expand Up @@ -326,6 +336,9 @@ impl Node for PluginNodeServer {
.div_ceil(1_000_000);
std::cmp::max(min_fee, proportional_fee)
};

let requested_amount_msat = req.amount_msat.clone();
let invoice_label = req.label.clone();

// Use the new RPC method name for versions > v25.05gl1
let mut res = if *version > *"v25.05gl1" {
Expand All @@ -343,6 +356,20 @@ impl Node for PluginNodeServer {
};

res.opening_fee_msat = opening_fee_msat;

// A JIT channel has now been negotiated with the LSP for this
// invoice. So, we're storing some data with the original requested
// amount.
let meta = LspInvoiceMeta {
label: invoice_label.clone(),
payment_hash: res.payment_hash.clone(),
requested_amount_msat,
bolt11: res.bolt11.clone(),
};

write_lsp_invoice_meta(rpc, meta).await
.map_err(|e| Status::new(Code::Internal, e.to_string()))?;
Comment on lines +370 to +371

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to fail the whole RPC call in the (unexpected) case that we can't write the meta data? We already negotiated a jit-channel invoice and stored it on CLN. I think it's enough to just log::warn and continue. We won't loose much.


Ok(Response::new(res.into()))
}

Expand Down Expand Up @@ -837,6 +864,33 @@ impl Node for PluginNodeServer {
}
}

/// Writes `LspInvoiceMeta` using datastore request. `LspInvoiceMeta` is useful for defining
/// some additional information regarding invoice being requested trough Greenlight.
async fn write_lsp_invoice_meta(mut rpc: tokio::sync::MutexGuard<'_, ClnRpc>, meta: LspInvoiceMeta)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The signature is a bit weird, compared to the other callers of ClnRpc in this file. This function takes ownership of the lock, which means that the caller can't use rpc afterwards. Every other helper in this file takes &mut rpc:

async write_lsp_invoice_meta(rpc: &mut ClnRpc, meta: LspInvoiceMeta)

-> Result<()> {
let record_serialized = serde_json::to_string(&meta)
.map_err(|e| Status::new(Code::Internal, e.to_string()))?;
Comment on lines +871 to +872

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This function returns an anyhow::Result. The extra serialization into a tonic::Status seems a bit useless. It's coerced into anyhow::Error and the caller converts it back into a tonic::Status.

Suggested change
let record_serialized = serde_json::to_string(&meta)
.map_err(|e| Status::new(Code::Internal, e.to_string()))?;
let record_serialized =
serde_json::to_string(&meta).context("failed to serialize LspInvoiceMeta")?;


let datastore_req = cln_rpc::model::requests::DatastoreRequest {
key: vec![
"gl".to_string(),
"jit_channels".to_string(),
meta.label,
],
Comment on lines +874 to +879

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on what info the reading part actually has, I'd rather use the payment_hash as the preferred key instead of the label - which by the way can be an ugly (maybe even unbounded) string.

Suggested change
let datastore_req = cln_rpc::model::requests::DatastoreRequest {
key: vec![
"gl".to_string(),
"jit_channels".to_string(),
meta.label,
],
let datastore_req = cln_rpc::model::requests::DatastoreRequest {
key: vec![
"gl".to_string(),
"lsp_invoices".to_string(),
meta.payment_hash,
],

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I thought of using payment hashes here but was aware of some ambiguity between invoices.

Since the original and reduced invoices have different amounts, payment hashes will also be different. In other words, we're storing an invoice with the key containing the original invoice. On GL client side when we intercept the invoice payment, the payment hash corresponds to the reduced invoice. Therefore, we'll fail to retrieve the original invoice by the reduced payment hash.

My intention about using invoice label was in it's nature - it is unique among all created invoices. That means, both original and reduced invoice should have the same label (uniqueness applies between original and reduced invoices because the original invoice invoice is not stored on client). It's indeed an ugly way, but I'm not sure about any other ways

string: Some(record_serialized),
hex: None,
mode: Some(cln_rpc::model::requests::DatastoreMode::CREATE_OR_REPLACE),
generation: None,
};

rpc.call_typed(&datastore_req).await.map_err(
|e|
anyhow!("Failed to store JIT channel negotiation data in datastore: {}", e)
)?;

Ok(())
}

use cln_grpc::pb::node_server::NodeServer;

#[derive(Clone, Debug)]
Expand Down
20 changes: 20 additions & 0 deletions libs/gl-plugin/src/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

pub use gl_client::persist::State;
use log::debug;
use serde::{Deserialize, Serialize};
use thiserror::Error;
use tonic::async_trait;

Expand Down Expand Up @@ -66,3 +67,22 @@ impl StateStore for SledStateStore {
.map_err(|e| e.into())
}
}

/// A structure that is used for storing invoices that are requested through
/// [Node::lsp_invoice](pb::node_server::Node::lsp_invoice) RPC call. lsp_invoice
/// call does not guarantee that the returned invoice is for requesting JIT
/// channel - if there is a channel with enough liquidity, a simple bolt11 invoice
/// is created.
///
/// This structure is stored in CLN datastore. The reason of why do we need this
/// structure instead of querying invoices table is that we want to distinguish
/// incomming payments whether they were for JIT channel opening or just a simple
/// payment. Currently, CLN does not allow to do so, that's why this workaround
/// exists.
#[derive(Serialize, Deserialize)]
pub struct LspInvoiceMeta {
pub label: String,
pub payment_hash: String,
pub requested_amount_msat: u64,
pub bolt11: String,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we could also store the peer_id and the expected_amount_msat. But feel free to leave it out if it's not required.

}
Loading