feat: lightpush new protobuf proposal #20

NagyZoltanPeter · 2024-05-29T16:49:51Z

Based on waku-org/pm#93 this new version of lightpush protobuf protocol definition is proposed.

More specific fail case definition in response
- error status description is now optional rather than hold the factual error cause.
- error codes are more-or-less matching HTTP error code means
Response contains the number of successful relay peers.
Placeholder for request type - for future enhancements

As this is a breaking change compared to the original definition, we may consider to version the protocol allow clients to get adapted to new protocol format.

Open discussion topic: Can't we make pubsub_topic optional for lightpush protocol, indicating to handle message as auto-sharded routing? Currently we require from clients to know about how to compute proper shard from content-topic and make light push request properly with that computed shard topic. Although this can be simple handled on the service side. WDYT?

…ol definition is proposed.

standards/core/lightpush.md

alrevuelta

thanks, left some comments.

standards/core/lightpush.md

jm-clius

Thanks! A couple of comments below, but direction lgtm

standards/core/lightpush.md

… definition as requested.

Ivansete-status

Thanks for it! 🥳
Just added some comments so far that I hope you find useful

standards/core/lightpush.md

NagyZoltanPeter · 2024-06-09T19:17:32Z

#20 (comment)
@Ivansete-status: I think its better idea to put less such logic into the service side. I feel it more client side one. IMHO we might embed it into SDK rather than to the raw protocol.
Because what does LOW_NUMBER_OF_PEERS implies? Is it a fixed number by configuration which the client side might not know about or depends on other factors like due to bandwidth restriction we might decide to unsubscribe from some of the heavy load shards?
Counting with DOS protection, following bandwidth management and other features may come (e.g. light push request type: push to store) that will impose other considerations.

Ivansete-status · 2024-06-09T22:17:50Z

#20 (comment) @Ivansete-status: I think its better idea to put less such logic into the service side. I feel it more client side one. IMHO we might embed it into SDK rather than to the raw protocol.

hahah yes I've noticed that :) The following comment is related to that and why I consider we should have fewer assumptions in light-node side and more on the full-node side: waku-org/nwaku#1004 (comment)

Because what does LOW_NUMBER_OF_PEERS implies? Is it a fixed number by configuration which the client side might not know about or depends on other factors like due to bandwidth restriction we might decide to unsubscribe from some of the heavy load shards?

<<Is it a fixed number by configuration which the client side might not know about>> --> Yes exactly. In that case, the light node may decide to try again to the same full-node in the near future or try to use another full-node.

Counting with DOS protection, following bandwidth management and other features may come (e.g. light push request type: push to store) that will impose other considerations.

Sorry, but I can't quite follow that statement.

SionoiS

LGTM except topic see my comment

jm-clius

Approving as I will be AFK for a couple of days and don't want to be a bottleneck. A couple of comments below. :)

standards/core/lightpush.md

weboko

looks good and added some comments

NagyZoltanPeter · 2024-06-19T14:12:35Z

@weboko , @SionoiS, @Ivansete-status : Please take a last look if acceptable. I'm heading to merge this PR. It's time to make some coding soon. :-)

…arguments

NagyZoltanPeter · 2024-06-20T12:38:07Z

I changed my mind as was thinking of comments from @jm-clius and @weboko... #20 (comment)

I removed SERVICE_UNAVAILABLE and replaced with NO_PEERS_TO_RELAY as in this particular application for lightpush it means exactly that.

Ivansete-status

LGTM! Thanks so much for it and the patience 🙌 !
I'm just leaving a couple of minor comments.
From my PoV we need to make it a bit clearer the meaning of relay_peer_count.

standards/core/lightpush.md

Co-authored-by: Ivan FB <128452529+Ivansete-status@users.noreply.github.com>

NagyZoltanPeter · 2024-06-21T13:51:15Z

@Ivansete-status yes you are right, I will lately extent explanation of relay peer count as it may not matching mesh node count.

…ield.

The ref. comment is answered after others opinions are taken into account. I see not other request up front.

NagyZoltanPeter · 2024-06-21T23:33:28Z

@SionoiS: Please take a look and approve if it is now in acceptable form. Thank you.

standards/core/lightpush.md

jm-clius

Thanks! I have added some minor comments, but lgtm now. :)

standards/core/lightpush.md

Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>

fryorcraken · 2024-07-26T06:16:04Z

standards/core/lightpush.md

+
+## Motivation and Goals
+
+Light nodes with short connection windows and limited bandwidth wish to push messages to other nodes in the Waku network to request message services.<br>


instead of <br> (<br /> seems to be the correct tag), just put an empty new line next.

fryorcraken · 2024-07-26T06:16:54Z

standards/core/lightpush.md

+Additionally, there is sometimes a need for confirmation that a message has been received "by the network"
+(here, at least one node).
+
+`WAKU-LIGHTPUSH` is a request/response protocol for this.


Are we officially moving from WAKU2 @jm-clius ?

Since this spec is not numbered yet (a.k.a. it's not a Vac RFC), the title is not critical.
For this to be promoted to Vac RFC, I suggest we keep using WAKU2 in the official title (also for consistency with other protocols, including the new Store). We can drop the "v2" from the name though - this is simply Waku Lightpush.

The title will be:
XX/WAKU2-LIGHTPUSH

where XX is the Vac-assigned RFC number.

standards/core/lightpush.md

fryorcraken · 2024-07-26T06:19:26Z

standards/core/lightpush.md

+
+If the node is unable to perform the request for some reason, it SHOULD return an error code in `LightPushResponse`.<br>
+
+Once the relay is successful, the `relay_peer_count` will indicate the number of peers that the node has managed to relay the message to. It's important to note that this number may vary depending on the node subscriptions and support for the requested pubsub_topic. The client can use this information to either consider the relay as successful or take further action, such as switching to a lightpush service peer with better connectivity.<br>


fryorcraken · 2024-07-26T06:21:10Z

standards/core/lightpush.md

+
+> The list of error codes is not complete and can be extended in the future.
+
+## Security Considerations


I would add that the content of the response node does not provide guarantees that the messages was relayed and that the requesting node should mitigate appropriately.

fryorcraken · 2024-07-26T06:21:32Z

standards/core/lightpush.md

+
+- Add support attaching RLN proof for the message requested to be relayed.
+- Add support for other request types.
+- Incentivization of the service


Definition of strategy to improve reliable usage of this service

NagyZoltanPeter · 2024-07-26T10:00:08Z

@fryorcraken thank you for the comments, let's continue tracking those here: #29

Based on waku-org/pm#93 this new version of lightpush protobuf protoc…

f7e1057

…ol definition is proposed.

NagyZoltanPeter requested review from richard-ramos, SionoiS, alrevuelta, kaichaosun, chaitanyaprem, jm-clius, gabrielmer, fryorcraken, weboko and Ivansete-status May 29, 2024 16:49

NagyZoltanPeter self-assigned this May 29, 2024

SionoiS reviewed May 29, 2024

View reviewed changes

alrevuelta reviewed May 31, 2024

View reviewed changes

standards/core/lightpush.md Outdated Show resolved Hide resolved

standards/core/lightpush.md Outdated Show resolved Hide resolved

standards/core/lightpush.md Outdated Show resolved Hide resolved

jm-clius requested changes May 31, 2024

View reviewed changes

NagyZoltanPeter mentioned this pull request Jun 7, 2024

chore: update lightpush client API waku-org/nwaku#2774

Open

2 tasks

Answer review notices, rephrase of some sections. Rework the protobuf…

13a3cc4

… definition as requested.

NagyZoltanPeter requested review from SionoiS, jm-clius and alrevuelta June 9, 2024 17:37

Ivansete-status reviewed Jun 9, 2024

View reviewed changes

SionoiS approved these changes Jun 10, 2024

View reviewed changes

SionoiS previously requested changes Jun 10, 2024

View reviewed changes

jm-clius approved these changes Jun 13, 2024

View reviewed changes

weboko reviewed Jun 17, 2024

View reviewed changes

standards/core/lightpush.md Outdated Show resolved Hide resolved

weboko approved these changes Jun 17, 2024

View reviewed changes

Finalize with incorporating review findings.

9a4f375

NagyZoltanPeter requested review from Ivansete-status, jm-clius, SionoiS and weboko June 19, 2024 14:12

Changed SERVICE_UNAVAILABLE to NO_PEERS_TO_RELAY according to review …

ed4e646

…arguments

Ivansete-status approved these changes Jun 21, 2024

View reviewed changes

standards/core/lightpush.md Outdated Show resolved Hide resolved

standards/core/lightpush.md Outdated Show resolved Hide resolved

Update standards/core/lightpush.md

1278f84

Co-authored-by: Ivan FB <128452529+Ivansete-status@users.noreply.github.com>

NagyZoltanPeter added 2 commits June 22, 2024 01:19

Wrap up with last grammar check and explanation of relay_peer_count f…

b2fd96a

…ield.

rename UNSUPPORTED_TOPIC to UNSUPPORTED_PUBSUB_TOPIC

6127e7a

chaitanyaprem reviewed Jun 24, 2024

View reviewed changes

standards/core/lightpush.md Outdated Show resolved Hide resolved

Made relay_peer_count field optional

103b6fc

NagyZoltanPeter requested a review from chaitanyaprem June 26, 2024 12:06

jm-clius approved these changes Jun 27, 2024

View reviewed changes

standards/core/lightpush.md Outdated Show resolved Hide resolved

standards/core/lightpush.md Outdated Show resolved Hide resolved

standards/core/lightpush.md Outdated Show resolved Hide resolved

standards/core/lightpush.md Outdated Show resolved Hide resolved

NagyZoltanPeter and others added 2 commits June 28, 2024 02:56

Apply suggestions from code review

b53d962

Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>

Formatting, added semantic breaks for better readability

7536fb6

NagyZoltanPeter merged commit ffc9d3a into master Jun 28, 2024
0 of 2 checks passed

NagyZoltanPeter mentioned this pull request Jul 2, 2024

Enhance light push protocol waku-org/pm#93

Open

8 tasks

fryorcraken reviewed Jul 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: lightpush new protobuf proposal #20

feat: lightpush new protobuf proposal #20

NagyZoltanPeter commented May 29, 2024 •

edited

Loading

alrevuelta left a comment

jm-clius left a comment

Ivansete-status left a comment

NagyZoltanPeter commented Jun 9, 2024

Ivansete-status commented Jun 9, 2024

SionoiS left a comment •

edited

Loading

jm-clius left a comment

weboko left a comment

NagyZoltanPeter commented Jun 19, 2024

NagyZoltanPeter commented Jun 20, 2024

Ivansete-status left a comment

NagyZoltanPeter commented Jun 21, 2024

NagyZoltanPeter commented Jun 21, 2024

jm-clius left a comment

fryorcraken Jul 26, 2024

fryorcraken Jul 26, 2024

jm-clius Jul 26, 2024

fryorcraken Jul 26, 2024

fryorcraken Jul 26, 2024

fryorcraken Jul 26, 2024

NagyZoltanPeter commented Jul 26, 2024


		## Motivation and Goals

		Light nodes with short connection windows and limited bandwidth wish to push messages to other nodes in the Waku network to request message services.<br>


		If the node is unable to perform the request for some reason, it SHOULD return an error code in `LightPushResponse`.<br>

		Once the relay is successful, the `relay_peer_count` will indicate the number of peers that the node has managed to relay the message to. It's important to note that this number may vary depending on the node subscriptions and support for the requested pubsub_topic. The client can use this information to either consider the relay as successful or take further action, such as switching to a lightpush service peer with better connectivity.<br>


		> The list of error codes is not complete and can be extended in the future.

		## Security Considerations

feat: lightpush new protobuf proposal #20

feat: lightpush new protobuf proposal #20

Conversation

NagyZoltanPeter commented May 29, 2024 • edited Loading

alrevuelta left a comment

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

NagyZoltanPeter commented Jun 9, 2024

Ivansete-status commented Jun 9, 2024

SionoiS left a comment • edited Loading

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

weboko left a comment

Choose a reason for hiding this comment

NagyZoltanPeter commented Jun 19, 2024

NagyZoltanPeter commented Jun 20, 2024

Ivansete-status left a comment

Choose a reason for hiding this comment

NagyZoltanPeter commented Jun 21, 2024

NagyZoltanPeter commented Jun 21, 2024

jm-clius left a comment

Choose a reason for hiding this comment

fryorcraken Jul 26, 2024

Choose a reason for hiding this comment

fryorcraken Jul 26, 2024

Choose a reason for hiding this comment

jm-clius Jul 26, 2024

Choose a reason for hiding this comment

fryorcraken Jul 26, 2024

Choose a reason for hiding this comment

fryorcraken Jul 26, 2024

Choose a reason for hiding this comment

fryorcraken Jul 26, 2024

Choose a reason for hiding this comment

NagyZoltanPeter commented Jul 26, 2024

NagyZoltanPeter commented May 29, 2024 •

edited

Loading

SionoiS left a comment •

edited

Loading