Repository navigation
fix(peer_store): upsert peer address when re-adding known peer - #1175
vincenzopalazzo wants to merge 1 commit into
Conversation
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)
|
I've assigned @tnull as a reviewer! |
There was a problem hiding this comment.
🤖 Potential bug: an unverified address can overwrite a working peer address
connect_peer_if_necessary returns Ok(()) without dialing when the peer is already connected, so the subsequent upsert_peer can persist an address we never connected to. For example, if a peer is connected at good:9735, calling node.connect(X, "typo:9735", true) succeeds and replaces the working address. After a restart, the reconnect loop uses the typo and fails to reconnect. The same issue applies to open_channel_inner.
Could we only upsert when we actually dialed the requested address? Comparing with PeerDetails::socket_address won't work because, for inbound peers, it's the ephemeral source address rather than the listening address. A test covering an already-connected peer with a different requested address would help prevent this regression.
| } | ||
| } | ||
|
|
||
| self.persist_and_apply(peer_info).await |
3 participants
Supersedes #1002 and #1115. Addresses the peer-store side of #700; the
liquidity-source side is tracked in #1147.
Problem
PeerStore::add_peerreturned early when the pubkey was alreadystored, so
Node::connect(node_id, new_address, persist = true)silentlykept the stale address for reconnection.