RFC: `Identify` should remember discovered peers and assist in peer discovery using `addresses_of_peer`

[thomaseizinger]

While implementing an address book here, I realized that we might be able to achieve the same functionality by adjusting how our existing protocols work. In particular, not all of our discovery-like protocols are currently assisting in peer discovery via addresses_of_peer. Identify for example does not override addresses_of_peer.

What do people think about changing that so that we remember the reported addresses of peers that we have discovered? I think the only issue here is that we potentially have an unbounded memory growth because there isn't really a natural event that we could leverage to clean-up old addresses.

Solutions I could think of:

1. Use a LRU cache and simply cap its storage at some number of entries.
2. Assign a TTL to each entry and have it expire automatically.

Ideally, Identify would also react to dial failures and remove entries that it reported but which turned out to be useless.

Having Identify remember reported addresses is particularly useful if:

1. Node A knows about its own, externally reachable address
2. Node B does not know about external addresses of node A
3. Node A dials node B
4. Node B learns via /identify about A's external addresses
5. The Connection between node A and B gets closed

If Identify would remember the externally reachable addresses that have previously been identified, Node B would now be able to establish a connection to A by simply dialing its PeerId.
Currently, users of rust-libp2p have to actively handle IdentifyEvent and add all reported addresses to some other NetworkBehaviour that will report it as part of addresses_of_peer later.

[mxinden]

Thanks for the proposal in #2232.

In general I am fine with the solution proposed in #2232. It is simple and non-intrusive.

At the same time I do find the proposal to slightly violate concerns. I.e. libp2p-identify today solely identifies peers. It is not concerned with caching addresses. Also, what if I would like to cache addresses discovered by another NetworkBehaviour implementation?

Overall I am more in favor of an approach similar to #2133, extending NetworkBehaviourAction with a ReportPeerAddr, introducing a AddressBook NetworkBehaviour. Arguments speaking in favor of this approach:

• Non intrusive. Only need to introduce NetworkBehaviourAction::ReportPeerAddr.
• Simple. A single LRU cache would do.
• Isolated. Thoss that don't want to use it don't include the behaviour.
• Single responsibility (caching addresses) in a single place.

[thomaseizinger]

At the same time I do find the proposal to slightly violate concerns. I.e. libp2p-identify today solely identifies peers. It is not concerned with caching addresses.

I think the way I look at it is that libp2p-identify has discovery features and hence can assist in peer discovery. The fact that this happens via caching is an implementation detail IMO.

Also, what if I would like to cache addresses discovered by another NetworkBehaviour implementation?

If that is desired, we can extract the LRU functionality into a component of libp2p-core (or a different crate?) that other implementations can make use of.

#2133 is more invasive as it changes public APIs which IMO might need a bit more discussion. For example, to avoid having multiple ways of achieving the same thing, we would have to discuss removing addresses_of_peer in favor of ReportPeerAddr.

[thomaseizinger]

In general I am fine with the solution proposed in #2232. It is simple and non-intrusive.

Can I consider this to be an approval of #2232 then or how would you like to move forward here? :)