Repository navigation
Conversation
When an existing LSP node_id is re-added with a new address/token/0conf, update the existing entry in place rather than silently dropping the new values. Previously, duplicates were ignored; now the config is updated and the node is reconnected. Related to lightningdevkit#700 (peer_store upsert in lightningdevkit#1002; this fixes the LSP config side). Test: add_liquidity_source_updates_existing_lsp_in_place verifies re-add updates address, token, and 0conf settings. AI-assisted (Sera). Signed-off-by: Bartok9 <259807879+Bartok9@users.noreply.github.com>
|
I've assigned @tnull as a reviewer! |
| log_info!(self.logger, "LSP node {} already added, skipping.", node_id); | ||
| return Ok(()); | ||
| if let Some(existing) = lsp_nodes.iter_mut().find(|n| n.node_id == node_id) { | ||
| if existing.address == address { |
There was a problem hiding this comment.
Potential bug: token and trust_peer_0conf updates are silently dropped when the address is unchanged.
The current check only compares the address, so re-adding the same node_id with a new token or trust_peer_0conf returns early and discards the changes. This means token rotation or revoking 0-conf trust at runtime has no effect.
Could we compare all three fields and only reset protocols or reconnect when the address changes?
let addr_changed = existing.address != address;
let changed = addr_changed
|| existing.token != token
|| existing.trust_peer_0conf != trust_peer_0conf;
if !changed {
return Ok(());
}There was a problem hiding this comment.
Good catch — the early return only compared the address, so a token rotation or a 0conf trust change on the same node_id was dropped.
Pushed 3be1fb5: we now treat the entry as changed if address, token, or trust_peer_0conf differs, and we only reconnect / reset supported_protocols when the address actually changes. Token- or 0conf-only updates are applied in place and left alone if connect/discover is not needed.
| let prev_trust = | ||
| std::mem::replace(&mut existing.trust_peer_0conf, trust_peer_0conf); | ||
| // Force rediscovery after config/address change. | ||
| let prev_protocols = std::mem::take(&mut existing.supported_protocols); |
There was a problem hiding this comment.
🟠 High: mem::take(supported_protocols) makes a healthy LSP "unknown" for the whole connect + discovery window → breaks LSPS2 acceptance / JIT flows
After this take() (L180) supported_protocols is None immediately, before connect + LSPS0 discovery run (up to LIQUIDITY_REQUEST_TIMEOUT_SECS). get_lsp_config / select_lsps_for_protocol (mod.rs L84-100) only match an LSP whose supported_protocols contains the protocol, and get_lsp_config doesn't wait because discovery_done_rx is already true. So during the window:
get_lsp_config(..)→None.- In
event.rs(~L1637),get_lsp_config(&counterparty_node_id, 2).is_some()gatesaccept_underpaying_htlcs. An inbound JIT channel from this LSP arriving now is accepted without that override, so the LSP's fee-skimmed HTLC is rejected as underpaying: a failed or stuck JIT payment with the LSPS2 lease already consumed. - Outgoing LSPS1/LSPS2 requests fail with "no LSP" although the LSP didn't change.
Suggested fix: don't clear supported_protocols up front. Keep the old value until rediscovery succeeds (discover_lsp_protocols already overwrites it). If you want to invalidate on address change, clear it only after a successful reconnect, just before discovery. At minimum, make the event.rs LSPS2 check tolerant of "known LSP, discovery in flight".
There was a problem hiding this comment.
Agreed — clearing supported_protocols up front made this LSP invisible to get_lsp_config for the whole connect + LSPS0 window.
46f911f keeps the previous protocols until discover_lsp_protocols overwrites them, and only reconnects/rediscovers when the address actually changed. Failure cleanup still restores the prior snapshot.
Also fixes the E0382 from 3be1fb5 (previous moved into the cleanup closure, then borrowed for the reconnect check) that failed check-python / macOS stable.
Re-adding an existing LSP compared only the address, so token rotation and 0conf trust changes were dropped. Compare all three fields and reconnect/rediscover only when the address changes.
Do not mem::take supported_protocols before connect+discover, and decide reconnect from a flag instead of borrowing the moved snapshot.
| // Keep supported_protocols until rediscovery overwrites them. Clearing here | ||
| // makes get_lsp_config miss this LSP for the whole connect+discover window. | ||
| let prev_protocols = existing.supported_protocols.clone(); | ||
| previous = Some((prev_address, prev_token, prev_trust, prev_protocols)); |
There was a problem hiding this comment.
Thanks, the take() removal (L192-194) resolves the JIT-window concern.
One thing still open: the snapshot rollback can race with a concurrent update for the same node_id. The lock is released at L205, then cleanup (L209-221) restores the snapshot taken at L195:
- A snapshots addr0, sets addr1
- B snapshots addr1, sets addr2
- A fails → L213-216 write addr0 (and stale
supported_protocols) over B's in-flight addr2 - B succeeds →
Ok(()), but the stored config is addr0
Suggested fix: serialize updates per LSP (lock held across the whole call), or connect + discover first and commit to lsp_nodes only on success, which makes rollback unnecessary. If concurrent re-adds aren't a supported use, a doc note would also be acceptable.
There was a problem hiding this comment.
Good catch — the lock is dropped before connect/discover, so a failed older update could write its snapshot over a newer re-add.
0499288 stamps each write with config_generation. Cleanup restores the snapshot (or removes a fresh insert) only when that generation is still current; a later add for the same node_id is left alone. Combined with the disconnect-then-dial fix so an address change actually hits the new endpoint before we treat the update as committed.
| /// `trust_peer_0conf` controls whether the node will accept 0-confirmation channels opened by this | ||
| /// LSP. Note this supersedes [`Config::trusted_peers_0conf`] for this peer. | ||
| /// Duplicate `node_id`s are ignored. | ||
| /// Re-adding an existing `node_id` updates its address/token/0conf settings. A changed address reconnects and rediscovers protocols; token or 0conf-only updates are applied in place. |
There was a problem hiding this comment.
🟡 Medium: "A changed address reconnects" (L157) / "…and reconnecting" (L178) isn't true for an already-connected LSP
connect_peer_if_necessary (L227) returns Ok(()) immediately if the peer is connected (connection.rs:81-83). When updating a live LSP, the new address is never dialled or validated, and rediscovery (L236) runs over the old session. A wrong address is accepted and only fails at the next disconnect, and the rollback at L229/L238 never fires.
Suggested fix: on address change, disconnect_peer(node_id) then do_connect_peer(node_id, address). Otherwise, reword the doc and log to "applied on next reconnect".
There was a problem hiding this comment.
Agreed — connect_peer_if_necessary returns immediately while the peer is up, so a live LSP never dialed the new address and the failure path never ran.
0499288 disconnects first, then do_connect_peer with the new address, and only rediscovers after that dial succeeds. Token/0conf-only updates still skip the reconnect. Doc and log now say we disconnect and reconnect, not that a no-op connect counts.
…rollback connect_peer_if_necessary no-ops while the peer is up, so a live LSP kept the old session. Disconnect, then dial the new address. Failure cleanup restores or removes only if this write's generation is still current. Signed-off-by: Bartok9 <259807879+Bartok9@users.noreply.github.com>
3 participants
When an existing LSP node_id is re-added with a new address/token/0conf, update the existing entry in place instead of silently dropping the new values. Extends the peer_store upsert pattern (#1002) to the liqudity config path.
Test: add_liquidity_source_updates_existing_lsp_in_place.
Refs #700.
AI-assisted (Sera).
Agent-Owner: sera
Claim: sera