Skip to content
This repository has been archived by the owner on Mar 26, 2019. It is now read-only.

Add Whisper implementation (WIP) #52

Merged
merged 6 commits into from Nov 27, 2018
Merged

Conversation

kdeme
Copy link
Contributor

@kdeme kdeme commented Nov 16, 2018

I've been working on the Whisper implementation, issue #43.

It is still WIP (see the notes in comments for missing things and improvements) but is in a fairly decent working state so I thought to do a PR and get some first remarks.

In general I do have following questions/remarks:

  • Is the approach where the 'runners' starts automatic (after the general p2p startup) OK? Or rather separate calls?

    @zah's answer: I've left some comments in the review. The init code will be reworked to solve WhisperConfig problem and runners will have to be started in a more manual fashion. The general approach of defined the Whisper procs over the EthereumNode type is fine.

  • Is the approach where calls have to be made with an EthereumNode as parameter OK? Or should there be a Whisper object that needs to be initialised with an EthereumNode and possible WhisperConfig settings? I'd have to figure out how to make that work within the rlpxProtocol shh macro parts though. But on the other hand, at this moment, it is actually not possible to initialise certain config settings before you start the network.

  • The current testing (the non mocking part) is actually rather integration testing, but quite tricky due to the sleeps that are needed. Probably not always working and will need improvement (if this way is deemed useful anyhow). However, it was the easiest & quickest way for me to have some automated testing while developing, so I added it. The mocking is currently limited but I had some trouble there getting things running, TBI.

  • Next to that there is also a very basic client that I often used to connect on a private network. Tested this also with geth -shh clients. There I noticed a PoW calculation difference with geth, issue created: Whisper: PoW calculation different from EIP-627 ethereum/go-ethereum#18070 . Asymmetric encryption and signatures, topics, bloomfilters work. Symmetric encryption is not tested yet. Direct P2P communication is also untested. I was wondering if some CI setup needs to be made to run tests on a private network together with geth (and parity) nodes?

    @zah: Integration tests with other clients are definitely a good thing to have, but probably the code for this should be based on the JSON-RPC interface of Nimbus and the work for it is tracked here:
    Create a sharable debugging and testing environment for Ethereum nimbus-eth1#172

  • Also used this client to connect to a public network (but the main network boot nodes don't get the client connected to any other Whisper clients, can it be that there are too few that support Whisper? TBI). However, when directly connecting to a known Whisper client list, messages are send from 1 client to another without the two clients being directly connected.

    @zah: It's true that there aren't many nodes running Whisper in the mainnet. For this reason, Status is maintain its own cluster of Whisper nodes. We have to ask someone for suitable bootstrap addresses, so we can find these nodes.

@kdeme
Copy link
Contributor Author

kdeme commented Nov 26, 2018

FYI, in the meanwhile I have also tested symmetric encryption with geth successfully.
And added + tested padding access together with geth.

@zah zah merged commit 1e5eeec into status-im:master Nov 27, 2018
eth_p2p/rlpx_protocols/shh_protocol.nim Show resolved Hide resolved
eth_p2p/rlpx_protocols/shh_protocol.nim Show resolved Hide resolved
eth_p2p/rlpx_protocols/shh_protocol.nim Show resolved Hide resolved
eth_p2p/rlpx_protocols/shh_protocol.nim Show resolved Hide resolved
eth_p2p/rlpx_protocols/shh_protocol.nim Show resolved Hide resolved
while peer.state(shh).running:
if not peer.networkState(shh).config.isLightNode:
peer.processQueue()
await sleepAsync(300)
Copy link
Member

Choose a reason for hiding this comment

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

Is this how the other clients do it? The polling interval seems to be quite high and I'm wondering whether it's possible to use a more event-driven way to handle everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I only checked geth, and that is how it is done there, with those intervals. The polling interval on the processQueue can be lowered, of course, finding the balance to remain a good 'citizen' on the network.

Initially I had in mind of doing it more event-driven, e.g. forwarding messages on the event of receiving them (it does make things a little more complex, like processing the queue on joining peers, peers that change bloomfilter, PoW, etc.). Is this what you mean for example?

It is not in the spec (EIP-627), however it is mentioned here that messages should be added and then processed in bulk: https://github.com/ethereum/go-ethereum/wiki/Whisper-Overview#creating-and-sending-messages

While it is not mentioned there why, I assumed that it is done most likely to maintain "darkness". Imagine the implementation always forwards messages directly when they are being received. Then if your own "posted" message is send in solo the peers can probably know it originates from you more easily by doing some network statistics.

After some very quick thought some example ideas:

  1. Some network monitoring could show that you posted a message without initially receiving any other messages? (perhaps invalid due to high amount of traffic)
  2. Most other peers send (bigger) lists of envelopes (as it is implemented like this now to stack them up for 300 ms in geth), thus you will also forward these lists of envelopes. Your single message post will be spotted easily. (I guess this does not hold if every implementation does it more event driven)

These are just some very quick thoughts and I'm not sure how valid they are but I guess what I am saying is that it sure feels that this way of queuing the messages up is hiding the own posted messages better.

Perhaps more complex tricks could be done like sending message at random time after receiving and such, but I am not sure if this will benefit the solution in the end.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks for the detailed explanation. I guess copying Geth is the safest route right now.

eth_p2p/rlpx_protocols/shh_protocol.nim Show resolved Hide resolved
# In its current blocking state, it could be noticed by a peer that no
# messages are send for a while, and thus that mining PoW is done, and that
# next messages contains a message originated from this peer
env.nonce = env.minePow(powTime)
Copy link
Member

Choose a reason for hiding this comment

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

It would be hard to execute this in a background thread at the moment. We'll need a way to send custom "tasks" to the async message loop (e.g. similar to C++'s asio::post). In asyncdispatch2, this is probably going to be represented by an AsyncChannel object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and I think that geth doesn't do this either (from seeing its behaviour when asking high PoW, I didn't check in the code yet).

eth_p2p/rlpx_protocols/shh_protocol.nim Show resolved Hide resolved
eth_p2p/rlpx_protocols/shh_protocol.nim Show resolved Hide resolved
zah added a commit that referenced this pull request Nov 28, 2018
kdeme added a commit to kdeme/nim-eth-p2p that referenced this pull request Nov 28, 2018
@kdeme kdeme deleted the feature/whisper-impl branch November 28, 2018 14:23
zah pushed a commit that referenced this pull request Nov 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants