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

feat: Improve filter peer management based on shards #4463

Closed
1 task
chaitanyaprem opened this issue Dec 14, 2023 · 4 comments · Fixed by #4531
Closed
1 task

feat: Improve filter peer management based on shards #4463

chaitanyaprem opened this issue Dec 14, 2023 · 4 comments · Fixed by #4531
Assignees
Labels
status-waku-integ All issues relating to the Status Waku integration.

Comments

@chaitanyaprem
Copy link
Contributor

Problem

While going through filter code in status-go, i had noticed that the way filter subscriptions are done towards peers does not take sharding into consideration.

Referring to below code where filter peers are found, few observations that can be improved are

  1. When looking for filter peers, not sure why relay protocol ID is also checked for. If a peer supports filter subscribe, that should be sufficient right?
  2. The peer lookup doesn't consider shards supported by a peer. Which means it is possible that the selected peer does not support the required shard and hence filter subscribe might fail. This can be addressed in 2 ways, either use Automatic peer selection provided by go-waku by passing flag WithAutomaticPeerSelection, or add logic that would also consider shards and bucket peers as per shards.

func (mgr *FilterManager) findFilterPeers() []peer.ID {
allPeers := mgr.node.Host().Peerstore().Peers()
peers := make([]peer.ID, 0)
for _, peer := range allPeers {
protocols, err := mgr.node.Host().Peerstore().SupportsProtocols(peer, filter.FilterSubscribeID_v20beta1, relay.WakuRelayID_v200)

Note that point-2 may not impact as of now, because the fleet nodes support all shards. But as communities are created and assigned to different shards where various peers only support a subset of them, this will start causing issues.

Implementation

Explained approaches on how this can be implemented above.

Acceptance Criteria

  • Filter peers are to be selected including the shards into consideration
@jm-clius
Copy link

This is indeed a possible improvement, but note that some level of inefficiency can be tolerated here - i.e. if you try to subscribe to a filter service node that doesn't support the shard you're interested in, it will reject your request and you could simply continue to another one until you reach your desired level of redundancy. This is not unlike e.g. relay protocol being pruned by peers for various reasons (e.g. capacity) and having to continue trying to graft to discovered peers until it finds a peer that has slots available.

@chair28980 chair28980 added the status-waku-integ All issues relating to the Status Waku integration. label Dec 19, 2023
@kaichaosun kaichaosun self-assigned this Jan 4, 2024
@kaichaosun kaichaosun moved this from Todo to In Progress in Status Waku Integration & Chat Reliability Jan 4, 2024
@kaichaosun
Copy link
Contributor

  1. When looking for filter peers, not sure why relay protocol ID is also checked for. If a peer supports filter subscribe, that should be sufficient right?

My understanding is that when a client decides to subscribe a filter servicer, the filter service should have the capability to gossip messages, and the WakuRelayID_v200 is the protocol that identify such capability. Does it make sense?

  1. The peer lookup doesn't consider shards supported by a peer.

In go-waku code comments WithAutomaticPeerSelection is an option used to randomly select a peer from the peer store and quick go through the code, I don't see how WithAutomaticPeerSelection helps with the sharded peer lookup. Can you provide more guidance on it? @chaitanyaprem

@chaitanyaprem
Copy link
Contributor Author

  1. When looking for filter peers, not sure why relay protocol ID is also checked for. If a peer supports filter subscribe, that should be sufficient right?

My understanding is that when a client decides to subscribe a filter servicer, the filter service should have the capability to gossip messages, and the WakuRelayID_v200 is the protocol that identify such capability. Does it make sense?

Yes, but it doesn't have to use relay rather it can use another filter node for the same(which inturn does relay). But i guess for status use-case this is fine since the fleets are filter service nodes which also support relay.
This is more of an observation i had, which may not have any impact as of now.
In future, if someone wants to run a filter service node, without supporting relay that also is possible while running a Waku node. Then this situation would have to be handled.

@chaitanyaprem
Copy link
Contributor Author

In go-waku code comments WithAutomaticPeerSelection is an option used to randomly select a peer from the peer store and quick go through the code, I don't see how WithAutomaticPeerSelection helps with the sharded peer lookup. Can you provide more guidance on it? @chaitanyaprem

It not only selects a random peer from peer-store rather it filters the peers based on shards they support. Refer these snippets

https://github.com/waku-org/go-waku/blob/846183d515db42ac5338a64311888563efb281a3/waku/v2/peermanager/peer_selection.go#L62-L64

https://github.com/waku-org/go-waku/blob/846183d515db42ac5338a64311888563efb281a3/waku/v2/peermanager/peer_selection.go#L76-L92

If pubsub topics are passed to peerSelection then peers are selected based on pubsubTopics (which are made of cluster and a shard). But now that i think about it, maybe the selection criteria API can be enhanced to specify just shards itself and internally it can translate to pubsubTopics.

In case of status-go, since it already has a filterPeerManager there are two ways to go about it:

  1. Entirely rely on go-waku peer-manager to manage filter peers and use AutomaticPeerSelection with shard
  2. Or just add shard based peer maintenance logic in status-go.

Personally i think approach 1 is better, because it would reduce duplication of logic.
But I am not sure how much impact this will have on status-go.

Approach-2 would be easier to modify and test since it just needs to consider shard as well while doing going through peers in peerstore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status-waku-integ All issues relating to the Status Waku integration.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants