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

fix: add option to specify preferred peers for filter and use bad peer removal logic only for lightpush and filter #1244

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chaitanyaprem
Copy link
Collaborator

@chaitanyaprem chaitanyaprem commented Oct 18, 2024

Description

Bunch of fixes and changes that help with offline state.

Changes

  • fix: filter stats mismatch and add bad peer check for light mode #1241 had some side-effects where-in store nodes were being removed from peerstore which we do not want because in status store nodes are fixed (i.e fleet nodes).
  • Apart from that i had noticed that when node is offline, connections are being triggered which result in failures and many good peers such as fleet nodes are getting removed from peer-store.
  • Added a more strict check and now migrated the bad peer removal logic to only be done from lightpush and filter and that too based on specific errors to avoid such side-effects.
  • Found an issue with offline to online state change handling in filter-manager where filter ping to existing subs will never happen.
  • Added a check to filter ping error handling in case node is already offline so that sub are not cleared
  • Added functionality to specify preferred-peers for filter. One of the peers used for filter subscription out of n will be randomly chosen from list of preferred peers.
  • Added a fix for missing messages to stop it when node goes offline in order to reduce backlog it accumulates and never being able to catch-up.

Tests

Being dogfooded status-im/status-mobile#21172 for various network disconnection/switch scenarios.

@chaitanyaprem chaitanyaprem changed the title not to merge: pr to test filter changes fix: pr to test filter changes (not to merge) Oct 18, 2024
@status-im-auto
Copy link

status-im-auto commented Oct 18, 2024

Jenkins Builds

Click to see older builds (9)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 8842d00 #1 2024-10-18 10:54:54 ~1 min unknown 📄log
✔️ 103d26c #2 2024-10-23 11:19:40 ~2 min unknown 📄log
✔️ 4e3d73c #3 2024-10-24 09:38:27 ~2 min unknown 📄log
✔️ be7eb68 #4 2024-10-24 09:57:25 ~3 min unknown 📄log
✔️ 5322d34 #5 2024-10-24 10:07:16 ~2 min unknown 📄log
✔️ 484e141 #6 2024-10-29 10:45:05 ~2 min unknown 📄log
✔️ 9cdf9ec #7 2024-10-29 11:40:07 ~2 min unknown 📄log
✔️ 213d382 #8 2024-11-05 17:48:44 ~2 min unknown 📄log
✔️ aadfbd6 #9 2024-11-09 10:32:46 ~2 min unknown 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 26bbb69 #10 2024-11-29 04:53:40 ~1 min unknown 📄log
✔️ 313c6bf #11 2024-12-02 01:49:54 ~2 min unknown 📄log

@chaitanyaprem chaitanyaprem force-pushed the test/filter-bad-peers branch 3 times, most recently from be7eb68 to 5322d34 Compare October 24, 2024 10:04
@chaitanyaprem chaitanyaprem changed the title fix: pr to test filter changes (not to merge) fix: add option to specify preferred peers for filter and use bad peer removal logic only for lightpush and filter Nov 29, 2024
@chaitanyaprem chaitanyaprem marked this pull request as ready for review November 29, 2024 04:56
params.peersToExclude = make(peermanager.PeerSet)
}
//exclude peers that are preferredpeers so that they don't get selected again.
if _, ok := params.peersToExclude[p]; !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe selectedPeers are the peers that we want to request, put selectedPeers further in another param doesn't make sense to me, and the naming peersToExclude further confuses me.
You probably want an internal state (not from params) like peersAlreadySelected to help selecting the peers?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants