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.74 KB (-2.3% 🔽)	1.7 s (-2.3% 🔽)	2.1 s (+93.36% 🔺)	3.8 s
Waku Simple Light Node	135.73 KB (-1.04% 🔽)	2.8 s (-1.04% 🔽)	3.2 s (+65.17% 🔺)	5.9 s
ECIES encryption	22.73 KB (-0.65% 🔽)	455 ms (-0.65% 🔽)	921 ms (+64.74% 🔺)	1.4 s
Symmetric encryption	22.23 KB (-0.63% 🔽)	445 ms (-0.63% 🔽)	789 ms (-11.5% 🔽)	1.3 s
DNS discovery	70.8 KB (+0.37% 🔺)	1.5 s (+0.37% 🔺)	1.5 s (+5.86% 🔺)	2.9 s
Peer Exchange discovery	71.8 KB (-0.03% 🔽)	1.5 s (-0.03% 🔽)	1.3 s (-28.12% 🔽)	2.8 s
Local Peer Cache Discovery	65.39 KB (+0.34% 🔺)	1.4 s (+0.34% 🔺)	1.8 s (+1.51% 🔺)	3.1 s
Privacy preserving protocols	76.52 KB (+0.39% 🔺)	1.6 s (+0.39% 🔺)	2.3 s (+57.99% 🔺)	3.8 s
Waku Filter	78.11 KB (-3.6% 🔽)	1.6 s (-3.6% 🔽)	1.9 s (+48.63% 🔺)	3.4 s
Waku LightPush	75.6 KB (-0.18% 🔽)	1.6 s (-0.18% 🔽)	2 s (-13.97% 🔽)	3.5 s
History retrieval protocols	75.78 KB (-2.47% 🔽)	1.6 s (-2.47% 🔽)	1.7 s (-2.86% 🔽)	3.2 s
Deterministic Message Hashing	7.32 KB (-0.96% 🔽)	147 ms (-0.96% 🔽)	319 ms (-35.12% 🔽)	465 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)

we have had feedback in the past that convinces us that consumers want to be able to check the information

that should be tackled by health state of the node, otherwise I don't see a need for consumers for do waku.lightPush.connectedPeers() - if that is something they need to do we can tell them:

do waku.getConnectedPeers().filter(byMulticodec);

expose connectionManager so that they can do the same from it;
Back then this functionality was added without clear need.

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?

as I explained it in the other comment - connectionManager.getPeers(multicodec) - will return peers for metadata and other protocols where as Protocols is for consumers use and has only LightPush/Filter etc.

Just to clarify here - let's not have unnecessary entry points and not needed code. This will be easier for us as to support, bug fix and improve our own code base.
I see us adding one method that does the job and then improving it if we find more evidence.
Of course I might be wrong as to this particular case, but then we can add it once requested.

@danisharora099 let me know what you think.

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

packages/interfaces/src/connection_manager.ts

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

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

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

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

…-manager

danisharora099

nice work! lgtm

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

packages/sdk/src/protocols/peer_manager.ts

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

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

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

Closed

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

packages/sdk/src/protocols/filter/subscription_monitor.ts Outdated Show resolved Hide resolved

packages/sdk/src/protocols/filter/subscription_monitor.ts Outdated Show resolved Hide resolved

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

packages/sdk/src/protocols/peer_manager.ts Outdated Show resolved Hide resolved

packages/sdk/src/protocols/peer_manager.spec.ts Show resolved Hide resolved

packages/tests/tests/filter/peer_management.spec.ts Outdated Show resolved Hide resolved

weboko added 8 commits January 30, 2025 19:37

merge with master

b1f4adf

make peer retrieval probabilistic

93eebfa

add comments

070b5a4

up lightpush tests

38067b3

add tests for peer_manager, improve folder structure

fb9556a

create named files for protocols

e84d0db

create named files, simplify project structure

8aac817

remove only

da1ffe6

weboko merged commit 62f93dc into master Jan 30, 2025
12 of 14 checks passed

weboko deleted the weboko/peer-manager branch January 30, 2025 23:16

This was referenced Jan 30, 2025

feat: better create protocol options #2072

Closed

chore: release master #2195

Open

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

weboko Jan 30, 2025

weboko Jan 30, 2025

weboko commented Jan 28, 2025 •

edited

Loading

danisharora099 left a comment

danisharora099 left a comment •

edited

Loading

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

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

danisharora099 Nov 27, 2024

Choose a reason for hiding this comment

weboko Jan 28, 2025

Choose a reason for hiding this comment

danisharora099 Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

weboko Jan 29, 2025

Choose a reason for hiding this comment

danisharora099 Jan 30, 2025

Choose a reason for hiding this comment

danisharora099 Jan 30, 2025

Choose a reason for hiding this comment

weboko Jan 30, 2025

Choose a reason for hiding this comment

weboko Jan 30, 2025

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

danisharora099 left a comment • 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

weboko commented Jan 28, 2025 •

edited

Loading

danisharora099 left a comment •

edited

Loading