fix: remove initialising peer-exchange while creating a node #1158

danisharora099 · 2023-02-08T15:38:04Z

Problem

PeerExchange is being initialised when a node was created, similar to other protocols like Filter and LightPush. This is however incorrect as PeerExchange is a discovery protocol and is to be initialised through libp2p's peerDiscovery ~~or manually.~~

Solution

Remove initialising PeerExchange by default while creating nodes

Notes

Unblocks feat: add peer exchange compliance test #1143
Resolves a warning log in console which was repeating twice earlier (was non breaking until the above PR was tested - god bless testing 🎉 )
~~- blocked by fix!: bump libp2p dependencies #1160 (comment)~~

github-actions · 2023-02-08T15:43:06Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
Waku core	114.94 KB (-0.06% 🔽)	2.3 s (-0.06% 🔽)	2.4 s (+29.16% 🔺)	4.7 s
Waku default setup	386.13 KB (-0.18% 🔽)	7.8 s (-0.18% 🔽)	4.2 s (-4.27% 🔽)	11.9 s
ECIES encryption	27.88 KB (0%)	558 ms (0%)	953 ms (-27.72% 🔽)	1.6 s
Symmetric encryption	27.88 KB (0%)	558 ms (0%)	1.1 s (-14.08% 🔽)	1.6 s
DNS discovery	108.79 KB (0%)	2.2 s (0%)	2.1 s (-15.53% 🔽)	4.2 s
Privacy preserving protocols	114.01 KB (-0.01% 🔽)	2.3 s (-0.01% 🔽)	1.5 s (-6.87% 🔽)	3.8 s
Light protocols	116.07 KB (-0.01% 🔽)	2.4 s (-0.01% 🔽)	1.6 s (-2.21% 🔽)	4 s
History retrieval protocols	115.91 KB (-0.01% 🔽)	2.4 s (-0.01% 🔽)	1.4 s (-2.72% 🔽)	3.7 s

fryorcraken · 2023-02-09T03:27:25Z

Wow, I actually have been thinking of doing this PR because it creates conflict with the new libp2p API so it's great to see that it also makes sense from your POV.

However, you only updated the create package here and I think the change needs an update on interfaces, core and tests too where peerExchange should be removed from the node interfaces.

danisharora099 · 2023-02-09T07:33:09Z

Wow, I actually have been thinking of doing this PR because it creates conflict with the new libp2p API so it's great to see that it also makes sense from your POV.

However, you only updated the create package here and I think the change needs an update on interfaces, core and tests too where peerExchange should be removed from the node interfaces.

based on your reply, im confused now as it makes sense.

it's a protocol that we need to mount to our node to be able to dial relevant px nodes
however, the discovery should only start if specified through libp2p's peerDiscovery
the discovery class in itself creates another instance of the PeerExchange class, while it should ideally reuse the already initialised instance (which can't be accessed as the node creation and peerDiscovery setup happens in the same line
eg:

    waku = await createLightNode({
      libp2p: {
        peerDiscovery: [
          bootstrap({ list: getPredefinedBootstrapNodes(Fleet.Test) }),
          wakuPeerExchangeDiscovery(),
        ],
      },
    });

i might be missing something but thought it might be better to get a fresh perspective on this

fryorcraken · 2023-02-09T09:11:17Z

it's a protocol that we need to mount to our node to be able to dial relevant px nodes

That's the thing, we don't need the waku node to expose the peer exchange API. We just need peer exchange to access the right libp2p components to operate.
Hence, I would remove it from the waku node interface and just build it as a normal libp2p peer discovery module that can be passed to libp2p.

fryorcraken

Did not do a full review as there is a fundamental show stopper.

packages/core/package.json

fryorcraken · 2023-02-16T02:37:56Z

packages/core/src/lib/wait_for_remote_peer.ts

@@ -61,11 +62,11 @@ export async function waitForRemotePeer(
  }

  if (protocols.includes(Protocols.PeerExchange)) {


We do not expect users to use peer exchange directly but only as a libp2p peer discovery protocol. Hence peer exchange support can be dropped from waitForRemotePeer and also potentially from the Protocols enum.

based on this comment, #1160 (comment) and libp2p/js-libp2p-interfaces#338 should no longer be a problem for us at this moment.

based on this comment, #1160 (comment) and libp2p/js-libp2p-interfaces#338 should no longer be a problem for us at this moment.

Yep I agree.

- also removes the manual test for peer-exchange (assumption is that the only way to initialise peer-exchange is through libp2p's peerDiscovery and not manually) (ref: #1158 (comment)) # Please enter the commit message for your changes. Lines starting

danisharora099 · 2023-02-16T06:35:55Z

@fryorcraken it's ready for review again.
the best way to review is by going to "files changed" instead of per commit

packages/peer-exchange/src/waku_peer_exchange_discovery.ts

danisharora099 · 2023-02-16T06:37:06Z

packages/tests/tests/peer_exchange.node.spec.ts

-    );
-
-    expect(receivedCallback).to.be.true;
+    const pxPeers = waku.libp2p


might help to abstract this into a generic helper like getPeersForProtocols

packages/peer-exchange/package.json

fryorcraken · 2023-02-17T02:13:49Z

packages/tests/tests/peer_exchange.node.spec.ts

-
-    expect(receivedCallback).to.be.true;
+    const pxPeers = waku.libp2p
+      .getConnections()


This strategies makes the test slow to run every time.

Instead of waiting 40s and then checking connection do this: listen on new connection events on libp2p. for each new connection, check tag. As soon as one with a peer exchange tag is found, test can stop.

You can also wrap that in a timeout so it does not wait forever.

great idea!

i saw a flaw while i revisited this: we assume that a peer exchange node will definitely connect but as we know (ref: waku-org/nwaku#1521), it's very probable that no connections are made. thus, i have refactored this to listen for discovery instead of connections

danisharora/fix-px

packages/tests/tests/peer_exchange.node.spec.ts

danisharora099 requested a review from weboko February 8, 2023 15:38

danisharora099 requested a review from fryorcraken as a code owner February 8, 2023 15:38

danisharora099 mentioned this pull request Feb 8, 2023

feat: add peer exchange compliance test #1143

Closed

danisharora099 marked this pull request as draft February 9, 2023 07:33

danisharora099 mentioned this pull request Feb 15, 2023

fix!: bump libp2p dependencies #1160

Merged

fryorcraken requested changes Feb 16, 2023

View reviewed changes

danisharora099 force-pushed the danisharora/fix-px branch from efbdb99 to 10249b7 Compare February 16, 2023 06:15

danisharora099 closed this Feb 16, 2023

danisharora099 force-pushed the danisharora/fix-px branch from 10249b7 to c6bd894 Compare February 16, 2023 06:19

danisharora099 reopened this Feb 16, 2023

fix: build

80af12b

danisharora099 marked this pull request as ready for review February 16, 2023 06:34

danisharora099 requested a review from fryorcraken February 16, 2023 06:35

danisharora099 commented Feb 16, 2023

View reviewed changes

fryorcraken approved these changes Feb 17, 2023

View reviewed changes

danisharora099 added 4 commits February 17, 2023 12:18

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

f979b94

danisharora/fix-px

update interop test

8cf30dd

decrease test duration for px auto discovery

9313180

rm: only for tests

ffedf4b

fryorcraken reviewed Feb 17, 2023

View reviewed changes

packages/tests/tests/peer_exchange.node.spec.ts Outdated Show resolved Hide resolved

address comment

dc88ae3

danisharora099 merged commit 1b41569 into master Feb 17, 2023

danisharora099 deleted the danisharora/fix-px branch February 17, 2023 07:57

github-actions bot mentioned this pull request Feb 17, 2023

chore: release master #1176

Closed

fryorcraken mentioned this pull request Mar 14, 2023

chore: release master #1237

Merged

fryorcraken mentioned this pull request Mar 31, 2023

chore: release master #1286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove initialising peer-exchange while creating a node #1158

fix: remove initialising peer-exchange while creating a node #1158

danisharora099 commented Feb 8, 2023 •

edited

Loading

github-actions bot commented Feb 8, 2023 •

edited

Loading

fryorcraken commented Feb 9, 2023

danisharora099 commented Feb 9, 2023

fryorcraken commented Feb 9, 2023

fryorcraken left a comment

fryorcraken Feb 16, 2023

danisharora099 Feb 16, 2023

fryorcraken Feb 17, 2023

danisharora099 commented Feb 16, 2023

danisharora099 Feb 16, 2023

fryorcraken Feb 17, 2023

danisharora099 Feb 17, 2023

		@@ -61,11 +62,11 @@ export async function waitForRemotePeer(
		}

		if (protocols.includes(Protocols.PeerExchange)) {

fix: remove initialising peer-exchange while creating a node #1158

fix: remove initialising peer-exchange while creating a node #1158

Conversation

danisharora099 commented Feb 8, 2023 • edited Loading

Problem

Solution

Notes

github-actions bot commented Feb 8, 2023 • edited Loading

size-limit report 📦

fryorcraken commented Feb 9, 2023

danisharora099 commented Feb 9, 2023

fryorcraken commented Feb 9, 2023

fryorcraken left a comment

Choose a reason for hiding this comment

fryorcraken Feb 16, 2023

Choose a reason for hiding this comment

danisharora099 Feb 16, 2023

Choose a reason for hiding this comment

fryorcraken Feb 17, 2023

Choose a reason for hiding this comment

danisharora099 commented Feb 16, 2023

danisharora099 Feb 16, 2023

Choose a reason for hiding this comment

fryorcraken Feb 17, 2023

Choose a reason for hiding this comment

danisharora099 Feb 17, 2023

Choose a reason for hiding this comment

danisharora099 commented Feb 8, 2023 •

edited

Loading

github-actions bot commented Feb 8, 2023 •

edited

Loading