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

Discovery v5: separate Waku network #770

Closed
jm-clius opened this issue Nov 15, 2021 · 7 comments
Closed

Discovery v5: separate Waku network #770

jm-clius opened this issue Nov 15, 2021 · 7 comments
Assignees

Comments

@jm-clius
Copy link
Contributor

Background

Recently we added a new waku-specific ENR field to identify Waku 2 nodes within a larger ENR-based discovery mechanism, such as Discovery v5.

In a Discovery v5 context, this addition allows Waku nodes to filter discovered nodes to only use/connect to other Waku nodes, but it does not affect which nodes are stored in the underlying routing table. In practice, then, the Waku 2 discv5 network won't be kept separate from other discv5 networks (e.g. for eth2).

Proposed solution

Add a validator function to the discovery v5 implementation in nim-eth, that:

  • belongs to the Protocol object
  • is optional and defaults to none (in order not to affect existing implementations)
  • prohibits nodes from being added to the routing table if validation fails
  • is externally defined, so that Waku v2 can provide its own implementation to filter on the waku2-field

It should also be possible to fork nim-eth for the nim-waku use case above, though I'd like to avoid this in the long term.

cc @oskarth @kdeme

@oskarth oskarth added this to New in Deprecated: nwaku Nov 15, 2021
@kdeme
Copy link
Contributor

kdeme commented Nov 19, 2021

Proposed solution looks good to me, and I had something very similar in mind. I was however going to propose the validation function as part of the routing table object instead, and do the validation in the AddNode there.

Few additional notes:

  • This does mean that nodes which don't do the extra validation call (say for example eth2 nodes) will still have waku nodes in their routing table and occasionally ping them or send findnode requests. I think that this is fine, as the Waku nodes will be a minority (But they will give useless nodes for these eth2 nodes of course). But the networks would not be completely separated. One could also add the validation in the message handler to not reply to such nodes, but I'm not sure yet if we should add that (In this case the validation call should probably be part of the Protocol object indeed).
  • This will lower the amount of nodes in your routing table and thus leaving you more susceptible to attacks. However, I guess it can be argued that not being able to find nodes properly would also allow for attacks (i.e. someone forcing incoming connections and us not finding good peers for outgoing connections).

@jm-clius
Copy link
Contributor Author

Thanks, @kdeme. Good points.

It makes sense to me to have this be part of routing table instead of Protocol (at least for now) if we start by only validating before adding to the table.

I imagine some "leakage" will inevitably occur, but if all Waku nodes maintain a separate routing table and validate their bootstrap nodes also before contacting them, wouldn't the networks remain separate at least in theory?

@kdeme
Copy link
Contributor

kdeme commented Nov 26, 2021

I imagine some "leakage" will inevitably occur, but if all Waku nodes maintain a separate routing table and validate their bootstrap nodes also before contacting them, wouldn't the networks remain separate at least in theory?

Without modifications, "leakage" could happen for example when:

  1. Some node is created supporting eth2 and waku2 and connecting to both a node on the waku network and one on the eth2 network (launching for example with a bootstrap node of both). This can happen also simply accidental, adding a wrong bootstrap node on cli. Once this is done, nodes will immediately be interchanging nodes and the networks are pretty much linked forever.
  2. Node A is started on network A with a specific network key and IP:Port combination in the ENR. Then one starts a node B for network B (and stops node A) but reuses the same network key and IP:Port combination in the ENR as for node A. Some node(s) of network A still has the ENR of node A in its routing table and contacts the, now new, node B, which is connected already with network B.

Now with these suggested modifications, one network (waku) will filter out the nodes. However the other network(s) will not. If the same leakage as in 1. or 2. occurs, the other networks will for example accidentally contact a node on the waku network, and this node will still reply to the request with a bunch of (waku only) nodes. These will still reply to ping requests and thus the nodes in the other networks will still store these in the routing table. They will still go around in that network, and thus they will also frequently be contacted. I don't really see an issue with that, aside from bandwidth / cpu usage, which might be of concern of course for the Waku use cases.

To fully seperate those, the waku filtering would also have to be applied on the message responses, basically ignoring messages from nodes of the other networks. This is however not possible without more changes in the current implementation as ENRs are not necessarily (They might be, depends on network key, routing table buckets, etc.) stored for each "session" with a node. This too would have to be added.

Perhaps at that point a better solution is to make the protocols (slightly) incompatible. I'm thinking then specifically of making the protocol-id or version different in the static-header: https://github.com/ethereum/devp2p/blob/master/discv5/discv5-wire.md#protocol-header. This too could be made configurable in our discv5 implementation.

@jm-clius
Copy link
Contributor Author

Thanks for the very thorough summary, @kdeme.
Indeed, we'd have to introduce a further incompatibility then to ensure networks remain separate.
I agree with your proposal in status-im/nim-eth#435 that we test nim-waku with the validator then as a first step.

@arnetheduck
Copy link
Contributor

shared discovery is something of a feature in that it's harder to take down - how big of a problem is this actually? ie what happens is you connect to a node, figure out that it doesn't support the features you want and discard it - this shouldn't be very expensive

@jm-clius
Copy link
Contributor Author

For me, the unknown would be how easily you'd then find interesting Waku nodes to connect to, especially if you have some further requirements re capabilities, store size, etc. I know this needle-in-a-haystack is something we'd like to test in any case, but it may be worth maintaining a separate discv5 network, at least while we are testing/have only very few nodes of interest to us. wdyt?

@oskarth oskarth moved this from New to In Progress in Deprecated: nwaku Nov 29, 2021
@jm-clius jm-clius self-assigned this Dec 6, 2021
@jm-clius jm-clius moved this from In Progress to Backlog in Deprecated: nwaku Jan 10, 2022
@kaiserd kaiserd self-assigned this Jan 17, 2022
@jm-clius
Copy link
Contributor Author

jm-clius commented May 9, 2022

Note that we have proceeded to separate the Waku discv5 network as discussed in #837 with unification with Ethereum on the roadmap once a critical mass is reached.

@jm-clius jm-clius closed this as completed May 9, 2022
Deprecated: nwaku automation moved this from Backlog to Done May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

4 participants