findProviders should return PeerIDs

[vasco-santos]

@Gozala commented on Wed Mar 10 2021

findProviders returns { id: PeerId, multiaddrs: Multiaddr[] }

https://github.com/libp2p/js-libp2p/blob/8e1fc78353cba194c86102dafcd68373b3f9a796/src/content-routing/index.js#L43-L52

While bitswap seems to assume it returns PeerIds instead. For more context please see

https://github.com/ipfs/js-ipfs-bitswap/pull/261/files#r587862380

---

@vasco-santos commented on Thu Mar 11 2021

The link does not properly route me to the correct place I think.

This returns an async iterable if {{ id: PeerId, multiaddrs: Multiaddr[] }. In bitswap it seems to return @returns {AsyncIterable<Provider>} where Provider is:

 * @typedef {Object} Provider
 * @property {PeerId} id
 * @property {Multiaddr[]} multiaddrs

I did not understand the inconsistency here. Let me know what I am missing

---

@Gozala commented on Thu Mar 11 2021

Here is the snapshot of the conversation in that pull

As per @achingbrain comment

libp2p.contentRouting.findProviders should return only PeerIDs.

The idea is that libp2p manages the actual multiaddrs internally and we don't worry about them at this level.

Maybe it's all correct, because types seem to align now ipfs/js-ipfs-bitswap#304

---

@vasco-santos commented on Thu Mar 11 2021

I did not see that comment. So, you mean that findProviders should return {AsyncIterable<{peerId: PeerId>}

Ok, so @achingbrain is right in the sense that libp2p should handle internally the multiaddrs storage in the AddressBook, which is what we currently do.

The interface for contentRouting and peerRouting returns {AsyncIterable<{peerId: PeerId, multiaddrs: Multiaddr[]>}

We should discuss this further, I would like to keep the multiaddrs to be returned. Currently, libp2p will auto-dial when multiaddrs are added to the PeerStore (and is a new peer). But this will hopefully be removed later on in favour of the connection manager overhaul with proactive dial strategies. With this in mind, it is worth to return the multiaddrs to enable the user to be able to select a specific multiaddr if the user wants to do it. The drawback here is that we might know other multiaddrs from the peer that will not be returned (only the router provider records are returned).

Ideally, users will just dial with peerId and libp2p will handle everything given that we in theory will have addresses. But, we might also discover peers that are not advertising any multiaddr, so we will have no multiaddrs for it, and the user can try to dial it and libp2p will fail with "no addresses for the peer", which will be odd.

With all things considered, and as bitswap is not blocked on this, I would punt a decision here for after the connManager work + rendezvous/DHT

---

@vasco-santos commented on Thu Mar 11 2021

I will move the issue to the interfaces repo, as this will mean changing the interface

[Gozala]

@vasco-santos thanks for elaboration, I think it would be good to at least capture

it is worth to return the multiaddrs to enable the user to be able to select a specific multiaddr if the user wants to do it. The drawback here is that we might know other multiaddrs from the peer that will not be returned (only the router provider records are returned).

In the findProviders method description here

https://github.com/libp2p/js-libp2p/blob/8e1fc78353cba194c86102dafcd68373b3f9a796/src/content-routing/index.js#L43-L52

With such context it would have been a lot more clear why things are the way they are.

[vasco-santos]

Would you like to PR that? findPeer would probably need the same

[vasco-santos]

and the user can try to dial it and libp2p will fail with "no addresses for the peer", which will be odd.

Bear in mind, that if a user does libp2p.dial(peerId) and we have no addresses for the peer, we should try to do findPeer if we have no addresses using DHT/Delegates.

We currently don't do this as we had no DHT, but as we now use delegates by default in js-ipfs, it might be good to add that fallback in libp2p's dialer

[Gozala]

With this in mind, it is worth to return the multiaddrs to enable the user to be able to select a specific multiaddr if the user wants to do it. The drawback here is that we might know other multiaddrs from the peer that will not be returned (only the router provider records are returned).

Should users instead query address book to get the addresses in those cases instead ? That way it would also lead to following behavior we seem to want

Ideally, users will just dial with peerId and libp2p will handle everything given that we in theory will have addresses.

[Gozala]

Bear in mind, that if a user does libp2p.dial(peerId) and we have no addresses for the peer, we should try to do findPeer if we have no addresses using DHT/Delegates.

We currently don't do this as we had no DHT, but as we now use delegates by default in js-ipfs, it might be good to add that fallback in libp2p's dialer

I feel like there should be two different methods, one that fails if no address is available and other that tries to discover a peer addresses and then dial. I expect that different use cases would demand different behavior and it might be best to put users in the position where they consider which one fits theirs as opposed to discover at some point that things don't exactly do an ideal thing for their use case. E.g we could have

1. connectTo(peer, ...) that attempts to discover and connect to a peer.
2. dialPeer(peer, ...) that attempts to dail peer, but fails if no address is known.

Or better yet might be to do something like the following (which is a lot more code, but is a lot more clear on what's going under the hood)

try {
  await dialPeer(peerId)
} catch (error) {
  if (haveNoAddress(error)) {
    await discoverPeer(peerId)
    await dialPeer(peerId)
  }
}

[achingbrain]

This is correct, findProviders returns PeerInfo objects, the typings hopefully make this clear.