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
6 changes: 6 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ pub(crate) const HRN_RESOLUTION_TIMEOUT_SECS: u64 = 5;
// The timeout after which we abort an LNURL-auth operation.
pub(crate) const LNURL_AUTH_TIMEOUT_SECS: u64 = 15;

// The initial delay before retrying a failed liquidity protocol discovery operation.
pub(crate) const LIQUIDITY_DISCOVERY_RETRY_INITIAL_DELAY_SECS: u64 = 5;

// The maximum delay before retrying a failed liquidity protocol discovery operation.
pub(crate) const LIQUIDITY_DISCOVERY_RETRY_MAX_DELAY_SECS: u64 = 3600;

#[derive(Debug, Clone)]
#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
/// Represents the configuration of an [`Node`] instance.
Expand Down
11 changes: 6 additions & 5 deletions src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1317,14 +1317,15 @@ where
);
let mut allow_0conf =
self.config.trusted_peers_0conf.contains(&counterparty_node_id);
let mut channel_override_config = None;

// If the peer is a configured LSP node, additionally honor its trust_peer_0conf flag.
if let Some(lsp) =
self.liquidity_source.get_lsp_config(&counterparty_node_id, 2).await
{
allow_0conf = allow_0conf || lsp.trust_peer_0conf;
if self.liquidity_source.get_lsp_trust_0conf(&counterparty_node_id) == Some(true) {
allow_0conf = true;
}

let mut channel_override_config = None;

if self.liquidity_source.get_lsp_config(&counterparty_node_id, 2).await.is_some() {
// When we're an LSPS2 client, allow claiming underpaying HTLCs as the LSP will skim off some fee. We'll
// check that they don't take too much before claiming.
channel_override_config = Some(ChannelConfigOverrides {
Expand Down
59 changes: 59 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@ pub use types::{
};
pub use vss_client;

use crate::config::{
LIQUIDITY_DISCOVERY_RETRY_INITIAL_DELAY_SECS, LIQUIDITY_DISCOVERY_RETRY_MAX_DELAY_SECS,
};
use crate::ffi::maybe_wrap;
use crate::liquidity::Liquidity;
use crate::scoring::setup_background_pathfinding_scores_sync;
Expand Down Expand Up @@ -748,6 +751,62 @@ impl Node {
}
});

// Background retry for LSPs that failed the initial protocol-discovery batch, so a
// transient startup failure doesn't leave a configured LSP permanently unusable.
let mut stop_retry = self.stop_sender.subscribe();
let retry_ls = Arc::clone(&self.liquidity_source);
let retry_logger = Arc::clone(&self.logger);
let retry_cm = Arc::clone(&self.connection_manager);
self.runtime.spawn_background_task(async move {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That likely should be a cancellable task?

let mut backoff = LIQUIDITY_DISCOVERY_RETRY_INITIAL_DELAY_SECS;
loop {
let undiscovered_lsps = retry_ls.get_undiscovered_lsps();
if undiscovered_lsps.is_empty() {
tokio::select! {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm? If there's nothing to do, why do need this select?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is there to stop the loop from busy-spinning when there's nothing to discover. The idea was to keep the task alive in case an LSP became undiscovered later, but a runtime add_liquidity_source cleans up the node if its discovery fails, so nothing new ever lands in the undiscovered set after startup. So once it's empty, there's nothing to wait for, and it should just return. I'll clean it up as part of the re-discovery rework, for re-discovering already-discovered LSPs.

_ = stop_retry.changed() => return,
_ = tokio::time::sleep(Duration::from_secs(
LIQUIDITY_DISCOVERY_RETRY_MAX_DELAY_SECS,
)) => continue,
}
}

tokio::select! {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems an tokio::time::interval would be more suitable?

_ = stop_retry.changed() => return,
_ = tokio::time::sleep(Duration::from_secs(backoff)) => {},
}

for (node_id, address) in undiscovered_lsps {
if let Err(e) =
retry_cm.connect_peer_if_necessary(node_id, address.clone()).await

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm, this seems redundant to our general reconnection loop, or not?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Configured LSPs aren't added to the peer store at startup, so the reconnection loop doesn't keep them connected. This is what connects us to run discovery.

{
log_debug!(
retry_logger,
"Discovery retry: failed to connect to LSP {}: {}",
node_id,
e
);
continue;
}
match retry_ls.discover_lsp_protocols(&node_id).await {
Ok(protocols) => log_info!(
retry_logger,
"Discovery retry: discovered protocols for LSP {}: {:?}",
node_id,
protocols
),
Err(e) => log_debug!(
retry_logger,
"Discovery retry: failed for LSP {}: {:?}",
node_id,
e
),
}
}

backoff = (backoff * 2).min(LIQUIDITY_DISCOVERY_RETRY_MAX_DELAY_SECS);
}
});

log_info!(self.logger, "Startup complete.");
*is_running_lock = true;
Ok(())
Expand Down
22 changes: 11 additions & 11 deletions src/liquidity/client/mod.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// This file is Copyright its original authors, visible in version control history.
//
// This file is licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license <LICENSE-MIT or
// http://opensource.org/licenses/MIT>, at your option. You may not use this file except in
// accordance with one or both of these licenses.
pub(crate) mod lsps1;
pub(crate) mod lsps2;
pub use lsps1::LSPS1OrderStatus;
// This file is Copyright its original authors, visible in version control history.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This commit diff seems to have a lot of unnecessary whitespace/line ending changes. Please amend to avoid that, will re-review then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the CRLF→LF normalization, which was already flagged as a nit in the earlier review. I'll amend it to pull it out of this commit and into a separate one.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is the CRLF→LF normalization, which was already flagged as a nit in the earlier review. I'll amend it to pull it out of this commit and into a separate one.

I think it's been fixed by #965, so shouldn't be necessary if you rebase.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I’ll rebase

//
// This file is licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license <LICENSE-MIT or
// http://opensource.org/licenses/MIT>, at your option. You may not use this file except in
// accordance with one or both of these licenses.

pub(crate) mod lsps1;
pub(crate) mod lsps2;

pub use lsps1::LSPS1OrderStatus;
64 changes: 64 additions & 0 deletions src/liquidity/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,40 @@ impl Liquidity {
Ok(())
}

/// Re-runs bLIP-50 / LSPS0 protocol discovery for an already-configured LSP.
///
/// Use this to recover an LSP that failed protocol discovery at startup or to
/// pick up newly supported protocols after the LSP has rolled out an upgrade.
///
/// The `node_id` must belong to an LSP configured at build time or added via
/// [`Liquidity::add_liquidity_source`]; otherwise [`Error::LiquiditySourceUnavailable`]
/// is returned.
pub fn retry_discovery(&self, node_id: PublicKey) -> Result<(), Error> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not a fan of exposing this publicly. Retrying should not be the concern of a user?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was meant to let a user trigger re-discovery for an LSP they know has rolled out a new protocol, the background retry only covers LSPs that never completed discovery in the first place, so an already-discovered LSP adding a protocol wouldn't get picked up.
But thinking about it more, the user usually won't know when an LSP adds a protocol, so leaning on them to call this isn't great. Probably makes more sense to have the background task periodically re-run discovery for already-discovered LSPs too, so new protocols get picked up automatically and there's nothing to expose. I will extend the background check

let (_, address) = self
.liquidity_source
.get_single_lsp_details(&node_id)
.ok_or(Error::LiquiditySourceUnavailable)?;

let con_cm = Arc::clone(&self.connection_manager);
let connect_addr = address.clone();
self.runtime.block_on(async move {
con_cm.connect_peer_if_necessary(node_id, connect_addr).await
})?;
log_info!(
self.logger,
"Connected to LSP {}@{} for protocol re-discovery.",
node_id,
address
);

let protocols = self
.runtime
.block_on(async { self.liquidity_source.discover_lsp_protocols(&node_id).await })?;
log_info!(self.logger, "Re-discovered protocols for LSP {}: {:?}", node_id, protocols);

Ok(())
}

/// Returns a liquidity handler allowing to request channels via the [bLIP-51 / LSPS1] protocol.
///
/// [bLIP-51 / LSPS1]: https://github.com/lightning/blips/blob/master/blip-0051.md
Expand Down Expand Up @@ -432,6 +466,17 @@ where
.collect()
}

pub(crate) fn get_single_lsp_details(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please avoid such ~single-call helpers (especially if they are only used in one place). Please rather inline them so they don't clutter up the file as much.

&self, node_id: &PublicKey,
) -> Option<(PublicKey, SocketAddress)> {
self.lsp_nodes
.read()
.expect("lock")
.iter()
.find(|n| &n.node_id == node_id)
.map(|n| (n.node_id, n.address.clone()))
}

pub(crate) async fn discover_lsp_protocols(
&self, node_id: &PublicKey,
) -> Result<Vec<u16>, Error> {
Expand Down Expand Up @@ -517,6 +562,25 @@ where
select_lsps_for_protocol(&self.lsp_nodes, protocol, Some(node_id))
}

pub(crate) fn get_undiscovered_lsps(&self) -> Vec<(PublicKey, SocketAddress)> {
self.lsp_nodes
.read()
.expect("lock")
.iter()
.filter(|n| n.supported_protocols.is_none())
.map(|n| (n.node_id, n.address.clone()))
.collect()
}

pub(crate) fn get_lsp_trust_0conf(&self, node_id: &PublicKey) -> Option<bool> {
self.lsp_nodes
.read()
.expect("lock")
.iter()
.find(|n| &n.node_id == node_id)
.map(|n| n.trust_peer_0conf)
}

/// Flips the `discovery_done` watch to `true`.
///
/// Called once after the *initial* batch of LSPs configured at build time has been
Expand Down
Loading
Loading