chore: improve the metadata protocol implementation

danisharora099 commented 1 year ago

Background

In the existing interaction between nwaku and js-waku concerning the Metadata protocol, a level of friction has been identified, particularly within the client-server architecture context. When js-waku makes a metadata request to nwaku, the latter opens a new stream to send back its response, diverging from the anticipated behavior where the response should be delivered on the same stream as the request. Moreover, nwaku has been observed to initiate metadata requests towards js-waku. This approach conflicts with the conventional client-server model, where js-waku, as a client, should be the entity initiating requests for metadata from nwaku.

Details

The desired outcome is to amend the Metadata protocol implementation on nwaku to adhere to a request-response model within a single stream, similar to the interaction pattern observed in the Peer Exchange protocol. This adjustment will ensure that responses to metadata requests from js-waku are dispatched on the same stream as the requests. Also, perhaps we need to revisit and potentially modify the behavior of nwaku wanting to initiate metadata requests towards js-waku to ensure alignment with the client-server model, where js-waku should be the entity making requests to nwaku.

Additionally, the current architecture requires js-waku (and perhaps all implementations) to have 2 request-response iterations: this is confusing since the shard info is already shared during the 1st request, but is expected to be sent again during the 2nd iteration. This is possibly related to how nwaku wants to handle requesting other nodes for its metadata.

Acceptance Criteria

Modify the Metadata protocol implementation in nwaku to ensure that responses to metadata requests from js-waku are transmitted on the same stream as the requests.
Examine and amend, if necessary, the practice of nwaku initiating metadata requests towards js-waku to align with the client-server model.
Conduct testing to validate the updated behavior, ensuring that the interactions between nwaku and js-waku on the Metadata protocol are now predictable and aligned with the anticipated client-server model.
Update the RFC to clearly explain the new behavior and provide guidance for interacting with the Metadata protocol, especially from js-waku's standpoint being a client.

cc @waku-org/research @waku-org/js-waku-developers

SionoiS commented 1 year ago

For context in nwaku peer_manager send a metadata request when a peer connects, in addition of the metadata protocol itself, hence the duplicate req/res.

fryorcraken commented 1 year ago

When js-waku makes a metadata request to nwaku, the latter opens a new stream to send back its response, diverging from the anticipated behavior where the response should be delivered on the same stream as the request.

This is not the first time we have such issue with protocol implementations in nwaku. All request-response protocol should be done on the same stream.

I am not sure how much @jm-clius or @richard-ramos remember the last time we faced the issue but it would be good if a singular template/function can be provided by @waku-org/nwaku-developers to ensure this does not happen every time we create a new request-response protocol. Cc @ABresting who is implementing the next req/res protocol I am aware of

fryorcraken commented 1 year ago

Moreover, nwaku has been observed to initiate metadata requests towards js-waku. This approach conflicts with the conventional client-server model, where js-waku, as a client, should be the entity initiating requests for metadata from nwaku.

I am not convinced this is an issue per se, filter protocol pushes messages from server to client. Identify protocol is initiated by all parties. I don't see a problem in this pattern.

Thinking of it, why is it a request-response protocol? Can't it be a one shot protocol where upon connection, it is expected for a peer to send the info and if it does not happen then we should just drop the connection? cc @alrevuelta

alrevuelta commented 1 year ago

For context in nwaku peer_manager send a metadata request when a peer connects, in addition of the metadata protocol itself, hence the duplicate req/res.

we could save this extra interaction easily by doing this metadata request only if the new connection != metadata protocol. but unsure how easy is that in nim-libp2p. Perhaps that's the intention of this issue @danisharora099. Note that that's not a metada protocol modification, but how its used.

All request-response protocol should be done on the same stream.

afaik metadata protocol does that. Same stream is reused.

Thinking of it, why is it a request-response protocol? Can't it be a one shot protocol where upon connection, it is expected for a peer to send the info and if it does not happen then we should just drop the connection? cc @alrevuelta

IMHO it should be req/resp. Imagine that p1 wants to know p2 cluster id. But p1 is the one starting the connection. In that case p2 has to respond to p1 request. In other words, sometimes it can be done in one shot, but sometimes not.

danisharora099 commented 1 year ago

update:

afaik metadata protocol does that. Same stream is reused.

checking against the latest release candidate, i can confirm that this is the case. thank you!

fryorcraken commented 1 year ago

update:

afaik metadata protocol does that. Same stream is reused.

checking against the latest release candidate, i can confirm that this is the case. thank you!

Can this issue be closed then?

fryorcraken commented 1 year ago

But p1 is the one starting the connection.

So actually, the identify protocol uses the "open a stream" as the request:

The protocol works by opening a stream to the remote peer you want to query, using /ipfs/id/1.0.0 as the protocol id string. The peer being identified responds by returning an Identify message and closes the stream.

When you want to push information, you open a push stream:

When a peer's basic information changes, for example, because they've obtained a new public listen address, they can use identify/push to inform others about the new information.

The push variant works by opening a stream to each remote peer you want to update, using /ipfs/id/push/1.0.0 as the protocol id string. When the remote peer accepts the stream, the local peer will send an Identify message and close the stream.

https://github.com/libp2p/specs/blob/master/identify/README.md

I am not going to push for this pattern simply because I imagine the gain over request response seems marginal.

danisharora099 commented 11 months ago

I'd also propose to make an update to the RFC

The node that makes the request, includes its metadata so that the receiver is aware of it, without requiring an extra interaction.

While the node that is making the request includes its metadata, the receiver (in this case nwaku), opens a new connection and expects the metadata again.

waku-org / nwaku