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

Add optional node validation to routing table #435

Closed
wants to merge 3 commits into from

Conversation

jm-clius
Copy link

This PR addresses waku-org/nwaku#770

It adds an optional node validator to the discv5 routing table, useful for client-defined rules to prevent certain nodes from being added for discovery v5. This can be used i.a. to create a separate discv5 network for Waku v2 based on the Waku v2 ENR key.

@kdeme, assuming that we reach consensus on this PR's approach, I'm not sure what the suggested way would be to proceed:
a) merge this to nim-eth master if it's "safe enough", or
b) rather run Waku v2 discovery v2 on a forked implementation for a while until we see how well it works.

cc @richard-ramos for future reference

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.

Seems reasonable to me.

Question: Are we "validating" a node or are we "filtering"? E.g. what's the proper model to use here? For validating, it seems like we'd kick off those nodes completely. For filtering, it seems like we'd mostly hide from view, and one could imagine this filter growing into something more dynamic (say capability based). But perhaps that should happen at a higher level too, not sure.

Copy link
Contributor

@kdeme kdeme left a comment

Choose a reason for hiding this comment

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

@kdeme, assuming that we reach consensus on this PR's approach, I'm not sure what the suggested way would be to proceed:
a) merge this to nim-eth master if it's "safe enough", or
b) rather run Waku v2 discovery v2 on a forked implementation for a while until we see how well it works.

I do think it is safe enough, and for that I'd be fine with having it merged.

I'm however not fully sure if it is sufficient, see my answer to your question here: waku-org/nwaku#770 (comment)

Perhaps it should first be tried out because of this? We could leave this branch alive and it can be used in nim-waku. WDYT?

@jm-clius
Copy link
Author

Thanks, @kdeme and also for your through answer in waku-org/nwaku#770

Perhaps it should first be tried out because of this? We could leave this branch alive and it can be used in nim-waku. WDYT?

I agree, and think it's relatively cheap to test on this feature branch for a while.

Question: Are we "validating" a node or are we "filtering"?

@oskarth fair point. In my mind, preventing a node from being added to the local routing table is slightly closer to "validating" than "filtering". Maybe this is only because discv5 already allows for "filtering" on enr field when querying for nodes, and I wanted to keep this a separate concept. I'd say capability filtering is closer to the latter. No strong opinion though.

@jm-clius
Copy link
Author

Closing, as we've gone a different way. cc @kaiserd

@jm-clius jm-clius closed this May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants