-
Notifications
You must be signed in to change notification settings - Fork 154
Follow-up to #792 (multi-LSP support) #956
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 all commits
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 |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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 { | ||
| 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! { | ||
|
Collaborator
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. Hmm? If there's nothing to do, why do need this select?
Contributor
Author
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. 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 |
||
| _ = stop_retry.changed() => return, | ||
| _ = tokio::time::sleep(Duration::from_secs( | ||
| LIQUIDITY_DISCOVERY_RETRY_MAX_DELAY_SECS, | ||
| )) => continue, | ||
| } | ||
| } | ||
|
|
||
| tokio::select! { | ||
|
Collaborator
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. It seems an |
||
| _ = 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 | ||
|
Collaborator
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. Hmm, this seems redundant to our general reconnection loop, or not?
Contributor
Author
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. 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(()) | ||
|
|
||
| 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. | ||
|
Collaborator
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 commit diff seems to have a lot of unnecessary whitespace/line ending changes. Please amend to avoid that, will re-review then.
Contributor
Author
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 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.
Collaborator
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 think it's been fixed by #965, so shouldn't be necessary if you rebase.
Contributor
Author
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. 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; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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> { | ||
|
Collaborator
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. Not a fan of exposing this publicly. Retrying should not be the concern of a user?
Contributor
Author
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. 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. |
||
| 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 | ||
|
|
@@ -432,6 +466,17 @@ where | |
| .collect() | ||
| } | ||
|
|
||
| pub(crate) fn get_single_lsp_details( | ||
|
Collaborator
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. 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> { | ||
|
|
@@ -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 | ||
|
|
||
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.
That likely should be a cancellable task?