-
Notifications
You must be signed in to change notification settings - Fork 247
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_: no peers available supporting LightPush protocol after network restored from disabled state #6153
base: develop
Are you sure you want to change the base?
Conversation
…restored from disabled state
Jenkins Builds
|
@@ -1707,6 +1707,10 @@ func (w *Waku) handleNetworkChangeFromApp(state connection.State) { | |||
|
|||
func (w *Waku) ConnectionChanged(state connection.State) { | |||
isOnline := !state.Offline | |||
if isOnline && !w.onlineChecker.IsOnline() { // we are online and we were offline before |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the duplicated predicate logic and the comment, one idea is to create a predicate function, something like:
modified wakuv2/waku.go
@@ -1705,9 +1705,13 @@ func (w *Waku) handleNetworkChangeFromApp(state connection.State) {
}
}
+func (w *Waku) isGoingOnline(state connection.State) bool {
+ return !state.Offline && !w.onlineChecker.IsOnline()
+}
+
func (w *Waku) ConnectionChanged(state connection.State) {
isOnline := !state.Offline
- if isOnline && !w.onlineChecker.IsOnline() { // we are online and we were offline before
+ if w.isGoingOnline(state) {
//TODO: analyze if we need to discover and connect to peers for relay.
w.discoverAndConnectPeers()
}
@@ -1720,8 +1724,7 @@ func (w *Waku) ConnectionChanged(state connection.State) {
w.handleNetworkChangeFromApp(state)
} else {
// for lightClient state update and onlineChange is handled in filterManager.
- // going online
- if isOnline && !w.onlineChecker.IsOnline() {
+ if w.isGoingOnline(state) {
select {
case w.goingOnline <- struct{}{}:
default:
The root-cause for issues status-im/status-mobile#21452 / status-im/status-mobile#21394 could not be due to peers not being connected after coming online. In light-clients peers can be connected as required i.e when we want to send messages via lightpush or subscribe to a filter to receive messages. And store nodes are pre-configured and they get connected to as and when we want to do store queries automatically. Hence the discovery of peers and connecting to them need not be done explicitly after coming back online. So far one thing that i had noticed is that the peers that we discover may not be reachable and hence it takes time to make successful connections for filter and lightpush. In order to fix this i had worked on waku-org/go-waku#1244 which would remove peers from local store if they are not reachable. This doesn't solve the problem completely but atleast prevents the node from trying to reuse peers to which connection has failed recently. @qfrank Can you give me the reasoning as to how you identified this as the root-cause for the issues? |
I identified it by debug status backend server ... what I found was: when user back to online from offline(keep offline before login), there's no peers available supporting LightPush protocol ( |
BTW, I found |
ah...interesting. got it since bootstrap nodes were not reachable during login and then they were post network is back online. Got it, this fix would definitely address the issue. Thanks for this info, i will start using status backend server to simulate these scenarios. Any instructions on how to use and run it? |
This is also a good point, in case of mobile we can use network connectivity available from device to determine network status and not just peer connectivity alone. Is there a way to access this info from this layer? That way we can use either The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@chaitanyaprem, do you know if we considered other ways to reliably detect network connectivity in desktop environments? I know there's no multiplatform way to solve this problem, but for such an important problem I would lean on platform specific solutions, going from most reliable to least reliable or a combo of 2 solutions (where checking the number of peers would be somewhere in the middle I guess). Probably such solutions would be better implemented outside the Waku's umbrella (?) @osmaczko @igor-sirotin do you remember past discussions about network connectivity for the desktop app? |
fix relate mobile issues: 21452 / 21394, and might also fix part(missing messages) of 21172
relate mobile PR