Skip to content

multi: write connect --perm address into the peer's LinkNode record#10974

Closed
ZZiigguurraatt wants to merge 2 commits into
lightningnetwork:masterfrom
ZZiigguurraatt:remember_peer_addresses
Closed

multi: write connect --perm address into the peer's LinkNode record#10974
ZZiigguurraatt wants to merge 2 commits into
lightningnetwork:masterfrom
ZZiigguurraatt:remember_peer_addresses

Conversation

@ZZiigguurraatt

Copy link
Copy Markdown
Contributor

lncli connect --perm marks a peer as persistent in memory, but before this commit the requested address was not written to any on-disk store. On restart lnd had no record that the user had ever asked for the peer, so the persistent-reconnect intent was lost.

This commit routes --perm through the existing LinkNode store via a new channeldb.LinkNodeDB.AddAddressForPeer helper. The helper creates a fresh LinkNode if none exists for the peer, or appends the address to an existing one (deduped by string). The Go-side wiring is minimal — the existing establishPersistentConnections startup path already reads LinkNodes and reconnects to each entry, so no new startup logic is required.

ConnectToPeer(perm=true) now:

  • writes the address to the LinkNode store, and
  • for peers we are already connected to, marks them as user-persistent in the in-memory set without erroring or opening a duplicate connection (previously --perm against an already-connected peer always returned errPeerAlreadyConnected). It still errors when the call has nothing to do — i.e. the peer is already connected and the address is already stored in the LinkNode entry — matching the plain-connect behaviour of erroring on a no-op request.

The remembered_addresses field on the Peer proto message now notes --perm alongside "first channel open" as a source.

Scope / known limitation

This is a partial fix for #10870. PruneLinkNodes still runs at startup and deletes LinkNode entries for peers with no open channels, so a --perm on a peer we have never opened a channel with is still lost across restart. The dominant use case — remembering an address for a peer we do have a channel with (e.g. because the peer stopped announcing that address) — is now covered.

Lifting the non-channel-peer limitation may be addressed as a follow-up once the migration to native SQL is complete.

The current semantic is add-only: repeated --perm calls with different addresses accumulate in the LinkNode, and there is no supported way to remove a stored address without disconnecting the peer. A separate follow-up commit will add a way to remove individual stored addresses manually.

itests:

  • perm adds remembered address opens a channel between two nodes, then --perms a second (unreachable) address for the peer and asserts that both the original and the new address appear in the peer's remembered_addresses list.
  • perm non-channel peer lost on restart pins down the limitation above: two nodes, no channel, --perm connect, restart, assert the peer no longer appears in the persistent reconnect set (queried via include_offline_persistent_peers=true). If PruneLinkNodes is ever taught to spare user-persistent entries, this test should be flipped.

Depends on #10973 (ListPeers extension) for the remembered_addresses and include_offline_persistent_peers surfaces used by the itests.

Contributes to #10870.
Contributes to #10871.

ListPeers previously returned only currently-connected peers with a single
`address` field, leaving no way to see:

  1. Why lnd is maintaining a connection to a given peer.
  2. What addresses lnd has stored for reconnecting to the peer.
  3. What the peer currently advertises in its NodeAnnouncement.
  4. Which peers lnd is auto-reconnecting to but is not currently connected
     to.
  5. Whether a reconnect attempt is in flight.

This commit fills those gaps by adding to the `Peer` message:

  * `is_persistent` — true if lnd is auto-reconnecting to the peer.
  * `remembered_addresses` — addresses lnd has stored for the peer (from
    the LinkNode record captured at first channel open).
  * `gossip_addresses` — addresses from the peer's current
    NodeAnnouncement, populated inline instead of requiring a separate
    GetNodeInfo call per peer.
  * `reconnect_pending` — true if lnd currently has an in-flight reconnect
    attempt for the peer.

`ListPeersRequest` gains `include_offline_persistent_peers`. When set, the
response also includes peers that are in lnd's auto-reconnect set but not
currently connected. Per-connection fields (address, bytes_sent, ping_time,
etc.) are empty/zero for those entries.

`lncli listpeers` gains an `--include_offline_persistent_peers` flag.

itest: `listpeers address fields` opens a channel between two nodes,
suspends the peer, and asserts the new field state in both the connected
and offline-persistent cases.

Partial fix for lightningnetwork#10871
`lncli connect --perm` marks a peer as persistent in memory, but before
this commit the requested address was not written to any on-disk store.
On restart lnd had no record that the user had ever asked for the peer,
so the persistent-reconnect intent was lost.

This commit routes `--perm` through the existing LinkNode store via a
new `channeldb.LinkNodeDB.AddAddressForPeer` helper. The helper creates
a fresh LinkNode if none exists for the peer, or appends the address to
an existing one (deduped by string). The Go-side wiring is minimal — the
existing `establishPersistentConnections` startup path already reads
LinkNodes and reconnects to each entry, so no new startup logic is
required.

`ConnectToPeer(perm=true)` now:

  * writes the address to the LinkNode store, and
  * for peers we are already connected to, marks them as user-persistent
    in the in-memory set without erroring or opening a duplicate
    connection (previously `--perm` against an already-connected peer
    always returned `errPeerAlreadyConnected`). It still errors when the
    call has nothing to do — i.e. the peer is already connected *and*
    the address is already stored in the LinkNode entry — matching the
    plain-`connect` behaviour of erroring on a no-op request.

The `remembered_addresses` field on the `Peer` proto message now notes
`--perm` alongside "first channel open" as a source.

Scope / known limitation
------------------------

This is a partial fix for lightningnetwork#10870. `PruneLinkNodes` still runs at startup
and deletes LinkNode entries for peers with no open channels, so a
`--perm` on a peer we have never opened a channel with is still lost
across restart. The dominant use case — remembering an address for a
peer we do have a channel with (e.g. because the peer stopped
announcing that address) — is now covered.

Lifting the non-channel-peer limitation may be addressed as a follow-up
once the migration to native SQL is complete.

The current semantic is add-only: repeated `--perm` calls with different
addresses accumulate in the LinkNode, and there is no supported way to
remove a stored address without disconnecting the peer. A separate
follow-up commit will add a way to remove individual stored addresses
manually.

itests:

  * `perm adds remembered address` opens a channel between two nodes,
    then `--perm`s a second (unreachable) address for the peer and
    asserts that both the original and the new address appear in the
    peer's `remembered_addresses` list.
  * `perm non-channel peer lost on restart` pins down the limitation
    above: two nodes, no channel, `--perm` connect, restart, assert the
    peer no longer appears in the persistent reconnect set (queried via
    `include_offline_persistent_peers=true`). If PruneLinkNodes is ever
    taught to spare user-persistent entries, this test should be
    flipped.

Depends on lightningnetwork#10973 (ListPeers extension) for the `remembered_addresses`
and `include_offline_persistent_peers` surfaces used by the itests.

Contributes to lightningnetwork#10870.
Contributes to lightningnetwork#10871.
@github-actions github-actions Bot added the severity-critical Requires expert review - security/consensus critical label Jul 15, 2026
@github-actions

Copy link
Copy Markdown

🔴 PR Severity: CRITICAL

gh pr view | 11 files | 718 lines changed

🔴 Critical (3 files)
  • channeldb/nodes.go - LinkNode store change (channel state / peer address persistence); channeldb/* is core channel-state persistence
  • rpcserver.go - explicitly listed critical file; modifies ConnectToPeer RPC handling for --perm
  • server.go - explicitly listed critical file; changes persistent-connection/reconnect bookkeeping
🟠 High (2 files)
  • lnrpc/lightning.pb.go - generated code under lnrpc/* (RPC/API definitions); regenerated from the .proto change
  • lnrpc/lightning.swagger.json - generated swagger spec under lnrpc/*, not on the auto-generated exclusion list so still counted here
🟡 Medium (2 files)
  • cmd/commands/commands.go - CLI client code (cmd/*), classified by path regardless of the server-side feature it exposes
  • lnrpc/lightning.proto - explicit *.proto API-definition rule takes precedence over the general lnrpc/* High bucket
🟢 Low (4 files)
  • docs/release-notes/release-notes-0.22.0.md - release notes
  • itest/list_on_test.go - test-only
  • itest/lnd_network_test.go - test-only
  • lntest/rpc/lnd.go - test harness code (lntest/*)

Analysis

The highest-severity files are channeldb/nodes.go, rpcserver.go, and server.go — all explicitly CRITICAL. channeldb/nodes.go adds a new LinkNodeDB.AddAddressForPeer helper that mutates the on-disk LinkNode record used to drive persistent-reconnect behavior at startup; server.go changes how --perm interacts with already-connected peers and the in-memory persistent-peer set; rpcserver.go wires the new behavior into the ConnectToPeer RPC. These are core connection-management / channel-state-persistence paths, so expert review is warranted (e.g. verifying the dedup logic on addresses, the interaction with PruneLinkNodes at startup per the PR's own stated limitation, and correctness of the "already connected + already persisted" no-op error path).

Excluding tests and generated files, this PR touches 7 files and ~378 lines — under the >20 files / >500 lines bump thresholds. It does touch more than one critical area (channeldb persistence and core server/rpcserver connection logic), which would normally justify a one-level bump, but severity is already at the maximum tier (CRITICAL), so no further bump applies.


To override, add a severity-override-{critical,high,medium,low} label.

@ZZiigguurraatt

Copy link
Copy Markdown
Contributor Author

closing in favor of #10975

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

severity-critical Requires expert review - security/consensus critical

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant