Strip PeerKeyLocation payloads from Subscribe messages

[sanity]

Problem

Subscribe messages still carry full PeerKeyLocation for target/subscriber, even though routing is now connection-based by socket address. Using PeerKeyLocation (with embedded addresses) for connected peers is redundant and risks stale/wrong addresses being propagated.

Why it matters

• We identify live peers by SocketAddr now; the transport source (upstream_addr) is authoritative for routing ReturnSub.
• Embedded addresses can be wrong (NAT loopback/private) and should not be trusted.
• Carrying a full location in payload forces us to thread addresses we don’t actually use.
• PR fix: allow parallel connection attempts (max 3 concurrent) #2174 surfaced a regression where trusting a Known-but-wrong embedded address breaks NAT peers; overriding from transport source fixed it, but removing embedded addresses would prevent the class entirely.

Proposal

• Keep subscriber identity (pubkey/PeerId), but drop/ignore embedded addresses in Subscribe payloads.
• Treat routing as fully connection-based: return path uses upstream_addr; forward path uses selected next-hop socket address.
• Adjust add_subscriber/ring/seeding code to accept identity + observed upstream address instead of a full PeerKeyLocation payload.
• Consider merging RequestSub/SeekNode (see Unify Subscribe RequestSub and SeekNode message variants #2182) so the message shape is simplified in one pass.

Scope

• Out of scope for PR stack 2167–2175; track as follow-up cleanup.

[github-actions]

🤖 Auto-labeled

Applied labels:

• T-enhancement (98% confidence)
• E-medium (80% confidence)
• A-networking (95% confidence)

Reasoning: This issue requests an optimization/cleanup to existing Subscribe message payloads (dropping embedded addresses) and adjusting routing to be connection-based. That is an enhancement (not a bug or entirely new feature). The change touches networking/routing code (A-networking) and requires protocol/message-shape and ring/seeding adjustments, so effort is moderate (E-medium). Because it affects protocol design and message semantics, it warrants a design discussion/consideration (S-needs-design).

Previous labels: none

If these labels are incorrect, please update them. This helps improve the auto-labeling system.

[iduartgomez]

@sanity is this relevant anymore?

[iduartgomez]

@claude has this been addressed and fixed already?

[claude]

Claude finished @iduartgomez's task in 2m 5s —— View job

---

Analysis Complete

Todo List

• Search for Subscribe message definitions
• Check PeerKeyLocation usage in subscription code
• Analyze current routing implementation
• Review recent changes that might have addressed this
• Provide status answer

---

Answer

This issue has been PARTIALLY addressed, but the full cleanup proposed here is NOT yet complete.

What has been done:

The recent work (particularly commits 188a484c and b9bb43fc) has made significant progress toward the goal of this issue:

1. Connection-based routing is now in place - The upstream_addr field tracks where messages came from (see subscribe.rs:362-363)

2. Addresses are handled dynamically - Subscribe messages now use PeerAddr::Unknown for subscribers who don't know their own external address (especially NAT peers), and the first recipient (gateway) fills in the observed address from the packet source:

   • See subscribe.rs:295-298, subscribe.rs:467-476, subscribe.rs:720-734

3. Seeding manager prefers transport addresses - When adding subscribers, the code now prioritizes the observed transport-level address over embedded addresses (see seeding.rs:104-109)

What is NOT done yet (what this issue asks for):

The message payloads still carry full PeerKeyLocation structures with addresses. The issue proposes:

• ✅ Use source_addr from transport for routing (DONE)
• ✅ Handle cases where embedded addresses are wrong/unknown (DONE via PeerAddr::Unknown)
• ❌ Actually strip/simplify the message payloads to only carry identity (pubkey) without address fields (NOT DONE)
• ❌ Change add_subscriber/ring/seeding APIs to accept identity + observed address separately (PARTIAL - they still accept full PeerKeyLocation but override with upstream_addr)

Current state:

The code currently has a hybrid approach:

• Messages still include PeerKeyLocation with target and subscriber fields containing addresses
• But these addresses can be PeerAddr::Unknown
• And the actual routing uses upstream_addr from the transport layer
• The seeding manager overrides embedded addresses with observed ones when available

This achieves the functional goal (correct routing for NAT peers) but not the architectural cleanup proposed in this issue.

Recommendation:

This issue is still valid and relevant as a follow-up refactoring task. The current code works correctly, but could be simplified by:

1. Removing address fields from Subscribe message payloads entirely (keep only identity)
2. Changing APIs to accept separate identity + observed address parameters
3. Potentially merging RequestSub/SeekNode as mentioned in Unify Subscribe RequestSub and SeekNode message variants #2182

The issue should remain open as a tracked enhancement for future cleanup.

---

[sanity]

Addressed by #2243