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
1 change: 1 addition & 0 deletions bindings/ldk_node.udl
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ dictionary LSPS2ServiceConfig {
u32 max_client_to_self_delay;
u64 min_payment_size_msat;
u64 max_payment_size_msat;
boolean client_trusts_lsp;
};

enum LogLevel {
Expand Down
1 change: 1 addition & 0 deletions src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1613,6 +1613,7 @@ fn build_with_store_internal(
Arc::clone(&kv_store),
Arc::clone(&config),
Arc::clone(&logger),
Arc::clone(&tx_broadcaster),
);

lsc.lsps1_client.as_ref().map(|config| {
Expand Down
51 changes: 42 additions & 9 deletions src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ where
counterparty_node_id,
channel_value_satoshis,
output_script,
..
user_channel_id,
} => {
// Construct the raw transaction with the output that is paid the amount of the
// channel.
Expand All @@ -506,12 +506,43 @@ where
locktime,
) {
Ok(final_tx) => {
// Give the funding transaction back to LDK for opening the channel.
match self.channel_manager.funding_transaction_generated(
temporary_channel_id,
counterparty_node_id,
final_tx,
) {
let needs_manual_broadcast =
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Would be cleaner if we did:

diff --git a/src/event.rs b/src/event.rs
index 7f467977..9a5516d8 100644
--- a/src/event.rs
+++ b/src/event.rs
@@ -506,20 +506,17 @@ where
                                        locktime,
                                ) {
                                        Ok(final_tx) => {
-                                               let needs_manual_broadcast =
-                                                       match self.liquidity_source.as_ref().map(|ls| {
-                                                               ls.as_ref().lsps2_channel_needs_manual_broadcast(
+                                               let needs_manual_broadcast = self
+                                                       .liquidity_source
+                                                       .as_ref()
+                                                       .and_then(|ls| {
+                                                               ls.lsps2_channel_needs_manual_broadcast(
                                                                        counterparty_node_id,
                                                                        user_channel_id,
                                                                )
-                                                       }) {
-                                                               Some(Ok(v)) => v,
-                                                               Some(Err(e)) => {
-                                                                       log_error!(self.logger, "Failed to determine if channel needs manual broadcast: {:?}", e);
-                                                                       false
-                                                               },
-                                                               None => false,
-                                                       };
+                                                               .ok()
+                                                       })
+                                                       .unwrap_or(false);

                                                let result = if needs_manual_broadcast {
                                                        self.liquidity_source.as_ref().map(|ls| {

match self.liquidity_source.as_ref().map(|ls| {
ls.as_ref().lsps2_channel_needs_manual_broadcast(
counterparty_node_id,
user_channel_id,
)
}) {
Some(Ok(v)) => v,
Some(Err(e)) => {
log_error!(self.logger, "Failed to determine if channel needs manual broadcast: {:?}", e);
false
},
None => false,
};

let result = if needs_manual_broadcast {
self.liquidity_source.as_ref().map(|ls| {
ls.lsps2_store_funding_transaction(
user_channel_id,
counterparty_node_id,
final_tx.clone(),
);
});
self.channel_manager.funding_transaction_generated_manual_broadcast(
temporary_channel_id,
counterparty_node_id,
final_tx,
)
} else {
self.channel_manager.funding_transaction_generated(
temporary_channel_id,
counterparty_node_id,
final_tx,
)
};

match result {
Ok(()) => {},
Err(APIError::APIMisuseError { err }) => {
log_error!(self.logger, "Panicking due to APIMisuseError: {}", err);
Expand Down Expand Up @@ -550,8 +581,10 @@ where
},
}
},
LdkEvent::FundingTxBroadcastSafe { .. } => {
debug_assert!(false, "We currently only support safe funding, so this event should never be emitted.");
LdkEvent::FundingTxBroadcastSafe { user_channel_id, counterparty_node_id, .. } => {
self.liquidity_source.as_ref().map(|ls| {
ls.lsps2_funding_tx_broadcast_safe(user_channel_id, counterparty_node_id);
});
},
LdkEvent::PaymentClaimable {
payment_hash,
Expand Down
84 changes: 81 additions & 3 deletions src/liquidity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@ use std::time::Duration;

use bitcoin::hashes::{sha256, Hash};
use bitcoin::secp256k1::{PublicKey, Secp256k1};
use bitcoin::Transaction;
use chrono::Utc;
use lightning::events::HTLCHandlingFailureType;
use lightning::ln::channelmanager::{InterceptId, MIN_FINAL_CLTV_EXPIRY_DELTA};
use lightning::ln::msgs::SocketAddress;
use lightning::ln::types::ChannelId;
use lightning::routing::router::{RouteHint, RouteHintHop};
use lightning::util::errors::APIError;
use lightning_invoice::{Bolt11Invoice, Bolt11InvoiceDescription, InvoiceBuilder, RoutingFees};
use lightning_liquidity::events::LiquidityEvent;
use lightning_liquidity::lsps0::ser::{LSPSDateTime, LSPSRequestId};
Expand Down Expand Up @@ -51,7 +53,6 @@ use crate::{total_anchor_channels_reserve_sats, Config, Error};
const LIQUIDITY_REQUEST_TIMEOUT_SECS: u64 = 5;

const LSPS2_GETINFO_REQUEST_EXPIRY: Duration = Duration::from_secs(60 * 60 * 24);
const LSPS2_CLIENT_TRUSTS_LSP_MODE: bool = true;
const LSPS2_CHANNEL_CLTV_EXPIRY_DELTA: u32 = 72;

struct LSPS1Client {
Expand Down Expand Up @@ -130,6 +131,8 @@ pub struct LSPS2ServiceConfig {
pub min_payment_size_msat: u64,
/// The maximum payment size that we will accept when opening a channel.
pub max_payment_size_msat: u64,
/// Use the client trusts lsp model
pub client_trusts_lsp: bool,
}

pub(crate) struct LiquiditySourceBuilder<L: Deref>
Expand All @@ -147,6 +150,7 @@ where
kv_store: Arc<DynStore>,
config: Arc<Config>,
logger: L,
broadcaster: Arc<Broadcaster>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to add this field if we already have tx_broadcaster: Arc<Broadcaster> above.

}

impl<L: Deref> LiquiditySourceBuilder<L>
Expand All @@ -156,7 +160,7 @@ where
pub(crate) fn new(
wallet: Arc<Wallet>, channel_manager: Arc<ChannelManager>, keys_manager: Arc<KeysManager>,
chain_source: Arc<ChainSource>, tx_broadcaster: Arc<Broadcaster>, kv_store: Arc<DynStore>,
config: Arc<Config>, logger: L,
config: Arc<Config>, logger: L, broadcaster: Arc<Broadcaster>,
) -> Self {
let lsps1_client = None;
let lsps2_client = None;
Expand All @@ -173,6 +177,7 @@ where
kv_store,
config,
logger,
broadcaster,
}
}

Expand Down Expand Up @@ -305,6 +310,79 @@ where
self.lsps2_client.as_ref().map(|s| (s.lsp_node_id, s.lsp_address.clone()))
}

pub(crate) fn lsps2_channel_needs_manual_broadcast(
&self, counterparty_node_id: PublicKey, user_channel_id: u128,
) -> Result<bool, APIError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Result<.., APIError> on lightning-liquidity pattern isn't great to begin with, but we should def. not have any methods in LDK Node using APIError. Please define an error variant if you really need one, but I suspect we don't actually need to.

Please clean this up, for example something like this

diff --git a/src/liquidity.rs b/src/liquidity.rs
index a293311d..ddd517b1 100644
--- a/src/liquidity.rs
+++ b/src/liquidity.rs
@@ -312,22 +312,18 @@ where

        pub(crate) fn lsps2_channel_needs_manual_broadcast(
                &self, counterparty_node_id: PublicKey, user_channel_id: u128,
-       ) -> Result<bool, APIError> {
-               // if we are not in a client_trusts_lsp model, we don't check and just return false
-               if !self.is_client_trusts_lsp() {
-                       log_debug!(self.logger, "Skipping funding transaction broadcast as client trusts LSP.");
-                       return Ok(false);
-               }
-
-               // if we are in a client_trusts_lsp model, then we check if the LSP has an LSPS2 operation in progress
-               self.lsps2_service.as_ref().map_or(Ok(false), |_| {
-                       let lsps2_service_handler = self.liquidity_manager.lsps2_service_handler();
-                       if let Some(handler) = lsps2_service_handler {
-                               handler.channel_needs_manual_broadcast(user_channel_id, &counterparty_node_id)
-                       } else {
-                               log_error!(self.logger, "LSPS2 service handler is not available.");
-                               Ok(false)
-                       }
+       ) -> bool {
+               self.lsps2_service.as_ref().map_or(false, |lsps2_service| {
+                       lsps2_service.service_config.client_trusts_lsp
+                               || self
+                                       .liquidity_manager()
+                                       .lsps2_service_handler()
+                                       .and_then(|handler| {
+                                               handler
+                                                       .channel_needs_manual_broadcast(user_channel_id, &counterparty_node_id)
+                                                       .ok()
+                                       })
+                                       .unwrap_or(false)
                })
        }

.. would be much more concise and readable, IMO. Also no need for 3-line util methods like is_client_trusts_lsp that just complicate things in this case.

(likewise below)

// if we are not in a client_trusts_lsp model, we don't check and just return false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please drop this redundant comment, it doesn't explain anything beyond the code below, just potentially adds to the confusion.

if !self.is_client_trusts_lsp() {
log_debug!(self.logger, "Skipping funding transaction broadcast as client trusts LSP.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, this is very confusing/wrong. We're actually auto-broadcasting, not manually/witholding the broadcast because we're in LSP-trusts-client, right?

Same below

return Ok(false);
}

// if we are in a client_trusts_lsp model, then we check if the LSP has an LSPS2 operation in progress
self.lsps2_service.as_ref().map_or(Ok(false), |_| {
let lsps2_service_handler = self.liquidity_manager.lsps2_service_handler();
if let Some(handler) = lsps2_service_handler {
handler.channel_needs_manual_broadcast(user_channel_id, &counterparty_node_id)
} else {
log_error!(self.logger, "LSPS2 service handler is not available.");
Ok(false)
}
})
}

pub(crate) fn lsps2_store_funding_transaction(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that we use them both at the same callsite and for the same purpose, should we just merge lsps2_channel_needs_manual_broadcast / lsps2_store_funding_transaction to broadcast_or_store_funding_transaction and call that in the event handler? Then most of this redundant boilerplate goes away, we just check once what we should do and call the respective method on channelmanager here?

&self, user_channel_id: u128, counterparty_node_id: PublicKey, funding_tx: Transaction,
) {
if !self.is_client_trusts_lsp() {
log_debug!(self.logger, "Skipping funding transaction broadcast as client trusts LSP.");
return;
}
self.lsps2_service.as_ref().map(|_| {
let lsps2_service_handler = self.liquidity_manager.lsps2_service_handler();
if let Some(handler) = lsps2_service_handler {
handler
.store_funding_transaction(user_channel_id, &counterparty_node_id, funding_tx)
.unwrap_or_else(|e| {
debug_assert!(false, "Failed to store funding transaction: {:?}", e);
log_error!(self.logger, "Failed to store funding transaction: {:?}", e);
});
} else {
log_error!(self.logger, "LSPS2 service handler is not available.");
}
});
}

pub(crate) fn lsps2_funding_tx_broadcast_safe(
&self, user_channel_id: u128, counterparty_node_id: PublicKey,
) {
if !self.is_client_trusts_lsp() {
log_debug!(self.logger, "Skipping funding transaction broadcast as client trusts LSP.");
return;
}
self.lsps2_service.as_ref().map(|_| {
let lsps2_service_handler = self.liquidity_manager.lsps2_service_handler();
if let Some(handler) = lsps2_service_handler {
handler
.set_funding_tx_broadcast_safe(user_channel_id, &counterparty_node_id)
.unwrap_or_else(|e| {
debug_assert!(false, "Failed to store funding transaction: {:?}", e);
log_error!(self.logger, "Failed to store funding transaction: {:?}", e);
});
} else {
log_error!(self.logger, "LSPS2 service handler is not available.");
}
});
}

fn is_client_trusts_lsp(&self) -> bool {
if let Some(lsps2_service) = self.lsps2_service.as_ref() {
lsps2_service.service_config.client_trusts_lsp
} else {
false
}
}

pub(crate) async fn handle_next_event(&self) {
match self.liquidity_manager.next_event_async().await {
LiquidityEvent::LSPS1Client(LSPS1ClientEvent::SupportedOptionsReady {
Expand Down Expand Up @@ -594,7 +672,7 @@ where
request_id,
intercept_scid,
LSPS2_CHANNEL_CLTV_EXPIRY_DELTA,
LSPS2_CLIENT_TRUSTS_LSP_MODE,
service_config.client_trusts_lsp,
user_channel_id,
)
.await
Expand Down
Loading
Loading