Improve ObservedAddrSet behaviour

p2p/protocol/identify/obsaddr.go

@@ -8,15 +8,32 @@ import (
 	ma "github.com/multiformats/go-multiaddr"
 )
 
+const ActivationThresh = 4
+
 // ObservedAddr is an entry for an address reported by our peers.
 // We only use addresses that:
-// - have been observed more than once. (counter symmetric nats)
-// - have been observed recently (10min), because our position in the
+// - have been observed at least 4 times in last 1h. (counter symmetric nats)
+// - have been observed at least once recently (1h), because our position in the
 //   network, or network port mapppings, may have changed.
 type ObservedAddr struct {
-	Addr     ma.Multiaddr
-	SeenBy   map[string]struct{}
-	LastSeen time.Time
+	Addr      ma.Multiaddr
+	SeenBy    map[string]time.Time
+	LastSeen  time.Time
+	Activated bool
+}
+
+func (oa *ObservedAddr) TryActivate(ttl time.Duration) bool {
+	// cleanup SeenBy set
+	now := time.Now()
+	for k, t := range oa.SeenBy {
+		if now.Sub(t) > ttl*ActivationThresh {
+			delete(oa.SeenBy, k)
+		}
+	}
+
+	// We only activate if in the TTL other peers observed the same address
+	// of ours at least 4 times.
+	return len(oa.SeenBy) >= ActivationThresh
 }
 
 // ObservedAddrSet keeps track of a set of ObservedAddrs
@@ -46,16 +63,7 @@ func (oas *ObservedAddrSet) Addrs() []ma.Multiaddr {
 			continue
 		}
 
-		// we only use an address if we've seen it more than once
-		// because symmetric nats may cause all our peers to see
-		// different port numbers and thus report always different
-		// addresses (different ports) for us. These wouldn't be
-		// very useful. We make the assumption that if we've
-		// connected to two different peers, and they both have
-		// reported seeing the same address, it is probably useful.
-		//
-		// Note: make sure not to double count observers.
-		if len(a.SeenBy) > 1 {
+		if a.Activated || a.TryActivate(oas.ttl) {
 			addrs = append(addrs, a.Addr)
 		}
 	}
@@ -79,13 +87,13 @@ func (oas *ObservedAddrSet) Add(addr ma.Multiaddr, observer ma.Multiaddr) {
 	if !found {
 		oa = &ObservedAddr{
 			Addr:   addr,
-			SeenBy: make(map[string]struct{}),
+			SeenBy: make(map[string]time.Time),
 		}
 		oas.addrs[s] = oa
 	}
 
 	// mark the observer
-	oa.SeenBy[observerGroup(observer)] = struct{}{}
+	oa.SeenBy[observerGroup(observer)] = time.Now()
 	oa.LastSeen = time.Now()
 }
 
@@ -100,6 +108,7 @@ func (oas *ObservedAddrSet) Add(addr ma.Multiaddr, observer ma.Multiaddr) {
 // Here, we use the root multiaddr address. This is mostly
 // IP addresses. In practice, this is what we want.
 func observerGroup(m ma.Multiaddr) string {
+	//TODO: If IPv6 rolls out we should mark /64 routing zones as one group
 	return ma.Split(m)[0].String()
 }
 

p2p/protocol/identify/obsaddr_test.go

@@ -18,6 +18,10 @@ func TestObsAddrSet(t *testing.T) {
 	}
 
 	addrsMarch := func(a, b []ma.Multiaddr) bool {
+		if len(a) != len(b) {
+			return false
+		}
+
 		for _, aa := range a {
 			found := false
 			for _, bb := range b {
@@ -38,8 +42,12 @@ func TestObsAddrSet(t *testing.T) {
 	a3 := m("/ip4/1.2.3.4/tcp/1233")
 	a4 := m("/ip4/1.2.3.4/tcp/1234")
 	a5 := m("/ip4/1.2.3.4/tcp/1235")
-	a6 := m("/ip4/1.2.3.6/tcp/1236")
-	a7 := m("/ip4/1.2.3.7/tcp/1237")
+
+	b1 := m("/ip4/1.2.3.6/tcp/1236")
+	b2 := m("/ip4/1.2.3.7/tcp/1237")
+	b3 := m("/ip4/1.2.3.8/tcp/1237")
+	b4 := m("/ip4/1.2.3.9/tcp/1237")
+	b5 := m("/ip4/1.2.3.10/tcp/1237")
 
 	oas := ObservedAddrSet{}
 
@@ -72,28 +80,32 @@ func TestObsAddrSet(t *testing.T) {
 		t.Error("addrs should _still_ be empty (same obs group)")
 	}
 
-	oas.Add(a1, a6)
+	oas.Add(a1, b1)
+	oas.Add(a1, b2)
+	oas.Add(a1, b3)
 	if !addrsMarch(oas.Addrs(), []ma.Multiaddr{a1}) {
 		t.Error("addrs should only have a1")
 	}
 
 	oas.Add(a2, a5)
 	oas.Add(a1, a5)
 	oas.Add(a1, a5)
-	oas.Add(a2, a6)
-	oas.Add(a1, a6)
-	oas.Add(a1, a6)
-	oas.Add(a2, a7)
-	oas.Add(a1, a7)
-	oas.Add(a1, a7)
+	oas.Add(a2, b1)
+	oas.Add(a1, b1)
+	oas.Add(a1, b1)
+	oas.Add(a2, b2)
+	oas.Add(a1, b2)
+	oas.Add(a1, b2)
+	oas.Add(a2, b4)
+	oas.Add(a2, b5)
 	if !addrsMarch(oas.Addrs(), []ma.Multiaddr{a1, a2}) {
 		t.Error("addrs should only have a1, a2")
 	}
 
 	// change the timeout constant so we can time it out.
 	oas.SetTTL(time.Millisecond * 200)
 	<-time.After(time.Millisecond * 210)
-	if !addrsMarch(oas.Addrs(), []ma.Multiaddr{nil}) {
+	if !addrsMarch(oas.Addrs(), nil) {
 		t.Error("addrs should have timed out")
 	}
 }