diff --git a/p2p/protocol/identify/obsaddr.go b/p2p/protocol/identify/obsaddr.go index 72263e3763..0e72ab3433 100644 --- a/p2p/protocol/identify/obsaddr.go +++ b/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() } diff --git a/p2p/protocol/identify/obsaddr_test.go b/p2p/protocol/identify/obsaddr_test.go index ac3b239f0e..acf3d30d00 100644 --- a/p2p/protocol/identify/obsaddr_test.go +++ b/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,7 +80,9 @@ 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") } @@ -80,12 +90,14 @@ func TestObsAddrSet(t *testing.T) { 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") } @@ -93,7 +105,7 @@ func TestObsAddrSet(t *testing.T) { // 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") } }