bug: metadata protocol will disconnect light clients

[richard-ramos]

Problem

The function proc onPeerMetadata(pm: PeerManager, peerId: PeerId) {.async.} uses the metadata protocol to determine whether a connected peer supports a shard or not, and disconnect the peer otherwise.

This works fine for nodes that support relay. However in the case of nodes that support only light protocols (like mobile on status, which only consumes lightpush/filter/peerexchange, it will fail.

The metadata protocol relies on the ENR containing the shards, yet in the case of request/response protocols, the ENR is not updated at all, so the peer get disconnected

[SionoiS]

Light nodes don't use discovery, so the easy fix it to trigger a shard update in Waku Metadata when using filter subscribe.

Does it make sense to also update ENRs when a node uses Relay AND Filter as client? 🤔 is that even a valid use case?

[richard-ramos]

Something else to take into account: When using lightmode, peer exchange will be used instead of discv5 to retrieve peers. At that point no subscription to filter protocol has happened yet, so a shard update triggered by filter subscribe will not have happened. Peer exchange also does not use shard data at all so we can't trigger an update when doing a request, can we? 🤔

[alrevuelta]

mm unsure if bug or feature. even light clients should have some identifier (eg mounted waku metadata) because otherwise they may be interacting with nodes in other networks.

so i would say light clients should mount waku metadata (which is dead simple).

[richard-ramos]

@alrevuelta: so i would say light clients should mount waku metadata (which is dead simple).

Light clients (using go-waku) already mount waku metadata, and the logic that deals with the clusterID is executed correctly (I assume by network you mean nodes sharing the same clusterID?).

The issue described here happens due to this code

nwaku/waku/node/peer_manager/peer_manager.nim

Lines 360 to 362 in 55ff667

	if not metadata.shards.anyIt(pm.wakuMetadata.shards.contains(it)):
	reason = "no shards in common"
	break guardClauses

It assumes that the nodes will have a list of shards at all times, yet this is an undefined behaviour for light clients as they do not update their ENR nor are discoverable, and you can see here how metadata protocol is instantiated using the node's own ENR as well as the list of topics you subscribe to in relay (both of these not really available while using light protocols).

nwaku/waku/node/waku_node.nim

Line 191 in 55ff667

let metadata = WakuMetadata.new(clusterId, node.enr, node.topicSubscriptionQueue)

(This behavior is also shared by go-waku),

[SionoiS]

Would there be any problem having light nodes support all shards by default? They don't use relay anyway.

[fryorcraken]

Would there be any problem having light nodes support all shards by default? They don't use relay anyway.

I believe a light client would want to be connected to a service node that support the shard they are interested in.
It does differ from the concept of "supporting a shard in relay" but the end goal should be similar.

[SionoiS]

Would there be any problem having light nodes support all shards by default? They don't use relay anyway.

I believe a light client would want to be connected to a service node that support the shard they are interested in. It does differ from the concept of "supporting a shard in relay" but the end goal should be similar.

That's not the problem. If a node is not interested any shards then it can't connect to any other nodes because they have no shard in common.

No being able to connect to other nodes before you select shards is bad UX IMO.

[SionoiS]

Per Nwaku meet today we decided to only check shards in common for Relay connections. We keep the cluster id check for every connection.