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: don't dial discovered peers if have already been attempted dial #1657

Merged
merged 9 commits into from
Oct 20, 2023

Conversation

danisharora099
Copy link
Collaborator

@danisharora099 danisharora099 commented Oct 12, 2023

Problem

When we discover new peers, a dial attempt is fired. If this discovery is emitted multiple times, it currently does not check if there is an active dial attempt for this peer, or if we've already tried to dial this peer before.

This is unwanted behaviour as we might trigger dial attempts for peer that have already been attempted.

Solution

Do not dial peers if they have already been attempted dial before (will be handled with reconnections), or if they have a dial attempt in progress

Notes

@danisharora099 danisharora099 requested a review from a team as a code owner October 12, 2023 09:20
@github-actions
Copy link

github-actions bot commented Oct 12, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 83.87 KB (+0.08% 🔺) 1.7 s (+0.08% 🔺) 1.7 s (-35.69% 🔽) 3.4 s
Waku Simple Light Node 264.11 KB (+0.03% 🔺) 5.3 s (+0.03% 🔺) 4 s (-5.2% 🔽) 9.3 s
ECIES encryption 79.48 KB (0%) 1.6 s (0%) 2.8 s (-2.15% 🔽) 4.4 s
Symmetric encryption 79.49 KB (0%) 1.6 s (0%) 2.8 s (+2.73% 🔺) 4.4 s
DNS discovery 111.24 KB (0%) 2.3 s (0%) 3.1 s (-13.53% 🔽) 5.3 s
Privacy preserving protocols 131.8 KB (0%) 2.7 s (0%) 3.7 s (-7.6% 🔽) 6.3 s
Light protocols 81.5 KB (0%) 1.7 s (0%) 1.6 s (-36.58% 🔽) 3.2 s
History retrieval protocols 80.55 KB (0%) 1.7 s (0%) 1.8 s (-2.43% 🔽) 3.4 s
Deterministic Message Hashing 5.65 KB (0%) 113 ms (0%) 453 ms (+124.31% 🔺) 566 ms

);
await delay(100);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see it was before here - but how do you think - is it possible to avoid?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved this out of the loop, right before we check the call count.
We need to add a little delay before we check for the call count to ensure the async operations within have completed.

Also refactored the test to check for callCount to be >= discovery events fired instead of callCount === events emitted as attemptDial should take care of the redundant cases that may arise from dialPeerStorePeers

@danisharora099 danisharora099 merged commit 1892f50 into master Oct 20, 2023
10 of 11 checks passed
@danisharora099 danisharora099 deleted the feat/peer-mgmt-dont-dial branch October 20, 2023 12:46
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.

2 participants