feat: add new metadata protocol #2062

alrevuelta · 2023-09-21T10:35:11Z

Description:

Add cluster-id flag to wakunode2.
Create WakuMetadata protocol. See feat(66/WAKU2-METADATA): add WakuMetadata Protocol vacp2p/rfc#619
Use WakuMetadata and cluster-id in the peer manager, triggering disconnecitons with peers that support a different cluster-id than us. This is applied only if cluster-id!=0 so since cluster-id=0 by default, this prevents from breaking backwards compat.

github-actions · 2023-09-21T10:41:57Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2062

Built from a6ad8ce

github-actions · 2023-09-28T15:01:38Z

This PR may contain changes to configuration options of one of the apps.

If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed.

Please also make sure the label release-notes is added to make sure any changes to the user interface are properly announced in changelog and release notes.

SionoiS

Looks good!

I would be curious to know how much longer does a full connection cycle take if we have to wait for an exchange of metadata.

We could wait 2 Waku releases before disconnecting from peer who don't support the protocol.
I feel like it would smooth the transition.

As for the live updating of supported shards, let's just do the same as with discv5.

richard-ramos

Should this implementation include some kind of backoff mechanism to avoid reconnecting to the same peer frequently? (which is something that could happen with discv5 as it frequently returns the same peers)

alrevuelta · 2023-10-04T07:51:23Z

I would be curious to know how much longer does a full connection cycle take if we have to wait for an exchange of metadata.

Guess 1 RTT. For relay not something that matters imho, since its doesnt block anything. But for req/resp protocols like store or filter, it will indeed add a small delay = to 1 RTT i guess.

We could wait 2 Waku releases before disconnecting from peer who don't support the protocol.
I feel like it would smooth the transition.

Now that I think about it, another option would be to only use this protocol and force a disconnection if clusterId=1 (aka The Waku Network). Or well, if clusterId!=0. And use clusterId=0 as default. With this:

old nodes can connect to new nodes that haven't changed the deployment (clusterId is not set and defaults to 0), so no disconnection happens.
new nodes explicitly deployed in TWN clusterId=1 will trigger the disconnection, so they won't connect to i) old nodes or ii) new nodes configured in a different cluster.

Then in future releases we can remove this check to enforce it always. wdyt @SionoiS ?

As for the live updating of supported shards, let's just do the same as with discv5.

Sure, leaving it for another PR. Tracker: #2105

Should this implementation include some kind of backoff mechanism to avoid reconnecting to the same peer frequently? (which is something that could happen with discv5 as it frequently returns the same peers)

imho its not needed, since cluster-ids are also filtered in the discv5 layer. I mean, you won't try to connect to a peer in a different cluster, since we filter out that peer based on the ENR. This protocol is specially usefull in inbound connections, since you dont have any control of that, and most likely you dont have the enr to know theh cluster.

SionoiS · 2023-10-04T11:54:41Z

Guess 1 RTT. For relay not something that matters imho, since its doesnt block anything. But for req/resp protocols like store or filter, it will indeed add a small delay = to 1 RTT i guess.

How many round trips happen currently? Do you know? What's the % increase.

Now that I think about it, another option would be to only use this protocol and force a disconnection if clusterId=1 (aka The Waku Network). Or well, if clusterId!=0. And use clusterId=0 as default. With this:

old nodes can connect to new nodes that haven't changed the deployment (clusterId is not set and defaults to 0), so no disconnection happens.

new nodes explicitly deployed in TWN clusterId=1 will trigger the disconnection, so they won't connect to i) old nodes or ii) new nodes configured in a different cluster.

Then in future releases we can remove this check to enforce it always. wdyt @SionoiS ?

Good idea! Do you want to change the RFC to include cluster id 0 == default?

alrevuelta · 2023-10-05T11:19:40Z

PR is ready for review. As discussed, disconnections are only triggered if clusterId!=0 AND myClusterId!=remoteClusterId. This prevents from breaking old/new nodes compatibility. clusterId=0 is used as default, so acts as a "feature flag" by now.

jm-clius

LGTM! One question re housekeeping, but the approach makes sense to me.

jm-clius · 2023-10-05T12:48:28Z

waku/node/peer_manager/peer_manager.nim

+        conn = await pm.switch.dial(peerId, WakuMetadataCodec)
+      except CatchableError:
+        info "disconnecting from peer", peerId=peerId, reason="waku metadata codec not supported"
+        asyncSpawn(pm.switch.disconnect(peerId))


Without having looked into the details it's not immediately clear to me if this disconnect would also trigger a proper cleanup of the peer in the peer store/manager so that we don't keep stale peers?

indeed, good catch. removing the peer here:
dbd692a

SionoiS

Looks good overall but I feel like we should keep in mind the apps that will not use TWN or use their own discovery process.

This metadata protocol should not be a requirement, just like relay is not required.

Also various nitpicks.

tests/test_peer_manager.nim

waku/node/peer_manager/peer_manager.nim

waku/node/waku_node.nim

waku/node/peer_manager/peer_manager.nim

SionoiS

👍

Thanks.

I'm still not sure about the use case where Waku is used without TWN. I'll think about it.

status-im/infra-waku#3 waku-org/nwaku#2062

alrevuelta force-pushed the poc-metadata-protocol branch 2 times, most recently from 11475b4 to 6914cde Compare September 28, 2023 15:01

alrevuelta force-pushed the poc-metadata-protocol branch from ff7c927 to 2a3755a Compare October 3, 2023 15:04

chair28980 assigned chair28980 and alrevuelta and unassigned chair28980 Oct 3, 2023

SionoiS reviewed Oct 3, 2023

View reviewed changes

richard-ramos reviewed Oct 3, 2023

View reviewed changes

alrevuelta force-pushed the poc-metadata-protocol branch from 2a3755a to 78eb939 Compare October 5, 2023 11:11

alrevuelta marked this pull request as ready for review October 5, 2023 11:17

alrevuelta requested review from SionoiS, jm-clius and richard-ramos October 5, 2023 11:17

jm-clius approved these changes Oct 5, 2023

View reviewed changes

SionoiS requested changes Oct 5, 2023

View reviewed changes

alrevuelta added 4 commits October 6, 2023 09:12

feat: add metadata protocol with clusterId report

fc67383

disconnect in different cases

2460a43

Disconnect only when clusterId not 0

9c2f34c

Fix comments

dbd692a

alrevuelta force-pushed the poc-metadata-protocol branch from 78eb939 to dbd692a Compare October 6, 2023 07:43

alrevuelta requested a review from SionoiS October 6, 2023 07:47

Merge branch 'master' into poc-metadata-protocol

f8f5abb

SionoiS approved these changes Oct 10, 2023

View reviewed changes

Merge branch 'master' into poc-metadata-protocol

1e0b6ff

chair28980 mentioned this pull request Oct 11, 2023

feat: add new metadata protocol waku-org/js-waku#1652

Closed

chair28980 mentioned this pull request Oct 11, 2023

feat: add new metadata protocol waku-org/go-waku#799

Closed

alrevuelta merged commit d5c3ade into master Oct 11, 2023
9 of 10 checks passed

alrevuelta deleted the poc-metadata-protocol branch October 11, 2023 06:58

fryorcraken added the E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details label Oct 27, 2023

yakimant added a commit to status-im/infra-role-nim-waku that referenced this pull request Nov 7, 2023

docker: add cluster-id docker_add_cluster_id_flag

1bb5afe

status-im/infra-waku#3 waku-org/nwaku#2062

yakimant added a commit to status-im/infra-role-nim-waku that referenced this pull request Nov 7, 2023

docker: add cluster-id docker_add_cluster_id_flag (#20)

af9a380

status-im/infra-waku#3 waku-org/nwaku#2062

SionoiS mentioned this pull request Nov 10, 2023

chore: improving node setup w.r.t sharding information #2187

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add new metadata protocol #2062

feat: add new metadata protocol #2062

alrevuelta commented Sep 21, 2023 •

edited

Loading

github-actions bot commented Sep 21, 2023 •

edited

Loading

github-actions bot commented Sep 28, 2023

SionoiS left a comment

richard-ramos left a comment

alrevuelta commented Oct 4, 2023

SionoiS commented Oct 4, 2023

alrevuelta commented Oct 5, 2023

jm-clius left a comment

jm-clius Oct 5, 2023

alrevuelta Oct 6, 2023

SionoiS left a comment

SionoiS left a comment

feat: add new metadata protocol #2062

feat: add new metadata protocol #2062

Conversation

alrevuelta commented Sep 21, 2023 • edited Loading

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

github-actions bot commented Sep 28, 2023

SionoiS left a comment

Choose a reason for hiding this comment

richard-ramos left a comment

Choose a reason for hiding this comment

alrevuelta commented Oct 4, 2023

SionoiS commented Oct 4, 2023

alrevuelta commented Oct 5, 2023

jm-clius left a comment

Choose a reason for hiding this comment

jm-clius Oct 5, 2023

Choose a reason for hiding this comment

alrevuelta Oct 6, 2023

Choose a reason for hiding this comment

SionoiS left a comment

Choose a reason for hiding this comment

SionoiS left a comment

Choose a reason for hiding this comment

alrevuelta commented Sep 21, 2023 •

edited

Loading

github-actions bot commented Sep 21, 2023 •

edited

Loading