multi: peer address management — persist, list, forget#10931
multi: peer address management — persist, list, forget#10931ZZiigguurraatt wants to merge 1 commit into
Conversation
|
PR Severity: CRITICAL. Files: channeldb/db.go, channeldb/persistent_peers.go, rpcserver.go, server.go are all CRITICAL. Label severity-critical applied. <!-- pr-severity-bot --> |
|
PR Severity: CRITICAL Highest-severity file rule | 15 files total | 1013 lines changed (excl. tests and auto-generated) Critical (4 files):
High (1 file):
Medium (4 files):
Low / Excluded:
Analysis This PR introduces persistent peer address management across three areas that individually each trigger CRITICAL classification: (1) channeldb/persistent_peers.go (new file) adds a new top-level bucket to the channel database for storing user-requested persistent peer addresses keyed by compressed pubkey. Any new channeldb schema warrants expert review for correctness, migration safety, and potential data corruption scenarios. (2) server.go (~394 lines changed) is the largest change in the PR. It rewires establishPersistentConnections to read the new persistent-peers bucket on startup, modifies peer reconnection logic, and adds ConnectToPeer return-status semantics. This is core peer lifecycle code. (3) rpcserver.go (~118 lines) updates ConnectPeer and DisconnectPeer RPC handlers with new wait_for_dial, forget, and force flags, including a new synchronous dial path in --perm mode. The combination of a new channeldb storage structure, significant server-side peer lifecycle changes, and new RPC semantics across multiple distinct critical packages warrants thorough expert review. To override, add a severity-override-{critical,high,medium,low} label. |
This PR adds three capabilities for managing peer addresses:
1. Persist addresses across restarts. `lncli connect --perm` now stores
the address on disk so the connection is re-established
automatically after an lnd restart, even when no open channel pins
the peer.
2. List all known addresses associated with a peer. `lncli listpeers`
gains three new per-peer address lists — perm_addresses (what the
user typed at --perm time), channel_peer_addresses (the LinkNode
record captured at first channel open), and gossip_addresses (the
current NodeAnnouncement) — plus is_persistent and reconnect_pending
status flags, and an --include_offline_persistent_peers request flag
to also surface peers in the reconnect set we are not currently
connected to.
3. Allow completely forgetting all of a peer's addresses. `lncli
disconnect` gains --forget (remove the persistent-peer bucket
entry; clears perm_addresses) and --force (also remove the LinkNode
record even when open channels exist; clears
channel_peer_addresses).
---
Implementation details:
Store user-requested perm peers in a new top-level
persistent-peers-bucket in channeldb (keyed by compressed pubkey, value
is the union of addresses typed at --perm time). On startup,
establishPersistentConnections reads this bucket alongside LinkNode and
graph addresses, and adds the entries to the persistent reconnect set
with perm=true so they are not pruned when the channel count drops to
zero.
`--forget` (DisconnectPeerRequest.forget) removes the on-disk perm-bucket
entry. By default it keeps the LinkNode record while open channels
exist; `--force` (DisconnectPeerRequest.force) removes the LinkNode too.
The handler rejects --force without --forget.
`--wait_for_dial` (ConnectPeerRequest.wait_for_dial) makes `--perm`
synchronous: the RPC blocks on the initial dial and only persists the
address on success. The async --perm semantics (return immediately,
persist regardless of dial success) remain the default.
When --perm targets a peer we are already connected to on a new address,
lnd now persists the new address and lets OutboundPeerConnected's
existing duplicate-handling logic swap the active connection over once
the new dial succeeds.
Address sources are intentionally not deduped across perm_addresses,
channel_peer_addresses, and gossip_addresses; dial-time dedup happens
internally before any outbound connection.
ConnectToPeer now returns descriptive status strings so the
ConnectPeerResponse distinguishes "marked as persistent", "already
persistent", and "swap initiated" cases from the default "connection
initiated".
itests: persistent peer survives restart, persistent peer address swap,
persistent peer wait for dial, forget persistent peer with channel.
Fixes lightningnetwork#10870.
Fixes lightningnetwork#10871.
92b3b1d to
34b2a06
Compare
ziggie1984
left a comment
There was a problem hiding this comment.
Concept NACK, this approach looks more like a band-aid to a broader address lifecycle gap
In my opinion if not really needed right now we should aim for a broader fix.
Given the current LND code we are missing proper address lifecycle management, so I see the value this PR provides by allowing manual management of addresses. But I don't think we should only aim for manual address management but rather introduce a proper lifecycle with garbage collection, priority of addresses and more. Adding an additional KV bucket although we are on our way to Native SQL adds additional burden to maintain this code in the future.
I don't think this feature is critical to not go with a proper clean approach rather than fixing the connect --perm issue. Open to discuss a narrower fix if you really need this feature for smoke testing environment or other parts where you encountered this gap.
This PR adds three capabilities for managing peer addresses:
Implementation details:
Store user-requested perm peers in a new top-level
persistent-peers-bucket in channeldb (keyed by compressed pubkey, value
is the union of addresses typed at --perm time). On startup,
establishPersistentConnections reads this bucket alongside LinkNode and
graph addresses, and adds the entries to the persistent reconnect set
with perm=true so they are not pruned when the channel count drops to
zero.
--forget(DisconnectPeerRequest.forget) removes the on-disk perm-bucketentry. By default it keeps the LinkNode record while open channels
exist;
--force(DisconnectPeerRequest.force) removes the LinkNode too.The handler rejects --force without --forget.
--wait_for_dial(ConnectPeerRequest.wait_for_dial) makes--permsynchronous: the RPC blocks on the initial dial and only persists the
address on success. The async --perm semantics (return immediately,
persist regardless of dial success) remain the default.
When --perm targets a peer we are already connected to on a new address,
lnd now persists the new address and lets OutboundPeerConnected's
existing duplicate-handling logic swap the active connection over once
the new dial succeeds.
Address sources are intentionally not deduped across perm_addresses,
channel_peer_addresses, and gossip_addresses; dial-time dedup happens
internally before any outbound connection.
ConnectToPeer now returns descriptive status strings so the
ConnectPeerResponse distinguishes "marked as persistent", "already
persistent", and "swap initiated" cases from the default "connection
initiated".
itests: persistent peer survives restart, persistent peer address swap,
persistent peer wait for dial, forget persistent peer with channel.
Fixes #10870.
Fixes #10871.