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)
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)
| 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.
There was a problem hiding this comment.
Now keying pruning on haveClearnetConfig. we made pruning eligibility derive from the resolved address set, which is a superset fix for the NAT concern.
ab9c445 to
24274ac
Compare
|
/gateway dismiss F5 per #10968 (comment) |
|
🚫 Dismissed F5 (minor) by @saubyk — per #10968 (comment) Open findings on this PR: 🟡 F6 (minor) · 🟡 F7 (minor) · 🟡 F8 (minor) |
|
/gateway re-review |
|
@Abdulkbk please note that we moved this pr to 0.22 so release notes would need to be updated |
There was a problem hiding this comment.
Gateway review — 3 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 3 Minor · 🔵 0 Nit
Summary
Every prior finding is now resolved. F6, F7, and F8 — the three that were unresolved at the last review — are all addressed: the redundant second DNS resolution is gone (host addresses are fed directly into the config set), pruneStale is now gated on haveClearnetConfig so pruning can never leave the node advertising zero clearnet addresses, and the onion/I2P-retention behavioral change for externalip nodes is documented in the release note with an operator remediation. F1–F4 remain addressed, and F5 was dismissed by the maintainer.
Three minor new observations surfaced, none blocking: the removal of the exported AdvertisedIPs field is a source-breaking change to the reusable netann package that warrants a changelog note; the pruneStale derivation in setSelfNode (the branch protecting against dropping live addresses) has no direct test coverage; and single-address host resolution can flap the advertised set across restarts for a host behind round-robin DNS.
One latent sharp edge is worth keeping in mind even though it is not a finding: the InitialAddrs invariants are enforced only by documentation, so a downstream netann consumer can still hold the field wrong and silently regress. F1's accepted fix was to document them, which was done; a structural fix (validate at startup, or seed lazily via LookupHost) remains a possible future hardening.
Status of prior findings
- F1 addressed: Fixed in
netann/host_ann.go:24— both invariants (keys byte-identical toHosts, values sharingLookupHost'sString()normalization) are now documented on theInitialAddrsfield. - F2 addressed: Fixed in
server.go— the all-or-nothing pruning limitation (one failed lookup suppresses pruning for every host, and a host that reliably fails at boot keeps accumulating) is now documented in thesetSelfNodecomment block, per the finding's minimum remediation. - F3 addressed: Fixed in
server.go—hostResolvedis now sorted withsort.Slicebefore being appended, so an unchanged host set yields a stable announcement across restarts. - F4 addressed: Fixed in
netann/host_ann.go— removal is now guarded bystillMapped: an old address is dropped only when no entry in the fully-updatedipMappingstill resolves to it, so a shared address a second host still points at is retained. - F6 addressed: Fixed in
server.go— the resolvedhostAddrsare now appended directly toaddrs(sorted) rather than round-tripped throughlncfg.NormalizeAddressesa second time, removing both the redundant lookup and theString()-coupling risk. - F7 addressed: Fixed in
server.go— pruning eligibility is nowpruneStale := haveClearnetConfig && allHostsResolved, wherehaveClearnetConfigrequires an actual*net.TCPAddrin the config-derived set. Pruning therefore cannot run without a surviving clearnet entry, closing the "prune every persisted clearnet address and advertise none" path (e.g. NAT mapping no ports). - F8 addressed: Fixed in
docs/release-notes/release-notes-0.21.2.md— the release note now states that anexternalipnode previously dropped persisted onion/I2P addresses at startup and now retains them, and tells an operator who removed a Tor/I2P service to clear a leftover withlncli peers updatenodeannouncement --address_remove=.
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)
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.
24274ac to
5e465df
Compare
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