Follow-up to #792 (multi-LSP support)#956
Conversation
- Add a background task that retries discovery for any LSP whose protocols are still undiscovered, using exponential backoff (5s up to 1h). - Expose `Liquidity::retry_discovery(node_id)` so users can trigger re-discovery on demand, both to recover a stuck LSP and to pick up newly supported protocols after an LSP upgrade.
Look up trust_peer_0conf by node id via a protocol-independent helper that does not depend on discovery
|
I've assigned @tnull as a reviewer! |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Excuse the delay here. Second commit seems broken, please avoid the unrelated changes as otherwise it's not really reviewable. Also needs a minor rebase by now.
| 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 { |
There was a problem hiding this comment.
That likely should be a cancellable task?
| /// 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> { |
There was a problem hiding this comment.
Not a fan of exposing this publicly. Retrying should not be the concern of a user?
| .collect() | ||
| } | ||
|
|
||
| pub(crate) fn get_single_lsp_details( |
There was a problem hiding this comment.
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.
| loop { | ||
| let undiscovered_lsps = retry_ls.get_undiscovered_lsps(); | ||
| if undiscovered_lsps.is_empty() { | ||
| tokio::select! { |
There was a problem hiding this comment.
Hmm? If there's nothing to do, why do need this select?
| } | ||
| } | ||
|
|
||
| tokio::select! { |
There was a problem hiding this comment.
It seems an tokio::time::interval would be more suitable?
|
|
||
| for (node_id, address) in undiscovered_lsps { | ||
| if let Err(e) = | ||
| retry_cm.connect_peer_if_necessary(node_id, address.clone()).await |
There was a problem hiding this comment.
Hmm, this seems redundant to our general reconnection loop, or not?
| pub(crate) mod lsps2; | ||
|
|
||
| pub use lsps1::LSPS1OrderStatus; | ||
| // This file is Copyright its original authors, visible in version control history. |
There was a problem hiding this comment.
This commit diff seems to have a lot of unnecessary whitespace/line ending changes. Please amend to avoid that, will re-review then.
3 participants
Some minor follow-ups after #792 landed.
Retry LSP protocol discovery when it fails at startup, so a transient connection failure doesn't leave a configured LSP permanently unusable. Adds a background retry with backoff and a
Liquidity::retry_discovery(node_id)method for on-demand re-discovery (also useful when an LSP rolls out a new protocol).Honor
trust_peer_0confindependent of the LSP's supported protocols, It was previously only applied to LSPS2 peers, so LSPS1-only or undiscovered LSPs silently lost their configured 0-conf trust.Leaving the LSP feature-gating for #900
Fixes - #936