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

Start relay protocol on wakunode2 #446

Merged
merged 3 commits into from
Apr 1, 2021

Conversation

jm-clius
Copy link
Contributor

This PR fixes #445

It ensures that relay protocol, and the underlying gossipsub, is started on a wakunode2.

This fix assumes the following:

  1. A node can be started and relay protocol mounted in any order.
  2. When relay is mounted, it should start if and only if the node was already started. Otherwise it should start on node start.

Note: I'm not sure if the original intention was to be somewhat more strict in terms of the ordering above, e.g. that protocols should always be mounted before the node starts listening. In our current examples, tests and scripts, the mount()<->start() order seems to be interchangeable, which allows for mounting protocols on live nodes.

@jm-clius jm-clius requested review from oskarth and staheri14 March 31, 2021 12:04
@@ -416,6 +419,13 @@ proc mountRelay*(node: WakuNode, topics: seq[string] = newSeq[string](), rlnRela
# TODO currently the message validator is set for the defaultTopic, this can be configurable to accept other pubsub topics as well
addRLNRelayValidator(node, defaultTopic)
info "WakuRLNRelay is mounted successfully"

if node.libp2pTransportLoops.len > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, is this the idiomatic way of doing this? How does it work in nimbus-eth2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nimbus-eth2 follows a strict order where the protocol is always mounted before starting the node. In fact, they mount GossipSub when creating the eth2 node, implying that my assumptions in the description above don't hold for them. We may consider doing the same (perhaps only for WakuRelay?), but that would require a fairly significant change in approach.

Seeing if related futures are set to know if the protocol/node has started doesn't feel idiomatic, but is at least not without precedent - this is similar to how GossipSub itself avoids a "double" start. A cleaner alternative, of course, is to just add started state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, I've changed this now to rather use an explicit started state variable (31700c8). Lmk if you have strong feelings about either the order in which we mount/start or what type of state we keep.

@jm-clius jm-clius merged commit 683577f into master Apr 1, 2021
@jm-clius jm-clius deleted the fix/relay-gossipsub-never-started branch April 1, 2021 09:37
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.

wakurelay, and underlying gossipsub, is never started
2 participants