Follow-up to #792 (multi-LSP support)#956
Conversation
|
👋 Thanks for assigning @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?
There was a problem hiding this comment.
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
| .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?
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| 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?
There was a problem hiding this comment.
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.
| 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks, I’ll rebase
Add a background task that retries discovery for any LSP whose protocols are still undiscovered, using exponential backoff (5s up to 1h). Once no undiscovered LSPs remain (or the backoff is exhausted), the task settles into a fixed interval (24h) and re-runs discovery for all configured LSPs, so we also pick up protocols an LSP rolls out after we first connected.
Look up trust_peer_0conf by node id via a protocol-independent helper that does not depend on discovery
Camillarhi
force-pushed
the
multi-lsp-support-follow-up
branch
from
da5bcdc to
b6da9a3
Compare
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