RFC: centralize PeerBook.put logic

[jacobheun]

Throughout the libp2p modules, especially when peer discovery is involved, modules will often do some combination of adding the peer to the PeerBook and/or emitting the peer event for discovery.

I'd like to suggest we change the behavior to emit only, and have libp2p process the necessary modifications to the PeerBook. The reason for this, is that by centralizing the decision to update the PeerBook, libp2p will have a clearer understanding of what peers are actually new, versus those that were already discovered. In turn, it can emit the peer discovery event to listening application code.

Currently, libp2p has to emit everything, because the underlying modules may have already added the peer to the PeerBook, which causes a lot of duplicate peer discovery events.

Reference libp2p/js-libp2p-kad-dht#93 (comment)

[JustMaier]

I'm still trying to grapple how everything works in libp2p, but the duplicate/infinite peer:discovery events were confusing and I thought for sure I was doing something wrong. If I'm reading this correctly though, it sounds like it's just a flaw in the current design.

Recently, @raulk's proposed a peerstore v2 (which I'm assuming is synonymous with PeerBook, but just what it's called for the Go library, sorry if I'm wrong). He suggested making it an "autonomous agent inside the libp2p stack, capable of taking decisions and reacting to environment changes." A big part of that change would be emitting events about things happening within the PeerBook. With that in mind, It sounds like it might make more sense to centralize this logic inside the PeerBook.

One of the first changes we could make to the PeerBook is applying what you suggested here in the PeerBook instead of libp2p, making it the central point for announcing peer discovery. Basically, rather than changing all of the libraries to skip adding to the peer book, they could stay as is and the PeerBook would handle announcing new peers via EventEmitters that could be handled by libp2p.

I took a stab at just moving the emission of the peer:discovery event and it seems to eliminate duplicates.

• JustMaier/js-peer-book@a2165c3
• JustMaier@7f6eec9

[jacobheun]

@JustMaier yes I think this approach is the right way to go and fits in nicely with v2 of the peer store. One side effect of removing the duplicate events that we'll need to account for is that currently it also gives us peer redialing. For example, if we are leveraging Bootstrap peers and are below are peer threshold, if we get disconnected the duplicate event will cause us to reconnect. The current logic is by no means ideal, but something we'll need to account for nonetheless or we risk peers losing all their connections if they're not actively discovering the network. Connection tagging is the right counterpart to this, but we don't currently have this in js yet (it's planned as part of the Connection Manager v2 work, which is still in spec design).

[JustMaier]

Seems like there could be a lot of overlap between v2 peerstore and connection manager. Both would be playing a role in network maintenance, connection manager focused on resource allocation and peerstore on connection quality/confidence. After reading more about the connection manager, I wonder if peerstore really just needs to remain as storage with a few changes around what it stores, how it stores it, and the addition of event emitting. Then the connection manager could be the single entity that maintains network quality and resource distribution using the peerstore as the place to keep peer info, metrics, and settings.

So, until connection manager v2 and an updated peerstore, is the best way to handle the dupe peer:discovery events to check for IsConnected and write a custom ignore property to apply to peers that you don't want to connect to?

[jacobheun]

So, until connection manager v2 and an updated peerstore, is the best way to handle the dupe peer:discovery events to check for IsConnected and write a custom ignore property to apply to peers that you don't want to connect to?

Yes, this is what we currently do internally with the maybeConnect method if autoDial is enabled (it is by default). If you're disabling autoDial to control your own dialing logic this is a reasonable approach to do that.

[vasco-santos]

This should be done with the changes in PeerStore over the last year.