Identify overwrites peerstore addresses TTL

[hsanjuan]

Some IPFS Cluster users complain that some their peers are no longer part of the other peers peerstore file after they crashed or were unreachable. This causes some problems when trying to recover the situation (ipfs-cluster/ipfs-cluster#648).

Cluster adds peer addresses to the Peerstore with PermanentAddrTTL. Therefore they should never be removed even if a peer becomes unreachable for a time. Doing some grepping on libp2p threw this line:

https://github.com/libp2p/go-libp2p-identify/blob/master/id.go#L223

In would seem that the identify protocol sets the TTL for addresses announced by a peer to ConnectedAddrTTL. Therefore, when peers go down they will be GC'ed. This is consistent to what users have reported and the only place in the library that I can find that an address added to the peerstore is modified.

If this is doing what I think is doing, I'd like that we either:

• Address this in the peerstore so that TTL cannot be lowered (I'm not sure if with a new method or exposing ttlWriteMode (https://github.com/libp2p/go-libp2p-peerstore/blob/master/pstoreds/addr_book.go#L2390) or with a special flag that says that TTL is manual/fixed).
• Address this here in identify. Identify should not be messing up with addresses added with PermanentTTL.

Also, I don't know but chances are this causes problems when connectivity drops on any libp2p peer.

[vyzo]

I think we need to file this as a bug in the peerstore; cc @raulk

[Stebalien]

Note: this repo is unused.

---

I believe this is a bug in the peerstore. AddAddrs is never supposed to reduce the TTL of an address. However, it looks like both the datastore backed and the memory backed address books have the same bug.

Docs:

If the manager has a longer TTL, the operation is a no-op for that address.

[raulk]

Exactly my thought, @Stebalien.

I believe we have unit tests for the TTL management, so this seems odd.

@hsanjuan are you guys picking up the identify protocol from this repo, by any chance? This repo contains an outdated version that uses SetAddrs() which does override the TTL unconditionally.

The version of identify we should be using is https://github.com/libp2p/go-libp2p/blob/master/p2p/protocol/identify/id.go#L247-L250 (highlighting the relevant lines).

[raulk]

@hsanjuan Had a quick look at ipfs-cluster, and I don't think you guys are picking up this repo. You don't configure the peerstore to use the datastore-backed one either, so it defaults to the in-mem peerstore.

AddAddrs() there only extends the TTL.

We have a test that checks if TTLs are extended (yay!), but we don't have a test that checks that TTLs are not shortened. I'll work on that now.

[Stebalien]

Yeah, it looks like the in-memory version is replacing a longer TTL with a shorter TTL if the shorter TTL yields a later expiration.

[raulk]

@Stebalien the test came back green locally: libp2p/go-libp2p-peerstore#52; let's wait for CI but unless I'm thick and missing something obvious, it seems the in-mem store is behaving as advertised.

[Stebalien]

The bug, as far as I can tell, is https://github.com/libp2p/go-libp2p-peerstore/blob/99108065a0f625eebc2bf7ad235edacd56d0c83a/pstoremem/addr_book.go#L111. We're comparing expiration, not TTLs. If some time has passed, a shorter TTL may expire after the longer TTL,

[raulk]

@Stebalien in my view, that's correct. The expiration of the shorter TTL whose resulting expiration is later than the current, will still extend the lifetime of the entry, as expected. If we compare only TTLs, this could happen (timescale in mm:ss):

t+00:00: AddAddr(addr, 10 * time.Minutes) => entry expires at t+10:00
(wait until just before the TTL expires)
t+09:59: AddAddr(addr, 10 * time.Minutes) => would do nothing, and the entry would still expire in 1 second; instead, it should've extended the expiration to t+19:59

[Stebalien]

I think we need to do both. That is,

• If the new TTL is greater than the existing TTL, replace it.
• If the new expiration is greater than the existing expiration, replace it.

This comes down to the fact that we're abusing TTLs as "tags" to identify different types of records. There's probably an issue discussing this but I can't find it...

[raulk]

Yeah, we've discussed that TTL is really a bad proxy for address quality/confidence. However, I still fail to see how the current behaviour can shorten the TTL of an address that was initially set with a permanent TTL.

[Stebalien]

On identify, we'll try to set it to ConnectedTTL. While that TTL is technically shorter by one nanosecond, the new expiration will be longer (because more than one nanosecond will have passed). Therefore, we'll "extend" the expiration and set the TTL to ConnectedTTL. Finally, when we disconnect, we'll revert anything with a ConnectedTTL to RecentlyConnectedAddrTTL.

[raulk]

Finally, when we disconnect, we'll revert anything with a ConnectedTTL to RecentlyConnectedAddrTTL.

That's the key element I was missing! Let me give it some thought and see how/if we can solve this nicely.

[hsanjuan]

Note: this repo is unused.

Ouch. My bad. Thanks for finding the actual trigger in https://github.com/libp2p/go-libp2p/blob/master/p2p/protocol/identify/id.go#L247-L250

[hsanjuan]

@Stebalien @raulk would you say this was fixed?

[Stebalien]

It fell off our radar. I believe libp2p/go-libp2p-peerstore#92 should fix this.