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: fix peer renewal, change Filter keep alive #2065

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

weboko
Copy link
Collaborator

@weboko weboko commented Jul 16, 2024

Problem

Peer renewal doesn't work as intended.

Current behavior - it renews a peer to a one with which light node has a connection already.
Expected behavior - renews to a peer which we don't have a connection.

This PR also adds decrease for Filter keep alive queries to 30 seconds as was proposed in one of the threads in waku-org/specs#18, and some code improvements.

Solution

Fix the behavior and add some improvements.

Notes

Tested in the dogfooding app - waku-org/lab.waku.org#68

@weboko weboko requested a review from a team as a code owner July 16, 2024 00:12
@@ -0,0 +1,22 @@
import type { Connection } from "@libp2p/interface";

export function selectConnection(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was moved from @waku/utils

@waku-org/js-waku-developers let's be cautious of not placing non public utils in @waku/utils

non public utils are those that are needed only by @waku/* packages family and/or test package.

Copy link

github-actions bot commented Jul 16, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 183.4 KB (+0.01% 🔺) 3.7 s (+0.01% 🔺) 18.6 s (-14.78% 🔽) 22.3 s
Waku Simple Light Node 183.53 KB (+0.06% 🔺) 3.7 s (+0.06% 🔺) 24.8 s (-0.68% 🔽) 28.4 s
ECIES encryption 23.12 KB (0%) 463 ms (0%) 10.5 s (+210.21% 🔺) 11 s
Symmetric encryption 22.58 KB (0%) 452 ms (0%) 4.7 s (-2.25% 🔽) 5.2 s
DNS discovery 72.49 KB (0%) 1.5 s (0%) 8.7 s (-51.92% 🔽) 10.2 s
Peer Exchange discovery 74.15 KB (0%) 1.5 s (0%) 17.4 s (+93.17% 🔺) 18.9 s
Local Peer Cache Discovery 67.68 KB (0%) 1.4 s (0%) 18 s (+1.39% 🔺) 19.4 s
Privacy preserving protocols 38.87 KB (0%) 778 ms (0%) 10.7 s (+20.76% 🔺) 11.5 s
Waku Filter 112.89 KB (+0.04% 🔺) 2.3 s (+0.04% 🔺) 23.3 s (+42.18% 🔺) 25.5 s
Waku LightPush 111.2 KB (-0.1% 🔽) 2.3 s (-0.1% 🔽) 20.8 s (+12.88% 🔺) 23 s
History retrieval protocols 111.73 KB (+0.11% 🔺) 2.3 s (+0.11% 🔺) 14.4 s (-26.06% 🔽) 16.6 s
Deterministic Message Hashing 7.29 KB (0%) 146 ms (0%) 3 s (+133.23% 🔺) 3.2 s

Copy link
Collaborator

@danisharora099 danisharora099 left a comment

Choose a reason for hiding this comment

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

I'm curious how the peer renewal failure was observed/debugged? Any failing test cases? Should we add new tests that would reproduce it?

@weboko
Copy link
Collaborator Author

weboko commented Jul 16, 2024

I'm curious how the peer renewal failure was observed/debugged? Any failing test cases? Should we add new tests that would reproduce it?

Peer renewal was working by dropping connection and then taking a peer from those with whom we have a connection. What we actually need to do is to get a peer to which we are not connected yet. Otherwise we need to always have connections to peers that we are not using in order to be able to use them as a replacement.

As for repro - you can do this by running dogfooding app now and seeing that at dome point there are no peers to be used for light push

@weboko weboko merged commit 00635b7 into master Jul 16, 2024
10 of 11 checks passed
@weboko weboko deleted the weboko/get-connected-peers branch July 16, 2024 16:35
@weboko weboko mentioned this pull request Jul 16, 2024
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