From 59062c6d6d6dd5ae3c8cd6c53637f98070740d10 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 27 May 2020 16:22:39 -0700 Subject: [PATCH 1/2] 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. --- p2p/protocol/identify/id.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/p2p/protocol/identify/id.go b/p2p/protocol/identify/id.go index 1eabf9eacf..01bc818b4d 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) From be70fa2460e7baf320efe0e29ad1e647b546b78a Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 27 May 2020 16:58:53 -0700 Subject: [PATCH 2/2] test: fix test for immediately forgetting addresses --- p2p/protocol/identify/id_test.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/p2p/protocol/identify/id_test.go b/p2p/protocol/identify/id_test.go index 4b79567f1a..ff34ee7bce 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))