-
Notifications
You must be signed in to change notification settings - Fork 418
Support client_trusts_lsp on LSPS2 #3838
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: main
Are you sure you want to change the base?
Changes from 3 commits
0ffe4af
bd0892d
6412945
89f6d59
b043972
3562aa8
644cec2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ use lightning::util::logger::Level; | |
use lightning_types::payment::PaymentHash; | ||
|
||
use bitcoin::secp256k1::PublicKey; | ||
use bitcoin::Transaction; | ||
|
||
use crate::lsps2::msgs::{ | ||
LSPS2BuyRequest, LSPS2BuyResponse, LSPS2GetInfoRequest, LSPS2GetInfoResponse, LSPS2Message, | ||
|
@@ -107,6 +108,89 @@ struct ForwardPaymentAction(ChannelId, FeePayment); | |
#[derive(Debug, PartialEq)] | ||
struct ForwardHTLCsAction(ChannelId, Vec<InterceptedHTLC>); | ||
|
||
#[derive(Debug, Clone)] | ||
enum TrustModel { | ||
ClientTrustsLsp { | ||
funding_tx_broadcast_safe: bool, | ||
payment_claimed: bool, | ||
funding_tx: Option<Arc<Transaction>>, | ||
}, | ||
LspTrustsClient, | ||
} | ||
|
||
impl TrustModel { | ||
fn should_manually_broadcast(&self) -> bool { | ||
match self { | ||
TrustModel::ClientTrustsLsp { | ||
funding_tx_broadcast_safe, | ||
payment_claimed, | ||
funding_tx, | ||
} => *funding_tx_broadcast_safe && *payment_claimed && funding_tx.is_some(), | ||
// in lsp-trusts-client, the broadcast is automatic, so we never need to manually broadcast. | ||
TrustModel::LspTrustsClient => false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this always be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually the method name is confusing. this should be false because in lsp-trusts-client, the broadcast is automatic, so we should return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixup commit changes this but will revert in a future commit, that will also include an e2e test |
||
} | ||
} | ||
|
||
fn new(client_trusts_lsp: bool) -> Self { | ||
if client_trusts_lsp { | ||
TrustModel::ClientTrustsLsp { | ||
funding_tx_broadcast_safe: false, | ||
payment_claimed: false, | ||
funding_tx: None, | ||
} | ||
} else { | ||
TrustModel::LspTrustsClient | ||
} | ||
} | ||
|
||
fn set_funding_tx(&mut self, funding_tx: Arc<Transaction>) { | ||
match self { | ||
TrustModel::ClientTrustsLsp { funding_tx: tx, .. } => { | ||
*tx = Some(funding_tx); | ||
}, | ||
TrustModel::LspTrustsClient => { | ||
// No-op | ||
}, | ||
} | ||
} | ||
|
||
fn set_funding_tx_broadcast_safe(&mut self, funding_tx_broadcast_safe: bool) { | ||
match self { | ||
TrustModel::ClientTrustsLsp { funding_tx_broadcast_safe: safe, .. } => { | ||
*safe = funding_tx_broadcast_safe; | ||
}, | ||
TrustModel::LspTrustsClient => { | ||
// No-op | ||
}, | ||
} | ||
} | ||
|
||
fn set_payment_claimed(&mut self, payment_claimed: bool) { | ||
match self { | ||
TrustModel::ClientTrustsLsp { payment_claimed: claimed, .. } => { | ||
*claimed = payment_claimed; | ||
}, | ||
TrustModel::LspTrustsClient => { | ||
// No-op | ||
}, | ||
} | ||
} | ||
|
||
fn get_funding_tx(&self) -> Option<Arc<Transaction>> { | ||
match self { | ||
TrustModel::ClientTrustsLsp { funding_tx: Some(tx), .. } => Some(Arc::clone(&tx)), | ||
_ => None, | ||
} | ||
} | ||
|
||
fn is_client_trusts_lsp(&self) -> bool { | ||
match self { | ||
TrustModel::ClientTrustsLsp { .. } => true, | ||
TrustModel::LspTrustsClient => false, | ||
} | ||
} | ||
} | ||
|
||
/// The different states a requested JIT channel can be in. | ||
#[derive(Debug)] | ||
enum OutboundJITChannelState { | ||
|
@@ -377,18 +461,20 @@ struct OutboundJITChannel { | |
user_channel_id: u128, | ||
opening_fee_params: LSPS2OpeningFeeParams, | ||
payment_size_msat: Option<u64>, | ||
trust_model: TrustModel, | ||
} | ||
|
||
impl OutboundJITChannel { | ||
fn new( | ||
payment_size_msat: Option<u64>, opening_fee_params: LSPS2OpeningFeeParams, | ||
user_channel_id: u128, | ||
user_channel_id: u128, client_trusts_lsp: bool, | ||
) -> Self { | ||
Self { | ||
user_channel_id, | ||
state: OutboundJITChannelState::new(), | ||
opening_fee_params, | ||
payment_size_msat, | ||
trust_model: TrustModel::new(client_trusts_lsp), | ||
} | ||
} | ||
|
||
|
@@ -414,6 +500,9 @@ impl OutboundJITChannel { | |
|
||
fn payment_forwarded(&mut self) -> Result<Option<ForwardHTLCsAction>, LightningError> { | ||
let action = self.state.payment_forwarded()?; | ||
if action.is_some() { | ||
self.trust_model.set_payment_claimed(true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we currently test whether the HTLC that caused us to open the channel was forwarded or just any channel. I imagine someone could get enough in pending HTLCs to get a channel, then route a single sat to themselves, claim that, and get the funding broadcasted (this should probably be explicitly tested). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what I will do here is track an once it's <= 0, it means that it's now safe for the LSP to proceed and broadcast the funding tx. I did it this way because the PaymentForward does not tell what htlc was forwarded (I guess that's intentional?), so I cannot tell what HTLC exactly the client is claiming. it just tells me the channel and the skimmed fees. I have the code ready, I'm improving the tests to cover some edge cases. @TheBlueMatt please let me know if this makes sense conceptually or if I'd need to do something else |
||
} | ||
Ok(action) | ||
} | ||
|
||
|
@@ -427,6 +516,26 @@ impl OutboundJITChannel { | |
let is_expired = is_expired_opening_fee_params(&self.opening_fee_params); | ||
self.is_pending_initial_payment() && is_expired | ||
} | ||
|
||
fn set_funding_tx(&mut self, funding_tx: Arc<Transaction>) { | ||
self.trust_model.set_funding_tx(funding_tx); | ||
} | ||
|
||
fn set_funding_tx_broadcast_safe(&mut self, funding_tx_broadcast_safe: bool) { | ||
self.trust_model.set_funding_tx_broadcast_safe(funding_tx_broadcast_safe); | ||
} | ||
|
||
fn should_broadcast_funding_transaction(&self) -> bool { | ||
self.trust_model.should_manually_broadcast() | ||
} | ||
|
||
fn get_funding_tx(&self) -> Option<Arc<Transaction>> { | ||
self.trust_model.get_funding_tx() | ||
} | ||
|
||
fn client_trusts_lsp(&self) -> bool { | ||
self.trust_model.is_client_trusts_lsp() | ||
} | ||
} | ||
|
||
struct PeerState { | ||
|
@@ -666,6 +775,12 @@ where | |
/// | ||
/// Should be called in response to receiving a [`LSPS2ServiceEvent::BuyRequest`] event. | ||
/// | ||
/// `client_trusts_lsp`: | ||
/// * false (default) => "LSP trusts client": LSP broadcasts the funding | ||
/// transaction as soon as it is safe and forwards the payment normally. | ||
/// * true => "Client trusts LSP": LSP may defer broadcasting the funding | ||
/// transaction until after the client claims the forwarded HTLC(s). | ||
/// | ||
/// [`ChannelManager::get_intercept_scid`]: lightning::ln::channelmanager::ChannelManager::get_intercept_scid | ||
/// [`LSPS2ServiceEvent::BuyRequest`]: crate::lsps2::event::LSPS2ServiceEvent::BuyRequest | ||
pub fn invoice_parameters_generated( | ||
|
@@ -692,6 +807,7 @@ where | |
buy_request.payment_size_msat, | ||
buy_request.opening_fee_params, | ||
user_channel_id, | ||
client_trusts_lsp, | ||
); | ||
|
||
peer_state_lock | ||
|
@@ -926,6 +1042,11 @@ where | |
}) | ||
}, | ||
} | ||
|
||
self.emit_broadcast_funding_transaction_event_if_applies( | ||
jit_channel, | ||
counterparty_node_id, | ||
); | ||
} | ||
} else { | ||
return Err(APIError::APIMisuseError { | ||
|
@@ -1412,6 +1533,130 @@ where | |
peer_state_lock.is_prunable() == false | ||
}); | ||
} | ||
|
||
/// Checks if the JIT channel with the given `user_channel_id` needs manual broadcast. | ||
/// Will be true if client_trusts_lsp is set to true | ||
pub fn channel_needs_manual_broadcast( | ||
&self, user_channel_id: u128, counterparty_node_id: &PublicKey, | ||
) -> Result<bool, APIError> { | ||
let outer_state_lock = self.per_peer_state.read().unwrap(); | ||
let inner_state_lock = | ||
outer_state_lock.get(counterparty_node_id).ok_or_else(|| APIError::APIMisuseError { | ||
err: format!("No counterparty state for: {}", counterparty_node_id), | ||
})?; | ||
let peer_state = inner_state_lock.lock().unwrap(); | ||
|
||
let intercept_scid = peer_state | ||
.intercept_scid_by_user_channel_id | ||
.get(&user_channel_id) | ||
.copied() | ||
.ok_or_else(|| APIError::APIMisuseError { | ||
err: format!("Could not find a channel with user_channel_id {}", user_channel_id), | ||
})?; | ||
|
||
let jit_channel = peer_state | ||
.outbound_channels_by_intercept_scid | ||
.get(&intercept_scid) | ||
.ok_or_else(|| APIError::APIMisuseError { | ||
err: format!( | ||
"Failed to map intercept_scid {} for user_channel_id {} to a channel.", | ||
intercept_scid, user_channel_id, | ||
), | ||
})?; | ||
|
||
Ok(jit_channel.client_trusts_lsp()) | ||
} | ||
|
||
/// Called to store the funding transaction for a JIT channel. | ||
/// This should be called when the funding transaction is created but before it's broadcast. | ||
pub fn store_funding_transaction( | ||
&self, user_channel_id: u128, counterparty_node_id: &PublicKey, | ||
funding_tx: Arc<Transaction>, | ||
) -> Result<(), APIError> { | ||
let outer_state_lock = self.per_peer_state.read().unwrap(); | ||
let inner_state_lock = | ||
outer_state_lock.get(counterparty_node_id).ok_or_else(|| APIError::APIMisuseError { | ||
err: format!("No counterparty state for: {}", counterparty_node_id), | ||
})?; | ||
let mut peer_state = inner_state_lock.lock().unwrap(); | ||
|
||
let intercept_scid = peer_state | ||
.intercept_scid_by_user_channel_id | ||
.get(&user_channel_id) | ||
.copied() | ||
.ok_or_else(|| APIError::APIMisuseError { | ||
err: format!("Could not find a channel with user_channel_id {}", user_channel_id), | ||
})?; | ||
|
||
let jit_channel = peer_state | ||
.outbound_channels_by_intercept_scid | ||
.get_mut(&intercept_scid) | ||
.ok_or_else(|| APIError::APIMisuseError { | ||
err: format!( | ||
"Failed to map intercept_scid {} for user_channel_id {} to a channel.", | ||
intercept_scid, user_channel_id, | ||
), | ||
})?; | ||
|
||
jit_channel.set_funding_tx(funding_tx); | ||
|
||
self.emit_broadcast_funding_transaction_event_if_applies(jit_channel, counterparty_node_id); | ||
Ok(()) | ||
} | ||
|
||
/// Called when the funding transaction is safe to broadcast. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs a discussion of what "safe to broadcast" means, cause its not actually "safe to broadcast" in the context of LSPS, only in the context of lightning. Should at least link to the |
||
/// This marks the funding_tx_broadcast_safe flag as true for the given user_channel_id. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will improve the name and docs of this function, but I'm still confused with what you mean by "funding_tx_broadcast_safe is not a public thing" I'm making it necessary to call |
||
pub fn funding_tx_broadcast_safe( | ||
&self, user_channel_id: u128, counterparty_node_id: &PublicKey, | ||
) -> Result<(), APIError> { | ||
let outer_state_lock = self.per_peer_state.read().unwrap(); | ||
let inner_state_lock = | ||
outer_state_lock.get(counterparty_node_id).ok_or_else(|| APIError::APIMisuseError { | ||
err: format!("No counterparty state for: {}", counterparty_node_id), | ||
})?; | ||
let mut peer_state = inner_state_lock.lock().unwrap(); | ||
|
||
let intercept_scid = peer_state | ||
.intercept_scid_by_user_channel_id | ||
.get(&user_channel_id) | ||
.copied() | ||
.ok_or_else(|| APIError::APIMisuseError { | ||
err: format!("Could not find a channel with user_channel_id {}", user_channel_id), | ||
})?; | ||
|
||
let jit_channel = peer_state | ||
.outbound_channels_by_intercept_scid | ||
.get_mut(&intercept_scid) | ||
.ok_or_else(|| APIError::APIMisuseError { | ||
err: format!( | ||
"Failed to map intercept_scid {} for user_channel_id {} to a channel.", | ||
intercept_scid, user_channel_id, | ||
), | ||
})?; | ||
|
||
jit_channel.set_funding_tx_broadcast_safe(true); | ||
|
||
self.emit_broadcast_funding_transaction_event_if_applies(jit_channel, counterparty_node_id); | ||
Ok(()) | ||
} | ||
|
||
fn emit_broadcast_funding_transaction_event_if_applies( | ||
&self, jit_channel: &OutboundJITChannel, counterparty_node_id: &PublicKey, | ||
) { | ||
if jit_channel.should_broadcast_funding_transaction() { | ||
let funding_tx = jit_channel.get_funding_tx(); | ||
|
||
if let Some(funding_tx) = funding_tx { | ||
let event_queue_notifier = self.pending_events.notifier(); | ||
let event = LSPS2ServiceEvent::BroadcastFundingTransaction { | ||
counterparty_node_id: *counterparty_node_id, | ||
user_channel_id: jit_channel.user_channel_id, | ||
funding_tx: funding_tx.as_ref().clone(), | ||
}; | ||
event_queue_notifier.enqueue(event); | ||
} | ||
} | ||
} | ||
} | ||
|
||
impl<CM: Deref> LSPSProtocolMessageHandler for LSPS2ServiceHandler<CM> | ||
|
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 see a reason this should be in an Arc.