#35578 net: don’t self advertise tor exit node ip addresses in outbound connections
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35578 net: don’t self advertise tor exit node ip addresses in outbound connections | Ready | 0 | Strong | 21 + 29 tests | |||
|
|
|
| ||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Suppresses self-advertisement of peer-reported addresses on outbound connections made through a proxy. In `GetLocalAddrForPeer`, the node skips using the address observed by an outbound peer if a proxy is configured for that network, preventing proxy exit IPs from being gossiped. Operators with reachable proxies can still advertise them using `-externalip`.
Problem: Nodes making outbound clearnet connections through a SOCKS5 proxy (such as Tor) receive the proxy exit node's IP address in the peer's view of them. If address discovery remains enabled, the node self-advertises this proxy exit IP, polluting peers' address managers with unreachable addresses.
Evidence of importance:
- The author reports that '55-76% of all tor exit nodes were present in various addrman snapshots I checked (my node and the ones from b10c’s peer.observer) from now and few years ago' (PR description).
Risk of a mistake: low (p2p, tests, docs). A bug in local address selection could cause a node to fail to self-advertise its reachable address or advertise an incorrect address to outbound peers.
Changed since the previous assessment: The earlier documentation update for Tor configuration parameters is no longer present; the diff now only contains the net logic change, `ResetProxy()` test helper, unit tests, and release notes.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: review of the latest push which added unit test, comments, and release notes 2026-09-15
- reviewer: review of the updated branch which added tests, comments, and release notes 2026-09-15
Agreement
Strong
substantive support, no open objection (0xB10C, naiyoma)
Thread notes: Brought claims up to date across all comments and reviews under current schema. Added questions from 0xB10C and mzumsande, and suggestions from naiyoma (tests and code comments addressed in push). / Reread the discussion under the current rules: added questions from 0xB10C and mzumsande, suggestions from naiyoma (code structure, test, inline comment, and out-of-scope check), and updated statuses based on author responses and pushes.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| 0xB10C (member) | question, approach | Are we sure this is being used like this, since with just -proxy set we would not be listening or self-announcing? | answered · author explained users often override listen=1 and discover=1, or use onion=127.0.0.1:9050 | answered | yes | 2026-06-26: 'Hm are we sure this is being used like this? With just `-proxy` set, we wouldn't be listening and not self-announcing.' Status from: 2026-06-30: stratospher: 'good point. it's either this or people using `onion=127.0.0.1:9050`.' |
| naiyoma (member) | suggestion, scope | add a check so that an outbound peer does not send us their own address as our address | open | answered | no | 2026-07-10: 'maybe we should also add check so that a peer does not send us their own address.' |
| naiyoma (member) | suggestion, style | Add a comment explaining why there is a proxy check in GetLocalAddrForPeer | fixed · fix pushed · added comment in latest push | changed | yes | 2026-07-10: 'Maybe add a comment? to explain a bit on why there's a proxy check here' Status from: 2026-09-14: stratospher: 'done both in the latest push. thanks!' |
| naiyoma (member) | suggestion, approach | Check whether an outbound connection is proxied before IsPeerAddrLocalGood to avoid unnecessary checks | open · author replied noting impact on tor exits running nodes and added release note suggestion | changed | no | 2026-07-16: 'An alternative approach would be to check whether an outbound connection is proxied before we even check if `IsPeerAddrLocalGood()`' |
| naiyoma (member) | suggestion, correctness | Add a unit test in net_tests.cpp using SetProxy() to verify peer's reported address is not relayed | fixed · fix pushed · test added in push | changed | no | 2026-07-16: 'I think its possible to test this in `net_tests.cpp` similar to `get_local_addr_for_peer_port` but using `SetProxy()`' Status from: 2026-09-14: stratospher: 'done both in the latest push. thanks!' |
| mzumsande (member) | question, approach | Could transparent torification (e.g. Whonix) where defaults are not changed account for exit nodes being relayed, making the fix incomplete? | answered · author agreed that transparent torification won't be helped, but the patch still helps when users override defaults | answered | yes | 2026-07-27: 'Maybe it could also due to transparent torification where users do not change any defaults but the traffic is rerouted via tor externally, e.g. by an OS such as Whonix? In that case, the fix might not solve the problem completely.' Status from: 2026-09-11: stratospher: 'good point! I don't think we have a way to know unless we see it in action in the P2P network and this patch obviously won't work for systems running transparent torrification. but we can try to reduce it from core's end when people override defaults.' |
Support:
- 0xB10C (member) (Concept ACK, 2026-06-26): it makes sense to me to not advertise an address we know we can't be reached on
- naiyoma (member) (Concept ACK, 2026-07-10) [tested]: It makes sense not to advertise these addresses, as they are not useful
- mzumsande (member) (Concept ACK, 2026-07-27): (no reason given) [not substantive]
Participants:
- 0xB10C (member, support, 2 statements 2026-06-26 to 2026-06-26): Concept ACK; questioned whether default configs without overriding listen/discover actually self-advertise
- naiyoma (member, support, 4 statements 2026-07-10 to 2026-07-16): Concept ACK, tested; suggested early check for proxy, code comment, and unit test
- mzumsande (member, support, 1 statements 2026-07-27 to 2026-07-27): Concept ACK; asked whether transparent torification accounts for exit node propagation
- sedited (member, neutral, 1 statements 2026-09-08 to 2026-09-08): asked for PR status regarding unaddressed comments
Files
29 lines under test/bench/ci.
- src/test/net_tests.cpp +29/-0
- doc/release-notes-35578.md +10/-0
- src/netbase.cpp +7/-0
- src/net.cpp +2/-1
- src/netbase.h +2/-0
Card
The short text the ranking pass compares across PRs.
PR 35578 stops nodes from self-advertising peer-observed IP addresses on outbound connections routed through a proxy like Tor. Currently, outbound peers see the proxy or Tor exit node IP and report it back, causing nodes to gossip unreachable proxy IPs across the P2P network. Addrman snapshots cited by the author showed 55-76% of all Tor exit nodes present in address tables due to this behavior. Operators running reachable proxies can still explicitly advertise them using -externalip.
Data
thread read through 2026-10-20 (14 statements), head 38f2716520
labels: P2P
Categories
P2P
P3 · bug fix (ranking pass: Broadest effect of the P3 address-handling fixes (network-wide addrman pollution).)
Prevents nodes from gossiping unreachable proxy exit IPs and polluting peers' address managers. The author notes that 55-76% of Tor exit nodes were present in observed addrman snapshots due to this self-advertisement behavior, making this a clear network hygiene improvement that is reasonably deferrable.
Membership: Has the P2P label and modifies address advertisement in src/net.cpp and src/netbase.cpp.
Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 0