This repository has been archived by the owner on Jun 14, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat(66/WAKU2-METADATA): add WakuMetadata Protocol #619
feat(66/WAKU2-METADATA): add WakuMetadata Protocol #619
Changes from 4 commits
24a6e28
97dc818
3f01712
d381ee1
2856707
7d8cbec
79831da
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jm-clius I'm not fully aware on the implications of static sharding in here. Any thought on wether it should be considered here? Or these shards should be only for auto sharding?
In other words. If a node is running with static sharding, should the metadata protocol be able to "advertise" those shards?
Another way could be to use
string
instead ofuint32
and just put the whole topic there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it stands, this would work for static sharding as well (as the cluster ID and shard numbers are encoded exactly the same). The one outstanding thing to decide is between
network_id
vs.cluster_id
as terminology. I think there are OK arguments either side, but we should pick one and stick with it. Perhaps let's just put it up for a vote somewhere?Interesting idea re
string
, but I see this exchange as more valuable (at least for now) for the numbered sharding as in the Waku Network. Exchanging the whole topic could as well just reuse thehello
messages that already exist in some libp2p implementations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure. Will ask.
Not sure i follow. If shard is just an int, how do we know if that refers to a shard or to a static "shard"?
mm why then using this metadata protocol for the shards? Perhaps it makes more sense to use the hello message and the whole topics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The network ID would be different. Static shards (despite the different name) follows the same pattern - the shard numbers are fully qualified only in conjunction with the network ID. In other words, the static sharded networks are just Waku Networks using their own network IDs (!=
1
). In fact, network IDs 0-15 are (potentially) reserved so we have some scope to define our own main/test networks, but any other network ID can be used by applications interested in creating their own Waku Networks.Well, why use it for network ID if that is also encoded in the topic? My understanding was that the intention is to conveniently exchange the minimum information that is necessary to determine if a connection is interesting or not - that would be network ID in conjunction with shard numbers. Neither of these pieces of metadata is currently enough on its own to make any final conclusions about a connection. The reason not to simply have this be implicit in a list of subscribed topics is to be explicit about our structured approach to network ID and shard numbers in topic construction. Or am I missing something behind the intention of the metadata protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm because there are some protocols that don't know about "topics". I mean, if i dial with
store
protocol, how do i know that the store protocol is running in the same network?. afaik right now there is no way?but yeah, agree with "exchange the minimum information to determine if a connection is interesting or not". But networkid is one level up than shards. Different network id is a deal breaker. Different shard(s), well guess we are not interested, but not as "strong".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but again I would need to know what shards within the network the store protocol supports. In all (current) cases, the network ID is just a logical grouping of shards. Since the shards (in fact the pubsub topics constructed from these) define how messages are routed, this is the minimum information I would need to know if a node is interesting for relay, store, filter and lightpush. (A future exception would be that network-ID defines the RLN contract regardless of shard, although this wouldn't affect my decision about which nodes I connect to or not. But RLN contract is the only protocol element that I can think of that belongs to a network ID and not to specific shards within the network.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed 2856707
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still make this
optional
in the proto3 definition (both in request and response).This is not because populating the field is necessarily optional in the protocol, but because
proto3
limitations currently imply that there will be no way for us to know if the remote party populated the field or not without theoptional
keyword. As it stands, if this field is not explicitly populated it will always deserialise as network ID0
, whether that was the intention or not.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be clear already, but just thought of a better way to state the implication. If we have a statement in an RFC such as:
there is no way for us to check this stipulation without the
optional
keyword. A bit silly in proto3 IMO.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed 2856707
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed 2856707