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

33/WAKU-DISCV5: Add first raw version #487

Merged
merged 16 commits into from
Mar 7, 2022
Merged

33/WAKU-DISCV5: Add first raw version #487

merged 16 commits into from
Mar 7, 2022

Conversation

kaiserd
Copy link
Contributor

@kaiserd kaiserd commented Feb 11, 2022

This PR addresses #486 introducing a first version of an RFC specifying a WAKU2 ambient discovery protocol based on discv5.
In its current state, it is meant as a basis for discussion.

@oskarth oskarth added this to New in vac-research Feb 11, 2022
@oskarth oskarth moved this from New to Review/QA in vac-research Feb 11, 2022
@kaiserd kaiserd marked this pull request as draft February 11, 2022 20:06
@oskarth oskarth changed the title add: first raw version of 32/WAKU-DISCV5 33/WAKU-DISCV5: Add first raw version Feb 14, 2022
Copy link
Contributor

@oskarth oskarth left a comment

Choose a reason for hiding this comment

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

Nice to see this coming along!

First glance with misc admin stuff, will have a look at details later

@@ -0,0 +1,102 @@
---
slug: 32
Copy link
Contributor

Choose a reason for hiding this comment

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

32 taken here #482

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


# References


Copy link
Contributor

Choose a reason for hiding this comment

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

CC0 copyright section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

`32/WAKU2-DISCV5` spans a discovery network isolated from the Etherium Discovery v5 network.
This separation allows for efficient queries.
Using a dedicated Waku2 discovery network, Waku2 nodes can query this discovery network for a random set of nodes and directly use these randomly distributed nodes as bootstrap into the Waku2 network.
If Waku2 would use the Etherium discovery v5 network a retrieved set of random nodes is not guaranteed to contain a Waku2 node leading to a needle-in-the-haystack problem.
Copy link
Contributor

@kdeme kdeme Feb 14, 2022

Choose a reason for hiding this comment

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

is not guaranteed to contain a Waku2 node`

I get what you are trying to say here, but perhaps it should be formulated differently. The way it is written now implies that with a "dedicated Waku2 discovery network" one will always find only Waku nodes, which can not really be enforced. Any node could run on this new network if they adjust the protocol.

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! I will address that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kdeme wdyt about the updated version?

* can be forked followed by changing the `protocol-id` string [link to go-discv5 once upstream]


# Security Considerations
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps something should be mentioned that due to running on a different network, (initial) small amount of nodes on the network will cause it to be at bigger risk for the attacks listed below.

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. I will add that.

Copy link
Contributor Author

@kaiserd kaiserd Feb 14, 2022

Choose a reason for hiding this comment

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

@kdeme I added a frist basic explanation. Wdyt? I will elaborate on this later. I did not metion the problem of the (initially) small network size yet (todo).

@kaiserd
Copy link
Contributor Author

kaiserd commented Feb 14, 2022

Thank you for the feedback :). I will apply it.

@kaiserd kaiserd mentioned this pull request Feb 14, 2022
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

This already lgtm as a raw spec. I've left some comments below. Feel free to mark PR as "ready for review" when you feel it is.

Comment on lines 22 to 34
* hard coded bootstrap nodes
* `DNS discovery`
* `peer-exchange` protocol
* `33/WAKU2-DISCV5` (specified in this document)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if a list like this is not better suited to the "Discovery" section in RFC 10/WAKU2? You could then just reference that section here to make the some point. For example, we don't really support "peer-exchange" yet, but will hopefully soon ™. Since the list of supported discovery protocols change frequently, we'll have to update this in several places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed. Maybe we could leave it here as long as the draft is in raw state, and once it advances we could update 10/WAKU and substitue a reference for this list. Wdyt?

content/docs/rfcs/33/README.md Outdated Show resolved Hide resolved
Comment on lines 40 to 53
### w.r.t. Ethereum Discovery v5
`33/WAKU2-DISCV5` spans a discovery network isolated from the Ethereum Discovery v5 network.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### w.r.t. Ethereum Discovery v5
`33/WAKU2-DISCV5` spans a discovery network isolated from the Ethereum Discovery v5 network.
### w.r.t. Ethereum Discovery v5
`33/WAKU2-DISCV5` spans a discovery network isolated from the Ethereum Discovery v5 network.

content/docs/rfcs/33/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

Nice one! please see my comments :)

The purpose of ambient node discovery within [10/WAKU2](/specs/10) is discovering Waku2 nodes in a decentralized way.
The unique selling point of `33/WAKU2-DISCV5` is its holistic view of the network, which allows avoiding hotspots and allows merging the network after a split.
While the other methods provide either a fixed or local set of nodes, `33/WAKU2-DISCV5` can provide a random sample of Waku2 nodes.
Future iterations of this document will add the possibility of efficiently discovering Waku2 nodes that have certain capabilities, e.g. holding messages of a certain time frame during which the querying node was offine.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Future iterations of this document will add the possibility of efficiently discovering Waku2 nodes that have certain capabilities, e.g. holding messages of a certain time frame during which the querying node was offine.
Future iterations of this document will add the possibility of efficiently discovering Waku2 nodes that have certain capabilities, e.g. holding messages of a certain time frame during which the querying node was offline.


* hard coded bootstrap nodes
* `DNS discovery`
* `peer-exchange` protocol
Copy link
Contributor

Choose a reason for hiding this comment

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

Any ref for this?


`33/WAKU2-DISCV5` spans an overlay network separate from the [GossipSub](https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/README.md) network [11/WAKU2-RELAY](/specs/11) builds on.
Being a P2P network on its own, it also depends on bootstrap nodes.
The advantage of having a separate discovery network is reducing load on the bootstrap nodes as the actual work is done by randomly discovered nodes, which in turn increases decentralization.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please break this sentence into shorter sentences, it is a bit hard to understand.


## Security Implications of a Separate Discovery Network

A dedicated Waku discovery network can be subject to eclipse attacks if not properly secured.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to briefly explain 1) the eclipse attack and how it harms the network and 2)why separating the waku discovery network from Eth discv5 make it vulnerable to this attack.

### w.r.t. Waku2 Relay Network

`33/WAKU2-DISCV5` spans an overlay network separate from the [GossipSub](https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/README.md) network [11/WAKU2-RELAY](/specs/11) builds on.
Being a P2P network on its own, it also depends on bootstrap nodes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we still need bootstrap nodes for a separate discovery network, wouldn't it be somewhat centralized? (similar to the discovery method#1 you listed above). How about a hybrid approach where some of the nodes in the waku discovery network also take part in the Eth discv5 net hence be discoverable there as well?

@kaiserd
Copy link
Contributor Author

kaiserd commented Feb 25, 2022

@staheri14 Thank you very much for the feedback. Because good and important comments in the discussion here. I am thinking about possible hybrid solutions, as you also pointed out in your feedback.

Imho, the best "hybrid" would be:

  • integrating efficient topic/capability discovery into Ethereum discv5
  • Waku being part of the Ethereum discv5 (not having a separate network at all)
  • making the waku-capability discoverable as a discv5 topic/capability
  • additionally, using random-walk discovery, as a fallback for mitigating eclipse attacks.

This would allow us integrating the benefits of having a separate discovery network into the existing Etherium discv5 network.

If the Ethereum discv5 community does not like this, another possible hybrid would be:

  • using a separate discv5 network as described in the PR
  • additionally allowing stronger nodes to participate in the Ethereum discv5 network and integrating discovered nodes into the waku-exclusive routing tables.

(Adaptive nodes can support

  • no discv5
  • separate discv5
  • both separate and Ethereum discv5 )

These are my next steps:

  • finish implementing and testing a first working Waku discv5 integration based on this PR
    • this allows early dogfooding and "advertising" this feature to Desktop
  • refine the first hybrid idea I described and post an issue on the Ethereum discv5 Github to start a discussion (I already talked about this with @kdeme)
  • apply your feedback and adapting this PR

@oskarth
Copy link
Contributor

oskarth commented Feb 28, 2022

Agree re raw spec comment above. What's blocking from this being ready to review and mergeable?

kaiserd and others added 6 commits March 3, 2022 09:52
Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>
Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>
@kaiserd
Copy link
Contributor Author

kaiserd commented Mar 3, 2022

I applied feedback and also reworked the raw RFC based on new results and insights. Imo, it is ready to be merged as a raw RFC after another review phase.

@kaiserd kaiserd marked this pull request as ready for review March 3, 2022 15:12

This version of `33/WAKU2-DISCV5` has a focus on timely deployment of an efficient discovery method for [10/WAKU2](/specs/10).
Establishing a separate discovery network is in line with this focus.
However, we are aware of potential resilience problems (see section on security considerations) and are [discussing](https://forum.vac.dev/t/waku-v2-discv5-roadmap-discussion/121/8)
Copy link
Contributor

@jm-clius jm-clius Mar 3, 2022

Choose a reason for hiding this comment

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

I like that this is specified.

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

A very good read and clear spec for waku2 discv5. Thanks!
My only comment would be that, since we have a reference implementation already and have demonstrated that this works, the status could likely be draft iso raw already. See https://rfc.vac.dev/spec/1/.

@oskarth, wdyt?

content/docs/rfcs/33/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/33/README.md Outdated Show resolved Hide resolved
kaiserd and others added 2 commits March 3, 2022 18:35
Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>
Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>
@oskarth
Copy link
Contributor

oskarth commented Mar 7, 2022

My only comment would be that, since we have a reference implementation already and have demonstrated that this works, the status could likely be draft iso raw already. See https://rfc.vac.dev/spec/1/.

Agree

Copy link
Contributor

@oskarth oskarth left a comment

Choose a reason for hiding this comment

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

Nice one!


Properly protecting against eclipse attacks is challenging and raises research questions that we will address in future stages of our discv5 roadmap.

# References
Copy link
Contributor

Choose a reason for hiding this comment

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

Can add links referenced throughout text here

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. Done.

kaiserd and others added 2 commits March 7, 2022 12:19
@kaiserd kaiserd merged commit 6498d92 into master Mar 7, 2022
vac-research automation moved this from Review/QA to Done Mar 7, 2022
@kaiserd kaiserd deleted the add/waku-discv5-rfc branch March 7, 2022 11:44
kaiserd added a commit that referenced this pull request Jul 16, 2022
* master:
  RFC16: add version call (#505)
  fix(noise): update RFC to implementation (#508)
  fixup: 37/WAKU2-NOISE fix images paths (#506)
  New RFC: 37/WAKU2-NOISE-SESSIONS (#504)
  36/WAKU2-BINDINGS-API (#501)
  docs(16/WAKU2-RPC): add ENR to waku info (#502)
  Adding 35/WAKU2-NOISE to menu (#500)
  add RFC33 to index (#499)
  feat: 32/RLN raw spec
  New RFC: 35/WAKU2-NOISE (#496)
  Update on the rln registration figure to match the current spec (#497)
  33/WAKU-DISCV5: Add first raw version (#487)
  Add pubsubTopic field to index (#492)
  Fix markdown links (#493)
  Categorize 22 & 31 (#490)
  Changed PB Timestamp index to 10 (#491)
  13/14/16/21: Change in timestamp format (#483)
  add: RFC31 copyright statement (#489)
  17/WAKU-RLN-RELAY: Revise spec for its draft version (#484)
kaiserd added a commit that referenced this pull request Jul 25, 2022
* master:
  RFC16: add version call (#505)
  fix(noise): update RFC to implementation (#508)
  fixup: 37/WAKU2-NOISE fix images paths (#506)
  New RFC: 37/WAKU2-NOISE-SESSIONS (#504)
  36/WAKU2-BINDINGS-API (#501)
  docs(16/WAKU2-RPC): add ENR to waku info (#502)
  Adding 35/WAKU2-NOISE to menu (#500)
  add RFC33 to index (#499)
  feat: 32/RLN raw spec
  New RFC: 35/WAKU2-NOISE (#496)
  Update on the rln registration figure to match the current spec (#497)
  33/WAKU-DISCV5: Add first raw version (#487)
  Add pubsubTopic field to index (#492)
  Fix markdown links (#493)
  Categorize 22 & 31 (#490)
  Changed PB Timestamp index to 10 (#491)
  13/14/16/21: Change in timestamp format (#483)
  add: RFC31 copyright statement (#489)
  17/WAKU-RLN-RELAY: Revise spec for its draft version (#484)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants