fullrt should maintain its own address book

[guillaumemichel]

Problem

fullrt currently depends on the go-libp2p peerstore to open connection and make requests through the ProtocolMessenger. The fullrt used to pollute the go-libp2p host peerstore with many stale addresses (fixed in #1053).

fullrt periodically runs a crawl, and tries to remember addresses between 2 crawls, to be able to dial all nodes directly. However, it fails at doing so, because addresses are flushed from the peerstore shortly after the connection to the remote peers closes. Which means that the peerstore doesn't remember the addresses during the full crawl interval. It cannot be addresses easily because of the peerstore current interface libp2p/go-libp2p#3218.

Potential solution

The best solution seems to be that fullrt maintains its own Address Book, and uses these addresses to dial remote peers when required. To achieve this, it is required to refactor the ProtocolMessenger logic, to depend on the fullrt address book (or provided addresses), rather than on the host peerstore to open a new stream.

go-libp2p-kad-dht/internal/net/message_manager.go

Line 209 in 64ea67f

nstr, err := ms.m.host.NewStream(ctx, ms.p, ms.m.protocols...)

see also #1053 (comment) for context.

[aschmahmann]

@guillaumemichel are you sure things are currently bugged? Looking at the code briefly:

• There's a pretty simple form of "address book" already

  go-libp2p-kad-dht/fullrt/dht.go

  Line 100 in 64ea67f

  peerAddrs map[peer.ID][]multiaddr.Multiaddr

• It's used for the bulk lookups and for GetClosestPeers, and together those cover all the requests
  • Note: GetClosestPeers does this somewhat janky hack to keep the peers around for the duration of the query

    go-libp2p-kad-dht/fullrt/dht.go

    Line 508 in 64ea67f

    dht.h.Peerstore().AddAddrs(p, peerAddrs, peerstore.TempAddrTTL)

    , but it doesn't influence much since go-libp2p will do this anyway as long as the query relying on the hack takes less than two minutes (https://github.com/libp2p/go-libp2p/blob/5e6f217d847e7c1c3f98197a54c9503d0303a7bb/core/peerstore/peerstore.go#L27) anyway https://github.com/libp2p/go-libp2p/blob/5e6f217d847e7c1c3f98197a54c9503d0303a7bb/p2p/host/basic/basic_host.go#L685

That being said if we want to formalize the addressbook and integrate it with a simpler / easier to use go-libp2p API is indicated in libp2p/go-libp2p#3218 that makes a lot of sense

[guillaumemichel]

Thanks for the quick feedback @aschmahmann !

I missed that the protoMessenger is only used after GetClosestPeers is called (or in bulk message send), so it should be good to leave it as is.

I think we can even remove the following, since it is a no-op (ttl of connected peer is higher than this one)

go-libp2p-kad-dht/crawler/crawler.go

Lines 303 to 307 in 64ea67f

	// Extend peerstore address ttl for addresses whose ttl is below
	// c.dialAddressExtendDur. By now identify has already cleaned up addresses
	// provided to Connect above and only kept the listen addresses advertised by
	// the remote peer
	c.host.Peerstore().AddAddrs(nextPeer.ID, c.host.Peerstore().Addrs(nextPeer.ID), c.dialAddressExtendDur)

[guillaumemichel]

Closing since fullrt already has a simple address book that works for its usage.

I think we can even remove the following, since it is a no-op (ttl of connected peer is higher than this one)

This partially related discussion can continue in #1063