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

Waku mail client implementation #141

Merged
merged 4 commits into from Dec 12, 2019
Merged

Waku mail client implementation #141

merged 4 commits into from Dec 12, 2019

Conversation

@kdeme
Copy link
Contributor

kdeme commented Dec 5, 2019

Starting on mail client as in https://github.com/vacp2p/specs/blob/master/waku.md#mailserver-and-client .

Some parts are unclear:

  • cursor -> This means it has to be send back, through the same packet id? Why not some other packet id (a reponse)? And why the array?
  • unclear which validation checking of envelope is done in the P2P Request packet. All like normal messages?
  • unclear which validation checking is done on envelopes for P2P Message packet. TTL for sure is skipped, but anything else?
@oskarth

This comment has been minimized.

Copy link
Member

oskarth commented Dec 6, 2019

(see below)

@adambabik

This comment has been minimized.

Copy link
Member

adambabik commented Dec 6, 2019

cursor -> This means it has to be send back, through the same packet id? Why not some other packet id (a reponse)? And why the array?

No, it's different packet code. The documentation misses a fragment about a packet 125 which is implemented in our for of Whisper.

I will take care of this today. Issue.

unclear which validation checking of envelope is done in the P2P Request packet. All like normal messages?

Here is how a regular envelope is validated vs how a p2p envelope is validated. p2p messages pretty much are not validated at all because they are assumed that they were already validated by mailserver.

@adambabik

This comment has been minimized.

Copy link
Member

adambabik commented Dec 6, 2019

The current spec explains how to handle this additional packet with the response.

kdeme added 2 commits Dec 5, 2019
@kdeme

This comment has been minimized.

Copy link
Contributor Author

kdeme commented Dec 11, 2019

@adambabik

Here is how a regular envelope is validated vs how a p2p envelope is validated. p2p messages pretty much are not validated at all because they are assumed that they were already validated by mailserver.

Yes, the first link is for regular message adding to the queue.
However, I don't think the P2P message request goes through that process.

I'm actually quite confused as there seem to be two implementations for this:
https://github.com/status-im/whisper/blob/25321b2c035b6e03dbae85a2f54cf89f9f873dd9/whisperv6/whisper.go#L449 & https://github.com/status-im/whisper/blob/25321b2c035b6e03dbae85a2f54cf89f9f873dd9/whisperv6/whisper.go#L465
One with an envelope, the other not?

From looking at the server counterpart, I have the impression it just checks the PoW value: https://github.com/status-im/status-go/blob/a636f33109130a9151553abf5e0253e35994ef60/mailserver/mailserver.go#L798

For the actual message envelopes send via P2P Message, it makes sense that validation is skipped.

@kdeme kdeme changed the title [WIP] Waku mail client implementation Waku mail client implementation Dec 11, 2019
@kdeme

This comment has been minimized.

Copy link
Contributor Author

kdeme commented Dec 11, 2019

If OK, this can be merged and I fill in the TODO gaps in a second part.

@kdeme kdeme requested a review from zah Dec 11, 2019
traceAsyncErrors peer.get().p2pRequest(env)

# Wait for the Request Complete packet
var f = peer.get().nextMsg(Waku.p2pRequestComplete)

This comment has been minimized.

Copy link
@zah

zah Dec 11, 2019

Member

If you have multiple concurrent requests, nextMsg(Waku.p2pRequestComplete) may be completed by a response from one of your other requests. Is this OK for the code here?

This comment has been minimized.

Copy link
@zah

zah Dec 11, 2019

Member

The solution would be to either add request IDs to the Waku messages or to implement some sort of queuing that will guarantee that only a single request can be in flight.

This comment has been minimized.

Copy link
@kdeme

kdeme Dec 12, 2019

Author Contributor

True, didn't think of that but this would be a problem when requestMail is called multiple times in parallel.
Could use the requestId (see TODO lower) to check for that, but it would still stop the flow.

# would be a cleaner separation between Waku and Mail server / client and then
# this could be moved to waku_mail.nim
P2PRequestCompleteObject* = object
requestId*: Hash

This comment has been minimized.

Copy link
@zah

zah Dec 11, 2019

Member

Does this need to be a hash? The framework has support for numeric request IDs through the requestResponse: helpers in the p2pProtocol DSL.

This comment has been minimized.

Copy link
@kdeme

kdeme Dec 12, 2019

Author Contributor

It doesn't need to be, it is just that this is how it is specified now in the Status mail server specification.
Ideally, for Waku, we can change this to what is more convenient.

This comment has been minimized.

Copy link
@zah

zah Dec 12, 2019

Member

I suggest following the practice of the LES protocol:
https://github.com/ethereum/devp2p/blob/master/caps/les.md

In our codebase, this looks like this:
https://github.com/status-im/nim-eth/blob/master/eth/p2p/rlpx_protocols/les_protocol.nim#L281

On the usage call-sites, this will automatically wait for the correct response.

This comment has been minimized.

Copy link
@kdeme

kdeme Dec 12, 2019

Author Contributor

So yes, ideally we could use the request IDs provided by the requestResponse mechanism. It was my original plan to use that mechanism, however it would not work due to non consecutive packet ID's, see comment lower.

eth/p2p/rlpx_protocols/waku_protocol.nim Outdated Show resolved Hide resolved
@zah zah merged commit b2656cc into master Dec 12, 2019
4 checks passed
4 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@delete-merged-branch delete-merged-branch bot deleted the waku-mail-client branch Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.