Repository: libp2p/go-libp2p
Pull request #949 (2020-05-20 18:53:23)

fix: group observations by zeroing port
In #917, we started dropping additional address observations if we had multiple for the same transport set. However, on further consideration, this isn't quite correct. We _want_ to keep additional observations for multiple IP addresses. The real issue is many observations for different ports.

So this patch simply changes the key with which we group observations from "address protocols" to "address without the port" (well, with the port set to 0).


Patch:
diff --git a/p2p/protocol/identify/obsaddr.go b/p2p/protocol/identify/obsaddr.go
index 5d339a80..1fd8eb71 100644
--- a/p2p/protocol/identify/obsaddr.go
+++ b/p2p/protocol/identify/obsaddr.go
@@ -70,15 +70,24 @@ func (oa *ObservedAddr) numInbound() int {
 	return count
 }
 
+// GroupKey returns the group in which this observation belongs. Currently, an
+// observed address's group is just the address with all ports set to 0. This
+// means we can advertise the most commonly observed external ports without
+// advertising _every_ observed port.
 func (oa *ObservedAddr) GroupKey() string {
-	key := ""
-	protos := oa.Addr.Protocols()
-
-	for i := range protos {
-		key = key + "/" + protos[i].Name
-	}
+	key := make([]byte, 0, len(oa.Addr.Bytes()))
+	ma.ForEach(oa.Addr, func(c ma.Component) bool {
+		switch proto := c.Protocol(); proto.Code {
+		case ma.P_TCP, ma.P_UDP:
+			key = append(key, proto.VCode...)
+			key = append(key, 0, 0) // zero in two bytes
+		default:
+			key = append(key, c.Bytes()...)
+		}
+		return true
+	})
 
-	return key
+	return string(key)
 }
 
 type newObservation struct {


Test patch:
diff --git a/p2p/protocol/identify/obsaddr_test.go b/p2p/protocol/identify/obsaddr_test.go
index 2beab1a6..a0f50cf9 100644
--- a/p2p/protocol/identify/obsaddr_test.go
+++ b/p2p/protocol/identify/obsaddr_test.go
@@ -314,17 +314,28 @@ func TestObservedAddrFiltering(t *testing.T) {
 }
 
 func TestObservedAddrGroupKey(t *testing.T) {
-	oa1 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/tcp/1231")}
-	require.Equal(t, "/ip4/tcp", oa1.GroupKey())
-
-	oa2 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.5/tcp/2222")}
-	require.Equal(t, "/ip4/tcp", oa2.GroupKey())
-
-	oa3 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/udp/1231")}
-	require.Equal(t, "/ip4/udp", oa3.GroupKey())
-	oa4 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.3.3.4/udp/1531")}
-	require.Equal(t, "/ip4/udp", oa4.GroupKey())
-
-	oa5 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.3.3.4/udp/1531/quic")}
-	require.Equal(t, "/ip4/udp/quic", oa5.GroupKey())
+	oa1 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/tcp/2345")}
+	oa2 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/tcp/1231")}
+	oa3 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.5/tcp/1231")}
+	oa4 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/udp/1231")}
+	oa5 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/udp/1531")}
+	oa6 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/udp/1531/quic")}
+	oa7 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/udp/1111/quic")}
+	oa8 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.5/udp/1111/quic")}
+
+	// different ports, same IP => same key
+	require.Equal(t, oa1.GroupKey(), oa2.GroupKey())
+	// different IPs => different key
+	require.NotEqual(t, oa2.GroupKey(), oa3.GroupKey())
+	// same port, different protos => different keys
+	require.NotEqual(t, oa3.GroupKey(), oa4.GroupKey())
+	// same port, same address, different protos => different keys
+	require.NotEqual(t, oa2.GroupKey(), oa4.GroupKey())
+	// udp works as well
+	require.Equal(t, oa4.GroupKey(), oa5.GroupKey())
+	// udp and quic are different
+	require.NotEqual(t, oa5.GroupKey(), oa6.GroupKey())
+	// quic works as well
+	require.Equal(t, oa6.GroupKey(), oa7.GroupKey())
+	require.NotEqual(t, oa7.GroupKey(), oa8.GroupKey())
 }