Add implementation recommendation for metadata - #168
Conversation
Based on recent learnings.
Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>
Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>
fryorcraken
changed the title
chaitanyaprem
left a comment
chaitanyaprem
left a comment
There was a problem hiding this comment.
Had one query, otherwise LGTM
Thanks for this!
Ivansete-status
left a comment
Ivansete-status
left a comment
There was a problem hiding this comment.
Thanks so much for it! 🙌
I added some comments that I hope you find useful
|
|
||
| A node SHOULD store the remote peer's metadata information for future reference. | ||
| A node MAY implement a TTL regarding a remote peer's metadata, and refresh it upon expiry by initiating another metadata request. | ||
| It is RECOMMENDED to set the TTL to 6 hours. |
There was a problem hiding this comment.
May I ask why 6h and not 2h or 20h? Just for learning
There was a problem hiding this comment.
I am happy to remove this recommendation as I am trying to infer too much on Chat sdk patterns.
There was a problem hiding this comment.
I think it's good to have an example number here. It means that we roughly think that, if implementing a TTL, it should be in the order of a few hours (as opposed to a few minutes or a few days). We can always adapt this number based on observation, but I think "friendly" specs can have our best current guesstimate, as long as it's merely a recommendation.
FWIW 4-6 hours is not an uncommon refresh period for network contexts. For example, this is more or less the interval a cellphone network will wait before refreshing the context of an idle phone.
| A node MAY proceed with metadata request upon reconnection to a remote peer. | ||
|
|
||
| A node SHOULD store the remote peer's metadata information for future reference. | ||
| A node MAY implement a TTL regarding a remote peer's metadata, and refresh it upon expiry by initiating another metadata request. |
There was a problem hiding this comment.
I can't quite see the need for that TTL concept if we perform metadata requests on first connection or reconnection.
There was a problem hiding this comment.
Remember that a node's metadata might change at any point. Metadata will eventually also be used to advertise more capabilities than just cluster + shard. It makes sense to assume that information like this may grow stale and needs to be refreshed from time to time.
There was a problem hiding this comment.
what other capabilities?
I feel like any additional capability would require nwaku to restart - which means to re-connect to the network
There was a problem hiding this comment.
what other capabilities? I feel like any additional capability would require
nwakuto restart - which means to re-connect to the network
A remote node, like a Status Desktop app, may change the relay shard they subscribe to dynamically.
This sentence is here exactly for that: remember implementers that metadata information may change.
There was a problem hiding this comment.
A remote node, like a Status Desktop app, may change the relay shard they subscribe to dynamically.
Then, having a TTL of 6h (or 1h) doesn't sound correct because if my Status Desktop app changes the shard dynamically, I want my app to start participating on that shard right away and not wait for TTL to expire. Then, an event-driven approach seems more convenient, instead of the TTL one, in this case.
There was a problem hiding this comment.
I want my app to start participating on that shard right away and not wait for TTL to expire.
What do you mean? What does "my app start participating" mean?
Then, an event-driven approach seems more convenient, instead of the TTL one, in this case.
So you are saying, if let's say a Status Desktop app that was doing relay on shard 1, stops to do so, then it should go through all the peers it has in its peer store (100s), and trigger a metadata request? do see you how this is not reasonable to have such massive network event triggered?
What we are saying is that the TTL + error handling is enough, because it's lazy.
- Alice discovers Bob
- Alice connects to Bob, do a metadata request and learns Bob is running relay on shard 2
- Alice disconnect to Bob, because we don't maintain connections by default
- Alice wants to send message on shard 2
- Alice re-connect to Bob
- Alice sends light push request to Bob for message on shard 2
- Bob stops subscribing to shard 2
- Alice wants to send new message on shard 2
- Alice reconnects to Bob
- Alice sends light push request to bob for message on shard 2
- Bob rejects, "Shard not supported"
- Alice connects to alternative node.
In the current recommendation, we suggest:
A node MAY implement a TTL regarding a remote peer's metadata, and refresh it upon expiry by initiating another metadata request.
A node MAY proceed with metadata request upon reconnection to a remote peer.
A node MAY trigger a metadata request after receiving an error response from a remote note stating they do not support a specific cluster or shard.
Which means:
At (5), and (9), we do not recommend a metadata request, before the light push request, every time.
If we were to recommend a metadata everytime we reconnect, it would mean we'd have 2 RTT every time we try to send a message over light push, adding necessary network calls to send a message
At (11), we recommend proceeding with a metadata request due to the error returned, so that Alice can avoid using Bob again for shard 2, while keeping Bob as a suitable service node for other shards (depending on metadata response).
At (7), we do not recommend Bob to initiate 100s of connection and metadata requests due to a local event.
| A node MAY trigger a metadata request after receiving an error response from a remote note | ||
| stating they do not support a specific shard or pubsub topic. |
There was a problem hiding this comment.
Typo fix and suggest to avoid using the pubsub topic term publicly.
| A node MAY trigger a metadata request after receiving an error response from a remote note | |
| stating they do not support a specific shard or pubsub topic. | |
| A node MAY trigger a metadata request after receiving an error response from a remote node | |
| stating they do not support a specific cluster-id or shard. |
Besides, I don't fully get why the metadata request is needed upon error reception :)
There was a problem hiding this comment.
Presumably we previously assumed they supported that shard, initiated a request and the error indicates that our information is stale.
There was a problem hiding this comment.
what if metadata and actual supported shard are differ?
I know we guarantee it in the code but bugs happen and malicious nodes too
There was a problem hiding this comment.
what if metadata and actual supported shard are differ? I know we guarantee it in the code but bugs happen and malicious nodes too
I think the recommendation can stop here, good engineering practices are still expected to avoid infinite loop of this kind.
There was a problem hiding this comment.
Besides, I don't fully get why the metadata request is needed upon error reception :)
answered in #168 (comment)
|
|
||
| A node SHOULD proceed with metadata request upon first connection to a remote node. | ||
| A node SHOULD use the remote node's libp2p peer id as identifier for this heuristic. | ||
|
|
There was a problem hiding this comment.
A change in the node's state should also trigger a metadata request to other peers.
| A node SHOULD proceed with metadata request upon change of any of its capabilities, such as the supported `cluster-id` or `shard` set. |
There was a problem hiding this comment.
This seems like a heavy requirement as it will likely affect all connections. We don't want a single subscription change to result in 300+ metadata requests, the vast majority of remote peers who may not even be affected by this change. I don't think it's necessary and we can leave it up to the remote parties to eventually get to know this information (with TTL and metadata-on-error implemented).
There was a problem hiding this comment.
Thanks for the comment @jm-clius !
I lean more to an event-driven approach, i.e., when something happens, this state change is spread everywhere has interest about it. That way will will have more real-time consistency.
Also, if someone configures a TTL==5 minutes, as an extreme example, then we might have metadata requests (+300) that are not needed because nothing changed.
IMHO, having the TTL concept will lead to more unneeded (+300) requests than having an event-driven approach because when a node starts with certain configuration, it is more likely to always run with that configuration forever instead of changing it every minute/hour/day. But I may miss something of course :)
There was a problem hiding this comment.
I lean more to an event-driven approach, i.e., when something happens, this state change is spread everywhere has interest about it.
We have to weigh in the implication. Yes, I agree event-driven approach within a given node, but not systematically when it implies network request.
In this case, a local-first, or lazy-push approach is preferred.
Also, if someone configures a TTL==5 minutes, as an extreme example, then we might have metadata requests (+300) that are not needed because nothing changed.
Note that 5 minutes is not the recommended TTL, but it's
It is RECOMMENDED to set the TTL to 6 hours.
IMHO, having the TTL concept will lead to more unneeded (+300) requests than having an event-driven approach
Let's see how the software behave but I disagree with your assumption.
But I may miss something of course :)
A desktop application that make full usage of autosharding may change relay subscription depending on user needs.
While this would not be expected to happen every 10min, it could happen.
There was a problem hiding this comment.
Remember that with decentralised network (and services) we don't expect everything to be consistent - we expect some peers to be misconfigured, some peers with buggy protocol behaviour, adversarial peers that try to trick us, etc. With metadata we can gain some probable information about peers' capabilities, but we should always assume that some of this information may be dishonest or outdated. A sensible heuristic for triggering metadata requests (e.g. with TTL, on error response, on connection) gives us more information, but never guarantees consistency. All our protocols should be able to deal with this ambiguity.
| ### Providing Shard Information | ||
|
|
||
| - Nodes that mount [`11/WAKU2-RELAY`](/waku/standards/core/11/relay.md) MAY include the shards they are subscribed to in their metadata payload. | ||
| - Nodes that mount [`11/WAKU2-RELAY`](/waku/standards/core/11/relay.md) and a shard-relevant service SHOULD include the shards they are subscribed to in their metadata payload. |
There was a problem hiding this comment.
What is "shard-relevant service" if may I ask?
I had an initial doubt, resolved in the next line :)
|
|
||
| - Nodes that mount [`11/WAKU2-RELAY`](/waku/standards/core/11/relay.md) MAY include the shards they are subscribed to in their metadata payload. | ||
| - Nodes that mount [`11/WAKU2-RELAY`](/waku/standards/core/11/relay.md) and a shard-relevant service SHOULD include the shards they are subscribed to in their metadata payload. | ||
| - Shard-relevant services are message related services, |
There was a problem hiding this comment.
Ah okay :D
Shall we move this line before Nodes that mount [11/WAKU2-RELAY](/waku/standards/core/11/relay.md) and a shard-relevant service SHOULD include the shards they are subscribed to in their metadata payload.
|
|
||
| ### Providing Cluster Id | ||
|
|
||
| A node SHOULD always include their cluster id into their metadata payload. |
There was a problem hiding this comment.
then it is a MUST, no?
| A node SHOULD always include their cluster id into their metadata payload. | |
| A node MUST include their cluster id into their metadata payload. |
There was a problem hiding this comment.
Not sure here why using SHOULD vs MUST @jm-clius any input?
There was a problem hiding this comment.
I would keep this SHOULD (even if one should always do something, it's not a "must" if some undefined reason exists for not doing this).
The default directive for specs IMO is usually SHOULD, unless the protocol breaks if this directive is not followed.
|
|
||
| ### Using Cluster Id | ||
|
|
||
| When reading the cluster id of a remote peer, the local node MAY disconnect if their cluster id is different from the remote peer. |
There was a problem hiding this comment.
I understand MAY but shouldn't we RECOMMEND to disconnect here?
There was a problem hiding this comment.
I would use RECOMMEND when it can negatively impact the overall system. I don't think it matters that much here.
| Ahead of doing a shard-relevant request, | ||
| a node MAY use the previously received metadata shard information to select a peer that support the targeted shard. | ||
|
|
||
| As part of peer management, a node MAY aim to maintain connections to a healthy pool of peers with different shards, |
There was a problem hiding this comment.
that seems to me to belong more for the scope of reliability than metadata
| As part of peer management, a node MAY aim to maintain connections to a healthy pool of peers with different shards, | ||
| based on the importance of shards for the application. | ||
|
|
||
| A node SHOULD maintain connection to some nodes in less relevant shards, to uses non-shard-related services such as [`34/WAKU2-PEER-EXCHANGE`](/waku/standards/core/34/peer-exchange.md). |
There was a problem hiding this comment.
do we need to specify this sentence? I am not use it is useful to define
There was a problem hiding this comment.
I think it is specifically useful to define because a knee-jerk implementer reaction we saw in both waku implementation is to disconnect if a node is not in the same shards.
Which is problematic at different level:
- an edge node should not really need a fixed "shard" concept, and instead, review request-by-request what shards are needed
- Relay node do not need dual management of shards, it is already handled by libp2p-gossipsub with pubsub topics
- In a network where the number of node per shard is not uniform, an edge node disconnected to any node not in its shard may run out of peer to perform peer-exchange with.
# Conflicts: # waku/standards/core/66/metadata.md
5 participants
Based on recent learnings.