Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: improve the metadata protocol implementation #2161

Open
danisharora099 opened this issue Oct 26, 2023 · 8 comments
Open

chore: improve the metadata protocol implementation #2161

danisharora099 opened this issue Oct 26, 2023 · 8 comments
Assignees
Labels
E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details effort/weeks Estimated to be completed in a few weeks

Comments

@danisharora099
Copy link

danisharora099 commented Oct 26, 2023

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
Copy link
Contributor

SionoiS commented Oct 26, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@fryorcraken 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
@alrevuelta
Copy link
Contributor

alrevuelta commented Oct 27, 2023

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

danisharora099 commented Dec 5, 2023

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.

@gabrielmer gabrielmer added the effort/weeks Estimated to be completed in a few weeks label Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E:1.4: Sharded peer management and discovery See https://github.com/waku-org/pm/issues/67 for details effort/weeks Estimated to be completed in a few weeks
Projects
Status: To Do
Development

No branches or pull requests

6 participants