Repository: libp2p/go-libp2p
Pull request #953 (2020-05-27 23:24:54)

fix: remove old addresses in identify immediately
Previously, we'd keep addresses discovered through the DHT for up to 2 minutes (temporary TTL) and previously seen addresses (recently connected) for up to 10 minutes (the TTL).

1. Make sure to downgrade both connected and recently connected addresses to the "temporary" ttl before adding new addresses.
2. Finally, downgrade addresses with the temporary TTL to 0.

This could be more efficient with a better peerstore abstraction, but this is better than nothing.


Patch:
diff --git a/p2p/protocol/identify/id.go b/p2p/protocol/identify/id.go
index 1eabf9ea..01bc818b 100644
--- a/p2p/protocol/identify/id.go
+++ b/p2p/protocol/identify/id.go
@@ -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
@@ -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())
@@ -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)


Test patch:
diff --git a/p2p/protocol/identify/id_test.go b/p2p/protocol/identify/id_test.go
index 8cc86d21..ff34ee7b 100644
--- a/p2p/protocol/identify/id_test.go
+++ b/p2p/protocol/identify/id_test.go
@@ -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)
@@ -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
 	testHasProtocolVersions(t, h1, h2p)
 	testHasPublicKey(t, h1, h2p, h2.Peerstore().PubKey(h2p)) // h1 should have h2's public key
@@ -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
@@ -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))
 
@@ -587,13 +584,11 @@ func TestIdentifyPushOnAddrChange(t *testing.T) {
 
 	h2pi := h2.Peerstore().PeerInfo(h2p)
 	require.NoError(t, h1.Connect(ctx, h2pi))
-	// h1 should immediately see a connection from h2
 	require.Len(t, h1.Network().ConnsToPeer(h2p), 1)
-	// wait for h2 to Identify itself so we are sure h2 has seen the connection.
-	ids1.IdentifyConn(h1.Network().ConnsToPeer(h2p)[0])
-
-	// h2 should now see the connection and we should wait for h1 to Identify itself to h2.
 	require.Len(t, h2.Network().ConnsToPeer(h1p), 1)
+
+	// wait for identify to complete and assert current addresses
+	ids1.IdentifyConn(h1.Network().ConnsToPeer(h2p)[0])
 	ids2.IdentifyConn(h2.Network().ConnsToPeer(h1p)[0])
 
 	testKnowsAddrs(t, h1, h2p, h2.Peerstore().Addrs(h2p))