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: use the lowest latency peer for protocols #1540

Merged
merged 15 commits into from
Sep 8, 2023

Conversation

danisharora099
Copy link
Collaborator

Problem

During selection of peers to be used for light protocols, the selection currently happens randomly. This is not ideal, as we might potentially be communicating with a higher latency node, while an option to use a faster node might exist.

Ref: #1465 #1497

Solution

  1. ping all peers and select the peer with the lowest latency.
  2. fallback to randomization (shouldn't happen)

@danisharora099 danisharora099 marked this pull request as ready for review September 7, 2023 08:24
@danisharora099 danisharora099 requested a review from a team as a code owner September 7, 2023 08:24
@github-actions
Copy link

github-actions bot commented Sep 7, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 28.79 KB (+0.2% 🔺) 576 ms (+0.2% 🔺) 1.8 s (-9.71% 🔽) 2.4 s
Waku Simple Light Node 309.78 KB (+0.03% 🔺) 6.2 s (+0.03% 🔺) 5 s (+27.25% 🔺) 11.2 s
ECIES encryption 28.68 KB (0%) 574 ms (0%) 1.3 s (-30.89% 🔽) 1.8 s
Symmetric encryption 28.68 KB (0%) 574 ms (0%) 965 ms (-43.33% 🔽) 1.6 s
DNS discovery 118.59 KB (0%) 2.4 s (0%) 2.9 s (-30.91% 🔽) 5.3 s
Privacy preserving protocols 122.6 KB (-0.01% 🔽) 2.5 s (-0.01% 🔽) 3 s (-5.71% 🔽) 5.5 s
Light protocols 29.22 KB (+0.95% 🔺) 585 ms (+0.95% 🔺) 1.6 s (-13.71% 🔽) 2.2 s
History retrieval protocols 28.34 KB (+0.94% 🔺) 567 ms (+0.94% 🔺) 1.3 s (+6.34% 🔺) 1.9 s
Deterministic Message Hashing 5.64 KB (0%) 113 ms (0%) 447 ms (-26.98% 🔽) 560 ms

@@ -340,7 +340,11 @@ export class ConnectionManager
void (async () => {
const peerId = evt.detail;

this.keepAliveManager.start(peerId, this.libp2p.services.ping);
this.keepAliveManager.start(
peerId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we can simplify and pass ‘libp2p’ directly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suggest passing ping and peerStore individually to KeepAliveManager instead of the entire libp2p object because:

  • by passing only what's needed, it's clear what KeepAliveManager depends on. This makes our code more readable and maintainable.
  • this approach reduces the dependency on the entire libp2p structure. If libp2p evolves, it minimizes the ripple effect on KeepAliveManager.
  • it follows best practices like the Single Responsibility and Interface Segregation Principles

Let me know your thoughts!

Copy link
Collaborator

Choose a reason for hiding this comment

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

SOLID is not applied here because it is more about how entity/object should be designed and not what it should be provided with. You might need thousand dependencies to do single thing and it is up to devs how easier/cleaner they should be provided.

My rule of thumb for dependencies - it's easier to grow the number of entities than methods.

And again, it is a nit, so feel free to keep the way you think reasonable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Understanding it's a nit, glad we're discussing to be on similar pages around these philosophies if we're maintaining big codebases :)

I understand that SOLID primarily focuses on the design and structure of classes. However, the Dependency Inversion Principle directly addresses how dependencies should be managed -- I interpret it as our classes are decoupled and that we're injecting only the necessary dependencies for this use case
Perhaps you have a different interpretation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it's exactly about necessary dependencies, and the necessary dependency here is libp2p and not its methods.

@danisharora099 danisharora099 merged commit 6f09fbf into master Sep 8, 2023
9 of 10 checks passed
@danisharora099 danisharora099 deleted the feat/lowest-latency-peer branch September 8, 2023 16:06
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