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

[Do not merge] Implement Waku mode PoC #114

Open
wants to merge 1 commit into
base: master
from

Conversation

@kdeme
Copy link
Contributor

kdeme commented Oct 28, 2019

Quick PoC of how the Waku mode could look. See status-im/specs#54

@kdeme kdeme force-pushed the waku-poc branch from abdadf8 to c0c82f3 Oct 29, 2019
@kdeme kdeme changed the title Implement Waku mode PoC [Do not merge] Implement Waku mode PoC Oct 30, 2019
@kdeme

This comment has been minimized.

Copy link
Contributor Author

kdeme commented Oct 30, 2019

This are currently purely the Whisper changes.

It would also require some forced setting to only connect with 1 other (trusted) waku node. And thus don't run additional discovery or listen for incoming connections.

Next to that, it would be best if we could have backwards compatibility with Whisper versions not having the Waku mode, but we would need this: #116

@oskarth

This comment has been minimized.

Copy link
Member

oskarth commented Oct 31, 2019

Nice one!

FYI @adambabik - do you know if #116 be an issue for existing geth based deployments?

@kdeme

This comment has been minimized.

Copy link
Contributor Author

kdeme commented Oct 31, 2019

Another, more ugly, way making sure we have backwards compatibility would be to not put the WakuMode and topics in the handshake, but having an additional message type which requests the WakuMode of the peer. If it turns out the be an "old" node without Waku support it will (must) simply ignore this message. Thus no reply will be send and the node can disconnect from this peer.

@adambabik

This comment has been minimized.

Copy link
Member

adambabik commented Nov 4, 2019

We already have incompatible number of arguments in Whisper handshake with regards to EIP-627:

From our fork of Whisper, I assume that we can handle that gracefully, i.e. if there is no additional param, ignore the error (https://github.com/status-im/whisper/blob/master/whisperv6/peer.go#L141-L144). It needs to be confirmed whether EIP-627 compatible node will handle properly a handshake with more params but it looks like it will.

In the Waku mode spec (status-im/specs#54), there are two problems: (1) adding a new packet code, (2) identifying as a waku node.

In my opinion, identifying as a Waku Node might be put into the handshake. It is very close logically to identifying as a Light Node. Generally, this light node identification could be extend to be an enum informing about the Whisper node mode (light, waku, possibly others). Alternatively, we can rely on the peers discovery mechanism. A peer would seek for Whisper nodes advertising themselves as Waku nodes, do a handshake and finally verify whether the advertisement is correct using a special packet or by examining its interface.

Adding a new packet is already explored by our Whisper fork, for example, packets to do the sync between mailservers.

So, there is a possibility to do everything we want by just introducing new packets and utilizing peers discovery mechanism and avoid changing the handshake at all.

@kdeme

This comment has been minimized.

Copy link
Contributor Author

kdeme commented Nov 5, 2019

I did not know about this change in the handshake. Only knew about the added packetids.

I agree on the identification in the handshake. The code change in this PR does that. It adds a WakuMode enum. It doesn't use the light node field as that would rather certain break things I believe. The list of topics is also immediately send in the handshake (similar like the bloom filter).

Having peer capability (supported protocols) checking in discovery would in general be better yes. But for us this is not yet possible. This would mean we need to implement discovery v5 or at least the ENR mechanism.
Also, I'm not sure if that will resolve the handshake here. Ideally you still send which type of WakuMode you are (client or node) and which topics you want. So purely the protocols supported is not enough. Sure, you could add the WakuMode in the discovery information also, but you have to draw a line somewhere. Where does it end?

Adding packetids is indeed never a problem. The node is supposed to ignore what it does not know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.