Fix mailserver spec #64

adambabik · 2019-11-21T12:58:59Z

Closes #55

kdeme

Looks clear enough to me to implement. Did mostly some language suggestions (feel free to improve).

However, symmetric key item + the issue of the single versus list of envelopes are still open questions to me.

status-whisper-mailserver-spec.md

kdeme · 2019-11-21T15:13:30Z

status-whisper-mailserver-spec.md

+`Limit`: 4-byte wide unsigned integer limiting the number of returned envelopes
+`Cursor`: 32-byte wide array of a cursor returned from the previous request (optional)
+
+The envelope MUST be signed with a symmetric key agreed between the requester and Mailserver.


How is this symmetric key agreed up on?

Just realized that this does not need to be part of the spec and can be arranged out of band (layer higher, etc.)
However, should we then not leave the option open to use either a symmetric or asymmetric key? Such as with the regular posting of messages?

Yes, I should make it more generic. It just must be encrypted somehow because each envelope requires that.

Seems like a feature enhancement request to spec to me, I suggest we note that it has to be agreed out of band in implementer notes in waku spec

For Status app it is hardcoded, iirc.

It is hardcoded, correct. I will keep it as it is then for now.

kdeme · 2019-11-21T15:17:25Z

status-whisper-mailserver-spec.md

+
+### Receiving historic messages
+
+Historic messages MUST be sent to a peer with a packet with a P2P Message code (`0x7f`) followed by a single Whisper envelope OR an array of Whisper envelopes. A peer receiving historic message MUST handle both cases.


This is a bit problematic.

I believe it goes against the waku/0 or Whisper/6 spec as there the P2P message code is defined with a single envelope as parameter.

In our nim implementation (DSL) it is not supported to do such thing (1 envelope or list of envelopes) .
So I would prefer to have waku/0 altered with either always a list of envelopes (can be 1). Or to add a message packet for multiple envelopes.

I believe it goes against the waku/0 or Whisper/6 spec as there the P2P message code is defined with a single envelope as parameter.

Correct but in the case when a mailserver returns a lot of messages, sending them one by one adds huge overhead. That's why we batched them.

In our nim implementation (DSL) it is not supported to do such thing (1 envelope or list of envelopes) .
So I would prefer to have waku/0 altered with either always a list of envelopes (can be 1). Or to add a message packet for multiple envelopes.

I would opt for multiple envelopes then. If one wants to send just one, they can do that as one-item array. The problem is that it makes Waku incompatible with Whisper... Can we solve this in the bridge?

Co-Authored-By: kdeme <kim.demey@gmail.com>

status-whisper-mailserver-spec.md

Co-Authored-By: Dean Eigenmann <dean.eigenmann@icloud.com>

oskarth

Much better than before, right level of abstraction now!

Some questions of clarifications on interface

oskarth · 2019-11-22T04:15:35Z

status-whisper-mailserver-spec.md

+
+Historic messages MUST be sent to a peer with as packet with a P2P Message code (`0x7f`) followed by a single Whisper envelope OR an array of Whisper envelopes. A peer receiving historic message MUST handle both cases.
+
+In order to accept a P2P Message packet, a node MUST trust a selected Mailserver.


Can we be more specific about what we mean by "trust"? I suppose this means making certain envelope invariants / checks optional?

E.g. here is roughly the logic in nim-eth as far as I can tell https://github.com/status-im/nim-eth/blob/master/eth/p2p/rlpx_protocols/whisper_protocol.nim#L223-L225

Good point. Generally, a trusted peer in Whisper terminology means it's allowed to send p2p messages.

oskarth · 2019-11-22T04:18:32Z

status-whisper-mailserver-spec.md

+`Upper`: 4-byte wide unsigned integer (UNIX time in seconds; newest requested envelope's creation time)
+`Bloom`: 64-byte wide array of Whisper topics encoded in a bloom filter to filter envelopes
+`Limit`: 4-byte wide unsigned integer limiting the number of returned envelopes
+`Cursor`: 32-byte wide array of a cursor returned from the previous request (optional)


How is this returned exactly?

What does it correspond to? I suppose an address of some kind that can be used for that specific mailserver to skip previously fetched messages? Does it send messages 'before' this cursor or after? How does it impact Lower and Upper, i.e. does it cap it to be higher than Lower and lower than Higher according to some logic?

It's up to the implementation. Also, it's not possible to query one mailserver and use the obtained cursor to query another one -- I will make it clear. It is so because each mailserver can obtain messages in different order or even may miss some. So it's Whisper property not this spec.

All other questions are up to the implementation due to what I wrote above.

oskarth · 2019-11-22T04:20:01Z

status-whisper-mailserver-spec.md

+`Limit`: 4-byte wide unsigned integer limiting the number of returned envelopes
+`Cursor`: 32-byte wide array of a cursor returned from the previous request (optional)
+
+The envelope MUST be signed with a symmetric key agreed between the requester and Mailserver.


Seems like a feature enhancement request to spec to me, I suggest we note that it has to be agreed out of band in implementer notes in waku spec

For Status app it is hardcoded, iirc.

oskarth · 2019-11-22T04:22:24Z

status-whisper-mailserver-spec.md

+
+### Receiving historic messages
+
+Historic messages MUST be sent to a peer with as packet with a P2P Message code (`0x7f`) followed by a single Whisper envelope OR an array of Whisper envelopes. A peer receiving historic message MUST handle both cases.


The single or array seems a bit suboptimal to me, ideally this should be just array and then it can even be empty, just like Messages packet.

Seems like an oversight in initial EIP627, perhaps this interface is something we can fix for later versions of waku.

Yes, agree. That's why I suggested either changing this packet like you say -> array which can be empty. Or adding a new packet id, which does this, to the waku spec.

I replied with the same suggestions. Just note that it will be incompatible with EIP-627.

kdeme · 2019-11-22T09:24:21Z

By the way, are any of the additional packet ids (still) used for mailserver functionality?

p2pRequestComplete, p2pSyncRequest, p2pSyncResponse sound related. (https://github.com/status-im/nim-eth/blob/master/eth/p2p/rlpx_protocols/whisper_protocol.nim#L228)

adambabik · 2019-11-22T12:36:02Z

@kdeme p2pSync* are not relevant. They are used to sync between mailservers like when you start a new one but want to get history from another one. I would not worry about this now.

p2pRequestComplete might be actually worth to implement. Otherwise, it's unknown when all messages were sent from a mailserver. Unless we have a better idea for this.

oskarth

LGTM

adambabik · 2019-11-25T07:55:52Z

@kdeme I describe P2P Request Complete code in 4eb554a.

kdeme

OK for me now, however I think once we move to a Waku version of this spec we should readdress this:
It is incompatible with the original Whisper spec (EIP-627) because it allows only a single envelope, however, an array of envelopes is much more performant. In order to stay compatible with EIP-627, a peer receiving historic message MUST handle both cases.

No need to mention that much then anymore about compatibility with EIP-627.
It can be done in the section which explains the differences between Whisper and Waku somewhere?

adambabik · 2019-11-25T09:04:19Z

@kdeme If we have a section about differences, we can reference it here. But I think we must mention what is incompatible for clarify in the relevant sections. I agree that explanation can be in a different place.

It can be done in the section which explains the differences between Whisper and Waku somewhere?

Waku is speced in a different repo. Here we just want to describe the current state of the protocol.

kdeme · 2019-11-25T09:15:50Z

@adambabik Yeah, for the current state of the protocol I'm perfectly fine with it. As we base ourselves on EIP-627 in those specs / implementations.

Just a reminder for later.

fix mailserver spec

9705c27

adambabik requested review from cammellos, oskarth, decanus and kdeme November 21, 2019 12:58

kdeme reviewed Nov 21, 2019

View reviewed changes

adambabik mentioned this pull request Nov 21, 2019

Waku 0: Mail Servers vacp2p/rfc#36

Closed

Apply suggestions from code review

f193ef8

Co-Authored-By: kdeme <kim.demey@gmail.com>

decanus suggested changes Nov 21, 2019

View reviewed changes

status-whisper-mailserver-spec.md Outdated Show resolved Hide resolved

status-whisper-mailserver-spec.md Outdated Show resolved Hide resolved

status-whisper-mailserver-spec.md Outdated Show resolved Hide resolved

Apply suggestions from code review

c65967c

Co-Authored-By: Dean Eigenmann <dean.eigenmann@icloud.com>

decanus approved these changes Nov 21, 2019

View reviewed changes

oskarth reviewed Nov 22, 2019

View reviewed changes

F

65c4b0a

oskarth approved these changes Nov 25, 2019

View reviewed changes

update P2P Request Complete code

4eb554a

kdeme approved these changes Nov 25, 2019

View reviewed changes

adambabik merged commit b8cabfd into master Nov 25, 2019

adambabik deleted the fix-mailserver-spec branch November 25, 2019 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mailserver spec #64

Fix mailserver spec #64

adambabik commented Nov 21, 2019

kdeme left a comment •

edited

Loading

kdeme Nov 21, 2019

kdeme Nov 21, 2019

adambabik Nov 21, 2019

oskarth Nov 22, 2019

adambabik Nov 22, 2019

kdeme Nov 21, 2019

adambabik Nov 21, 2019

oskarth left a comment

oskarth Nov 22, 2019

adambabik Nov 22, 2019

oskarth Nov 22, 2019

adambabik Nov 22, 2019

oskarth Nov 22, 2019

oskarth Nov 22, 2019

kdeme Nov 22, 2019 •

edited

Loading

adambabik Nov 22, 2019

kdeme commented Nov 22, 2019

adambabik commented Nov 22, 2019

oskarth left a comment

adambabik commented Nov 25, 2019

kdeme left a comment

adambabik commented Nov 25, 2019

kdeme commented Nov 25, 2019


		### Receiving historic messages

		Historic messages MUST be sent to a peer with a packet with a P2P Message code (`0x7f`) followed by a single Whisper envelope OR an array of Whisper envelopes. A peer receiving historic message MUST handle both cases.


		Historic messages MUST be sent to a peer with as packet with a P2P Message code (`0x7f`) followed by a single Whisper envelope OR an array of Whisper envelopes. A peer receiving historic message MUST handle both cases.

		In order to accept a P2P Message packet, a node MUST trust a selected Mailserver.

Fix mailserver spec #64

Fix mailserver spec #64

Conversation

adambabik commented Nov 21, 2019

kdeme left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarth 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

Choose a reason for hiding this comment

kdeme Nov 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdeme commented Nov 22, 2019

adambabik commented Nov 22, 2019

oskarth left a comment

Choose a reason for hiding this comment

adambabik commented Nov 25, 2019

kdeme left a comment

Choose a reason for hiding this comment

adambabik commented Nov 25, 2019

kdeme commented Nov 25, 2019

kdeme left a comment •

edited

Loading

kdeme Nov 22, 2019 •

edited

Loading