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 v0 PoC #120

Merged
merged 13 commits into from Nov 21, 2019

Conversation

@oskarth
Copy link
Member

oskarth commented Nov 15, 2019

Just a fork of Whisper with changed capabilities etc. Part of vacp2p/specs#27

@kdeme

This comment has been minimized.

Copy link
Contributor

kdeme commented Nov 16, 2019

Thanks for trying this out. Please note that I plan on splitting some of the code for more reuse.

@oskarth

This comment has been minimized.

Copy link
Member Author

oskarth commented Nov 18, 2019

@kdeme that's fine, I wanted to duplicate it to KISS for now. Can always optimize it later, perhaps for waku v1? (rule of three). Alternatively, if it makes it easier to have WakuWhisper capability inside Waku namespace and not pollute whisper_protocol.nim

@oskarth

This comment has been minimized.

Copy link
Member Author

oskarth commented Nov 19, 2019

From PM with Kim for reference

After spending a bit of time with the bridge stuff I agree that #122 is probably necessary.

My nim-foo isn't quite up to par, I had some issues with type duplication, essentially I tried to do let res = waku.queueWhisperMessage(node, waku.Message(msg)) inside Whisper queue message but the types are different
I started defining a whisper_types module but got stuck on some public/accessible field/constructor stuff that I couldn't resolve in a short timebox.

I think I'll wait until you've done the refactoring above, then above should essentially work. Do you want to work on the same branch? might be easier, also since I think we usually work slightly different hours.

@kdeme kdeme force-pushed the waku-zero branch 2 times, most recently from 5001bb5 to f19bedd Nov 19, 2019
@kdeme

This comment has been minimized.

Copy link
Contributor

kdeme commented Nov 19, 2019

I've removed the fix-119 merge commit and rebased to get that fix + refactor changes in. Adjusted the Waku/0 code to make use of whisper_types.

@oskarth I've dropped your last commit as it had merge issues with the above and I was planning to change the approach somewhat, see below. I hope that's OK.

Next, some bridging attempt:
Easy (but bit intrusive and ugly) option could be, two queues (1 Waku, 1 Whisper) and add to each
one when receiving messages by adding discard peer.networkState(Whisper).queue.add(msg) for Waku code and discard peer.networkState(Waku).queue.add(msg) for Whisper code, basically each time where the filter notify happens in the messages proc.
This however doesn't work as it requires to import the other protocol which creates a recursive module dependency error at compile time.

The original idea I had was to simply share the queue. I don't believe this will create problems in terms of network message forwarding.
So I did a quick (and dirty) implementation of this by making the queue a ref object in the protocol network state, and checking if the queue of Whisper isNil or not before creating a new queue. In order for this to work the order or adding capabilities does matter though, first Whisper then Waku. (we can't add the same code for both protocols due to same error as above, which is again just a good indication that this is dirty and that the protocol code should be agnostic to all this).
If this approach is taken, it should eventually be reworked so that default, the queues are separated, but that a queue can be added upfront somehow, in which case no new one is created in the initProtocolState.

If one shared queue would be a problem somehow, another option would be to allow for a custom function to be called in the messages proc (at the location of the filter notify). In that custom function the message could be added to the "other" queue.

I've adjusted the waku test to have a Whisper Node <-> Bridge (WhisperWaku) <-> Waku Node setup, where data is posted and received from both the Whisper and the Waku node.

Added to the regular p2p test cases, and dropped the commit of the extra test task.

@oskarth

This comment has been minimized.

Copy link
Member Author

oskarth commented Nov 20, 2019

Great, thanks!

In order for this to work the order or adding capabilities does matter though, first Whisper then Waku.

How about a separate fn for adding caps in right order? Strictly speaking, having WakuWhisper bridge is more than just having waku and whisper capability, so perhaps bridging should even be a flag.


What do we need to do to get this merged? Can we do the cleaner queue implementation in a follow up PR?

@oskarth oskarth marked this pull request as ready for review Nov 20, 2019
@oskarth oskarth changed the title [Do not merge] Waku v0 PoC Waku v0 PoC Nov 20, 2019
@kdeme

This comment has been minimized.

Copy link
Contributor

kdeme commented Nov 20, 2019

Yes, default we should not bridge in case we have both capabilities. This is now just a quick implementation to be able to test it. If we go for 1 shared queue we should have:

  • Default just Whisper and/or Waku.
  • Optionally provide them with a shared queue -> bridge functionality.

However, I think the last point requires DSL change, I'll think about it.

For merging I think I first have to figure out why appveyor test fails. And I think we can also drop the commits adding the waku_basic_client.nim? There is no real use for this now.
Ideally we also have a cleaner bridge solution, but as the waku_protocol is not really used yet, we could also do without and add a big TODO.

@kdeme kdeme force-pushed the waku-zero branch from fbe7ceb to 65ef598 Nov 21, 2019
oskarth and others added 9 commits Nov 15, 2019
According to `doc/p2p.md` capability should be three letters.  I can't see this
in https://github.com/ethereum/devp2p/blob/master/rlpx.md#capability-messaging
though, but better safe than sorry.

This also dismabiguates and stays somewhat consistent in vocabulary:

Whisper -> Waku
whisper -> waku
shh     -> wkk
- WakuWhisper and Whisper peers connect
- WakuWhisper node picks up Whisper messages
@kdeme kdeme force-pushed the waku-zero branch from 65ef598 to 21f543d Nov 21, 2019
@kdeme

This comment has been minimized.

Copy link
Contributor

kdeme commented Nov 21, 2019

Somewhat cleaner solution now for the bridge, it still suffers from vacp2p/research#14 (comment) though.
Rebased & dropped 4 commits related to waku_basic_client.nim

I think this can be merged for now.

@kdeme kdeme requested a review from zah Nov 21, 2019
@zah zah merged commit bc2b76f into master Nov 21, 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
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.