-
Notifications
You must be signed in to change notification settings - Fork 955
Add Experimental no-MPP, Lsp-Trusts-Client LSPS2 Support #8569
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
base: master
Are you sure you want to change the base?
Conversation
a5a1bc0
to
b64c866
Compare
Signed-off-by: Peter Neuroth <[email protected]>
We need to somehow access the peer id in the jrpc server to know where the response should go. This seems to be the most convenient way for now. We may unclutter this in the future if this results in performance issues. Signed-off-by: Peter Neuroth <[email protected]>
Signed-off-by: Peter Neuroth <[email protected]>
While this is still experimental, we only want to enable the client when explicitly defined! Signed-off-by: Peter Neuroth <[email protected]>
Adds some primitives defined in lsps0 for other protocol messages. Signed-off-by: Peter Neuroth <[email protected]>
Using lsp_id instead of peer as identifier Signed-off-by: Peter Neuroth <[email protected]>
the client is connected to the lsp before sending a request Signed-off-by: Peter Neuroth <[email protected]>
Move the handler to a separate file, and add lsps2_enabled flag to the handler. Signed-off-by: Peter Neuroth <[email protected]>
Add models and options to enable lsps2 on the lsp Signed-off-by: Peter Neuroth <[email protected]>
This commit adds the lsps2_get_info call defined by BLIP052. It also adds a test policy plugin that the LSP service plugin uses to fetch the actual fee menu from to separate the concerns of providing a spec compliant implementation of an LSP and making business decisions about fee prices. Signed-off-by: Peter Neuroth <[email protected]>
Adds the lsps2.buy request to the client and the lsps2.buy handler to the LSP service. Signed-off-by: Peter Neuroth <[email protected]>
Adds the service side (LSP) for a simple no-mpp trusted jit channel opening. This is only an intermediate step, we are going to add support for multiple htlcs. This is experimental and can drain on-chain fees from the LSP if used in public. Signed-off-by: Peter Neuroth <[email protected]>
Adds the full roundtrip to request a jit channel from the LSP. It approves the jit scid returned by the LSP and returns the invoice with the corresponding route-hint. Changelog-Added Experimental support for LSPS2 no-MPP, Lsps-trusts-client mode. See https://github.com/lightning/blips/blob/master/blip-0052.md for further details. Signed-off-by: Peter Neuroth <[email protected]>
We only allow zero_conf channels if we approved the a jit-channel from the LSP in advance. Signed-off-by: Peter Neuroth <[email protected]>
Calling lsps_jitchannel we want to pass through the label and description parameters used to call `invoice` to keep the api close to Core-Lightning Signed-off-by: Peter Neuroth <[email protected]>
b64c866
to
e1ed03f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work @nepet, here are some comments I had, once again, forgotten to submit, last week when I first looked at this. Nothing major, just small stuff really. Happy to see this merged as soon as possible 👍
} | ||
} | ||
|
||
/// Wraps a payload with a peer ID for internal LSPS message transmission. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use a real JSON build like serde_json::json!()
?
let mut json: Value = | ||
serde_json::from_slice(data).map_err(|e| UnwrapError::SerdeFailure(e.to_string()))?; | ||
|
||
if let Value::Object(ref mut map) = json { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially when you use serde_json here for parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be much more comfortable to use if we serde_json::Derive
a struct representing the payload.
/// Unwraps payload data and peer ID, panicking on error | ||
/// | ||
/// This is a convenience function for cases where one knows the data is valid. | ||
pub fn unwrap_payload_with_peer_id(data: &[u8]) -> (Vec<u8>, PublicKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't build these hidden panic wrappers, rather call the fallible version and deal with the fallout.
const MSAT_PER_SAT: u64 = 1_000; | ||
|
||
/// Represents a monetary amount as defined in LSPS0.msat. Is converted to a | ||
/// `String` in json messages with a suffix `_msat` or `_sat` and internally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They defined yet another serialization format? This is getting out of hand, the LSPS people really are reinventing the wheel several times over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have suggested that we should instead consolidate around a shared internal msat representation based on cln_rpc::primitives::Amount
so handing amounts back and forth becomes less of a hassle, but it is likely more work than it's worth...
/// provides more clarity. | ||
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] | ||
#[serde(transparent)] // Key attribute! Serialize/Deserialize as the inner u32 | ||
pub struct Ppm(pub u32); // u32 is sufficient as 1,000,000 fits easily |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are assuming node operators are going to be happy with a mere 100% fee? 😁
|
||
/// Checks that the node is connected to the peer and that it has the LSP | ||
/// feature bit set. | ||
async fn ensure_lsp_connected(cln_client: &mut ClnRpc, lsp_id: &str) -> Result<(), anyhow::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't ensuring, it's checking.
if plugin.option(&lsps2::OPTION_ENABLED)? { | ||
log::debug!("lsps2 enabled"); | ||
let secret_hex = plugin.option(&lsps2::OPTION_PROMISE_SECRET)?; | ||
if let Some(secret_hex) = secret_hex { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should likely have an exit:
if not secret_hex.is_some() {
return ...
}
// Continue unindented.
Otherwise you tend to nest very deeply which is hard to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea being that "if we got down here, then all requirements are set", whereas deeply nested if-else blocks tend to cause confusion "in which branch am I now".
log::debug!("lsps2 enabled"); | ||
let secret_hex = plugin.option(&lsps2::OPTION_PROMISE_SECRET)?; | ||
if let Some(secret_hex) = secret_hex { | ||
let secret_hex = secret_hex.trim().to_lowercase(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lowercase is superfluous.
// --- | ||
|
||
// Fixme: We only accept no-mpp for now, mpp and other flows will be added later on | ||
if ds_rec.expected_payment_size.is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this value being set alone already a signal to abort? I would have expected this if the HTLC amount is < than the expected amount only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice PR @nepet, I only left some minor nits, that can be addressed in followup PRs. Let's merge this and start testing it in real-world scenarios.
} | ||
|
||
/// Deserializes a lowercase hex string to a `Vec<u8>`. | ||
pub fn from_hex<'de, D>(deserializer: D) -> Result<Vec<u8>, D::Error> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hex::encode and hex::decode? Both are already used in many of our other crates.
I took the liberty of fixing up the linter complaints 😉 |
2e7513d
to
d16663e
Compare
const OPTION_ENABLED: options::FlagConfigOption = ConfigOption::new_flag( | ||
"dev-lsps-service-enabled", | ||
"Enables an LSPS service on the node.", | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be experimental-lsps-service
:
- enabled is redundant.
- dev options are supposed to rely on --developer: experimental options do not.
dev options are for internal testing and are completely undocumented. experimental are for documented-but-may-change options.
peer.features.as_deref().map_or(false, |f_str| { | ||
if let Some(feature_bits) = hex::decode(f_str).ok() { | ||
let mut fb = feature_bits.clone(); | ||
fb.reverse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This strikes me as dangerous; requiring byte reversal here in the caller. Would recommend a new feature_bits type or let is_feature_bit_set do this work...
Adds experimental support for LSPS2 [BLIP-0052](https://github.com/lightning/blips/blob/master/blip-0052.md)) in the no-MPP, LSP-trusts-client mode.
This is the first step in a series of PRs aimed at enabling full LSPS2 support. The focus here is just getting the skeleton in place so clients can start interacting with an LSP under the simplest trust model. At this stage it’s limited to single-part payments (no-MPP).
Still WIP:
The intention is to iterate on top of this, adding MPP, better validation, Client-trusts-LSP flow and more complete testing in follow-up PRs.