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

Early draft: Waku mode spec #54

Closed
wants to merge 3 commits into from
Closed

Early draft: Waku mode spec #54

wants to merge 3 commits into from

Conversation

oskarth
Copy link
Contributor

@oskarth oskarth commented Oct 17, 2019

See #53 and discuss post


Format TBD.

2. Provide way to identify oneself as a Waku node.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this where we need to start getting into transport negotiation or can a waku client talk to non waku using clients?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could be as simple as adding a field to the existing Whisper handshake, to identify as Waku-chan or Waku-san.
Behaviour at handshake would then be:

  • in case of Waku-chan -> disconnect unless talking to a Waku-san. Only make 1 connection.
  • in case of Waku-san:
    • when connecting with Waku-chan: apply Topic list filter when sending messages to this peer
    • when connecting with others: current behaviour (full messages or bloomfilter)

Problem with this might be how to do it without losing backwards compatibility (non waku nodes). But maybe it will just work as is, to be tested. I believe EIP-8 requests that you can add extra fields to the handshake, but that is specifically for rlpx/devp2p handshake. I don't think it mentions this for subprotocols such as Whisper.
And in fact, I think in our nim implementation that would be something we have to correct.
I'm not sure if geth its behaviour, something to test.

Additionally, I think the topic list could then also be added in the Whisper handshake (and the new packet code can be used to update it). This way we don't get a bunch of unwanted messages send right after the handshake.

Copy link
Collaborator

@kdeme kdeme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @yenda mentioned on discuss, it would be important to have secure communications between the waku-chan and waku-san, so that meta-data does not leak any further than the waku-san.

Considering the flaws of devp2p, perhaps this should not be allowed as a lower transport in this scenario? Or at least it should be clear of the possible consequences.

But maybe I'm stating the obvious as comparison with Infura is made and thus HTTPS and WebSockets was perhaps the default idea?


Format TBD.

2. Provide way to identify oneself as a Waku node.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could be as simple as adding a field to the existing Whisper handshake, to identify as Waku-chan or Waku-san.
Behaviour at handshake would then be:

  • in case of Waku-chan -> disconnect unless talking to a Waku-san. Only make 1 connection.
  • in case of Waku-san:
    • when connecting with Waku-chan: apply Topic list filter when sending messages to this peer
    • when connecting with others: current behaviour (full messages or bloomfilter)

Problem with this might be how to do it without losing backwards compatibility (non waku nodes). But maybe it will just work as is, to be tested. I believe EIP-8 requests that you can add extra fields to the handshake, but that is specifically for rlpx/devp2p handshake. I don't think it mentions this for subprotocols such as Whisper.
And in fact, I think in our nim implementation that would be something we have to correct.
I'm not sure if geth its behaviour, something to test.

Additionally, I think the topic list could then also be added in the Whisper handshake (and the new packet code can be used to update it). This way we don't get a bunch of unwanted messages send right after the handshake.

@oskarth
Copy link
Contributor Author

oskarth commented Oct 18, 2019

As @yenda mentioned on discuss, it would be important to have secure communications between the waku-chan and waku-san, so that meta-data does not leak any further than the waku-san.

Considering the flaws of devp2p, perhaps this should not be allowed as a lower transport in this scenario? Or at least it should be clear of the possible consequences.

Long term we want to move to libp2p. For the initial PoC, I'd suggest we stay with DevP2P and note possible consequences. In terms of threat matrix it is fairly low in terms of severity and probability, IMO.

@adambabik also suggested HTTPS. I'd like to see some very strong arguments for us to move to it, as it is not really on the path we want to take long term. What would it give us? What are the trade offs? How much time would it take to implement? How would we switch from it to devp2p/libp2p in adaptive node (note waka mode can be reset / fine-tuned)?

@oskarth
Copy link
Contributor Author

oskarth commented Oct 18, 2019

Maybe it could be as simple as adding a field to the existing Whisper handshake, to identify as Waku-chan or Waku-san.

True, how would this work if Whisper API were to change in the future (unlikely but eh)? I.e. can we ensure we don't take up a spot that might be used by other nodes?

Not too familiar with EIP-8 requests but sounds possibly useful.

Is this something you'd like to play around with further @kdeme to help tighten up the spec / get poc?

@cammellos
Copy link
Contributor

@adambabik also suggested HTTPS. I'd like to see some very strong arguments for us to move to it, as it is not really on the path we want to take long term. What would it give us?

I think we should simplify the current way of doing this, and have a simple request-response sync pattern, regarding HTTPS , sounds like a safe option, but don't have a strong opinion on the matter.

@adambabik
Copy link
Contributor

@adambabik also suggested HTTPS. I'd like to see some very strong arguments for us to move to it, as it is not really on the path we want to take long term. What would it give us? What are the trade offs? How much time would it take to implement? How would we switch from it to devp2p/libp2p in adaptive node (note waka mode can be reset / fine-tuned)?

I have a few :)

  1. Managing connections in p2p systems (including peers selection, reconnects) is much more problematic for client and server nodes compared to HTTPS or WebSockets. devp2p abstracts it to some degree and we have experience but still.
  2. HTTPS traffic is easy to load balance so the client does not need to implement the whole logic which in p2p systems is necessary: peers discovery protocol, peers pool, careful management of a number of connected peers to reduce traffic etc. Drawback is that HTTPS and load balancing is more centralized.
  3. I guess Waku node would require many new APIs so it does not matter which way we will go because in both the new methods must be implemented. Having said that using HTTPS requires to copy encryption/decryption and validation logic for messages from Whisper. Using devp2p we probably could stay with large portion of the current Whisper implementation.
  4. With HTTPS there is additional effort to bridge it with devp2p on Waku nodes.

To sump up, HTTPS is an easier communication protocol to work with from the client perspective. It allows us to scale Waku nodes transparently. It also allows more type of clients like pure browser one and should be more reliable in terms of faster reconnects, work better in the restricted environments like airport wifi etc. Cons, it likely requires a bit more work (but it's an opportunity to do it better :)). It also opens this "convenient mode" which once is implemented will be difficult to get rid of, although, from less aware clients perspective, it does not matter because they will have better experience using the app. Switching between these two modes would not be a bad idea.


## Roles and definitions

1. Waku-chan
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use more explicit client and server terms, even if it loses the "joke"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, waku client / waku node is better, otherwise we need to have context on which one is which

Copy link
Contributor Author

@oskarth oskarth Nov 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing, agree


Waku mode is compatible with [EIP-627](https://eips.ethereum.org/EIPS/eip-627) and simply extends its capabilities. It does this through a new packet code, as well as some client specific recommendations. Two specific protocol changes are done:

1. Modify EIP627 by adding a new packet code, e.g. `Topic List [101, bytes]` where bytes is a list of N topics.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not make a new protocol? what's the benefit here of trying to keep it compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the minimal amount of changes needed. It's only adding a capability to the current Whisper protocol, and a node can switch between the two modes. It doesn't impact the rest of the network, and they can keep using Whisper like they already are.

We can also base in on EIP-627 and call the subprotocol whisper-waku instead of whisper/6, and then simple have Waku nodes listen to whisper/6 traffic and relay it over the new subprotocol. Might be preferable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be preferable?

Intuitively I'd go in this direction but I haven't studied the specific details (ie what if someone else makes an "extension" to whisper/6, would the two be compatible? what if we want to change something about waku mode?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really clear to me what is meant here with "extension", I thought it is just extending a protocol with extra message types (new packetIds).

Your suggestion would mean a new subprotocol (with quite some overlap on Whisper yes, but I would not call it an extension). A Waku node would basically have to talk whisper/6 and whisper-waku, and be some sort of bridge / relay between those.

Practically, in the nim implementation a lot would be able to be reused, but there would be some extra work/complexity in sharing the message queue between protocols. Not impossible and perhaps preferable, but more work and bit more of complexity versus a better split off between protocols. Not sure if this is really that necessary though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already added many packets to our Whisper fork that are not specified in the EIP-627. That path has been already taken (unfortunately) and I don't see a reason to change it now. Introducing a new packet code is waaay simpler at this stage.

Copy link
Contributor Author

@oskarth oskarth Nov 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this in protocol call, and the consensus was to at least explore having a separate subprotocol. While it likely does lead to a bit of initial complexity, it's be better long term as it's clear what we are talking about when we mention a specific protocol, and it makes it easier for us to do future protocol changes, such as accounting for resources, moving to libp2p, changes to spam resistance, settlement, etc. Moving to libp2p we need a bridge between different subprotocols anyway. (If it does turn out to be impractical/take longer than expected, we can easily table it and go back to just hacking it in for the time being).

We already added many packets to our Whisper fork that are not specified in the EIP-627. That path has been already taken (unfortunately) and I don't see a reason to change it now. Introducing a new packet code is waaay simpler at this stage.

From what I can tell that's not really documented in the current specs very clearly, e.g. ctrl-f packet https://github.com/status-im/specs/blob/master/status-whisper-usage-spec.md and https://github.com/status-im/specs/blob/master/status-client-spec.md It should be. The fact that we still seem to have undocumented behaviour that makes us not spec compliant would count as an argument in favor of more clarity on the actual protocols we support and require.

EDIT: When you say many, how many do you mean? Aside from shh_markTrustedPeer and shhext_requestMessages (https://github.com/status-im/specs/blob/master/status-whisper-usage-spec.md#whisper-v6-extensions, I can't see many other modifications in spec. It's also in a different format than EIP627 (https://eips.ethereum.org/EIPS/eip-627) which makes spec diff even more difficult as a prospective client implementer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any advantage of having a separate subprotocol that would simplify transition to libp2p honestly. If we want to have a separate subprotocol, I expect that we actually write a spec similar to EIP-627. It needs to be that comprehensive otherwise we will end up in even bigger mess than the current situation with altered Whisper handshake and additional packet codes never documented. It my opinion it is a 10x bigger task. It sounds more reasonable long term but it's just different scope.

To clarify, I guess we are not talking about a subprotocol that runs along Whisper, right? It's gonna be a completely independent self-sufficient subprotocol?

Our packet codes: https://github.com/status-im/whisper/blob/master/whisperv6/doc.go#L51-L62 vs EIP-627 packet codes: https://github.com/ethereum/go-ethereum/blob/master/whisper/whisperv6/doc.go#L48-L53.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec similar to EIP-627

yep agree. A good starting point is for us to fork it and amend it.

It's gonna be a completely independent self-sufficient subprotocol?

yes, essentially a fork of Whisper that starts off identical. Essentially same thing we've already done except it isn't properly documented.

https://github.com/status-im/whisper/blob/master/whisperv6/doc.go#L51-L62 Why is this not documented in specs? How are we supposed to write another implementation in Nim if we just add random message types and don't document them? Not to mention other parties who are interested in building on "Whisper"/Status (e.g. team at Consensus who are evaluating it for enterprise). This is exactly what we are trying to get away from. Can we please document this in the specs repo?

Copy link
Contributor

@adambabik adambabik Nov 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍

Because these parts are not essential and they are not covered in the spec whatsoever:

  • batchAcknowledgedCode and messageResponseCode are used to confirm that the message is received by another peer, I believe it's sent only by mailservers. I don't know why there are two codes which look like doing the same thing.
  • Syncing mailservers (not sure we should even document it as it's technically not part of the protocol): p2pSyncRequestCode and p2pSyncResponseCode.
  • p2pRequestCompleteCode is a response from mailserver after sending all envelopes.

The first one and the last one could be documented.

EDIT: #58 and #59

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additional handshake parameter is something I didn't know about until now (which does mean it works in the geth fork when a client without these parameters makes a connection 👍). So yes, it would be good to have it written down somewhere even if it is not mandatory in the "forked Whisper spec".

@oskarth
Copy link
Contributor Author

oskarth commented Dec 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants