Refactor liquidity source to support multiple LSP nodes#792
Refactor liquidity source to support multiple LSP nodes#792Camillarhi wants to merge 5 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
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Cool to see this works, from what I see this already looks pretty good!
However, it's still very hard to check that most of the changes are actually just code moves and do not change the behavior. Actually reviewing this would mean to re-review ~all liquidity related logic. So, please split the second commit into multiple ones, preferably checking that git diff --color-moved --ignore-all-space [--patience] does indeed pick up the code moves as such.
Could e.g., do these commits, rougly:
- Just move LSPS1-related logic into new module, simply redirecting calls
- Then move all LSPS2-related logic into new module, simply redirecting calls
- Then start making behavioral changes, e.g., switching to the service discovery.
This might also need another rebase as we're now disallowing any unwraps since #868.
Thanks for the detailed breakdown! I'll split the second commit along the lines you suggested.
The branch has also been rebased and all |
Camillarhi
force-pushed
the
multi-lsp-support
branch
from
2cda868 to
ecfe0b5
Compare
Replace per-protocol single-LSP configuration `LSPS1Client, LSPS2Client` with a unified `Vec<LspNode>` model where users configure LSP nodes via `add_lsp()` and protocol support is discovered at runtime via LSPS0 `list_protocols`. - Replace separate `LSPS1Client/LSPS2Client` with global pending request maps keyed by `LSPSRequestId` - Add LSPS0 protocol discovery `discover_lsp_protocols` with event handling for `ListProtocolsResponse` - Update events to use is_lsps_node() for multi-LSP counterparty checks - Deprecate `set_liquidity_source_lsps1/lsps2` builder methods in favor of `add_lsp()` - LSPS2 JIT channels now query all LSPS2-capable LSPs and automatically select the cheapest fee offer across all of them - Add `request_channel_from_lsp()` for explicit LSPS1 LSP selection - Spawn background discovery task on `Node::start()`
Camillarhi
force-pushed
the
multi-lsp-support
branch
from
ecfe0b5 to
50c209d
Compare
Hi @tnull, I've split the second commit along the lines you suggested: |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks, def. more approachable!
| let discovery_cm = Arc::clone(&self.connection_manager); | ||
| self.runtime.spawn_background_task(async move { | ||
| let discovery_logger = Arc::clone(&liquidity_logger); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
We need to make sure that we cancel all spawned tasks on shutdown based on the stop signal.
| let liquidity_source = | ||
| self.liquidity_source.as_ref().ok_or(Error::LiquiditySourceUnavailable)?; | ||
| let lsp_config = LspConfig { node_id, address, token }; | ||
| liquidity_source.add_lsp_node(lsp_config.clone())?; |
There was a problem hiding this comment.
Can we just inline this helper and other very short helpers like is_lsps_node? They seem to just increas the clutter.
We should also abort here if we already have an entry for the node_id, not just add another entry at the end.
There was a problem hiding this comment.
Can we just inline this helper and other very short helpers like
is_lsps_node? They seem to just increas the clutter.
I have Inlined add_lsp_node directly into add_lsp, and kept LiquiditySource::is_lsps_node as a method since event.rs only holds an Arc<LiquiditySource> and can't reach the private lsp_nodes field without widening visibility.
We should also abort here if we already have an entry for the
node_id, not just add another entry at the end.
A duplicate check has been added that returns Error::DuplicateLspNode, so re-adding the same node_id aborts early instead of silently appending another entry.
| { | ||
| let mut pending_discovery = self.pending_lsps0_discovery.lock().expect("lock"); | ||
| lsps0_handler.list_protocols(node_id); | ||
| pending_discovery.insert(*node_id, sender); |
There was a problem hiding this comment.
Hmm, should we also abort here if we find a discovery is already in-flight before we just override it? Also, when would we expire stale requests, i.e., senders we add here but which never finish, e.g., by hitting an error below?
There was a problem hiding this comment.
Thanks for pointing that out. I hadn't considered either case. I'll look into it and push a fix.
| let lsps2_nodes = select_all_lsps_for_protocol(&self.lsp_nodes, 2); | ||
| if lsps2_nodes.is_empty() { | ||
| log_error!(self.logger, "No LSPs available for LSPS2 protocol.",); | ||
| return Err(Error::LiquiditySourceUnavailable); |
There was a problem hiding this comment.
Hmm, given the discovery will now take a bit after startup, do we see any way to make this not immediately fail, e.g., when a node comes up and calls receive_via_jit_channel? Maybe we could block on the discovery task, of course applying a timeout?
| ) -> &mut Self { | ||
| // Mark the LSP as trusted for 0conf | ||
| self.config.trusted_peers_0conf.push(node_id.clone()); | ||
| self.config.trusted_peers_0conf.push(node_id); |
There was a problem hiding this comment.
It seems we only set this here, but not for the Liquidity::add_lsp variant? Maybe they both should now take trust_peer_0conf: bool?
| /// This method is useful when the user wants to connect to an LSP but does not want to be concerned with | ||
| /// the specific protocol used for liquidity provision. The node will automatically detect and use the | ||
| /// appropriate protocol supported by the LSP. | ||
| pub fn add_lsp( |
There was a problem hiding this comment.
Hmm, the previous API (set_liquidity_source_lsps2) tried to somewhat disambiguate the case where we are the client vs. where we are the provider. We also intentionally had most builder methods start with set_ to reach a more homogeneous API. Breaking the latter is fine, but if we rename this it would be good to find a name that also works in context of set_liquidity_provider_lsps2, as add_lsp and set_liquidity_provider_lsps2 read now like they do very similar things, but they actually don't.
| let allow_0conf = self.config.trusted_peers_0conf.contains(&counterparty_node_id); | ||
| let mut channel_override_config = None; | ||
| if let Some((lsp_node_id, _)) = self | ||
| let is_lsp_node = self |
There was a problem hiding this comment.
Super confused here: why are we even updating the list in add_lsp if we now decide we trust every LSP for 0conf? Shouldn't we still maintain the list and only trust individual LSPs if the users configures them?
3 participants
Opening this as an early draft to get feedback on the overall API direction
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