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

Clean addresses with peer id before adding to addrbook #2007

Merged
merged 3 commits into from
Jan 25, 2023
Merged

Clean addresses with peer id before adding to addrbook #2007

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1929d4e
Clean addresses with peer id before adding to addrbook
sukunrt Jan 23, 2023
1e5c9d4
Filter p2p addresses with invalid peerid
sukunrt Jan 24, 2023
fb7d0a9
Nits
MarcoPolo Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions p2p/host/peerstore/pstoreds/addr_book.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,10 +601,12 @@ func (ab *dsAddrBook) deleteAddrs(p peer.ID, addrs []ma.Multiaddr) (err error) {
func cleanAddrs(addrs []ma.Multiaddr) []ma.Multiaddr {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should now take a peer id as well, and filter out multiaddrs that have an id component but don't match (and log an error when this happens)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed now

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm logging a warning which was previously logged for nil addresses, should I make it an error?

clean := make([]ma.Multiaddr, 0, len(addrs))
for _, addr := range addrs {
if addr == nil {
// Remove suffix of /p2p/peer-id from address
ad, _ := peer.SplitAddr(addr)
if ad == nil {
continue
}
clean = append(clean, addr)
clean = append(clean, ad)
}
return clean
}
21 changes: 12 additions & 9 deletions p2p/host/peerstore/pstoremem/addr_book.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,19 +238,21 @@ func (mab *memoryAddrBook) addAddrsUnlocked(s *addrSegment, p peer.ID, addrs []m

exp := mab.clock.Now().Add(ttl)
for _, addr := range addrs {
if addr == nil {
// Remove suffix of /p2p/peer-id from address
taddr, _ := peer.SplitAddr(addr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment as above about the peer id check. Also I think it's okay to shadow the addr variable with the new multiaddr.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be fixed

if taddr == nil {
log.Warnw("was passed nil multiaddr", "peer", p)
continue
}

// find the highest TTL and Expiry time between
// existing records and function args
a, found := amap[string(addr.Bytes())] // won't allocate.
a, found := amap[string(taddr.Bytes())] // won't allocate.
if !found {
// not found, announce it.
entry := &expiringAddr{Addr: addr, Expires: exp, TTL: ttl}
amap[string(addr.Bytes())] = entry
mab.subManager.BroadcastAddr(p, addr)
entry := &expiringAddr{Addr: taddr, Expires: exp, TTL: ttl}
amap[string(taddr.Bytes())] = entry
mab.subManager.BroadcastAddr(p, taddr)
} else {
// update ttl & exp to whichever is greater between new and existing entry
if ttl > a.TTL {
Expand Down Expand Up @@ -283,17 +285,18 @@ func (mab *memoryAddrBook) SetAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du

exp := mab.clock.Now().Add(ttl)
for _, addr := range addrs {
if addr == nil {
taddr, _ := peer.SplitAddr(addr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed now.

if taddr == nil {
log.Warnw("was passed nil multiaddr", "peer", p)
continue
}
aBytes := addr.Bytes()
aBytes := taddr.Bytes()
key := string(aBytes)

// re-set all of them for new ttl.
if ttl > 0 {
amap[key] = &expiringAddr{Addr: addr, Expires: exp, TTL: ttl}
mab.subManager.BroadcastAddr(p, addr)
amap[key] = &expiringAddr{Addr: taddr, Expires: exp, TTL: ttl}
mab.subManager.BroadcastAddr(p, taddr)
} else {
delete(amap, key)
}
Expand Down
8 changes: 8 additions & 0 deletions p2p/host/peerstore/test/addr_book_suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,14 @@ func testAddAddress(ab pstore.AddrBook, clk *mockClock.Mock) func(*testing.T) {
ab.AddAddrs("", addrs, time.Hour)
AssertAddressesEqual(t, addrs, ab.Addrs(""))
})

t.Run("add a /p2p address", func(t *testing.T) {
peerId := GeneratePeerIDs(1)[0]
addr := GenerateAddrs(1)
p2pAddr := addr[0].Encapsulate(Multiaddr("/p2p/" + peerId.String()))
ab.AddAddr(peerId, p2pAddr, time.Hour)
AssertAddressesEqual(t, addr, ab.Addrs(peerId))
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have a case that fails if the peer id in the multiaddr is != to the peer id passed in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The annoying thing is that AddAddr doesn't return an error. So the best we can do here is to fail silently and log an error in the implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added another test case with filtering on invalid peerId

}
}

Expand Down