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: remove old addresses in identify immediately #953

Merged
merged 2 commits into from
May 28, 2020
Merged

fix: remove old addresses in identify immediately #953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59062c6
fix: remove old addresses in identify immediately
Stebalien May 27, 2020
be70fa2
test: fix test for immediately forgetting addresses
Stebalien May 27, 2020
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
16 changes: 10 additions & 6 deletions p2p/protocol/identify/id.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,6 @@ func init() {
}
}

// transientTTL is a short ttl for invalidated previously connected addrs
const transientTTL = 10 * time.Second

type addPeerHandlerReq struct {
rp peer.ID
resp chan *peerHandler
Expand Down Expand Up @@ -546,9 +543,13 @@ func (ids *IDService) consumeMessage(mes *pb.Identify, c network.Conn) {
ttl = peerstore.ConnectedAddrTTL
}

// invalidate previous addrs -- we use a transient ttl instead of 0 to ensure there
// is no period of having no good addrs whatsoever
ids.Host.Peerstore().UpdateAddrs(p, peerstore.ConnectedAddrTTL, transientTTL)
// Downgrade connected and recently connected addrs to a temporary TTL.
for _, ttl := range []time.Duration{
peerstore.RecentlyConnectedAddrTTL,
peerstore.ConnectedAddrTTL,
} {
ids.Host.Peerstore().UpdateAddrs(p, ttl, peerstore.TempAddrTTL)
}

// add signed addrs if we have them and the peerstore supports them
cab, ok := peerstore.GetCertifiedAddrBook(ids.Host.Peerstore())
Expand All @@ -560,6 +561,9 @@ func (ids *IDService) consumeMessage(mes *pb.Identify, c network.Conn) {
} else {
ids.Host.Peerstore().AddAddrs(p, lmaddrs, ttl)
}

// Finally, expire all temporary addrs.
ids.Host.Peerstore().UpdateAddrs(p, peerstore.TempAddrTTL, 0)
ids.addrMu.Unlock()

log.Debugf("%s received listen addrs for %s: %s", c.LocalPeer(), c.RemotePeer(), lmaddrs)
Expand Down
15 changes: 6 additions & 9 deletions p2p/protocol/identify/id_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ func subtestIDService(t *testing.T) {
testKnowsAddrs(t, h2, h1p, []ma.Multiaddr{}) // nothing

// the forgetMe addr represents an address for h1 that h2 has learned out of band
// (not via identify protocol). Shortly after the identify exchange, it will be
// forgotten and replaced by the addrs h1 sends during identify
// (not via identify protocol). During the identify exchange, it will be
// forgotten and replaced by the addrs h1 sends.
forgetMe, _ := ma.NewMultiaddr("/ip4/1.2.3.4/tcp/1234")

h2.Peerstore().AddAddr(h1p, forgetMe, peerstore.RecentlyConnectedAddrTTL)
Expand All @@ -80,7 +80,7 @@ func subtestIDService(t *testing.T) {
// the IDService should be opened automatically, by the network.
// what we should see now is that both peers know about each others listen addresses.
t.Log("test peer1 has peer2 addrs correctly")
testKnowsAddrs(t, h1, h2p, h2.Peerstore().Addrs(h2p)) // has them
testKnowsAddrs(t, h1, h2p, h2.Addrs()) // has them
testHasCertifiedAddrs(t, h1, h2p, h2.Peerstore().Addrs(h2p)) // should have signed addrs also
Comment on lines +83 to 84
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand all the test changes. Why are we sometimes using h.Addrs() and sometimes using h.Peerstore().Addrs(h.ID)?

testHasProtocolVersions(t, h1, h2p)
testHasPublicKey(t, h1, h2p, h2.Peerstore().PubKey(h2p)) // h1 should have h2's public key
Expand All @@ -93,12 +93,9 @@ func subtestIDService(t *testing.T) {
}
ids2.IdentifyConn(c[0])

addrs := h1.Peerstore().Addrs(h1p)
addrs = append(addrs, forgetMe)

// and the protocol versions.
t.Log("test peer2 has peer1 addrs correctly")
testKnowsAddrs(t, h2, h1p, addrs) // has them
testKnowsAddrs(t, h2, h1p, h1.Addrs()) // has them
testHasCertifiedAddrs(t, h2, h1p, h1.Peerstore().Addrs(h1p))
testHasProtocolVersions(t, h2, h1p)
testHasPublicKey(t, h2, h1p, h1.Peerstore().PubKey(h1p)) // h1 should have h2's public key
Expand All @@ -112,8 +109,8 @@ func subtestIDService(t *testing.T) {

t.Log("testing addrs just after disconnect")
// addresses don't immediately expire on disconnect, so we should still have them
testKnowsAddrs(t, h2, h1p, addrs)
testKnowsAddrs(t, h1, h2p, h2.Peerstore().Addrs(h2p))
testKnowsAddrs(t, h2, h1p, h1.Addrs())
testKnowsAddrs(t, h1, h2p, h2.Addrs())
testHasCertifiedAddrs(t, h1, h2p, h2.Peerstore().Addrs(h2p))
testHasCertifiedAddrs(t, h2, h1p, h1.Peerstore().Addrs(h1p))

Expand Down