chore: refactor peer-exchange according to nwaku 0.15.0 #1193

danisharora099 · 2023-02-21T08:48:40Z

Problem

Peer Exchange is supposed to be a request-response protocol (https://rfc.vac.dev/spec/34/) but due to a bug in the earlier versions of nwaku, it would asynchronously send a callback on a different stream. To adapt to this, js-waku would have an event listener open.

With nwaku 0.15.0, this is addressed and peer-exchange works as the originally designed request-response protocol.

Solution

js-waku's peer exchange implementation now removes the event listener on the registrar, and use the response received in the request to dispatch the ENRs.

Notes

Resolves breaking change
re-enables peer-exchange test for CI
Related to:
unblocks feat: compliance test for peer-exchange discovery #1186
this also reduces a lot of code bloat, while also making the received response faster 🎉

github-actions · 2023-02-21T08:54:35Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
Waku core	116.71 KB (+0.07% 🔺)	2.4 s (+0.07% 🔺)	3.3 s (+6.63% 🔺)	5.7 s
Waku default setup	383.99 KB (+2.93% 🔺)	7.7 s (+2.93% 🔺)	7.6 s (+4.13% 🔺)	15.3 s
ECIES encryption	27.99 KB (0%)	560 ms (0%)	1.7 s (+0.3% 🔺)	2.3 s
Symmetric encryption	27.99 KB (0%)	560 ms (0%)	1.7 s (-8.47% 🔽)	2.2 s
DNS discovery	108.07 KB (+0.02% 🔺)	2.2 s (+0.02% 🔺)	4.3 s (+50.63% 🔺)	6.5 s
Privacy preserving protocols	116.19 KB (+0.06% 🔺)	2.4 s (+0.06% 🔺)	2.5 s (-4.01% 🔽)	4.8 s
Light protocols	117.96 KB (+0.04% 🔺)	2.4 s (+0.04% 🔺)	3.1 s (+10.09% 🔺)	5.5 s
History retrieval protocols	118.03 KB (+0.07% 🔺)	2.4 s (+0.07% 🔺)	2.3 s (+10.47% 🔺)	4.6 s

packages/peer-exchange/src/waku_peer_exchange_discovery.ts

weboko · 2023-02-21T15:12:50Z

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

@@ -18,19 +18,17 @@ import { Nwaku } from "../src/nwaku.js";
 describe("Peer Exchange", () => {
  let waku: LightNode;

-  before(async function () {
+  afterEach(async function () {
+    !!waku && waku.stop().catch((e) => console.log("Waku failed to stop", e));


async/await can be used to prettify code

How would it look like? You'd need to add a try/catch block which would make this more verbose than necessary, no?

something like

afterEach(async () => { try { await waku?.stop(); } catch (err) { console.log("Waku failed to stop", e); } });

It depends on perception what is verbose for this case.

packages/peer-exchange/src/waku_peer_exchange.ts

fryorcraken · 2023-02-22T02:38:57Z

packages/peer-exchange/src/waku_peer_exchange.ts

+      const enrs = await Promise.all(
+        decoded.peerInfos.map(
+          (peerInfo) => peerInfo.enr && ENR.decode(peerInfo.enr)
+        )
+      );
+
+      const peerInfos = enrs.map((enr) => {
+        return {
+          ENR: enr,
+        };
+      });


could probably merge both maps to avoid traversing the array twice

we need to wait for the promise resolution first before converting it into an array again

const peerInfos = await Promise.all( decoded.peerInfos.map( (peerInfo) => { if (!peerInfo.enr) return; const enr = await ENR.decode(peerInfo.enr); return { ENR: enr } } ) );

Probably need to use a typeguard:

export function isDefined<T>(msg: T | undefined): msg is T { return !!msg; }

Or use this lib: https://twitter.com/mattpocockuk/status/1627686864579600385?s=20

fryorcraken · 2023-02-22T02:42:24Z

packages/peer-exchange/src/waku_peer_exchange_discovery.ts

+    for (const _peerInfo of peerInfos) {
+      const { ENR } = _peerInfo;
+      if (!ENR) {
+        log("no ENR");


We can see that this should not be possible from the code in waku_peer_exchange.ts

I don't think this log line is useful.

because of the protobuf definitions and typescript interpretations, it interprets that the fields might be optional which makes it necessary for us to handle that scenario without a forbidden assertion

Should be sorted with #1196

fryorcraken · 2023-02-22T02:44:29Z

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

@@ -18,19 +18,17 @@ import { Nwaku } from "../src/nwaku.js";
 describe("Peer Exchange", () => {
  let waku: LightNode;

-  before(async function () {
+  afterEach(async function () {
+    !!waku && waku.stop().catch((e) => console.log("Waku failed to stop", e));


How would it look like? You'd need to add a try/catch block which would make this more verbose than necessary, no?

danisharora099 · 2023-02-26T19:31:31Z

Will aim to merge this for now with the comments addressed in the interest of time as this PR also unblocks #1186. Please feel free to tag me again in case we want to address some of the remaining open conversations.

danisharora099 · 2023-02-26T19:56:16Z

@fryorcraken are you aware why the px test with go-waku is failing with a timeout error, while it succeeds for node & nwaku? how can i run go-waku testing locally to reproduce the same?
(cc @richard-ramos)

fryorcraken · 2023-02-26T23:04:43Z

@fryorcraken are you aware why the px test with go-waku is failing with a timeout error, while it succeeds for node & nwaku?

I don't know if @richard-ramos has aligned go-waku's behaviour just yet.

how can i run go-waku testing locally to reproduce the same?

Just set some env vars (modify as needed).

WAKU_SERVICE_NODE_BIN=/home/fryorcraken/src/waku-org/go-waku/build/waku;WAKU_SERVICE_NODE_DIR=/home/fryorcraken/src/waku-org/go-waku/build;WAKU_SERVICE_NODE_PARAMS=--min-relay-peers-to-publish\=0

richard-ramos · 2023-02-27T14:22:15Z

go-waku's peer exchange implementation is not compatible with nwaku. I'll look at it this week

danisharora099 · 2023-03-13T06:20:55Z

go-waku's peer exchange implementation is not compatible with nwaku. I'll look at it this week

Hey @richard-ramos, is there an update yet? 😅

fryorcraken · 2023-03-14T02:29:35Z

go-waku's peer exchange implementation is not compatible with nwaku. I'll look at it this week

Hey @richard-ramos, is there an update yet? sweat_smile

Update CI to use go-waku 0.5.2 and see how it goes.

danisharora099 · 2023-03-14T08:20:33Z

Merging this PR for now.

Tracking the breaking go-waku CI test here: waku-org/go-waku#491

danisharora099 marked this pull request as ready for review February 21, 2023 08:54

danisharora099 requested a review from fryorcraken as a code owner February 21, 2023 08:54

weboko reviewed Feb 21, 2023

View reviewed changes

packages/peer-exchange/src/waku_peer_exchange_discovery.ts Outdated Show resolved Hide resolved

weboko reviewed Feb 21, 2023

View reviewed changes

packages/peer-exchange/src/waku_peer_exchange.ts Show resolved Hide resolved

fryorcraken approved these changes Feb 22, 2023

View reviewed changes

danisharora099 force-pushed the chore/refactor-px-nwaku0.15.0 branch from 69ba3b8 to eb885c5 Compare March 13, 2023 06:14

merge with master: adhere acc to updated API

55cc8f9

danisharora099 force-pushed the chore/refactor-px-nwaku0.15.0 branch from eb885c5 to 55cc8f9 Compare March 13, 2023 06:15

danisharora099 and others added 2 commits March 14, 2023 12:13

bump go-waku to 0.5.2

461386c

Merge branch 'master' into chore/refactor-px-nwaku0.15.0

4c58899

danisharora099 mentioned this pull request Mar 14, 2023

bug(peer-exchange): js-waku <> go-waku interop waku-org/go-waku#491

Closed

danisharora099 closed this Mar 14, 2023

danisharora099 reopened this Mar 14, 2023

danisharora099 merged commit a20b797 into master Mar 14, 2023

danisharora099 deleted the chore/refactor-px-nwaku0.15.0 branch March 14, 2023 08:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactor peer-exchange according to nwaku 0.15.0 #1193

chore: refactor peer-exchange according to nwaku 0.15.0 #1193

danisharora099 commented Feb 21, 2023 •

edited

Loading

github-actions bot commented Feb 21, 2023 •

edited

Loading

weboko Feb 21, 2023

fryorcraken Feb 22, 2023

weboko Feb 24, 2023

fryorcraken Feb 22, 2023

danisharora099 Feb 26, 2023

fryorcraken Feb 26, 2023

fryorcraken Feb 22, 2023

danisharora099 Feb 26, 2023

fryorcraken Feb 26, 2023

fryorcraken Feb 22, 2023

danisharora099 commented Feb 26, 2023 •

edited

Loading

danisharora099 commented Feb 26, 2023 •

edited

Loading

fryorcraken commented Feb 26, 2023 •

edited

Loading

richard-ramos commented Feb 27, 2023

danisharora099 commented Mar 13, 2023

fryorcraken commented Mar 14, 2023

danisharora099 commented Mar 14, 2023

chore: refactor peer-exchange according to nwaku 0.15.0 #1193

chore: refactor peer-exchange according to nwaku 0.15.0 #1193

Conversation

danisharora099 commented Feb 21, 2023 • edited Loading

Problem

Solution

Notes

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

size-limit report 📦

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

danisharora099 commented Feb 26, 2023 • edited Loading

danisharora099 commented Feb 26, 2023 • edited Loading

fryorcraken commented Feb 26, 2023 • edited Loading

richard-ramos commented Feb 27, 2023

danisharora099 commented Mar 13, 2023

fryorcraken commented Mar 14, 2023

danisharora099 commented Mar 14, 2023

danisharora099 commented Feb 21, 2023 •

edited

Loading

github-actions bot commented Feb 21, 2023 •

edited

Loading

danisharora099 commented Feb 26, 2023 •

edited

Loading

danisharora099 commented Feb 26, 2023 •

edited

Loading

fryorcraken commented Feb 26, 2023 •

edited

Loading