fix(liquidity): update LspNode in place on re-add by Bartok9 · Pull Request #1147 · lightningdevkit/ldk-node

Bartok9 · 2026-10-10T13:04:08Z

When an existing LSP node_id is re-added with a new address, token, or 0conf setting, update the existing LspNode in place instead of silently dropping the new values. A changed address disconnects the live session, dials the new address, and rediscovers protocols. Token/0conf-only updates stay in place. Failure cleanup is generation-guarded so a stale rollback cannot clobber a newer write.

This is the liquidity-config side of #700 only. The peer-store upsert previously tracked in #1002 was closed without merging, so a restart can still reconnect to a stale peer-store address. That is a separate follow-up, not part of this PR.

Dropped the proptest 1.11.0 workflow pin. Upstream already landed that in #1153, and it was unrelated to this change. Rebased onto current main.

Test: add_liquidity_source_updates_existing_lsp_in_place.

Refs #700.
AI-assisted (Sera).
Agent-Owner: sera
Claim: sera

ldk-reviews-bot · 2026-10-10T13:04:10Z

I've assigned @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

ajaysehwal · 2026-10-10T15:03:29Z

-				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 {


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(()); }

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.

ajaysehwal · 2026-10-10T15:04:53Z

+				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);


🟠 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() gates accept_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".

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.

ajaysehwal · 2026-10-10T15:16:14Z

+				// 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));


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.

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.

ajaysehwal · 2026-10-10T15:16:54Z

 	/// `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.


🟡 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".

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.

Camillarhi

The proptest pin across the workflow files looks unrelated to the PR.

Also, the description says the peer store side of #700 is handled in #1002, but that PR has been closed without being merged. With only the LSP entry in Liquidity updated, the node would still reconnect to the stale address stored in the peer store.

Could you drop the workflow changes and update the description as well?

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 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>

Bartok9 · 2026-10-15T18:11:16Z

Thanks @Camillarhi — both points taken.

Dropped the proptest pin across the workflow files. Upstream already landed that in #1153, so it was unrelated here. Rebased onto current main; the diff is now only src/liquidity/mod.rs and src/builder.rs.

Updated the description. #1002 was closed without merging, so this PR is the liquidity-config side of #700 only. You're right that a restart can still reconnect to a stale peer-store address — PeerStore::add_peer still no-ops when the node_id is already stored. I'll track that upsert as a separate follow-up rather than widening this PR.

468eb63

Camillarhi · 2026-10-15T18:18:10Z

Thanks for the updates. One broader question before going further. Ignoring a duplicate node_id came out of this review on #792 (#792 (comment)), and in #956 we dropped runtime rediscovery (#956 (comment)) in favour of a restart. This PR goes back to updating in place, with reconnect and rediscovery at runtime, so I'm not sure it's the direction we want. @tnull, what do you think?

Bartok9 · 2026-10-15T18:19:05Z

Fair question — happy to wait on @tnull before going further.

The in-place update is only trying to stop a re-add from silently dropping a new address/token/0conf. #792's ignore-duplicate and #956's restart-only rediscovery are the other half of that, and I don't want to reopen them if the project prefers a restart for an address change.

If the preferred shape is "persist the new Liquidity config, reconnect only on next start," I can drop the live disconnect/dial/rediscovery and leave this as a config-only write. If runtime reconnect is still wanted, I'll keep the generation-guarded path as-is. Either way I'll follow whatever you and @tnull land on.

Camillarhi asked whether live disconnect/dial/rediscovery belongs here given lightningdevkit#792 and lightningdevkit#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.

Bartok9 · 2026-10-16T03:42:30Z

Picked a direction rather than waiting: persist in place, reconnect on next start — matching #956's restart-only rediscovery and #792 not fighting a live duplicate session.

d9421f7 drops live disconnect/dial/rediscovery on re-add. Address/token/0conf still update in place. New node_ids still connect and discover immediately. A live peer is left as-is until restart.

Happy to restore the generation-guarded runtime reconnect path if you and @tnull want that instead.

Re-adding a known peer with a new address previously no-op'ed via a contains_key check, so Node::connect(..., persist=true) kept the stale address for reconnection (lightningdevkit#700). Split add_peer into two methods sharing a persist-then-apply helper: - upsert_peer updates the stored address on re-add. Used for explicitly supplied addresses: Node::connect with persist set, open_channel, and the LSPS2 payment path. - add_peer_if_missing only stores unknown peers. Used for the gossip-derived address of inbound channel counterparties, which must not override an explicitly stored address. In-memory state is only mutated after a successful write, matching remove_peer. To avoid cloning the entire peer map, the update is applied temporarily under the write lock for serialization and restored before persisting; readers cannot observe the temporary value as they need the same lock. The liquidity-side counterpart (NodeBuilder/Liquidity add_liquidity_source) is deliberately left out here; it is tracked in lightningdevkit#1147. Fixes lightningdevkit#700. Supersedes lightningdevkit#1002. Co-authored-by: Bartok <259807879+Bartok9@users.noreply.github.com> Assisted-by: goose (AI)

PeerStore::add_peer returned early when the pubkey was already stored, so Node::connect(..., persist=true) with a new address silently kept the stale address used for reconnection (lightningdevkit#700). Split add_peer into two methods sharing a persist-then-apply helper, following the maintainer review on lightningdevkit#1002: - upsert_peer updates the stored address on re-add. Used where the address is explicitly supplied: Node::connect with persist set, open_channel, and the LSPS2 payment path. - add_peer_if_missing only stores unknown peers. Used for the gossip-derived address of inbound channel counterparties, which must not override an explicitly stored address. In-memory state is only mutated after a successful write, matching remove_peer. Following the review suggestion on lightningdevkit#1002, avoid cloning the entire peer map: apply the update temporarily under the write lock for serialization, restore it, persist, then apply it permanently. Readers cannot observe the temporary value as they need the same lock. Re-adding an unchanged peer is a no-op. The liquidity-side counterpart (NodeBuilder/Liquidity add_liquidity_source) is deliberately left out; it is tracked in lightningdevkit#1147. Fixes lightningdevkit#700. Supersedes lightningdevkit#1002 and lightningdevkit#1115. Co-authored-by: Bartok <259807879+Bartok9@users.noreply.github.com> Assisted-by: goose (AI)

PeerStore::add_peer returned early when the pubkey was already stored, so Node::connect(..., persist=true) with a new address silently kept the stale address used for reconnection (see lightningdevkit#700). Split add_peer into two methods sharing a persist-then-apply helper, following the maintainer review on lightningdevkit#1002: - upsert_peer updates the stored address on re-add. Used where the address is explicitly supplied: Node::connect with persist set, open_channel, and the LSPS2 payment path. - add_peer_if_missing only stores unknown peers. Used for the gossip-derived address of inbound channel counterparties, which must not override an explicitly stored address. In-memory state is only mutated after a successful write, matching remove_peer. Following the review suggestion on lightningdevkit#1002, avoid cloning the entire peer map: apply the update temporarily under the write lock for serialization, restore it, persist, then apply it permanently. Readers cannot observe the temporary value as they need the same lock. Re-adding an unchanged peer is a no-op. Addresses the peer-store side of lightningdevkit#700. The liquidity-source side (NodeBuilder/Liquidity add_liquidity_source) is deliberately left out here; it is tracked in lightningdevkit#1147. Supersedes lightningdevkit#1002 and lightningdevkit#1115. Co-authored-by: Bartok <259807879+Bartok9@users.noreply.github.com> Assisted-by: goose (AI)

PeerStore::add_peer returned early when the pubkey was already stored, so Node::connect(..., persist=true) with a new address silently kept the stale address used for reconnection (see lightningdevkit#700). Split add_peer into two methods sharing a persist-then-apply helper, following the maintainer review on lightningdevkit#1002: - upsert_peer updates the stored address on re-add. Used where the address is explicitly supplied: Node::connect with persist set, open_channel, and the LSPS2 payment path. - add_peer_if_missing only stores unknown peers. Used for the gossip-derived address of inbound channel counterparties, which must not override an explicitly stored address. In-memory state is only mutated after a successful write, matching remove_peer. Following the review suggestion on lightningdevkit#1002, avoid cloning the entire peer map: apply the update temporarily under the write lock for serialization, restore it, persist, then apply it permanently. Readers cannot observe the temporary value as they need the same lock. Re-adding an unchanged peer is a no-op. remove_peer is switched to the same pattern for consistency. Addresses the peer-store side of lightningdevkit#700. The liquidity-source side (NodeBuilder/Liquidity add_liquidity_source) is deliberately left out here; it is tracked in lightningdevkit#1147. Supersedes lightningdevkit#1002 and lightningdevkit#1115. Co-authored-by: Bartok <259807879+Bartok9@users.noreply.github.com> Assisted-by: goose (AI)

ldk-reviews-bot requested a review from tnull October 10, 2026 13:14

ajaysehwal reviewed Oct 10, 2026

View reviewed changes

Bartok9 force-pushed the fix/ldp-liquidity-lsp-in-place-update branch 2 times, most recently from 4963c4e to 3e1ba79 Compare October 12, 2026 13:03

Bartok9 mentioned this pull request Oct 13, 2026

fix(peer_store): upsert peer address when re-adding known peer #1002

Closed

Camillarhi reviewed Oct 15, 2026

View reviewed changes

Bartok9 force-pushed the fix/ldp-liquidity-lsp-in-place-update branch from 3953a40 to 468eb63 Compare October 15, 2026 18:10

vincenzopalazzo mentioned this pull request Oct 16, 2026

fix(peer_store): upsert peer address when re-adding known peer #1175

Open

Conversation

Bartok9 commented Oct 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Oct 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Camillarhi left a comment

Choose a reason for hiding this comment

Uh oh!

Bartok9 commented Oct 15, 2026

Uh oh!

Camillarhi commented Oct 15, 2026

Uh oh!

Bartok9 commented Oct 15, 2026

Uh oh!

Bartok9 commented Oct 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Bartok9 commented Oct 10, 2026 •

edited

Loading

ldk-reviews-bot commented Oct 10, 2026 •

edited

Loading