Merge pull request #92 from libp2p/fix/identify-2

fix multiple TTL bugs

pstoreds/addr_book.go

@@ -347,11 +347,21 @@ Outer:
 		for _, have := range pr.Addrs {
 			if incoming.Equal(have.Addr) {
 				existed[i] = true
-				if mode == ttlExtend && have.Expiry > newExp {
-					// if we're only extending TTLs but the addr already has a longer one, we skip it.
-					continue Outer
+				switch mode {
+				case ttlOverride:
+					have.Ttl = int64(ttl)
+					have.Expiry = newExp
+				case ttlExtend:
+					if int64(ttl) > have.Ttl {
+						have.Ttl = int64(ttl)
+					}
+					if newExp > have.Expiry {
+						have.Expiry = newExp
+					}
+				default:
+					panic("BUG: unimplemented ttl mode")
 				}
-				have.Expiry = newExp
+
 				// we found the address, and addresses cannot be duplicate,
 				// so let's move on to the next.
 				continue Outer

pstoremem/addr_book.go

@@ -132,7 +132,7 @@ func (mab *memoryAddrBook) AddAddr(p peer.ID, addr ma.Multiaddr, ttl time.Durati
 
 // AddAddrs gives memoryAddrBook addresses to use, with a given ttl
 // (time-to-live), after which the address is no longer valid.
-// If the manager has a longer TTL, the operation is a no-op for that address
+// This function never reduces the TTL or expiration of an address.
 func (mab *memoryAddrBook) AddAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Duration) {
 	// if ttl is zero, exit. nothing to do.
 	if ttl <= 0 {
@@ -156,10 +156,19 @@ func (mab *memoryAddrBook) AddAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du
 		}
 		asBytes := addr.Bytes()
 		a, found := amap[string(asBytes)] // won't allocate.
-		if !found || exp.After(a.Expires) {
+		if !found {
+			// not found, save and announce it.
 			amap[string(asBytes)] = &expiringAddr{Addr: addr, Expires: exp, TTL: ttl}
-
 			mab.subManager.BroadcastAddr(p, addr)
+		} else {
+			// Update expiration/TTL independently.
+			// We never want to reduce either.
+			if ttl > a.TTL {
+				a.TTL = ttl
+			}
+			if exp.After(a.Expires) {
+				a.Expires = exp
+			}
 		}
 	}
 }

test/addr_book_suite.go

@@ -86,9 +86,13 @@ func testAddAddress(ab pstore.AddrBook) func(*testing.T) {
 			// after the initial TTL has expired, check that only the third address is present.
 			time.Sleep(1200 * time.Millisecond)
 			AssertAddressesEqual(t, addrs[2:], ab.Addrs(id))
+
+			// make sure we actually set the TTL
+			ab.UpdateAddrs(id, time.Hour, 0)
+			AssertAddressesEqual(t, nil, ab.Addrs(id))
 		})
 
-		t.Run("adding an existing address with an earlier expiration is noop", func(t *testing.T) {
+		t.Run("adding an existing address with an earlier expiration never reduces the expiration", func(t *testing.T) {
 			id := GeneratePeerIDs(1)[0]
 			addrs := GenerateAddrs(3)
 
@@ -102,6 +106,27 @@ func testAddAddress(ab pstore.AddrBook) func(*testing.T) {
 			time.Sleep(2100 * time.Millisecond)
 			AssertAddressesEqual(t, addrs, ab.Addrs(id))
 		})
+
+		t.Run("adding an existing address with an earlier expiration never reduces the TTL", func(t *testing.T) {
+			id := GeneratePeerIDs(1)[0]
+			addrs := GenerateAddrs(1)
+
+			ab.AddAddrs(id, addrs, 4*time.Second)
+			// 4 seconds left
+			time.Sleep(3 * time.Second)
+			// 1 second left
+			ab.AddAddrs(id, addrs, 3*time.Second)
+			// 3 seconds left
+			time.Sleep(2)
+			// 1 seconds left.
+
+			// We still have the address.
+			AssertAddressesEqual(t, addrs, ab.Addrs(id))
+
+			// The TTL wasn't reduced
+			ab.UpdateAddrs(id, 4*time.Second, 0)
+			AssertAddressesEqual(t, nil, ab.Addrs(id))
+		})
 	}
 }