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

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

Problem

After spending time with project that use js-waku we were able to determine entities in the code which behavior can be improve. Among them: PeerManager, ReliabilityManager etc.

Solution

For behavior this PR aims to add robustenes to the code by adding new configuration properties and making sure they are enforced.
This PR also re-works PeerManager and partially RelibilityManager.

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.79 KB (-2.24% 🔽)	1.7 s (-2.24% 🔽)	1.8 s (-15.03% 🔽)	3.5 s
Waku Simple Light Node	135.68 KB (-1.08% 🔽)	2.8 s (-1.08% 🔽)	2.4 s (+7.69% 🔺)	5.1 s
ECIES encryption	22.73 KB (-0.65% 🔽)	455 ms (-0.65% 🔽)	703 ms (+32.66% 🔺)	1.2 s
Symmetric encryption	22.23 KB (-0.63% 🔽)	445 ms (-0.63% 🔽)	888 ms (+27.12% 🔺)	1.4 s
DNS discovery	70.8 KB (+0.37% 🔺)	1.5 s (+0.37% 🔺)	1.5 s (-14.89% 🔽)	2.9 s
Peer Exchange discovery	71.8 KB (-0.03% 🔽)	1.5 s (-0.03% 🔽)	1.9 s (+23.48% 🔺)	3.3 s
Local Peer Cache Discovery	65.39 KB (+0.34% 🔺)	1.4 s (+0.34% 🔺)	1.9 s (+10.39% 🔺)	3.2 s
Privacy preserving protocols	76.52 KB (+0.39% 🔺)	1.6 s (+0.39% 🔺)	889 ms (-46.95% 🔽)	2.5 s
Waku Filter	78.11 KB (-3.6% 🔽)	1.6 s (-3.6% 🔽)	2 s (+4.64% 🔺)	3.6 s
Waku LightPush	75.6 KB (-0.18% 🔽)	1.6 s (-0.18% 🔽)	1.9 s (+17.72% 🔺)	3.4 s
History retrieval protocols	75.81 KB (-2.43% 🔽)	1.6 s (-2.43% 🔽)	859 ms (-40.13% 🔽)	2.4 s
Deterministic Message Hashing	7.32 KB (-0.96% 🔽)	147 ms (-0.96% 🔽)	415 ms (+41.19% 🔺)	561 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?

packages/core/src/index.ts

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

yes, in ConnectionManager there is method implemented for it
public async getConnectedPeers(codec?: string): Promise<Peer[]>

easier to do
waku.lightpush.connectedPeers()
vs
waku.connectionManager.getPeers(waku.lightpush.multicodec)

im in favor of not removing this
also is a breaking change

why waku.lightpush.connectedPeers() even needed?
I would assume to be used on the consumer side - but then it's something that we don't expect to be used

whereas waku.connectionManager.getPeers(waku.lightpush.multicodec) is for internal usage for protocols and shouldn't be used outside

also is a breaking change

agree, this PR is pretty much breaking change as we found out previous things were not working well enough

we have had feedback in the past that convinces us that consumers want to be able to check the information of connected peers on the protocol (cc @vpavlin)
we can add it on the SDK layer instead of the core layer

we can continue to use ConnectionManager to provide this API - do you see reason in switching out the use of multicodecs as arg for the function, with the enum type for protocols instead to help DX? (ref: #2191)

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

packages/interfaces/src/connection_manager.ts

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? :(

it is because mock is partial and there are two ways - mock everything or type casting
since test is a controlled env - type casting is good enough

curious how we were able to do this previously:

screen shot is broken

ah

(this is out of curiosity:D)

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?

Yes! But in order not to overcomplicate it even further (now there are 4 PRs) - I will make follow up with them

would be good to add tests in the same PR introducing the functionality

packages/sdk/src/protocols/peer_manager.ts

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

packages/sdk/src/reliability_monitor/receiver.ts

weboko · 2025-01-28T00:51:36Z

thank you @danisharora099

Are we planning to address the TODOs part of this PR, or follow up?

absolutely, I was looking at some last week

Do we need to introduce newer tests as we delete the functionalities and respective spec tests?

absolutely again, but I limit this PR to drastic changes + manual validation and dogfooding
tests will be a follow up on a case by case basis

How does the peer management work with this PR, considering the overhaul of BaseProtocol, PeerManager and ConnectionManager? Is it stable?

explained in the comment above
it is stable, last Thursday we didn't find bugs as well

danisharora099

did another review
left comments in individual threads

* add message cache to Filter * remove WakuOptions and use only ProtocolCreateOptions * move subscribe options to createLightNode Fitler protocol options * rename SubscriptionManager to Subscription * rename to CreateNodeOptions * add warning * feat: introduce subscription manager (#2202) * feat: inroduce subscription manager * fix: make pipeline succeed (#2238) * fix test * use hardcoded value * update playwright * fix test:browser

adklempner · 2025-01-29T19:11:17Z

packages/sdk/src/protocols/filter/subscription_monitor.ts

+
+  private async onPeerConnected(_event: CustomEvent<PeerId>): Promise<void> {
+    // TODO(weboko): use config.numOfUsedPeers here
+    if (this.peers.length > 0) {


A comment above the function to explain this check would be helpful

adklempner · 2025-01-29T19:13:45Z

packages/sdk/src/protocols/filter/subscription_monitor.ts

+    }
+
+    this.peers = await this.peerManager.getPeers();
+    await Promise.all(this.peers.map((peer) => this.subscribe(peer)));


similar to above, a comment would be helpful. it appears that if a disconnected peer had been used to establish a filter subscription, then this triggers an attempt to resubscribe to all peers?

…-manager

danisharora099

nice work! lgtm

primarily comments remaining are around not deleting tests, and adding new tests for new functionalities

danisharora099 · 2025-01-30T12:35:38Z

packages/sdk/src/protocols/peer_manager.ts

we want to follow up with integrating HealthManager back with PeerManager?

danisharora099 · 2025-01-30T12:37:50Z

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

we should re add tests for the refactored PeerManager entity

danisharora099 · 2025-01-30T12:40:27Z

packages/tests/tests/filter/peer_management.spec.ts

similar comment re not deleting tests

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

weboko mentioned this pull request Jan 14, 2025

feat: improve filter subscriptions #2193

Merged

weboko marked this pull request as ready for review January 15, 2025 23:17

weboko requested a review from a team as a code owner January 15, 2025 23:17

weboko mentioned this pull request Jan 23, 2025

chore: dogfood subscription and peer managment rework waku-org/lab.waku.org#111

Merged

merge with master

7258a01

weboko mentioned this pull request Jan 28, 2025

feat: make core protocols use PeerId only #2232

Open

weboko requested a review from danisharora099 January 28, 2025 00:51

weboko added 2 commits January 28, 2025 23:39

merge with master

7f4033d

rename to getConnectedPeers

994d05e

weboko mentioned this pull request Jan 28, 2025

fix: make pipeline succeed #2238

Merged

5 tasks

danisharora099 reviewed Jan 29, 2025

View reviewed changes

adklempner approved these changes Jan 29, 2025

View reviewed changes

weboko added 2 commits January 29, 2025 22:52

up lock

ebb00f4

Merge branch 'master' of github.com:waku-org/js-waku into weboko/peer…

b6ac054

…-manager

danisharora099 reviewed Jan 30, 2025

View reviewed changes

merge with master

b1f4adf

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 •

edited

Loading

github-actions bot commented Oct 20, 2024 •

edited

Loading

danisharora099 left a comment

danisharora099 Nov 27, 2024

weboko Jan 28, 2025

danisharora099 Jan 29, 2025 •

edited

Loading

weboko Jan 29, 2025

danisharora099 Jan 30, 2025

danisharora099 Jan 30, 2025

danisharora099 Nov 27, 2024

weboko Jan 28, 2025

danisharora099 Jan 29, 2025

weboko Jan 29, 2025

danisharora099 Jan 30, 2025 •

edited

Loading

danisharora099 Nov 27, 2024

weboko Jan 28, 2025

danisharora099 Jan 29, 2025

weboko commented Jan 28, 2025 •

edited

Loading

danisharora099 left a comment

adklempner Jan 29, 2025

adklempner Jan 29, 2025

danisharora099 left a comment •

edited

Loading

danisharora099 Jan 30, 2025

danisharora099 Jan 30, 2025

danisharora099 Jan 30, 2025 •

edited

Loading


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

		describe("PeerManager", () => {

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 • edited Loading

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

danisharora099 Jan 29, 2025 • edited Loading

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

danisharora099 Jan 30, 2025 • edited Loading

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

weboko commented Jan 28, 2025 • edited Loading

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

danisharora099 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danisharora099 Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

weboko commented Oct 20, 2024 •

edited

Loading

github-actions bot commented Oct 20, 2024 •

edited

Loading

danisharora099 Jan 29, 2025 •

edited

Loading

danisharora099 Jan 30, 2025 •

edited

Loading

weboko commented Jan 28, 2025 •

edited

Loading

danisharora099 left a comment •

edited

Loading

danisharora099 Jan 30, 2025 •

edited

Loading