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: use automatic peer selection for filter. #4531

Merged
merged 3 commits into from
Jan 12, 2024
Merged

Conversation

kaichaosun
Copy link
Contributor

Use automatic peer selection from waku helps reduce duplicate logic and increase performance when static shards apply.

Important changes:

  • use filter option WithAutomaticPeerSelection when subscribe

Closes #4463

@status-im-auto
Copy link
Member

status-im-auto commented Jan 5, 2024

Jenkins Builds

Click to see older builds (19)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 48d9a1e #1 2024-01-05 06:49:48 ~3 min linux 📦zip
✔️ 48d9a1e #1 2024-01-05 06:51:24 ~4 min ios 📦zip
✔️ 48d9a1e #1 2024-01-05 06:52:00 ~5 min android 📦aar
✖️ 48d9a1e #1 2024-01-05 07:07:57 ~21 min tests 📄log
✖️ 48d9a1e #2 2024-01-05 07:28:36 ~19 min tests 📄log
✖️ 48d9a1e #3 2024-01-05 07:51:38 ~18 min tests 📄log
✔️ a306492 #2 2024-01-08 03:58:39 ~1 min linux 📦zip
✔️ a306492 #2 2024-01-08 03:59:13 ~2 min android 📦aar
✔️ a306492 #2 2024-01-08 04:02:21 ~5 min ios 📦zip
✖️ a306492 #4 2024-01-08 04:34:00 ~37 min tests 📄log
✔️ 1795605 #3 2024-01-09 02:36:16 ~2 min android 📦aar
✔️ 1795605 #3 2024-01-09 02:37:08 ~3 min linux 📦zip
✔️ 1795605 #3 2024-01-09 02:37:39 ~3 min ios 📦zip
✖️ 1795605 #5 2024-01-09 02:55:04 ~21 min tests 📄log
✖️ 1795605 #6 2024-01-09 09:10:18 ~19 min tests 📄log
✖️ 1795605 #7 2024-01-09 12:52:27 ~18 min tests 📄log
✖️ 1795605 #9 2024-01-10 04:49:41 ~21 min tests 📄log
✖️ 1795605 #10 2024-01-10 05:22:24 ~19 min tests 📄log
✖️ 1795605 #11 2024-01-10 06:27:34 ~19 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ f8d2249 #4 2024-01-10 09:58:04 ~2 min android 📦aar
✔️ f8d2249 #4 2024-01-10 09:59:04 ~3 min linux 📦zip
✔️ f8d2249 #4 2024-01-10 09:59:14 ~3 min ios 📦zip
✖️ f8d2249 #12 2024-01-10 10:14:54 ~19 min tests 📄log
✖️ f8d2249 #13 2024-01-10 10:37:15 ~13 min tests 📄log
✖️ f8d2249 #14 2024-01-10 14:56:38 ~20 min tests 📄log
✖️ f8d2249 #15 2024-01-11 07:18:53 ~19 min tests 📄log
✔️ de02268 #5 2024-01-11 08:11:30 ~1 min linux 📦zip
✔️ de02268 #5 2024-01-11 08:12:59 ~3 min ios 📦zip
✖️ de02268 #16 2024-01-11 08:14:09 ~4 min tests 📄log
✔️ de02268 #5 2024-01-11 08:15:13 ~5 min android 📦aar
✔️ de02268 #17 2024-01-11 09:07:52 ~38 min tests 📄log

@vitvly
Copy link
Contributor

vitvly commented Jan 5, 2024

@kaichaosun @chaitanyaprem been trying to recall why exactly WithPeer was used in FilterManager, instead of WithAutomaticPeerSelection. Originally implemented here: #4020.

I think that it was planned to improve FilterManager.findPeerCandidate function, so that it would take distribute distinct filter subs among different peers better. Right now it's random, similarly to what WithAutomaticPeerSelection does. This results in FilterManager.resubscribe being not too smart when trying to establish MinPeersForFilter connections for any particular filter, as it might select the same peer twice, i think.

So maybe another option to try is improving findPeerCandidate, so that:

  • it does not pick the same peer for the same filter twice
  • it sorts FilterManager.peers somehow by priority. Before Add filter debugging logs and map locking #4020 there used to be a Waku.filterPeerDisconnectMap, which had put recently disconnected peers to the end of peers list. Definitely there can be a much better logic for peer sorting. Apologies if there is already one in go-waku that i'm not aware of.

Alternatively, go-waku's automatic peer selection could be modified in order to take into account the above two points.

@kaichaosun
Copy link
Contributor Author

kaichaosun commented Jan 8, 2024

The mgr.peers is maintained as the candidates for connecting to a filter service node, both the connected and disconnected peers should be removed from the candidates. It originally populated from peer store with protocol support Filter and relay.
mgr.subscribeToFilter is now changed to not use goroutine to avoid subscribe to the same peer.

For sorting FilterManager.peers by priority, this is another story, and I see it can be used not only by filter, should we design and spec it first before implement it? Go-waku already provides peer selection based on delay, I believe there could be other dimensions of priority besides delay. (use delay based selection probably not a good idea when we are not sure about the geolocation of filter services and application users.)

@vitvly @chaitanyaprem

@chaitanyaprem
Copy link
Contributor

For sorting FilterManager.peers by priority, this is another story, and I see it can be used not only by filter, should we design and spec it first before implement it? Go-waku already provides peer selection based on delay, I believe there could be other dimensions of priority besides delay. (use delay based selection probably not a good idea when we are not sure about the geolocation of filter services and application users.)

You are right. This is something that will have to be done eventually(but for now it makes sense to keep it simple since we are dealing with only fleet nodes which are not of large number).
This would require more thought as you had mentioned. There is already a similar feature planned on go-waku waku-org/go-waku#771 which will be taken up sometime this year based on the need.

For now, to keep it simple we can use RTT probably for this.

@kaichaosun
Copy link
Contributor Author

For now, to keep it simple we can use RTT probably for this.

Do you mean use SelectPeerWithLowestRTT for filter peer selection? This will probably add too many loads to the filter service nodes if most of the users are in same region (like Europe). @chaitanyaprem

@chaitanyaprem
Copy link
Contributor

For now, to keep it simple we can use RTT probably for this.

Do you mean use SelectPeerWithLowestRTT for filter peer selection? This will probably add too many loads to the filter service nodes if most of the users are in same region (like Europe). @chaitanyaprem

This would work better in a more decentralized operator environment where there are more service nodes which i think would be how the status communities would function in the long run.
But you are right that if we start using this with current fleet nodes only, this won't scale well. This can be taken up at a later stage on the RTT logic is complete and we have some data showing how much traffic it adds.

// Create sub placeholder in order to avoid potentially too many subs
tempID := uuid.NewString()
subs[tempID] = nil
mgr.subscribeToFilter(filterID, tempID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain a bit more on why removing goroutine helps with avoiding subscribing to the same peer? Having all calls to go-waku's FilterLightnode asynchronous is important for the functionality of FilterManager, as far as i recall - some of these subscribes/unsubscribes might take a long time.

Copy link
Contributor Author

@kaichaosun kaichaosun Jan 10, 2024

Choose a reason for hiding this comment

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

Added back goroutine and depend on go-waku for dedupliate subscribing to same peer.

@kaichaosun kaichaosun merged commit 0c474bb into develop Jan 12, 2024
7 checks passed
@kaichaosun kaichaosun deleted the filter-shards branch January 12, 2024 07:09
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.

feat: Improve filter peer management based on shards
4 participants