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

chore: use connected peers instead of discovered peers for protocols #1758

Merged
merged 4 commits into from
Jan 2, 2024

Conversation

danisharora099
Copy link
Collaborator

Problem

There is a problem in the peers we choose during a protocol request: we currently get discovered peers while we should be getting and using peers that we have an open connection with.

Solution

Use connected peers instead of discovered peers that might not be connected

Notes

Interesting how we didn't run into any problems with this until now

@danisharora099 danisharora099 requested a review from a team as a code owner December 14, 2023 19:40
Copy link

github-actions bot commented Dec 14, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 81.96 KB (0%) 1.7 s (0%) 2.4 s (+5.7% 🔺) 4 s
Waku Simple Light Node 261.34 KB (+0.03% 🔺) 5.3 s (+0.03% 🔺) 4.2 s (+44.37% 🔺) 9.4 s
ECIES encryption 31.94 KB (0%) 639 ms (0%) 1.7 s (+15.3% 🔺) 2.3 s
Symmetric encryption 31.93 KB (0%) 639 ms (0%) 1.2 s (-27.79% 🔽) 1.9 s
DNS discovery 123.17 KB (0%) 2.5 s (0%) 2.3 s (-25.52% 🔽) 4.8 s
Privacy preserving protocols 119.3 KB (-0.01% 🔽) 2.4 s (-0.01% 🔽) 1.9 s (-50.49% 🔽) 4.3 s
Light protocols 79.48 KB (+0.09% 🔺) 1.6 s (+0.09% 🔺) 2.1 s (-4.73% 🔽) 3.7 s
History retrieval protocols 78.38 KB (+0.09% 🔺) 1.6 s (+0.09% 🔺) 1.3 s (-19.38% 🔽) 2.8 s
Deterministic Message Hashing 5.92 KB (0%) 119 ms (0%) 405 ms (+151.04% 🔺) 523 ms

await tearDownNodes(nwaku, waku);
});

it("returns all connected peers that support the protocol", async function () {
Copy link
Member

Choose a reason for hiding this comment

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

Is there an easy way to have a discovered peer that's disconnected? Might be nice to have a test to ensure that peer isn't included

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hum can you clarify what you mean?
like for a peer that doesn't support the protocol but is discovered -- it should not be connected, right?

--
merging this for now, please feel free or let me know to add a test case for that as a followup

@danisharora099 danisharora099 merged commit 9983549 into master Jan 2, 2024
10 of 11 checks passed
@danisharora099 danisharora099 deleted the chore/fix-protocol-getPeers branch January 2, 2024 10:19
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