diff --git a/p2p/protocol/identify/id.go b/p2p/protocol/identify/id.go index 56dedf655..d9cc0b1e0 100644 --- a/p2p/protocol/identify/id.go +++ b/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 { diff --git a/p2p/protocol/identify/obsaddr.go b/p2p/protocol/identify/obsaddr.go index b2169057a..f4033949b 100644 --- a/p2p/protocol/identify/obsaddr.go +++ b/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) { diff --git a/p2p/protocol/identify/obsaddr_test.go b/p2p/protocol/identify/obsaddr_test.go index 8a5ef80fe..dba769594 100644 --- a/p2p/protocol/identify/obsaddr_test.go +++ b/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") }