Improve 14/WAKU2-MESSAGE RFC format etc #553

LNSD · 2022-11-10T23:24:01Z

The goal is to stabilize the 14/WAKU2-MESSAGE RFC making it comply with VAC's RFC template.

From @oskarth comment:

There's a lot of things going on here and I'm not sure I fully understand the purpose of some of the changes...

I'd separate out:

making it compliant with format (formatting)

functional change

promotion to stable (this should be a one line PR once everything has been stable for long time)

This PR touches a lot of things right now

This PR covers the first item. It improves formatting/compliance with the RFC template and some style improvements and paraphrasing for clarity.

This PR resolves #551

LNSD · 2022-11-10T23:26:39Z

content/docs/rfcs/14/README.md

-status: draft
+name: Waku v2 message RFC
+status: stable
+category: Standards Track


I am not sure if I picked the correct category for this RFC. Is this the right category for this RFC?

Where one can find descriptions of these categories?

Here you can find the template, but I cannot find the different categories description

think this is correct but defer to @kaiserd

I will add a category description to the COSS.

LNSD · 2022-11-10T23:30:47Z

content/docs/rfcs/14/README.md

 ```protobuf
 syntax = "proto3";

 message WakuMessage {
  bytes payload = 1;
-  string contentTopic = 2;
+  string content_topic = 2;
  uint32 version = 3;
  sint64 timestamp = 10;
  bool ephemeral = 31;
 }
 ```


TO-DO:

Add this protobuf to the Waku protocol buffers' repository

PR opened here: waku-org/waku-proto#1

Why? Shouldn't field names be under_score based on https://developers.google.com/protocol-buffers/docs/style?

Shouldn't field names be under_score based on developers.google.com/protocol-buffers/docs/style?

Yes, this is what I did here: contentTopic -> content_topic

LNSD · 2022-11-10T23:45:11Z

content/docs/rfcs/14/README.md

+# References

-Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/).
+TBD


I am unsure exactly what to put here 🤔

I'd say the links you have already put in the text.

It should be summarized links that are in text

staheri14

Thanks, LGTM!

content/docs/rfcs/14/README.md

staheri14 · 2022-11-10T23:49:31Z

content/docs/rfcs/14/README.md

-status: draft
+name: Waku v2 message RFC
+status: stable
+category: Standards Track


Where one can find descriptions of these categories?

content/docs/rfcs/14/README.md

staheri14 · 2022-11-11T00:04:18Z

content/docs/rfcs/14/README.md


-### Version 1
+- **Version 0:**  The payload SHOULD be interpreted as unencrypted; additionally, it CAN indicate that the message payload has been encrypted at the application layer.


Suggested change

- **Version 0:** The payload SHOULD be interpreted as unencrypted; additionally, it CAN indicate that the message payload has been encrypted at the application layer.

- **Version 0:** The `payload` SHOULD be interpreted as unencrypted; additionally, it CAN indicate that the message payload has been encrypted at the application layer.

staheri14 · 2022-11-11T00:04:44Z

content/docs/rfcs/14/README.md


-### Version 1
+- **Version 0:**  The payload SHOULD be interpreted as unencrypted; additionally, it CAN indicate that the message payload has been encrypted at the application layer.


Why not MUST? i.e., MUST be interpreted?

Same question for you, why not SHOULD? Can you elaborate?

Yes, sure; for reference, based on https://rfc.vac.dev/spec/1/#language and the document referred therein https://www.rfc-editor.org/rfc/rfc2119, the term SHOULD indicates recommendation whereas MUST indicates an absolute requirement. So in here, when the version is 0 (or 1), to me, the payload is absolutely unencrypted (or encrypted), i.e., there is no doubt. Hence the term MUST looks more appropriate.

Not sure I agree, @staheri14. When version is 0 it simply means that Waku is not enforcing some encryption scheme. Applications are free to encrypt the payload as they wish (and most do). SHOULD is therefore more appropriate, IMO.

Applications are free to encrypt the payload as they wish (and most do). SHOULD is therefore more appropriate, IMO.

I see what you mean, right, for version 0 it makes sense to use SHOULD. But in the other cases i.e., version value of 1 and 2, I think that needs to be updated to MUST following my previous reasoning.

Or, are we going to argue that if the version is e.g., 1, the application still can decide to use a different encryption scheme or none at all? @jm-clius if that is the case, then SHOULD is appropriate.

Indeed. I don't think from an RFC perspective we can force applications to use version in a certain way, although it's clearly recommended.

staheri14 · 2022-11-11T00:05:52Z

content/docs/rfcs/14/README.md

+- **Version 0:**  The payload SHOULD be interpreted as unencrypted; additionally, it CAN indicate that the message payload has been encrypted at the application layer.
+- **Version 1:** The payload SHOULD be encrypted using Waku v1 payload encryption. [26/WAKU-PAYLOAD](/spec/26).
+This provides asymmetric and symmetric encryption. The key agreement is performed out of band. And provides an encrypted signature and padding for some form of unlinkability.
+- **Version 2:** The payload SHOULD be encrypted using [35/WAKU2-NOISE](/spec/35).


Ditto about SHOULD vs MUST here

staheri14 · 2022-11-11T00:08:24Z

content/docs/rfcs/14/README.md

+# References

-Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/).
+TBD


I'd say the links you have already put in the text.

content/docs/rfcs/14/README.md

LNSD · 2022-11-11T01:01:31Z

@staheri14 I see some of your questions as too vague.

What do you expect me to do there? Could you be more precise about what you are suggesting? Or the things that are unclear to you?

For the sake of time, please let's try to be more concise in our review comments.

staheri14 · 2022-11-11T01:16:28Z

What do you expect me to do there? Could you be more precise about what you are suggesting? Or the things that are unclear to you?

@LNSD

I am not sure which comment needs more clarification. let me know, and I will be happy to explain. :)

staheri14 · 2022-11-11T01:29:55Z

@LNSD added further clarifications to my comments as I saw fit.

content/docs/rfcs/14/README.md

oskarth · 2022-11-11T03:29:37Z

content/docs/rfcs/14/README.md

-status: draft
+name: Waku v2 message RFC
+status: stable
+category: Standards Track


think this is correct but defer to @kaiserd

content/docs/rfcs/14/README.md

oskarth · 2022-11-11T03:33:30Z

content/docs/rfcs/14/README.md

 ```protobuf
 syntax = "proto3";

 message WakuMessage {
  bytes payload = 1;
-  string contentTopic = 2;
+  string content_topic = 2;
  uint32 version = 3;
  sint64 timestamp = 10;
  bool ephemeral = 31;
 }
 ```


Why? Shouldn't field names be under_score based on https://developers.google.com/protocol-buffers/docs/style?

content/docs/rfcs/14/README.md

oskarth · 2022-11-11T03:35:23Z

content/docs/rfcs/14/README.md


-This indicates that the payload SHOULD be either unencrypted or that encryption is done at a separate layer outside of Waku.
+The waku message payload MAY be encrypted. The message `version` attribute indicates the schema used to encrypt the payload data.


Oh diff is confusing, this line is no longer referring to version 0?

content/docs/rfcs/14/README.md

oskarth · 2022-11-11T03:36:40Z

content/docs/rfcs/14/README.md


-## Reliability of the WakuMessage timestamp


Why is this removed

It is not removed, but with the format changes, it seems that the diff is not detecting it. Check line 113.

oskarth · 2022-11-11T03:36:57Z

content/docs/rfcs/14/README.md

+# References

-Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/).
+TBD


It should be summarized links that are in text

oskarth · 2022-11-11T03:38:16Z

There's a lot of things going on here and I'm not sure I fully understand the purpose of some of the changes...

I'd separate out:

making it compliant with format (formatting)
functional change
promotion to stable (this should be a one line PR once everything has been stable for long time)

This PR touches a lot of things right now

jm-clius

Thanks! I think this is on track to getting our message spec to stable. See my comments below. I agree that this spec should be decoupled from other protocol specs (because it is the foundational one), but it may be worth still adding concrete examples to clarify concepts such as ephemeral.

content/docs/rfcs/14/README.md

jm-clius · 2022-11-11T09:03:47Z

content/docs/rfcs/14/README.md


-See [13/WAKU2-STORE](/spec/13) for more details.
+The `timestamp` attribute signifies the time at which the message was generated by its sender. This field SHOULD contain the Unix epoch time in nanoseconds. If the attribute is omitted, it SHOULD be interpreted as timestamp 0.


If the attribute is omitted, it SHOULD be interpreted as timestamp 0.

I'm not sure why this clause is necessary? Couldn't it also be interpreted as unset to the same effect by some clients?

jm-clius · 2022-11-11T09:05:22Z

content/docs/rfcs/14/README.md


-## Payloads
+The `ephemeral` attribute signifies the transient nature of the message. If this field is set to `true`, the message SHOULD be interpreted as ephemeral. Else, if the field is omitted or set to `false`, the message SHOULD be interpreted as non-ephemeral.


I understand that we probably don't want any coupling between this spec and the store specification, but I think it's OK to have an example here of what ephemeral MAY imply. Something like:
For example, nodes interested in storing historical messages MAY choose to ignore ephemeral messages.

jm-clius · 2022-11-11T09:10:12Z

content/docs/rfcs/14/README.md


-### Version 1
+- **Version 0:**  The payload SHOULD be interpreted as unencrypted; additionally, it CAN indicate that the message payload has been encrypted at the application layer.


Not sure I agree, @staheri14. When version is 0 it simply means that Waku is not enforcing some encryption scheme. Applications are free to encrypt the payload as they wish (and most do). SHOULD is therefore more appropriate, IMO.

jm-clius · 2022-11-11T09:11:09Z

content/docs/rfcs/14/README.md


-### Version 1
+- **Version 0:**  The payload SHOULD be interpreted as unencrypted; additionally, it CAN indicate that the message payload has been encrypted at the application layer.


Suggested change

- **Version 0:** The payload SHOULD be interpreted as unencrypted; additionally, it CAN indicate that the message payload has been encrypted at the application layer.

- **Version 0:** The payload SHOULD be interpreted as unencrypted; additionally, it MAY indicate that the message payload has been encrypted at the application layer.

CAN is not a recognised RFC keyword.

content/docs/rfcs/14/README.md

s1fr0

LGTM ! Left some comments/nitpick

staheri14

Thanks, LGTM!

kaiserd

LGTM 👍
I'd change some of the wording, especially the "One may" list.
I'll do an editing pass addressing that in a separate PR.
Generally, I suggest adding an editing pass by an editor (position we need to fill) before a PR can enter the stabalization phase.
As @oskarth suggested, the actual stabilization PR should just change the status, and it should be done after the respective RFC has been with out changes for a while.

LNSD added the track:waku-specs Waku specs track (RAD) label Nov 10, 2022

LNSD requested review from oskarth, kaiserd, s1fr0, staheri14 and jm-clius November 10, 2022 23:24

LNSD self-assigned this Nov 10, 2022

LNSD force-pushed the stabilize-14-waku-message branch from 95a6582 to c7cccae Compare November 10, 2022 23:24

LNSD commented Nov 10, 2022

View reviewed changes

LNSD force-pushed the stabilize-14-waku-message branch from c7cccae to f85d1d4 Compare November 10, 2022 23:27

LNSD commented Nov 10, 2022

View reviewed changes

LNSD force-pushed the stabilize-14-waku-message branch from f85d1d4 to 0b2283b Compare November 10, 2022 23:41

LNSD commented Nov 10, 2022

View reviewed changes

staheri14 approved these changes Nov 11, 2022

View reviewed changes

oskarth reviewed Nov 11, 2022

View reviewed changes

jm-clius reviewed Nov 11, 2022

View reviewed changes

LNSD force-pushed the stabilize-14-waku-message branch from 0b2283b to 4adaeb5 Compare November 14, 2022 09:37

LNSD commented Nov 14, 2022

View reviewed changes

content/docs/rfcs/14/README.md Show resolved Hide resolved

LNSD force-pushed the stabilize-14-waku-message branch 5 times, most recently from 10b6605 to c701d23 Compare November 14, 2022 12:06

LNSD requested review from oskarth, jm-clius and staheri14 November 14, 2022 14:07

LNSD force-pushed the stabilize-14-waku-message branch 3 times, most recently from 1ac49dd to c918ed2 Compare November 14, 2022 15:12

s1fr0 reviewed Nov 14, 2022

View reviewed changes

content/docs/rfcs/14/README.md Outdated Show resolved Hide resolved

s1fr0 reviewed Nov 14, 2022

View reviewed changes

content/docs/rfcs/14/README.md Outdated Show resolved Hide resolved

s1fr0 reviewed Nov 14, 2022

View reviewed changes

content/docs/rfcs/14/README.md Outdated Show resolved Hide resolved

s1fr0 reviewed Nov 14, 2022

View reviewed changes

content/docs/rfcs/14/README.md Outdated Show resolved Hide resolved

s1fr0 approved these changes Nov 14, 2022

View reviewed changes

promote 14/WAKU2-MESSAGE RFC to stable state

bbc5b98

LNSD force-pushed the stabilize-14-waku-message branch from c918ed2 to bbc5b98 Compare November 14, 2022 22:22

staheri14 approved these changes Nov 14, 2022

View reviewed changes

oskarth approved these changes Nov 15, 2022

View reviewed changes

oskarth merged commit 5b798cc into master Nov 15, 2022

oskarth deleted the stabilize-14-waku-message branch November 15, 2022 02:12

oskarth changed the title ~~Promote 14/WAKU2-MESSAGE RFC to stable state~~ Improve 14/WAKU2-MESSAGE RFC format etc Nov 15, 2022

kaiserd reviewed Nov 15, 2022

View reviewed changes

fryorcraken mentioned this pull request Jan 31, 2023

Protobuf decode absent values as default waku-org/js-waku#921

Closed


		### Version 1
		- Version 0: The payload SHOULD be interpreted as unencrypted; additionally, it CAN indicate that the message payload has been encrypted at the application layer.

	- Version 0: The payload SHOULD be interpreted as unencrypted; additionally, it CAN indicate that the message payload has been encrypted at the application layer.
	- Version 0: The `payload` SHOULD be interpreted as unencrypted; additionally, it CAN indicate that the message payload has been encrypted at the application layer.


		This indicates that the payload SHOULD be either unencrypted or that encryption is done at a separate layer outside of Waku.
		The waku message payload MAY be encrypted. The message `version` attribute indicates the schema used to encrypt the payload data.


		See [13/WAKU2-STORE](/spec/13) for more details.
		The `timestamp` attribute signifies the time at which the message was generated by its sender. This field SHOULD contain the Unix epoch time in nanoseconds. If the attribute is omitted, it SHOULD be interpreted as timestamp 0.


		## Payloads
		The `ephemeral` attribute signifies the transient nature of the message. If this field is set to `true`, the message SHOULD be interpreted as ephemeral. Else, if the field is omitted or set to `false`, the message SHOULD be interpreted as non-ephemeral.

Improve 14/WAKU2-MESSAGE RFC format etc #553

Improve 14/WAKU2-MESSAGE RFC format etc #553

Conversation

LNSD commented Nov 10, 2022 • edited Loading

LNSD Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LNSD Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

LNSD Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jm-clius Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

staheri14 Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LNSD commented Nov 11, 2022 • edited Loading

staheri14 commented Nov 11, 2022

staheri14 commented Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarth commented Nov 11, 2022

jm-clius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jm-clius Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1fr0 left a comment

Choose a reason for hiding this comment

staheri14 left a comment

Choose a reason for hiding this comment

kaiserd left a comment • edited Loading

Choose a reason for hiding this comment

LNSD commented Nov 10, 2022 •

edited

Loading

LNSD Nov 10, 2022 •

edited

Loading

LNSD Nov 10, 2022 •

edited

Loading

LNSD Nov 11, 2022 •

edited

Loading

jm-clius Nov 11, 2022 •

edited

Loading

staheri14 Nov 14, 2022 •

edited

Loading

LNSD commented Nov 11, 2022 •

edited

Loading

staheri14 commented Nov 11, 2022 •

edited

Loading

jm-clius Nov 11, 2022 •

edited

Loading

kaiserd left a comment •

edited

Loading