Skip to content

Update stored peer address on persist connect - #1115

Closed
vincenzopalazzo wants to merge 1 commit into
lightningdevkit:mainfrom
vincenzopalazzo:fix/peer-store-address-update
Closed

Update stored peer address on persist connect#1115
vincenzopalazzo wants to merge 1 commit into
lightningdevkit:mainfrom
vincenzopalazzo:fix/peer-store-address-update

Conversation

@vincenzopalazzo

@vincenzopalazzo vincenzopalazzo commented Sep 22, 2026

Copy link
Copy Markdown
Member

Summary

  • On ldk-server, connect(pubkey, new_addr, persist=true) succeeded and logged the new socket, but the peer store kept the old address. The reconnect task only reads peer_store.list_peers(), so the next dial (and every dial after restart) used the stale address.
  • Seen for 028ea2162c8cc5d9bd938c5b218a3432b1eae23f9d36f491d6d8a811b95578d494: stored 98.207.69.189:54324, connected at 98.234.61.157:54324 on 2026-09-21T17:43:24Z, then dialed the old address again from 2026-09-22T13:00:12Z about once a minute. A process restart only reloaded the stale entry.
  • PeerStore::add_peer returned Ok(()) when the pubkey was already present and never wrote the new address. It now replaces the address and persists it. Memory is updated only after the write succeeds, same rule as remove_peer. An unchanged address still skips the write.
  • Disconnect is not the fix. It drops the store entry and the live session.

Review Notes

  • Production-safety review passed (1 round).
  • Reconnect interval is unchanged. No gossip-address fallback in this PR.

Decision Log

Hardest decision: Persist the updated map before mutating self.peers, instead of inserting under the write lock and then writing. list_peers() is what the reconnect task dials, so memory has to move, but a failed KV write must not leave this process ahead of disk. Encoding a clone and inserting only after persist_peers matches remove_peer.

Alternatives rejected:

  • Leave the early return and tell operators to disconnect then connect: disconnect removes the persisted peer and drops the live session. That is a workaround, not the persist contract.
  • Update memory first, then persist: a failed write would make this process dial the new address and the next restart dial the old one.
  • After N failed dials, try a gossip address: out of scope until the persist update is correct.

Least confident about: HashMap iteration order in PeerStoreSerWrapper is not stable, so the unchanged-address test compares raw KV bytes. A future change that re-encodes an equal map could fail that assertion even though the logical address is unchanged. The reload assertions on 127.0.0.1:9739 are the ones that catch the ldk-server bug.

Test plan

  • add_peer_updates_stored_address fails on upstream/main (cd4fe79): reloaded address stays 127.0.0.1:9738
  • Same test passes on this branch, including reload of 127.0.0.1:9739
  • peer_info_persistence and remove_peer_does_not_mutate_memory_if_persist_fails pass
  • add_peer_does_not_mutate_memory_if_persist_fails passes
  • CI passes

Assisted-by: goose (AI)

@vincenzopalazzo
fix: update stored peer address on persist connect
9be876b 
PeerStore::add_peer returned early when the pubkey was already stored,
so Node::connect(..., persist=true) left a stale address for reconnect.
Replace the address and persist it when it changes, and leave memory
untouched if that write fails.

Assisted-by: goose (AI)
@ldk-reviews-bot

ldk-reviews-bot commented Sep 22, 2026 •
edited
Loading

Copy link
Copy Markdown

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.

@enigbe

enigbe commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

I think you might want to look at #1002. It addresses the same issue.

@vincenzopalazzo

vincenzopalazzo commented Sep 22, 2026

Copy link
Copy Markdown
Member Author

Oh thanks @enigbe for some reason my Ai did not find the duplicated

So closing in favor of #1002

vincenzopalazzo added a commit to vincenzopalazzo/ldk-node that referenced this pull request Oct 16, 2026
@vincenzopalazzo @Bartok9
fix(peer_store): upsert peer address when re-adding known peer
66feeaa 
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)
vincenzopalazzo added a commit to vincenzopalazzo/ldk-node that referenced this pull request Oct 16, 2026
@vincenzopalazzo @Bartok9
fix(peer_store): upsert peer address when re-adding known peer
dd45214 
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)
vincenzopalazzo added a commit to vincenzopalazzo/ldk-node that referenced this pull request Oct 16, 2026
@vincenzopalazzo @Bartok9
fix(peer_store): upsert peer address when re-adding known peer
1b28e63 
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)
@vincenzopalazzo vincenzopalazzo mentioned this pull request Oct 16, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tnull tnull Awaiting requested review from tnull

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vincenzopalazzo @ldk-reviews-bot @enigbe