feat: improve peer manager and re-integrate to light push #2191

weboko · 2024-10-20T23:10:57Z

Problem

TBD

Solution

TBD

Notes

Resolves feat: improve peer manager and re-integrate to light push #2186

…ionManager, remove not needed code

github-actions · 2024-10-20T23:14:56Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
Waku node	84.53 KB (-2.17% 🔽)	1.7 s (-2.17% 🔽)	2.7 s (-21.9% 🔽)	4.4 s
Waku Simple Light Node	134.9 KB (-1.5% 🔽)	2.7 s (-1.5% 🔽)	3 s (-14.09% 🔽)	5.7 s
ECIES encryption	22.88 KB (0%)	458 ms (0%)	1.2 s (+1.39% 🔺)	1.7 s
Symmetric encryption	22.37 KB (0%)	448 ms (0%)	923 ms (-21.12% 🔽)	1.4 s
DNS discovery	70.49 KB (-0.17% 🔽)	1.5 s (-0.17% 🔽)	1.8 s (-35.1% 🔽)	3.2 s
Peer Exchange discovery	71.47 KB (-0.53% 🔽)	1.5 s (-0.53% 🔽)	1.8 s (-18.62% 🔽)	3.2 s
Local Peer Cache Discovery	65.08 KB (-0.13% 🔽)	1.4 s (-0.13% 🔽)	2 s (-11.12% 🔽)	3.3 s
Privacy preserving protocols	76.14 KB (+0.22% 🔺)	1.6 s (+0.22% 🔺)	2.7 s (+82.67% 🔺)	4.2 s
Waku Filter	78.46 KB (-3.08% 🔽)	1.6 s (-3.08% 🔽)	2.4 s (+2.51% 🔺)	3.9 s
Waku LightPush	75.15 KB (-0.7% 🔽)	1.6 s (-0.7% 🔽)	3 s (-1.79% 🔽)	4.5 s
History retrieval protocols	75.25 KB (-3.17% 🔽)	1.6 s (-3.17% 🔽)	2.3 s (-9.96% 🔽)	3.8 s
Deterministic Message Hashing	7.39 KB (0%)	148 ms (0%)	533 ms (+182.67% 🔺)	681 ms

…s, add getPeers for IWaku

danisharora099

Nice PR! 🚀

Lots happening here.
A few pointers from my side:

Are we planning to address the TODOs part of this PR, or follow up?
Do we need to introduce newer tests as we delete the functionalities and respective spec tests?
How does the peer management work with this PR, considering the overhaul of BaseProtocol, PeerManager and ConnectionManager? Is it stable?

danisharora099 · 2024-11-27T08:11:10Z

packages/core/src/index.ts


 export { getHealthManager } from "./lib/health_manager.js";

-export { KeepAliveManager } from "./lib/keep_alive_manager.js";


why are we deleting the export for KeepAliveManager?

danisharora099 · 2024-11-27T08:12:11Z

packages/core/src/lib/base_protocol.ts

-    return getPeersForProtocol(this.components.peerStore, [this.multicodec]);
-  }
-
-  public async connectedPeers(): Promise<Peer[]> {


Do we still have access to this utility function from elsewhere?
Basically getting the connected peers for a particular protocol

danisharora099 · 2024-11-27T08:14:59Z

packages/core/src/lib/connection_manager/connection_manager.ts

-    relay?: IRelay,
-    options?: Partial<ConnectionManagerOptions>
-  ) {
+  public constructor(options: ConnectionManagerConstructorOptions) {


confirming that we are shifting out of the singleton pattern?

danisharora099 · 2024-11-27T08:16:21Z

packages/core/src/lib/connection_manager/connection_manager.ts

+    return peers
+      .filter((p) => !!p)
+      .filter((p) => (codec ? (p as Peer).protocols.includes(codec) : true))
+      .sort((left, right) => getPeerPing(left) - getPeerPing(right)) as Peer[];


we should probably abstract this logic into a private helper to differentiate functionalities, something like:

return filterPeersByProtocols(peers).sortPeersByLatency

reference:

public async getPeers( { numPeers, maxBootstrapPeers }: { numPeers: number; maxBootstrapPeers: number; } = { maxBootstrapPeers: 0, numPeers: 0 } ): Promise<Peer[]> { // Retrieve all connected peers that support the protocol & shard (if configured) const allAvailableConnectedPeers = await this.connectedPeers(); // Filter the peers based on discovery & number of peers requested const filteredPeers = filterPeersByDiscovery( allAvailableConnectedPeers, numPeers, maxBootstrapPeers ); // Sort the peers by latency const sortedFilteredPeers = await sortPeersByLatency( this.components.peerStore, filteredPeers ); if (sortedFilteredPeers.length === 0) { this.log.warn( "No peers found. Ensure you have a connection to the network." ); } if (sortedFilteredPeers.length < numPeers) { this.log.warn( `Only ${sortedFilteredPeers.length} peers found. Requested ${numPeers}.` ); } return sortedFilteredPeers; }

danisharora099 · 2024-11-27T08:19:56Z

packages/interfaces/src/connection_manager.ts

@@ -63,7 +85,8 @@ export interface IConnectionStateEvents {

 export interface IConnectionManager
  extends TypedEventEmitter<IPeersByDiscoveryEvents & IConnectionStateEvents> {
-  configuredPubsubTopics: PubsubTopic[];
+  pubsubTopics: PubsubTopic[];
+  getConnectedPeers(codec?: string): Promise<Peer[]>;


let's use the Protocols enum instead of the codec string here -- easier way to get information from the function without expecting the knowledge of exact codec (we already export the Protocols enum)

danisharora099 · 2024-11-27T08:24:05Z

packages/sdk/src/protocols/light_push/light_push.spec.ts

+    } as unknown as ConnectionManager,
+    {} as unknown as PeerManager,
+    options.libp2p


why doesn't typescript comply here/why does it expect an unknown typecase? :(

danisharora099 · 2024-11-27T08:24:40Z

packages/sdk/src/protocols/peer_manager.spec.ts

-
-import { PeerManager } from "./peer_manager.js";
-
-describe("PeerManager", () => {


should we add new tests for the PeerManager?

danisharora099 · 2024-11-27T08:25:57Z

packages/sdk/src/protocols/peer_manager.ts

 import { Logger } from "@waku/utils";
-import { Mutex } from "async-mutex";


were you able to validate the usage of manual locking instead of using the mutexes lib works well?

danisharora099 · 2024-11-27T08:26:29Z

packages/sdk/src/protocols/store/index.ts

-      })
-    )[0];
+    const peers = await this.peerManager.getPeers();
+    const peer = peers[0];


nit: let's add a comment here that store only uses one peer

danisharora099 · 2024-11-27T08:27:19Z

packages/sdk/src/reliability_monitor/receiver.ts

-    this.addLibp2pEventListener("peer:disconnect", (evt) => {
-      const peerId = evt.detail;
-      if (this.getPeers().some((p) => p.id.equals(peerId))) {
-        void this.renewAndSubscribePeer(peerId);
-      }
-    });
-  }


apologies if i missed it, how would renewal happen on a peer disconnection now?

weboko added 5 commits October 19, 2024 17:02

up lock

c93b58e

make ConnectionManager use ctor

7efbd26

reform connection manager configurations

a6dd499

remove log param from peerManager

e897d5c

make PeerManager use only ConnectionManager, move getPeers to Connect…

e1813bc

…ionManager, remove not needed code

weboko mentioned this pull request Oct 20, 2024

feat: improve peer manager and re-integrate to light push #2189

Closed

weboko added 8 commits October 21, 2024 01:34

remove allPeers and connectedPeers from BaseProtocolCore, update test…

9bdc2af

…s, add getPeers for IWaku

use only one peerManager from Waku object

2edd856

remove IBaseProtocolSDK and merge with PeerManager

7ae3b91

re-implement peerManager, remove ProtocolUseOptions

d0c6905

merge with master

1eaedb3

remove not needed test, up lock

984326b

update deps and lock

8a3337d

remove old test for peerManager, fix check and spell

d4210c7

danisharora099 reviewed Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve peer manager and re-integrate to light push #2191

feat: improve peer manager and re-integrate to light push #2191

weboko commented Oct 20, 2024

github-actions bot commented Oct 20, 2024 •

edited

Loading

danisharora099 left a comment

danisharora099 Nov 27, 2024

danisharora099 Nov 27, 2024

danisharora099 Nov 27, 2024

danisharora099 Nov 27, 2024

danisharora099 Nov 27, 2024

danisharora099 Nov 27, 2024

danisharora099 Nov 27, 2024

danisharora099 Nov 27, 2024

danisharora099 Nov 27, 2024

danisharora099 Nov 27, 2024

danisharora099 Nov 27, 2024


		export { getHealthManager } from "./lib/health_manager.js";

		export { KeepAliveManager } from "./lib/keep_alive_manager.js";


		import { PeerManager } from "./peer_manager.js";

		describe("PeerManager", () => {

		import { Logger } from "@waku/utils";
		import { Mutex } from "async-mutex";

feat: improve peer manager and re-integrate to light push #2191

Are you sure you want to change the base?

feat: improve peer manager and re-integrate to light push #2191

Conversation

weboko commented Oct 20, 2024

Problem

Solution

Notes

github-actions bot commented Oct 20, 2024 • edited Loading

size-limit report 📦

danisharora099 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 20, 2024 •

edited

Loading