Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: group observations by zeroing port #949

Merged
merged 1 commit into from
May 20, 2020
Merged

fix: group observations by zeroing port #949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 16 additions & 7 deletions p2p/protocol/identify/obsaddr.go
View file
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Why are we adding in the zero bytes here instead of just skipping them? Is this just a defensive measure to keep them as valid multiaddrs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, we could just skip it. But yeah, I wanted these to be valid multiaddrs.

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
View file
Open in desktop
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())
}