From 2fb6d7a48f732400fb0f4128256b15f08f724b81 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 24 Jul 2019 16:29:51 -0700 Subject: [PATCH] fix multiple TTL bugs The first fix independently extends the address expiration time and the address TTL: By example: * We have an address with a TTL of 4s that will expire in 1s. * We update it with a TTL of 3s. Before this change: * We end up with an address with a TTL of 3s that will expire in 3s. After this change: * We end up with an address with a TTL of 4s that will expire in 3s. --- The second fix prevents the in-memory addressbook from announcing existing addresses every time their TTLs get updated. --- The third fix correctly updates TTLs for existing addresses in the on-disk addressbook. This fixes https://github.com/libp2p/go-libp2p-identify/issues/2 --- pstoreds/addr_book.go | 18 ++++++++++++++---- pstoremem/addr_book.go | 15 ++++++++++++--- test/addr_book_suite.go | 27 ++++++++++++++++++++++++++- 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/pstoreds/addr_book.go b/pstoreds/addr_book.go index bba648e..2f75265 100644 --- a/pstoreds/addr_book.go +++ b/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 diff --git a/pstoremem/addr_book.go b/pstoremem/addr_book.go index 19b60f9..90ffb17 100644 --- a/pstoremem/addr_book.go +++ b/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 + } } } } diff --git a/test/addr_book_suite.go b/test/addr_book_suite.go index 72a899c..ea6078c 100644 --- a/test/addr_book_suite.go +++ b/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)) + }) } }