From 468eb635fda0ae2cba0674f2ea215fe65b818e8c Mon Sep 17 00:00:00 2001 From: Bartok9 <259807879+Bartok9@users.noreply.github.com> Date: Mon, 12 Oct 2026 08:59:53 -0400 Subject: [PATCH 1/2] fix(liquidity): update LspNode in place on re-add When an existing LSP node_id is re-added with a new address/token/0conf, update the existing LspNode entry in place rather than silently dropping the new values. Compare all three fields and reconnect/rediscover only when the address changes; do not mem::take supported_protocols before connect+discover; guard failure cleanup with a generation counter so a stale write cannot clobber newer state. Related to #700 (peer_store upsert in #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> --- src/builder.rs | 61 +++++++++++++++++++- src/liquidity/mod.rs | 129 ++++++++++++++++++++++++++++++++----------- 2 files changed, 156 insertions(+), 34 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 54e79f4b3a..205ca92cbd 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -529,7 +529,7 @@ impl NodeBuilder { /// 0-confirmation channels opened by this LSP. If `false`, 0-confirmation /// acceptance for this peer falls back to [`Config::trusted_peers_0conf`]. /// - /// May be called multiple times to register several LSPs. Duplicate `node_id`s are ignored. + /// May be called multiple times to register several LSPs. Re-adding an existing `node_id` updates its address, token, and 0conf trust settings. /// /// [bLIP-50 / LSPS0]: https://github.com/lightning/blips/blob/master/blip-0050.md pub fn add_liquidity_source( @@ -539,7 +539,12 @@ impl NodeBuilder { let liquidity_source_config = self.liquidity_source_config.get_or_insert(LiquiditySourceConfig::default()); - if liquidity_source_config.lsp_nodes.iter().any(|n| n.node_id == node_id) { + if let Some(existing) = + liquidity_source_config.lsp_nodes.iter_mut().find(|n| n.node_id == node_id) + { + existing.address = address; + existing.token = token; + existing.trust_peer_0conf = trust_peer_0conf; return self; } @@ -1189,7 +1194,7 @@ impl Builder { /// 0-confirmation channels opened by this LSP. If `false`, 0-confirmation /// acceptance for this peer falls back to [`Config::trusted_peers_0conf`]. /// - /// May be called multiple times to register several LSPs. Duplicate `node_id`s are ignored. + /// May be called multiple times to register several LSPs. Re-adding an existing `node_id` updates its address, token, and 0conf trust settings. /// /// [bLIP-50 / LSPS0]: https://github.com/lightning/blips/blob/master/blip-0050.md pub fn add_liquidity_source( @@ -2938,4 +2943,54 @@ mod tests { builder.set_runtime(multi_thread_runtime.handle().clone()).unwrap(); assert!(builder.setup_runtime(&logger).is_ok(), "a multi-threaded runtime should be used"); } + + #[test] + fn add_liquidity_source_updates_existing_lsp_in_place() { + use std::str::FromStr; + + use bitcoin::secp256k1::PublicKey; + use lightning::ln::msgs::SocketAddress; + + let mut builder = NodeBuilder::new(); + let node_id = PublicKey::from_str( + "0276607124ebe6a6c9338517b6f485825b27c2dcc0b9fc2aa6a4c0df91194e5993", + ) + .unwrap(); + let first_address = SocketAddress::from_str("127.0.0.1:9735").unwrap(); + let second_address = SocketAddress::from_str("127.0.0.2:9735").unwrap(); + + // Register an LSP, then re-add the same node_id with a new address and + // different 0conf/trust settings. Re-adding must update the existing + // entry in place rather than silently dropping the new values. + builder.add_liquidity_source(node_id, first_address.clone(), None, true); + builder.add_liquidity_source( + node_id, + second_address.clone(), + Some("token".to_owned()), + false, + ); + + let config = builder + .liquidity_source_config + .as_ref() + .expect("liquidity source config should be initialized"); + assert_eq!( + config.lsp_nodes.len(), + 1, + "re-adding the same node_id must not create a duplicate entry", + ); + assert_eq!( + config.lsp_nodes[0].address, second_address, + "re-adding must update the address in place", + ); + assert_eq!( + config.lsp_nodes[0].token, + Some("token".to_owned()), + "re-adding must update the token in place", + ); + assert!( + !config.lsp_nodes[0].trust_peer_0conf, + "re-adding must update the 0conf trust setting in place", + ); + } } diff --git a/src/liquidity/mod.rs b/src/liquidity/mod.rs index c1442b0fc9..955f26708a 100644 --- a/src/liquidity/mod.rs +++ b/src/liquidity/mod.rs @@ -156,7 +156,10 @@ impl Liquidity { /// 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 /// 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 + /// disconnects any live session, dials the new address, and rediscovers protocols. Token or + /// 0conf-only updates are applied in place. A failed update restores the previous config only + /// if no newer update for the same `node_id` has landed. pub fn add_liquidity_source( &self, node_id: PublicKey, address: SocketAddress, token: Option, trust_peer_0conf: bool, @@ -165,45 +168,105 @@ impl Liquidity { return Err(Error::NotRunning); } + let mut previous: Option<(SocketAddress, Option, bool, Option>)> = None; + let mut addr_changed = true; + // Generation of this write. Cleanup restores or removes only if it is still current, + // so a concurrent re-add for the same node_id is not rolled back. + // New entries start at generation 1; existing entries inherit the current generation. + let mut update_generation = 1u64; { let mut lsp_nodes = self.liquidity_source.lsp_nodes.write().expect("lock"); - if lsp_nodes.iter().any(|n| n.node_id == node_id) { - 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) { + addr_changed = existing.address != address; + let changed = addr_changed + || existing.token != token + || existing.trust_peer_0conf != trust_peer_0conf; + if !changed { + log_info!(self.logger, "LSP node {} already added, skipping.", node_id); + return Ok(()); + } + if addr_changed { + log_info!( + self.logger, + "Updating existing LSP node {} address/config; disconnecting and reconnecting.", + node_id + ); + } else { + log_info!( + self.logger, + "Updating existing LSP node {} token/0conf without reconnecting.", + node_id + ); + } + let prev_address = std::mem::replace(&mut existing.address, address.clone()); + let prev_token = std::mem::replace(&mut existing.token, token.clone()); + let prev_trust = + std::mem::replace(&mut existing.trust_peer_0conf, trust_peer_0conf); + // 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(); + existing.config_generation = existing.config_generation.wrapping_add(1); + update_generation = existing.config_generation; + previous = Some((prev_address, prev_token, prev_trust, prev_protocols)); + } else { + lsp_nodes.push(LspNode { + node_id, + address: address.clone(), + token: token.clone(), + trust_peer_0conf, + supported_protocols: None, + config_generation: update_generation, + }); } - - lsp_nodes.push(LspNode { - node_id, - address: address.clone(), - token: token.clone(), - trust_peer_0conf, - supported_protocols: None, - }); } - // If anything below fails, drop the half-initialized entry so the user can retry cleanly. + // On failure: remove half-initialized inserts; restore prior config for updates. + // Skip if a newer add for this node_id already committed a later generation. let lsp_nodes = Arc::clone(&self.liquidity_source.lsp_nodes); let cleanup = move || { - lsp_nodes.write().expect("lock").retain(|n| n.node_id != node_id); + let mut nodes = lsp_nodes.write().expect("lock"); + let still_current = nodes + .iter() + .find(|n| n.node_id == node_id) + .is_some_and(|n| n.config_generation == update_generation); + if !still_current { + return; + } + if let Some((prev_address, prev_token, prev_trust, prev_protocols)) = previous { + if let Some(existing) = nodes.iter_mut().find(|n| n.node_id == node_id) { + existing.address = prev_address; + existing.token = prev_token; + existing.trust_peer_0conf = prev_trust; + existing.supported_protocols = prev_protocols; + } + } else { + nodes.retain(|n| n.node_id != node_id); + } }; - let con_cm = Arc::clone(&self.connection_manager); - let connect_addr = address.clone(); - if let Err(e) = self - .runtime - .block_on(async move { con_cm.connect_peer_if_necessary(node_id, connect_addr).await }) - { - cleanup(); - return Err(e); - } - log_info!(self.logger, "Connected to LSP {}@{}.", node_id, address); - - if let Err(e) = self - .runtime - .block_on(async { self.liquidity_source.discover_lsp_protocols(&node_id).await }) - { - cleanup(); - return Err(e); + if addr_changed { + let con_cm = Arc::clone(&self.connection_manager); + let connect_addr = address.clone(); + // connect_peer_if_necessary no-ops while the peer is up, so a live LSP would keep + // the old session and never dial the new address. Drop it first, then dial. + if let Err(e) = self.runtime.block_on(async move { + con_cm.disconnect_peer(node_id); + con_cm.do_connect_peer(node_id, connect_addr).await + }) { + cleanup(); + return Err(e); + } + log_info!(self.logger, "Connected to LSP {}@{}.", node_id, address); + + if let Err(e) = self + .runtime + .block_on(async { self.liquidity_source.discover_lsp_protocols(&node_id).await }) + { + cleanup(); + return Err(e); + } + } else { + log_info!(self.logger, "Updated LSP {} config (address unchanged).", node_id); } Ok(()) @@ -239,6 +302,9 @@ pub(crate) struct LspNode { trust_peer_0conf: bool, // Protocol numbers discovered via LSPS0 (e.g., 1 = LSPS1, 2 = LSPS2, 5 = LSPS5). supported_protocols: Option>, + // Bumped on every in-place config write. Failure cleanup matches this so a newer + // concurrent update is not overwritten by an older rollback. + config_generation: u64, } pub(crate) struct LiquiditySourceBuilder @@ -338,6 +404,7 @@ where token: cfg.token, trust_peer_0conf: cfg.trust_peer_0conf, supported_protocols: None, + config_generation: 0, }) .collect(), )); From d9421f7e9cb539f46e7f4503e897caf2d614546f Mon Sep 17 00:00:00 2001 From: Bartok9 <259807879+Bartok9@users.noreply.github.com> Date: Thu, 15 Oct 2026 23:42:08 -0400 Subject: [PATCH 2/2] fix(liquidity): persist LSP re-add without runtime reconnect Camillarhi asked whether live disconnect/dial/rediscovery belongs here given #792 and #956. Persist address/token/0conf in place; leave any live peer connected and use the new address on the next start. New node_ids still connect and discover immediately. --- src/builder.rs | 4 +- src/liquidity/mod.rs | 130 +++++++++++++------------------------------ 2 files changed, 41 insertions(+), 93 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 205ca92cbd..7d53d4a7d5 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -529,7 +529,7 @@ impl NodeBuilder { /// 0-confirmation channels opened by this LSP. If `false`, 0-confirmation /// acceptance for this peer falls back to [`Config::trusted_peers_0conf`]. /// - /// May be called multiple times to register several LSPs. Re-adding an existing `node_id` updates its address, token, and 0conf trust settings. + /// May be called multiple times to register several LSPs. Re-adding an existing `node_id` updates its address, token, and 0conf trust settings. A running node uses the new address on the next start. /// /// [bLIP-50 / LSPS0]: https://github.com/lightning/blips/blob/master/blip-0050.md pub fn add_liquidity_source( @@ -1194,7 +1194,7 @@ impl Builder { /// 0-confirmation channels opened by this LSP. If `false`, 0-confirmation /// acceptance for this peer falls back to [`Config::trusted_peers_0conf`]. /// - /// May be called multiple times to register several LSPs. Re-adding an existing `node_id` updates its address, token, and 0conf trust settings. + /// May be called multiple times to register several LSPs. Re-adding an existing `node_id` updates its address, token, and 0conf trust settings. A running node uses the new address on the next start. /// /// [bLIP-50 / LSPS0]: https://github.com/lightning/blips/blob/master/blip-0050.md pub fn add_liquidity_source( diff --git a/src/liquidity/mod.rs b/src/liquidity/mod.rs index 955f26708a..ed1546e2ba 100644 --- a/src/liquidity/mod.rs +++ b/src/liquidity/mod.rs @@ -156,10 +156,9 @@ impl Liquidity { /// 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 /// LSP. Note this supersedes [`Config::trusted_peers_0conf`] for this peer. - /// Re-adding an existing `node_id` updates its address/token/0conf settings. A changed address - /// disconnects any live session, dials the new address, and rediscovers protocols. Token or - /// 0conf-only updates are applied in place. A failed update restores the previous config only - /// if no newer update for the same `node_id` has landed. + /// Re-adding an existing `node_id` updates its address/token/0conf settings in place. A live + /// peer is left connected; the new address is used on the next start (same restart-only + /// rediscovery as #956). New `node_id`s still connect and discover immediately. pub fn add_liquidity_source( &self, node_id: PublicKey, address: SocketAddress, token: Option, trust_peer_0conf: bool, @@ -168,105 +167,58 @@ impl Liquidity { return Err(Error::NotRunning); } - let mut previous: Option<(SocketAddress, Option, bool, Option>)> = None; - let mut addr_changed = true; - // Generation of this write. Cleanup restores or removes only if it is still current, - // so a concurrent re-add for the same node_id is not rolled back. - // New entries start at generation 1; existing entries inherit the current generation. - let mut update_generation = 1u64; { let mut lsp_nodes = self.liquidity_source.lsp_nodes.write().expect("lock"); if let Some(existing) = lsp_nodes.iter_mut().find(|n| n.node_id == node_id) { - addr_changed = existing.address != address; - let changed = addr_changed + let changed = existing.address != address || existing.token != token || existing.trust_peer_0conf != trust_peer_0conf; if !changed { log_info!(self.logger, "LSP node {} already added, skipping.", node_id); return Ok(()); } - if addr_changed { - log_info!( - self.logger, - "Updating existing LSP node {} address/config; disconnecting and reconnecting.", - node_id - ); - } else { - log_info!( - self.logger, - "Updating existing LSP node {} token/0conf without reconnecting.", - node_id - ); - } - let prev_address = std::mem::replace(&mut existing.address, address.clone()); - let prev_token = std::mem::replace(&mut existing.token, token.clone()); - let prev_trust = - std::mem::replace(&mut existing.trust_peer_0conf, trust_peer_0conf); - // 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(); - existing.config_generation = existing.config_generation.wrapping_add(1); - update_generation = existing.config_generation; - previous = Some((prev_address, prev_token, prev_trust, prev_protocols)); - } else { - lsp_nodes.push(LspNode { - node_id, - address: address.clone(), - token: token.clone(), - trust_peer_0conf, - supported_protocols: None, - config_generation: update_generation, - }); + log_info!( + self.logger, + "Updating existing LSP node {} config in place; reconnect on next start.", + node_id + ); + existing.address = address; + existing.token = token; + existing.trust_peer_0conf = trust_peer_0conf; + return Ok(()); } + lsp_nodes.push(LspNode { + node_id, + address: address.clone(), + token: token.clone(), + trust_peer_0conf, + supported_protocols: None, + }); } - // On failure: remove half-initialized inserts; restore prior config for updates. - // Skip if a newer add for this node_id already committed a later generation. + // If anything below fails, drop the half-initialized entry so the user can retry cleanly. let lsp_nodes = Arc::clone(&self.liquidity_source.lsp_nodes); let cleanup = move || { - let mut nodes = lsp_nodes.write().expect("lock"); - let still_current = nodes - .iter() - .find(|n| n.node_id == node_id) - .is_some_and(|n| n.config_generation == update_generation); - if !still_current { - return; - } - if let Some((prev_address, prev_token, prev_trust, prev_protocols)) = previous { - if let Some(existing) = nodes.iter_mut().find(|n| n.node_id == node_id) { - existing.address = prev_address; - existing.token = prev_token; - existing.trust_peer_0conf = prev_trust; - existing.supported_protocols = prev_protocols; - } - } else { - nodes.retain(|n| n.node_id != node_id); - } + lsp_nodes.write().expect("lock").retain(|n| n.node_id != node_id); }; - if addr_changed { - let con_cm = Arc::clone(&self.connection_manager); - let connect_addr = address.clone(); - // connect_peer_if_necessary no-ops while the peer is up, so a live LSP would keep - // the old session and never dial the new address. Drop it first, then dial. - if let Err(e) = self.runtime.block_on(async move { - con_cm.disconnect_peer(node_id); - con_cm.do_connect_peer(node_id, connect_addr).await - }) { - cleanup(); - return Err(e); - } - log_info!(self.logger, "Connected to LSP {}@{}.", node_id, address); - - if let Err(e) = self - .runtime - .block_on(async { self.liquidity_source.discover_lsp_protocols(&node_id).await }) - { - cleanup(); - return Err(e); - } - } else { - log_info!(self.logger, "Updated LSP {} config (address unchanged).", node_id); + let con_cm = Arc::clone(&self.connection_manager); + let connect_addr = address.clone(); + if let Err(e) = self + .runtime + .block_on(async move { con_cm.connect_peer_if_necessary(node_id, connect_addr).await }) + { + cleanup(); + return Err(e); + } + log_info!(self.logger, "Connected to LSP {}@{}.", node_id, address); + + if let Err(e) = self + .runtime + .block_on(async { self.liquidity_source.discover_lsp_protocols(&node_id).await }) + { + cleanup(); + return Err(e); } Ok(()) @@ -302,9 +254,6 @@ pub(crate) struct LspNode { trust_peer_0conf: bool, // Protocol numbers discovered via LSPS0 (e.g., 1 = LSPS1, 2 = LSPS2, 5 = LSPS5). supported_protocols: Option>, - // Bumped on every in-place config write. Failure cleanup matches this so a newer - // concurrent update is not overwritten by an older rollback. - config_generation: u64, } pub(crate) struct LiquiditySourceBuilder @@ -404,7 +353,6 @@ where token: cfg.token, trust_peer_0conf: cfg.trust_peer_0conf, supported_protocols: None, - config_generation: 0, }) .collect(), ));