-
Notifications
You must be signed in to change notification settings - Fork 3
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
14/WAKU2-MESSAGE: Move to Stable #120
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The only blocking comment is on a missing attribute from the wire protocol for RLN rate limit proofs
These protocols are designed to be secure, privacy-preserving, | ||
and censorship-resistant and can run in resource-restricted environments. | ||
At a high level, Waku v2 implements a Pub/Sub messaging pattern over libp2p and | ||
At a high level, | ||
[10/WAKU2](/waku/standards/core/10/waku2.md) implements a publish/subscribe messaging pattern over libp2p and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely sure, but should other RFCs referenced in a stable
RFC also be stable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a rule define in 1/COSS. Currently, once an RFC becomes stable only the links can be updated afterwards. So if an RFC, referenced in a stable RFC, becomes deprecated it can not be removed.
### Reliability of the `timestamp` attribute | ||
|
||
The Waku message `timestamp` attribute is set by the sender. | ||
The message `timestamp` attribute is set by the sender. | ||
Therefore, because message timestamps aren’t independently verified, | ||
this attribute is prone to exploitation and misuse. | ||
It should not solely be relied upon for operations such as message ordering. | ||
For example, a malicious actor can arbitrarily set the `timestamp` of a Waku message | ||
For example, a malicious actor can arbitrarily set the `timestamp` of a `WakuMessage` | ||
to a high value so that it always shows up as the most recent message | ||
in a chat application. | ||
Applications using Waku messages’ `timestamp` attribute | ||
are recommended to use additional methods for more robust message ordering. | ||
Applications using [10/WAKU2](/waku/standards/core/10/waku2.md) messages’ `timestamp` attribute | ||
are RECOMMENDED to use additional methods for more robust message ordering. | ||
An example of how to deal with message ordering against adversarial message timestamps | ||
can be found in the Status protocol, | ||
see [62/STATUS-PAYLOADS](../../../../status/62/payloads.md/#clock-vs-timestamp-and-message-ordering). | ||
see [62/STATUS-PAYLOADS](/status/62/payloads.md/#clock-vs-timestamp-and-message-ordering). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true anymore, as the timestamp
are checked together with the RLN proof. Perhaps we just remove this entire section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this should be removed. I believe this security consideration is still relevant if an implementation does not include RLN. But if the RLN proof timestamp
attribute is a solution to deter malicious actors, should this be described in this section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik the timestamp
validation is still WIP and not specified properly (so we can't reference it). I wouldn't suddenly introduce RLN concepts here. Perhaps it's enough to simply state the problem as is done in this section.
If this attribute is set to `true`, the message SHOULD be interpreted as ephemeral. | ||
If, instead, the attribute is omitted or set to `false`, | ||
the message SHOULD be interpreted as non-ephemeral. | ||
|
||
## Wire Format | ||
|
||
The Waku message wire format is specified using [protocol buffers v3](https://developers.google.com/protocol-buffers/). | ||
The `WakuMessage` wire format is specified using [protocol buffers v3](https://developers.google.com/protocol-buffers/). | ||
|
||
```protobuf | ||
syntax = "proto3"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I somehow can't add a suggestion directly in the wire protocol below. In order to be accurate, we should one more field between meta
and ephemeral
:
optional bytes rate_limit_proof = 21;
and a description under "Message Attributes":
- The
rate_limit_proof
attribute, if present, contains a rate limit proof encoded as per 17/WAKU2-RLN-RELAY
Co-authored-by: Hanno Cornelius <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. LGTM.
### Reliability of the `timestamp` attribute | ||
|
||
The Waku message `timestamp` attribute is set by the sender. | ||
The message `timestamp` attribute is set by the sender. | ||
Therefore, because message timestamps aren’t independently verified, | ||
this attribute is prone to exploitation and misuse. | ||
It should not solely be relied upon for operations such as message ordering. | ||
For example, a malicious actor can arbitrarily set the `timestamp` of a Waku message | ||
For example, a malicious actor can arbitrarily set the `timestamp` of a `WakuMessage` | ||
to a high value so that it always shows up as the most recent message | ||
in a chat application. | ||
Applications using Waku messages’ `timestamp` attribute | ||
are recommended to use additional methods for more robust message ordering. | ||
Applications using [10/WAKU2](/waku/standards/core/10/waku2.md) messages’ `timestamp` attribute | ||
are RECOMMENDED to use additional methods for more robust message ordering. | ||
An example of how to deal with message ordering against adversarial message timestamps | ||
can be found in the Status protocol, | ||
see [62/STATUS-PAYLOADS](../../../../status/62/payloads.md/#clock-vs-timestamp-and-message-ordering). | ||
see [62/STATUS-PAYLOADS](/status/62/payloads.md/#clock-vs-timestamp-and-message-ordering). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik the timestamp
validation is still WIP and not specified properly (so we can't reference it). I wouldn't suddenly introduce RLN concepts here. Perhaps it's enough to simply state the problem as is done in this section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this!
Updating 14/WAKU2-MESSAGE rfc. Suggesting a move to stable.