Merge pull request #1342 from ipfs/observe-addr-twice

Better symmetric NAT avoidance.

p2p/protocol/identify/id.go

@@ -256,7 +256,7 @@ func (ids *IDService) consumeObservedAddress(observed []byte, c inet.Conn) {
 
 	// ok! we have the observed version of one of our ListenAddresses!
 	log.Debugf("added own observed listen addr: %s --> %s", c.LocalMultiaddr(), maddr)
-	ids.observedAddrs.Add(maddr)
+	ids.observedAddrs.Add(maddr, c.RemoteMultiaddr())
 }
 
 func addrInAddrs(a ma.Multiaddr, as []ma.Multiaddr) bool {

p2p/protocol/identify/obsaddr.go

@@ -15,9 +15,9 @@ import (
 // - have been observed recently (10min), because our position in the
 //   network, or network port mapppings, may have changed.
 type ObservedAddr struct {
-	Addr      ma.Multiaddr
-	LastSeen  time.Time
-	TimesSeen int
+	Addr     ma.Multiaddr
+	SeenBy   map[string]struct{}
+	LastSeen time.Time
 }
 
 // ObservedAddrSet keeps track of a set of ObservedAddrs
@@ -25,7 +25,7 @@ type ObservedAddr struct {
 type ObservedAddrSet struct {
 	sync.Mutex // guards whole datastruct.
 
-	addrs map[string]ObservedAddr
+	addrs map[string]*ObservedAddr
 	ttl   time.Duration
 }
 
@@ -54,29 +54,54 @@ func (oas *ObservedAddrSet) Addrs() []ma.Multiaddr {
 		// 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.
-		if a.TimesSeen > 1 {
+		//
+		// Note: make sure not to double count observers.
+		if len(a.SeenBy) > 1 {
 			addrs = append(addrs, a.Addr)
 		}
 	}
 	return addrs
 }
 
-func (oas *ObservedAddrSet) Add(addr ma.Multiaddr) {
+func (oas *ObservedAddrSet) Add(addr ma.Multiaddr, observer ma.Multiaddr) {
 	oas.Lock()
 	defer oas.Unlock()
 
 	// for zero-value.
 	if oas.addrs == nil {
-		oas.addrs = make(map[string]ObservedAddr)
+		oas.addrs = make(map[string]*ObservedAddr)
 		oas.ttl = peer.OwnObservedAddrTTL
 	}
 
 	s := addr.String()
-	oas.addrs[s] = ObservedAddr{
-		Addr:      addr,
-		TimesSeen: oas.addrs[s].TimesSeen + 1,
-		LastSeen:  time.Now(),
+	oa, found := oas.addrs[s]
+
+	// first time seeing address.
+	if !found {
+		oa = &ObservedAddr{
+			Addr:   addr,
+			SeenBy: make(map[string]struct{}),
+		}
+		oas.addrs[s] = oa
 	}
+
+	// mark the observer
+	oa.SeenBy[observerGroup(observer)] = struct{}{}
+	oa.LastSeen = time.Now()
+}
+
+// observerGroup is a function that determines what part of
+// a multiaddr counts as a different observer. for example,
+// two ipfs nodes at the same IP/TCP transport would get
+// the exact same NAT mapping; they would count as the
+// same observer. This may protect against NATs who assign
+// different ports to addresses at different IP hosts, but
+// not TCP ports.
+//
+// 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 {
+	return ma.Split(m)[0].String()
 }
 
 func (oas *ObservedAddrSet) SetTTL(ttl time.Duration) {

p2p/protocol/identify/obsaddr_test.go

@@ -36,6 +36,10 @@ func TestObsAddrSet(t *testing.T) {
 	a1 := m("/ip4/1.2.3.4/tcp/1231")
 	a2 := m("/ip4/1.2.3.4/tcp/1232")
 	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")
 
 	oas := ObservedAddrSet{}
 
@@ -43,23 +47,45 @@ func TestObsAddrSet(t *testing.T) {
 		t.Error("addrs should be empty")
 	}
 
-	oas.Add(a1)
-	oas.Add(a2)
-	oas.Add(a3)
+	oas.Add(a1, a4)
+	oas.Add(a2, a4)
+	oas.Add(a3, a4)
 
 	// these are all different so we should not yet get them.
 	if !addrsMarch(oas.Addrs(), nil) {
 		t.Error("addrs should _still_ be empty (once)")
 	}
 
-	oas.Add(a1)
+	// same observer, so should not yet get them.
+	oas.Add(a1, a4)
+	oas.Add(a2, a4)
+	oas.Add(a3, a4)
+	if !addrsMarch(oas.Addrs(), nil) {
+		t.Error("addrs should _still_ be empty (same obs)")
+	}
+
+	// different observer, but same observer group.
+	oas.Add(a1, a5)
+	oas.Add(a2, a5)
+	oas.Add(a3, a5)
+	if !addrsMarch(oas.Addrs(), nil) {
+		t.Error("addrs should _still_ be empty (same obs group)")
+	}
+
+	oas.Add(a1, a6)
 	if !addrsMarch(oas.Addrs(), []ma.Multiaddr{a1}) {
 		t.Error("addrs should only have a1")
 	}
 
-	oas.Add(a2)
-	oas.Add(a1)
-	oas.Add(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)
 	if !addrsMarch(oas.Addrs(), []ma.Multiaddr{a1, a2}) {
 		t.Error("addrs should only have a1, a2")
 	}