addressservice: extract out addressing logic from basichost #3075

sukunrt · 2024-12-01T13:37:25Z

The goal is to keep modifying it and removing the dependency on basichost by relying on events. At that point, we can expose this as a separate service usable by both basic and blank hosts

Part of #2229

The goal is to keep modifying it and removing the dependency on basichost by relying on events. At that point, we can expose this as a separate service usable by both basic and blank hosts

closes: #3057 rest of the work is in: #3075 Co-authored-by: wlynxg <[email protected]>

Ideally, we should rewrite this to have the same semantics, opt into transports not opt out, as `libp2p.New`. But I need webtransport and webrtc support to write address inference tests for #3075 Depending on how disruptive this is to users, we can decide on whether to merge or drop this.

This adds support for `/webtransport` andn `/webrtc-direct` to GenSwarm. Ideally, we should rewrite this to have the same semantics, opt into transports not opt out, as `libp2p.New`. But I need webtransport and webrtc support to write address inference tests for #3075 Depending on how disruptive this is to users, we can decide on whether to merge or drop this.

MarcoPolo

I know the worst bits of this code are just copied over from the previous implementation. So apologies for picking too much into those.

This is a good point to remove some of the idiosyncrasies here.

MarcoPolo · 2025-01-30T22:25:26Z

p2p/host/basic/address_service.go

+var p2pCircuitAddr = ma.StringCast("/p2p-circuit")
+
+// AllAddrs returns all the addresses the host is listening on except circuit addresses.
+func (a *addressService) AllAddrs() []ma.Multiaddr {


Maybe this should be called AllExceptCirucitAddrs

Related. We should spend a bit of time thinking if these are the names we want
for these methods now while we're refactoring.

MarcoPolo · 2025-01-30T22:29:04Z

p2p/host/basic/address_service.go

+	return finalAddrs
+}
+
+func (a *addressService) appendInterfaceAddrs(result []ma.Multiaddr, listenAddrs []ma.Multiaddr) []ma.Multiaddr {


nit, slight preference for s/result/dst

MarcoPolo · 2025-01-30T22:31:45Z

p2p/host/basic/address_service.go

+}
+
+// getAllPossibleLocalAddrs gives all the possible address returned for `conn.LocalAddr` correspoinding
+// to the `listenAddr`


I'm having a hard time understanding this comment.

MarcoPolo · 2025-01-30T22:35:06Z

p2p/host/basic/basic_host.go


-		ar, err := autorelay.NewAutoRelay(h, opts.AutoRelayOpts...)
+	if !h.disableSignedPeerRecord {


Why did you move this?

MarcoPolo · 2025-01-30T22:36:37Z

p2p/host/basic/basic_host.go

@@ -872,15 +734,7 @@ func (h *BasicHost) ConnManager() connmgr.ConnManager {
 // When used with AutoRelay, and if the host is not publicly reachable,
 // this will only have host's private, relay, and no public addresses.


Is this comment still accurate?

MarcoPolo · 2025-01-30T22:56:16Z

p2p/host/basic/address_service.go

+		select {
+		case <-ticker.C:
+		case <-a.addrsChangeChan:
+		case <-a.autoRelayAddrsSub.Out():


We don't want to use the event here?

MarcoPolo · 2025-01-30T22:59:54Z

p2p/host/basic/address_service.go

+	}
+	// Make a copy. Consumers can modify the slice elements
+	addrs = slices.Clone(a.addrsFactory(addrs))
+	// Add certhashes for the addresses provided by the user via address factory.


This comment is wrong. The address factory is unused here.

MarcoPolo · 2025-01-30T23:14:07Z

p2p/host/basic/address_service.go

+	ifaceAddrs := a.ifaceAddrs.All()
+	if a.natmgr == nil || !a.natmgr.HasDiscoveredNAT() {
+		if a.observedAddrsService != nil {
+			result = append(result, a.observedAddrsService.OwnObservedAddrs()...)


Would it be better to use OwnObservedAddrFor per listen Addr? Seems surprising
that if we haven't discovered our nat we return all our observed addrs. Example:
It would be surprising if I called this method with just QUIC and got
back a tcp multiaddr.

MarcoPolo · 2025-01-30T23:18:02Z

p2p/host/basic/address_service.go

+			if ip == nil || !manet.IsPublicAddr(ip) {
+				continue
+			}
+			result = append(result, ma.Join(ip, extMaddrNoIP))


So we expect to have a valid external port mapping from the nat service, but be given a private IP Address, and we expect the that second NAT somehow preserves that port mapping? Does this actually work?

MarcoPolo · 2025-01-30T23:23:11Z

p2p/host/basic/address_service.go

+	// Use both for hole punching.
+	addrs = append(addrs, a.observedAddrsService.OwnObservedAddrs()...)
+	addrs = ma.Unique(addrs)
+	return slices.DeleteFunc(addrs, func(a ma.Multiaddr) bool { return !manet.IsPublicAddr(a) })


Is there any sorting that we want to do here?

sukunrt force-pushed the sukun/address-service-1 branch from d54166d to 3dbd24d Compare December 1, 2024 13:38

sukunrt mentioned this pull request Dec 1, 2024

introduce a proper address pipeline #2229

Open

1 task

sukunrt force-pushed the sukun/address-service-1 branch 5 times, most recently from 5896f14 to a9e6585 Compare December 2, 2024 14:29

sukunrt force-pushed the sukun/autorelay-event branch from 2b114a2 to 762a43e Compare December 3, 2024 08:39

sukunrt added 2 commits December 3, 2024 14:15

addressservice: extract out addressing logic from basichost

83991ff

The goal is to keep modifying it and removing the dependency on basichost by relying on events. At that point, we can expose this as a separate service usable by both basic and blank hosts

cleanup interface address handling

e8dba37

sukunrt force-pushed the sukun/address-service-1 branch from a9e6585 to e3367f1 Compare December 3, 2024 08:45

sukunrt added 2 commits December 3, 2024 14:37

cleanup nat addr handling; add tests

12e9110

make everything mockable

fd76100

sukunrt force-pushed the sukun/address-service-1 branch 4 times, most recently from d77303e to e45fe9e Compare December 3, 2024 16:59

sukunrt requested a review from MarcoPolo December 3, 2024 17:03

sukunrt added a commit that referenced this pull request Dec 8, 2024

nat: ignore mapping if external port is 0

2861e3d

closes: #3057 rest of the work is in: #3075 Co-authored-by: wlynxg <[email protected]>

sukunrt added a commit that referenced this pull request Dec 8, 2024

nat: ignore mapping if external port is 0

df48dde

closes: #3057 rest of the work is in: #3075 Co-authored-by: wlynxg <[email protected]>

This was referenced Dec 8, 2024

nat: ignore mapping if external port is 0 #3094

Merged

fix: don't use invalid port 0 when NATMap fails #3058

Closed

p-shahi mentioned this pull request Dec 16, 2024

v0.38 #3005

Closed

26 tasks

p-shahi mentioned this pull request Dec 26, 2024

v0.39 #3082

Open

21 tasks

add tests for present behavior

5fcce02

sukunrt force-pushed the sukun/address-service-1 branch from e45fe9e to 5fcce02 Compare December 30, 2024 15:49

sukunrt mentioned this pull request Dec 30, 2024

swarm_test: support more transports for GenSwarm #3130

Draft

MarcoPolo mentioned this pull request Jan 29, 2025

v0.40 #3164

Open

21 tasks

MarcoPolo requested changes Jan 30, 2025

View reviewed changes

typos

c0a894d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

addressservice: extract out addressing logic from basichost #3075

addressservice: extract out addressing logic from basichost #3075

sukunrt commented Dec 1, 2024 •

edited

Loading

MarcoPolo left a comment

MarcoPolo Jan 30, 2025

MarcoPolo Jan 30, 2025

MarcoPolo Jan 30, 2025

MarcoPolo Jan 30, 2025

MarcoPolo Jan 30, 2025

MarcoPolo Jan 30, 2025

MarcoPolo Jan 30, 2025

MarcoPolo Jan 30, 2025

MarcoPolo Jan 30, 2025

MarcoPolo Jan 30, 2025

MarcoPolo Jan 30, 2025


		ar, err := autorelay.NewAutoRelay(h, opts.AutoRelayOpts...)
		if !h.disableSignedPeerRecord {

		@@ -872,15 +734,7 @@ func (h *BasicHost) ConnManager() connmgr.ConnManager {
		// When used with AutoRelay, and if the host is not publicly reachable,
		// this will only have host's private, relay, and no public addresses.

addressservice: extract out addressing logic from basichost #3075

Are you sure you want to change the base?

addressservice: extract out addressing logic from basichost #3075

Conversation

sukunrt commented Dec 1, 2024 • edited Loading

MarcoPolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukunrt commented Dec 1, 2024 •

edited

Loading