Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: improving get_peer_ids_by_protocol in libwaku #3109

Merged

Conversation

gabrielmer
Copy link
Contributor

@gabrielmer gabrielmer commented Oct 10, 2024

Description

Currently, libwaku's GET_PEER_IDS_BY_PROTOCOL returns the peer ids of peers that have an open libp2p stream associated with a given protocol.

However, what's most interesting for us is the capabilities of our connected peers and not which protocols are we actually using.

For example, for req-res protocols such as Store, most of the time we don't have an open stream associated with the Store protocol - which means that with the current approach GET_PEER_IDS_BY_PROTOCOL won't return anything.

Therefore, changing the approach so we return all our connected peers that support a given protocol, even if we don't have open streams for that protocol in specific.

Changes

  • improving GET_PEER_IDS_BY_PROTOCOL so it returns our connected peers with a given capability

Issue

#3039

Copy link

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3109

Built from f793b40

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful! Thanks for it! 💯

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok, but left a question to consider.

return ok(allPeerIDs.join(","))
let connectedPeers = waku.node.peerManager.wakuPeerStore
.peers($self[].protocol)
.filterIt(it.connectedness == Connected)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just asking due to my missing knowledge here.
What is the aim of this req? Would not we extend the list with also CanConnect state peers?

+1: I learned that peers gathered with PX won't get protocols filled until they are not called. So maybe they are in a kind of stale state in peer manager. We know their capabilities (advertised) but we cannot filter them by protocol.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of the request is to get the peers we're connected to that support a given protocol. If for example we want to do a Store query, the first thing we would do is see if we're connected to a Store node and send them a Store query.

I think we can also separately, when needed, add a request that includes peers in the peer manager in CanConnect state, but in this case we want the information of already connected nodes.

I believe that the issue with PX, correct me if I'm wrong, is that we get addresses of peers that we store, but we don't really connect to them. Once we're connected to them, we get the protocols when performing Identify, and they get filled.

So in this case for example, if we are connected to a peer we found via PX, it will have the protocol filled, as we are already connected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it, thank you.
About PX. I think this issue there that we will never select such peers as potential service peers, because they have no protocols filled due not connected. But isn't that a trap of 22?
In js-waku they inherently fill the protocol derived from capability, so those peers get selectable for service.
This is not future proof but inevitably a better option IMHO.
Or is there any mechanism that will consider auto call those peers from PX randomly?

Copy link
Contributor Author

@gabrielmer gabrielmer Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm good question, I think it does make sense to fill the protocols supported according to ENRs capabilities until we can connect to the peer and update the info when performing Identify. I guess it's better to use the capability information rather than ignore. If we look at protocols when selecting peers then for sure it needs to have some value.

Although we could also prioritize the ones for which we have already performed identify, and if we can't connect to any of those, then try randomly with peers regardless of what they advertised.

For sure @jm-clius can give us valuable input for this case :)

On the other hand, I think we do have a mechanism that considers PX peers randomly:

let notConnectedPeers = pm.wakuPeerStore.getDisconnectedPeers()

In our connectivity loop that we run periodically to connect to peers, we don't take a look at protocols supported, we just check that we have them in the peer store, disconnected and not in backoff period. So in that case, we would try to connect to peers found in PX, although the initial connection would be via Relay.

But not completely sure on what's the logic like for choosing service nodes when one is the client, I guess we can't rely on Relay connection mechanisms and we need to have a way of choosing the service node to query. For sure you know better than me about this :)

Whichever algorithm we use, we can create a service for libwaku. I did this one in a very simplistic way to start, but if there's a specific algorithm to choose service peers, then it would make sense to add to libwaku :))

@gabrielmer gabrielmer merged commit ed0ee5b into master Oct 11, 2024
13 of 14 checks passed
@gabrielmer gabrielmer deleted the chore-libwaku-returning-peers-by-mounted-protocols branch October 11, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants