From 96ed20bffe377e56c2193c41e86581d97470b729 Mon Sep 17 00:00:00 2001 From: Juan Batiz-Benet Date: Sun, 7 Jun 2015 16:47:34 -0700 Subject: [PATCH 1/2] p2p/protocol/identify: dont double count observers If the same peer observed the same address twice, it would be double counted as different observations. This change adds a map to make sure we're counting each observer once. This is easily extended to require more than two observations, but i have not yet encountered NATs for whom this is relevant. --- p2p/protocol/identify/id.go | 2 +- p2p/protocol/identify/obsaddr.go | 33 ++++++++++++++++++--------- p2p/protocol/identify/obsaddr_test.go | 28 +++++++++++++++++------ 3 files changed, 44 insertions(+), 19 deletions(-) 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..c55757df3 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,40 @@ 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 } + + // Add current observer. + oa.SeenBy[observer.String()] = struct{}{} + oa.LastSeen = time.Now() } 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..416e8f681 100644 --- a/p2p/protocol/identify/obsaddr_test.go +++ b/p2p/protocol/identify/obsaddr_test.go @@ -36,6 +36,9 @@ 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.4/tcp/1236") oas := ObservedAddrSet{} @@ -43,23 +46,34 @@ 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)") + } + + oas.Add(a1, a5) 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) if !addrsMarch(oas.Addrs(), []ma.Multiaddr{a1, a2}) { t.Error("addrs should only have a1, a2") } From 500bb51759503cf5481d7150c8a4a1d791ba9078 Mon Sep 17 00:00:00 2001 From: Juan Batiz-Benet Date: Sun, 7 Jun 2015 17:00:56 -0700 Subject: [PATCH 2/2] p2p/net/identify: clump addr observers into groups Different mutliaddrs is not enough. Nodes may share transports. NAT port mappings will likely only work on the base IP/TCP port pair. We go one step further, and require different root (IP) addrs. Just in case some NATs group by IP. In practice, this is what we want: use addresses only if hosts that are on different parts of the network have seen this address. --- p2p/protocol/identify/obsaddr.go | 18 ++++++++++++++++-- p2p/protocol/identify/obsaddr_test.go | 14 +++++++++++++- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/p2p/protocol/identify/obsaddr.go b/p2p/protocol/identify/obsaddr.go index c55757df3..f4033949b 100644 --- a/p2p/protocol/identify/obsaddr.go +++ b/p2p/protocol/identify/obsaddr.go @@ -85,11 +85,25 @@ func (oas *ObservedAddrSet) Add(addr ma.Multiaddr, observer ma.Multiaddr) { oas.addrs[s] = oa } - // Add current observer. - oa.SeenBy[observer.String()] = struct{}{} + // 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) { oas.Lock() defer oas.Unlock() diff --git a/p2p/protocol/identify/obsaddr_test.go b/p2p/protocol/identify/obsaddr_test.go index 416e8f681..dba769594 100644 --- a/p2p/protocol/identify/obsaddr_test.go +++ b/p2p/protocol/identify/obsaddr_test.go @@ -38,7 +38,8 @@ 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.4/tcp/1236") + a6 := m("/ip4/1.2.3.6/tcp/1236") + a7 := m("/ip4/1.2.3.7/tcp/1237") oas := ObservedAddrSet{} @@ -63,7 +64,15 @@ func TestObsAddrSet(t *testing.T) { 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") } @@ -74,6 +83,9 @@ func TestObsAddrSet(t *testing.T) { 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") }