multi: fix stale address accumulation when using externalhosts#10968
multi: fix stale address accumulation when using externalhosts#10968Abdulkbk wants to merge 4 commits into
Conversation
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟡 Medium (1 file)
🟢 Low (3 files)
AnalysisThe highest-severity file touched is Checked bump conditions (excluding test files
No bump conditions were triggered, so severity remains at the base CRITICAL level driven by the To override, add a |
757796b to
2aeff09
Compare
|
/gateway review |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 3 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 3 Minor · 🔵 0 Nit
Summary
This PR fixes a real bug: nodes using externalhosts accumulated dead clearnet addresses in their NodeAnnouncement across restarts. The approach — resolve hosts up front, seed the HostAnnouncer with InitialAddrs so an IP change is treated as a swap rather than an append, and prune stale persisted clearnet addresses only when the config is a trustworthy source — is sound, and the fail-safe (never prune unless every host resolved) is a deliberate, defensible choice. Test coverage is strong: reconcileNodeAddrs, the restart swap, and the flip-back case are all exercised.
No blocking issues. The concerns below are the deliberate all-or-nothing prune gate (which partially re-opens the accumulation window under repeated boot-time DNS failure), the contract on the newly exported InitialAddrs field, and nondeterministic address ordering from ranging a map.
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
04a6c22 to
57bebbf
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 2 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 2 Minor · 🔵 0 Nit
Summary
Two of the three prior findings are resolved: the InitialAddrs invariants are now documented (F1), and the resolved host addresses are sorted before being appended so an unchanged set yields a stable announcement (F3). One prior finding is still open (F2): pruning remains all-or-nothing across all configured hosts, so a single unresolvable host suppresses stale-address pruning for every host that did resolve. One genuinely-new minor issue: hostWatcher can drop a still-live address when two hosts share an IP and only one changes.
Nothing here blocks merge — all open items are minor.
Status of prior findings
- F1 addressed: Addressed in
netann/host_ann.go— theInitialAddrsfield now documents both invariants (keys byte-identical toHostsentries; values sharingLookupHost'sString()normalization) and recommends deriving keys, values, andHostsfrom a single source. - F3 addressed: Addressed in
server.go:5859—setSelfNodenow collects the resolved host addresses intohostAddrStringsand callssort.Stringsbefore appending, so an unchanged host set serializes to the same announcement across restarts regardless of map iteration order.
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
b4f9d78 to
29965d7
Compare
The externalhosts config option only ever took effect at runtime, via the HostAnnouncer. At startup it was ignored entirely: cfg.ExternalIPs is populated only from externalip, so a node configured with just externalhosts took the "no external IPs configured" branch in setSelfNode and restored every address it had persisted on its previous run. Those addresses include the IPs the configured hostname resolved to in past lifetimes, which are no longer reachable once the ISP hands the node a new address. We now resolve the externalhosts entries up front and treat them as a source of our clearnet addresses, the same way externalip already is, along with any NAT-discovered IPs. A persisted clearnet address that none of these sources back is stale and gets dropped, while addresses we cannot re-derive from the config, such as onion or I2P addresses, are always carried over. We only prune when every configured host resolved. A host we could not resolve leaves us unable to tell which of our persisted addresses are its stale results, so we keep all of them rather than let a transient DNS failure strip the only address we're reachable on.
The announcer tracks the address each host last resolved to so that an IP change replaces the old address rather than adding to it. That mapping lived only in memory and started out empty, so the first resolution after a restart looked like a brand new address and was appended to whatever we had restored from disk. A node whose IP changed while it was down would therefore keep the dead IP and gain the new one, accumulating another dead address on every such restart. The server now hands us the addresses its hosts resolved to at startup, which we seed the mapping with, so a change we notice after a restart is recognised as a change. This also lets us drop AdvertisedIPs. It was a snapshot of our addresses taken at startup that was never updated afterwards, and the check that used it ran before we recorded the address to remove: if a host's IP changed and then changed back within the same lifetime, we skipped the address as already advertised and never removed the intermediate one, leaving us advertising an address we were no longer reachable on and not the one we were. The seeded mapping subsumes what the check was for.
When two externalhosts entries resolve to the same IP and only one of them later changes, we recorded the shared address for removal keyed solely on the changed host's previous value. The unchanged host hit the "nothing changed" early continue and never re-added it, so IPAnnouncer dropped an address we were still reachable on until the next restart. Defer the removal decision until every host has been resolved, and only drop the addresses that no host maps to any more. This also means a host we failed to resolve keeps its last known address advertised, since its stale mapping still counts, matching the guard we apply to the persisted addresses at startup.
29965d7 to
ab9c445
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 4 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 4 Minor · 🔵 0 Nit
Summary
The three prior blockers to closure have all landed: the HostAnnouncer.InitialAddrs invariants are now documented (F1), host addresses are sorted before advertising so an unchanged set is stable across restarts (F3), the shared-IP over-removal in hostWatcher is fixed with a stillMapped guard and a regression test (F4), and the all-or-nothing pruning limitation (F2) is now spelled out in a thorough comment on pruneStale. The core reconciliation logic (reconcileNodeAddrs, resolveExternalHosts, the rewritten hostWatcher) is correct and well-tested.
Four new minor observations remain, all in server.go and all fail-safe: an upgrade-time behavior change where persisted onion/I2P addresses are now carried over even when externalip is set; clearnet pruning keyed on a single concrete Go type; the new NAT-driven pruning trigger; and a redundant double-resolution of the configured hosts. None block merge.
Status of prior findings
- F1 addressed: Fixed in netann/host_ann.go:24-40 — the
InitialAddrsfield now documents both invariants (keys byte-identical toHosts; values sharingLookupHost'sString()normalization) and the derive-from-a-single-source guidance. Confirmed by the author's reply on the thread. - F2 addressed: Addressed in server.go:~5874-5891 — the
pruneStale := configuredSource && allHostsResolvedblock now carries a full comment explaining that pruning is all-or-nothing, that a host absent fromInitialAddrsre-appends without removing its stale entry, and that a host reliably failing at boot keeps accumulating until a clean boot. The prior finding's minimum ask (note the residual limitation) is met; the gating itself (never prune when any host failed to resolve) is correct. - F3 addressed: Fixed in server.go:~5859 — resolved host addresses are collected into
hostAddrStringsandsort.Strings-ordered before being appended, so the nondeterministic map-iteration ordering no longer perturbs the signed announcement. Confirmed by the author's reply. - F4 addressed: Fixed in netann/host_ann.go:~145-160 —
hostWatchernow buildsstillMappedfrom the fully-updatedipMappingand only moves a stale candidate intoaddrsToRemovewhen no other host still resolves to it, so a shared address is no longer dropped when one of two hosts changes. Covered by the new "both hosts resolve to the same IP" case inTestHostAnnouncerUpdatesand byTestHostAnnouncerAddrFlipBack.
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| } | ||
|
|
||
| addrs := make([]net.Addr, 0, len(configAddrs)+len(persisted)) | ||
| addrs = append(addrs, configAddrs...) |
There was a problem hiding this comment.
🟡 F5 (Minor) — *Clearnet pruning keyed on the net.TCPAddr concrete type
reconcileNodeAddrs decides whether a persisted address is prunable clearnet solely via _, isClearnet := addr.(*net.TCPAddr). Any clearnet endpoint that reaches srcNode.Addresses under a different concrete net.Addr type is treated as non-clearnet and unconditionally retained, so it would never be pruned — partially defeating the stale-address cleanup for that address kind. This fails safe (over-retention, never reachability loss), and for the address types this build persists today (*net.TCPAddr for IPv4/IPv6, *tor.OnionAddr for onion) the predicate is correct; onion/I2P retention is intended. I cannot confirm from the diff whether graph-db deserialization can yield a clearnet address as a non-*net.TCPAddr type — if it can, the classification should key off address family rather than a single Go type.
| hostAddrStrings := make([]string, 0, len(hostAddrs)) | ||
| for _, addr := range hostAddrs { | ||
| hostAddrStrings = append(hostAddrStrings, addr.String()) | ||
| } |
There was a problem hiding this comment.
🟡 F6 (Minor) — Configured hosts are resolved twice via a String() round-trip
Each externalhosts entry is resolved once in resolveExternalHosts (to build hostAddrs/InitialAddrs), then its .String() is pushed back through lncfg.NormalizeAddresses, which resolves it a second time to produce the configAddrs that feed reconcileNodeAddrs. Beyond the redundant lookup, this couples correctness to the two paths producing byte-identical String() output: if they ever diverge, the config-derived entry won't match the persisted one in the configured set and a still-backed address could be pruned or duplicated. Since hostAddrs already holds resolved net.Addr values, feeding them directly into the config set would remove both the second resolution and the coupling.
| // from the HostAnnouncer's InitialAddrs, so when it resolves later its | ||
| // address is appended without the stale one being removed, and a host | ||
| // that reliably fails at boot keeps accumulating until a boot where | ||
| // every host resolves. Pruning per host would need a persisted |
There was a problem hiding this comment.
🟡 F7 (Minor) — NAT-only nodes can now over-prune persisted clearnet addresses
configuredSource is set true by NAT discovery as well as by externalip/externalhosts, so pruneStale now fires for a node relying solely on NAT traversal — a configuration that previously never pruned (the old gate was len(cfg.ExternalIPs) != 0). If a NAT run reports a different external IP, the previously-advertised NAT address is pruned and re-added on recovery, churning the signed announcement; and if a run sets configuredSource=true while yielding no clearnet configAddrs (e.g. NAT discovery returns no IP, or externalhosts resolve only to onion), every persisted clearnet address is pruned and the node advertises none. Verify that the NAT block sets configuredSource only when it actually produced an address, so pruning cannot run without a surviving clearnet entry.
| // Merge the addresses backed by our config with the ones we | ||
| // persisted last run, dropping any persisted clearnet address | ||
| // the config no longer backs. | ||
| addrs = reconcileNodeAddrs( |
There was a problem hiding this comment.
🟡 F8 (Minor) — Persisted onion/I2P re-advertised even when externalip is set
The source-node branch previously discarded persisted addresses entirely whenever externalip was configured (if len(cfg.ExternalIPs) == 0 { addrs = withoutV2Onion(srcNode.Addresses) }); it now always calls reconcileNodeAddrs(addrs, srcNode.Addresses, pruneStale), which carries over every persisted onion/I2P address regardless of pruneStale. On upgrade, an operator who set externalip and had a Tor address persisted from an earlier config — including one they intentionally removed — will begin re-advertising that onion address again. This is consistent with the PR's "onion and I2P are always carried over" intent, but it is a behavioral change for existing externalip nodes worth calling out in the release note; confirm re-advertising a config-removed onion is acceptable.
2 participants
Change Description
Fixes #10952.
externalhostsentries are now resolved during startup and, together withexternalipand any NAT-discovered IPs, determine which clearnet addresses we advertise. A persisted clearnet address that none of these sources back is dropped, onion and I2P addresses are always carried over. Nothing is pruned unless every configured host is resolved, so a transient DNS failure can't strip the only address we're reachable on.Steps to Test
Steps for reviewers to follow to test the change.
Pull Request Checklist
Testing