Skip to content

Commit

Permalink
fix: group observations by zeroing port
Browse files Browse the repository at this point in the history
In #917, we started dropping additional address observations if we had multiple
for the same transport set. However, on further consideration, this isn't quite
correct. We _want_ to keep additional observations for multiple IP addresses.
The real issue is many observations for different ports.

So this patch simply changes the key with which we group observations from
"address protocols" to "address without the port" (well, with the port set to
0).
  • Loading branch information
Stebalien committed May 20, 2020
1 parent b396e69 commit 6ef5f5d
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 20 deletions.
23 changes: 16 additions & 7 deletions p2p/protocol/identify/obsaddr.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,24 @@ func (oa *ObservedAddr) numInbound() int {
return count
}

// GroupKey returns the group in which this observation belongs. Currently, an
// observed address's group is just the address with all ports set to 0. This
// means we can advertise the most commonly observed external ports without
// advertising _every_ observed port.
func (oa *ObservedAddr) GroupKey() string {
key := ""
protos := oa.Addr.Protocols()

for i := range protos {
key = key + "/" + protos[i].Name
}
key := make([]byte, 0, len(oa.Addr.Bytes()))
ma.ForEach(oa.Addr, func(c ma.Component) bool {
switch proto := c.Protocol(); proto.Code {
case ma.P_TCP, ma.P_UDP:
key = append(key, proto.VCode...)
key = append(key, 0, 0) // zero in two bytes
default:
key = append(key, c.Bytes()...)
}
return true
})

return key
return string(key)
}

type newObservation struct {
Expand Down
37 changes: 24 additions & 13 deletions p2p/protocol/identify/obsaddr_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -314,17 +314,28 @@ func TestObservedAddrFiltering(t *testing.T) {
}

func TestObservedAddrGroupKey(t *testing.T) {
oa1 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/tcp/1231")}
require.Equal(t, "/ip4/tcp", oa1.GroupKey())

oa2 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.5/tcp/2222")}
require.Equal(t, "/ip4/tcp", oa2.GroupKey())

oa3 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/udp/1231")}
require.Equal(t, "/ip4/udp", oa3.GroupKey())
oa4 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.3.3.4/udp/1531")}
require.Equal(t, "/ip4/udp", oa4.GroupKey())

oa5 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.3.3.4/udp/1531/quic")}
require.Equal(t, "/ip4/udp/quic", oa5.GroupKey())
oa1 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/tcp/2345")}
oa2 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/tcp/1231")}
oa3 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.5/tcp/1231")}
oa4 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/udp/1231")}
oa5 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/udp/1531")}
oa6 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/udp/1531/quic")}
oa7 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.4/udp/1111/quic")}
oa8 := &identify.ObservedAddr{Addr: ma.StringCast("/ip4/1.2.3.5/udp/1111/quic")}

// different ports, same IP => same key
require.Equal(t, oa1.GroupKey(), oa2.GroupKey())
// different IPs => different key
require.NotEqual(t, oa2.GroupKey(), oa3.GroupKey())
// same port, different protos => different keys
require.NotEqual(t, oa3.GroupKey(), oa4.GroupKey())
// same port, same address, different protos => different keys
require.NotEqual(t, oa2.GroupKey(), oa4.GroupKey())
// udp works as well
require.Equal(t, oa4.GroupKey(), oa5.GroupKey())
// udp and quic are different
require.NotEqual(t, oa5.GroupKey(), oa6.GroupKey())
// quic works as well
require.Equal(t, oa6.GroupKey(), oa7.GroupKey())
require.NotEqual(t, oa7.GroupKey(), oa8.GroupKey())
}

0 comments on commit 6ef5f5d

Please sign in to comment.