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

Add a flag to require minimum number of peers to publish a message on relay #1004

Closed
richard-ramos opened this issue Jun 14, 2022 · 6 comments
Assignees
Labels
effort/days Estimated to be completed in a few days, less than a week enhancement New feature or request

Comments

@richard-ramos
Copy link
Member

This should be used in Waku Relay to determine if a message should be broadcasted or not depending on the number of relay peers interested in a pubsub topic.

It is useful to avoid 'losing' messages due to these being broadcasted to no one, while requiring a minimum number of peers would at least inform the dev/user that a message cannot be broadcasted.

This should also be used for determining if a message was published or not in lightpush, since right now it will return a success message, regardless if the message was broadcasted to peers or not.

In go-waku I created the flag --min-relay-peers-to-publish with a default value of 1. Maybe the same flag could be used in nwaku?

@Ivansete-status
Copy link
Collaborator

I moved this to priority because I consider this will help us to enhance the message reliability

@Ivansete-status Ivansete-status added the effort/days Estimated to be completed in a few days, less than a week label May 8, 2024
@chair28980 chair28980 added the enhancement New feature or request label May 15, 2024
@NagyZoltanPeter
Copy link
Contributor

I moved this to priority because I consider this will help us to enhance the message reliability

@Ivansete-status @richard-ramos : Please consider this if we can close or still necessary in the scope of #2722

My opinion we don't need it if the protocol is enhanced that the reply will contain the number of peers the message successfully relayed to. WDYT?

@Ivansete-status
Copy link
Collaborator

I moved this to priority because I consider this will help us to enhance the message reliability

@Ivansete-status @richard-ramos : Please consider this if we can close or still necessary in the scope of #2722

My opinion we don't need it if the protocol is enhanced that the reply will contain the number of peers the message successfully relayed to. WDYT?

Hey there! IMO, this (#1004) is complementary to the #2722 and it seems pretty straightforward to implement. I think is good to have that instead of having the hard-coded value that we have nowadays. For example, I think we should avoid the following:

if publishedCount == 0:

... and instead, make the wakuRelay.publish to return a Result that will give all possible detail in case of error. The min-relay-peers-to-publish attribute value can be passed to relay on start up.

@NagyZoltanPeter
Copy link
Contributor

I moved this to priority because I consider this will help us to enhance the message reliability

@Ivansete-status @richard-ramos : Please consider this if we can close or still necessary in the scope of #2722
My opinion we don't need it if the protocol is enhanced that the reply will contain the number of peers the message successfully relayed to. WDYT?

Hey there! IMO, this (#1004) is complementary to the #2722 and it seems pretty straightforward to implement. I think is good to have that instead of having the hard-coded value that we have nowadays. For example, I think we should avoid the following:

if publishedCount == 0:

... and instead, make the wakuRelay.publish to return a Result that will give all possible detail in case of error. The min-relay-peers-to-publish attribute value can be passed to relay on start up.

Do you min-relay-peers-to-publish should be a startup option? It might work for Status but not that generic solution.
IMHO we should leave to decide success or failure to the client including number of peers the message is published.
From the service node point of view even one is success because it means there is no any hard obstacles in sending the message onto the network.

On the other suggestion, that can be harder, as current relay implementation completely relies on gossip-sub implementation and that handles all errors internally while returning number of peers only. But indeed we need to examine that option too.

@Ivansete-status
Copy link
Collaborator

Do you min-relay-peers-to-publish should be a startup option?

At first, I was thinking of considering it as a const but, having it as a parameter will give us more flexibility to tweak it while debugging reliability tasks.


It might work for Status but not that generic solution.
IMHO we should leave to decide success or failure to the client including number of peers the message is published.
From the service node point of view even one is success because it means there is no any hard obstacles in sending the message onto the network.

I see your point and I agree that making the light node to decide would bring more flexibility. IMO, the relay node is the entity that should decide whether the message was properly published or not because it will also perform the reliability protocol by leveraging on Store requests. Summarizing, I think the light node should not make any assumption as to whether a certain message was properly published or not.


On the other suggestion, that can be harder, as current relay implementation completely relies on gossip-sub implementation and that handles all errors internally while returning number of peers only. But indeed we need to examine that option too.

Excuse me, what other suggestion do you mean :D ?

@NagyZoltanPeter
Copy link
Contributor

I think this is covered with #2722, although there will be no minimum peers required but response will contain the number of peers the message is published.

@github-project-automation github-project-automation bot moved this from Priority to Done in Waku Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/days Estimated to be completed in a few days, less than a week enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

6 participants