Refactor liquidity source to support multiple LSP nodes#792
Refactor liquidity source to support multiple LSP nodes#792Camillarhi wants to merge 9 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
@tnull Early draft up for review, would appreciate feedback on the general API direction |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks! Some initial questions..
| supported_protocols: Mutex::new(None), | ||
| }) | ||
| .collect(), | ||
| pending_lsps1_opening_params_requests: Mutex::new(HashMap::new()), |
There was a problem hiding this comment.
Uh, no, I don't think we should just merge all into one. Especially given that we intend to add more logic on a per-spec basis, this will be will become even more confusing going forward. If anything, we should maybe start the refactoring by first moving the LSPS1/LSPS2 specific parts to src/liquidity/{lsps1,lsps2}.rs, or maybe even to client/service specific sub-modules like src/liquidity/{client,service}/{lsps1,lsps2}.rs.
There was a problem hiding this comment.
Alright, I will move the LSPS1/LSPS2 specific parts
| pub(crate) async fn lsps1_request_channel( | ||
| &self, lsp_balance_sat: u64, client_balance_sat: u64, channel_expiry_blocks: u32, | ||
| announce_channel: bool, refund_address: bitcoin::Address, | ||
| announce_channel: bool, refund_address: bitcoin::Address, node_id: &PublicKey, |
There was a problem hiding this comment.
Hmm, I do wonder if it would make sense to create something like an struct ServiceProvider and move the API methods to there. Then, each registered LSP would have a corresponding ServiceProvider that exposes a bunch of public and internal APIs, which would make the modularization cleaner and would avoid having to give node_id everywhere?
There was a problem hiding this comment.
Makes sense, I will have a look and see
Camillarhi
force-pushed
the
multi-lsp-support
branch
2 times, most recently
from
3cda7f7 to
8bbf55a
Compare
|
Hello @tnull, apologies for the delay. This is ready for another round of review |
Camillarhi
force-pushed
the
multi-lsp-support
branch
from
8bbf55a to
ea93afe
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Cool, already looks pretty good! Did a first higher-level pass, have yet to look into the details.
| @@ -1,1542 +0,0 @@ | |||
| // This file is Copyright its original authors, visible in version control history. | |||
There was a problem hiding this comment.
Would be good to structure this PR in a way that (as far as possible) makes any code moves dedicated commits that can be picked up by git diff --color-moved --patience, as otherwise reviewing this in detail will be very hard.
| } | ||
| } | ||
|
|
||
| discovery_ls.discover_all_lsp_protocols().await; |
There was a problem hiding this comment.
I think it would be good to make this part of the background task above? Also, can we spawn the discovery tasks in parallel rather than doing them sequentially?
| } | ||
|
|
||
| /// Configures the [`Node`] instance to source inbound liquidity from the given LSP at runtime, | ||
| /// without specifying the exact protocol used (e.g., LSPS1 or LSPS2). |
There was a problem hiding this comment.
I think we can drop the remark regarding 'without specifying the exact protocol' here and elsewhere, as the API already communicates that due to being generic. I do however wonder if we'd want to move this method to an API-extension object similar to what we do for the payment types? I.e., retrieve the API object via Node::liquidity()?
There was a problem hiding this comment.
Thanks. I will remove the remark and move this to a Node::liquidity() API-extension
| /// [`Bolt11Payment::receive_via_jit_channel`]: crate::payment::Bolt11Payment::receive_via_jit_channel | ||
| #[derive(Clone)] | ||
| #[cfg_attr(feature = "uniffi", derive(uniffi::Object))] | ||
| pub struct LSPS1Liquidity { |
There was a problem hiding this comment.
It seems this is currently not exposed in the API anymore?
| /// The given `token` will be used by the LSP to authenticate the user. | ||
| /// | ||
| /// [bLIP-51 / LSPS1]: https://github.com/lightning/blips/blob/master/blip-0051.md | ||
| #[deprecated(note = "Use `add_lsp` instead")] |
There was a problem hiding this comment.
Hmm, I think so far we've been fine with just breaking the APIs without deprecating them first. If we find a better API I'd be fine with just dropping the old ones to clean up.
Camillarhi
force-pushed
the
multi-lsp-support
branch
2 times, most recently
from
6061c6d to
a560764
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Cool. Docs CI is currently failing.
Would be great if you try once more to breakup the second commit into smaller chunks, but let me know if it's too cumbersome/impossible.
Feel free to mark ready for review then.
Camillarhi
force-pushed
the
multi-lsp-support
branch
from
a560764 to
c430e51
Compare
|
🔔 1st Reminder Hey @tnull @valentinewallace! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @tnull @valentinewallace! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull @valentinewallace! This PR has been waiting for your review. |
1 similar comment
|
🔔 2nd Reminder Hey @tnull @valentinewallace! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull @valentinewallace! This PR has been waiting for your review. |
1 similar comment
|
🔔 3rd Reminder Hey @tnull @valentinewallace! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull @valentinewallace! This PR has been waiting for your review. |
1 similar comment
|
🔔 4th Reminder Hey @tnull @valentinewallace! This PR has been waiting for your review. |
Camillarhi
force-pushed
the
multi-lsp-support
branch
2 times, most recently
from
22da16c to
2cda868
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks for splitting things out into prefactors! There's still a lot of things going on at the same time in the last commit, please break it up further if you see the opportunity. A few more comments, also some suggestions on how to reduce noisiness in the last commit.
| trust_peer_0conf: bool, | ||
| ) -> Result<(), Error> { | ||
| let liquidity_source = | ||
| self.liquidity_source.as_ref().ok_or(Error::LiquiditySourceUnavailable)?; |
There was a problem hiding this comment.
Hmm, wouldn't erroring out here mean that we can't add LSPs only at runtime, if we don't also add some during build time?
| let mut lsp_nodes = liquidity_source.lsp_nodes.write().expect("lock"); | ||
| if lsp_nodes.iter().any(|n| n.node_id == node_id) { | ||
| log_error!(self.logger, "LSP node {} is already added.", node_id); | ||
| return Err(Error::DuplicateLspNode); |
There was a problem hiding this comment.
Maybe we should just silently continue rather than erroring here?
| self.config.trusted_peers_0conf.contains(&counterparty_node_id); | ||
| let mut channel_override_config = None; | ||
| if let Some((lsp_node_id, _)) = self | ||
| if let Some(lsp2_node) = self |
There was a problem hiding this comment.
nit: Should be lsps2_node or similar?
| ); | ||
| let _ = tokio::time::timeout( | ||
| Duration::from_secs(LSPS_DISCOVERY_WAIT_TIMEOUT_SECS), | ||
| rx.wait_for(|done| *done), |
There was a problem hiding this comment.
Hmm, if there's nothing in-flight, we might just be waiting unnecessarily, no?
There was a problem hiding this comment.
That is true. The wait should only kick in when there's actually something in flight worth waiting for. LspNode.supported_protocols is None precisely while a node's protocol discovery hasn't completed or failed. So we can use that as the in-flight signal, if every node already has its protocols populated, there's nothing to wait for.
The previous shape selected all_lsps_for_protocol first and only waited if the result was empty. But that means if 1 of N LSPS2 capable nodes happened to respond quickly while the rest were still mid-discovery, we'd return a partial set and skip waiting which isn't ideal since we want to pick the best LSP from the full set. So I've inverted the structure to wait first if anything's pending, then select once.
| Ok(response) | ||
| } | ||
|
|
||
| pub(crate) async fn handle_next_event(&self, event: LSPS1ClientEvent) { |
There was a problem hiding this comment.
Can make moving the event handling to the sub-modules 3 more prefactor commits (at least LSPS1 client, LSPS2 client, LSPS2 service, maybe also for the LSPS0 variant)?
There was a problem hiding this comment.
I have broken this down into prefactor commits for LSPS1 client, LSPS2 client and LSPS2 service
| pub(crate) pending_lsps1_opening_params_requests: | ||
| Mutex<HashMap<LSPSRequestId, oneshot::Sender<LSPS1OpeningParamsResponse>>>, | ||
| pub(crate) pending_create_order_requests: | ||
| pub(crate) pending_lsps1_create_order_requests: |
There was a problem hiding this comment.
Why are these field renames necessary? They seem to only increase the noise in the diff?
There was a problem hiding this comment.
The field renames have been reverted
Camillarhi
force-pushed
the
multi-lsp-support
branch
5 times, most recently
from
af25bef to
bc2dde3
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
Camillarhi
force-pushed
the
multi-lsp-support
branch
2 times, most recently
from
0ab16a4 to
a84f8e5
Compare
| let cm = Arc::clone(&discovery_cm); | ||
| let logger = Arc::clone(&discovery_logger); | ||
| let ls = Arc::clone(&liquidity_handler); | ||
| discovery_set.spawn(async move { |
There was a problem hiding this comment.
We should probably make sure to cancel/abort all tasks we spawn here in case of a shutdown.
| /// and select the appropriate protocol per request automatically. | ||
| /// | ||
| /// The given `token` will be used by the LSP to authenticate the user. | ||
| /// `trust_peer_0conf` controls whether the node will accept 0-confirmation channels opened by this |
There was a problem hiding this comment.
Codex:
- Medium: trust_peer_0conf = false does not actually supersede Config::trusted_peers_0conf as documented. The event handler initializes allow_0conf from the global config and ORs in the LSP flag, so an LSP present in the global trust list is still accepted as zero-conf even when added with
trust_peer_0conf = false. See docs at src/builder.rs:448 and behavior at src/event.rs:1294.
| self.liquidity_source.as_ref().map(|ls| { | ||
| ls.lsps2_funding_tx_broadcast_safe(user_channel_id, counterparty_node_id); | ||
| }); | ||
| self.liquidity_source |
There was a problem hiding this comment.
Codex:
- Low: Non-LSPS2-service nodes now call the LSPS2 service broadcast-safe hook for every FundingTxBroadcastSafe event. Because lsps2_funding_tx_broadcast_safe treats lsps2_service: None as “do not early-return,” it reaches the missing service handler path and logs an error for ordinary nodes/
channels. See src/event.rs:681 and src/liquidity/service/lsps2.rs:164.
| res = discovery_set.join_next(), if !discovery_done => { | ||
| if res.is_none() { | ||
| liquidity_handler.mark_discovery_done(); | ||
| discovery_done = true; |
There was a problem hiding this comment.
Codex:
- High: A builder-configured LSP can become permanently unusable after a transient startup discovery failure. Node::start logs connect/discovery errors and still marks discovery done, while LSPS1/LSPS2 selection later only considers nodes with supported_protocols = Some(...) and never
retries undiscovered nodes. Runtime add_liquidity_source also skips the same node ID, so users cannot recover in-process. See src/lib.rs:682, src/lib.rs:725, src/liquidity/client/lsps2.rs:506, src/liquidity/client/lsps1.rs:400, src/liquidity/mod.rs:126.
Do we see an easy way to retry discovery during the runtime? If not/if it gets too complicated I'd rather leave it for a follow-up.
There was a problem hiding this comment.
I will look into it and if it gets too complicated, I will do it in a follow up PR
There was a problem hiding this comment.
I will handle this in a follow-up PR. I will have a background retry for failed startup discovery and also expose an API, something like retry_discovery(node_id), to cover the case where a user knows their configured LSP has rolled out a new protocol.
Camillarhi
force-pushed
the
multi-lsp-support
branch
2 times, most recently
from
606bcf2 to
a55f8b3
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Seems CI is unfortunately failing right now (CLN failure is unrelated though).
Replace per-protocol single-LSP configuration `LSPS1Client` and `LSPS2Client` with a unified `Vec<LspNode>` model where users configure LSP nodes via `add_liquidity_source()` at build time or runtime and per-LSP protocol support is discovered via the LSPS0 `list_protocols`. - Introduce a per-LSP `trust_peer_0conf` flag to `LspConfig`/`LspNode` structs that controls whether 0-conf channels from that LSP are accepted - Add LSPS0 protocol discovery `discover_lsp_protocols` with event handling for `ListProtocolsResponse` - Update events to also use each LSP's `trust_peer_0conf` flag when deciding whether to allow 0-conf channels - Replace `set_liquidity_source_lsps1` and `set_liquidity_source_lsps2` builder methods with a single `add_liquidity_source()` that takes a `trust_peer_0conf` flag - Rename `set_liquidity_provider_lsps2` to `enable_liquidity_provider` - LSPS2 JIT channels now query all LSPS2-capable LSPs and automatically select the cheapest fee offer across all of them - Spawn background discovery task on `Node::start()` and expose a watch channel so dependent flows can wait for discovery to complete - Add a new `Liquidity` handler `Node::liquidity()` exposing `add_liquidity_source()` API for adding LSPs at runtime, and `lsps1()` for the existing LSPS1 surface
Camillarhi
force-pushed
the
multi-lsp-support
branch
2 times, most recently
from
0c09af8 to
b5a5fa4
Compare
Remove the `Ignoring` variant now that the liquidity source is always built, so the enum and its match arms are now pure overhead. Replace it with a struct that holds the `LiquiditySource` directly and have each trait method delegate straight to `liquidity_manager()`.
Camillarhi
force-pushed
the
multi-lsp-support
branch
from
b5a5fa4 to
3c80fdc
Compare
The failing CI has been resolved |
3 participants
The current setup ties you to a single LSP per protocol via
set_liquidity_source_lsps1/set_liquidity_source_lsps2. This refactor replaces that with a unifiedVec<LspNode>model where LSP nodes are added viaadd_lsp()and protocol support is discovered at runtime through LSPS0list_protocols. Multi-LSP support has been requested previously in #529.set_liquidity_source_lsps1/set_liquidity_source_lsps2in favor ofadd_lsp()LSPS1Client/LSPS2Clientwith global pending request maps keyed byLSPSRequestIdListProtocolsResponseNode::start()request_channel_from_lsp()for explicit LSPS1 LSP selectionis_lsps_node()for multi-LSP counterparty checksThis sets the foundation for LSPS5 support currently being worked on in #729