wakurelay, and underlying gossipsub, is never started

[jm-clius]

Description

WakuRelay defines start() on the underlying GossipSub protocol, but this is never called for a wakunode2.

Impact

The impact is that (a) the GossipSub heartbeat never runs, and (b) direct peers are not maintained. The latter is not yet used for wakunode2, but heartbeat is required to maintain the mesh, send IHAVE and IWANT messages, etc.

[oskarth]

Huh, this is surprising to me. I wonder if this is a regression that was introduced as part of one of the wakunode init changes?

[jm-clius]

Yeah, I've been trying to trace this, but couldn't find where this was introduced yet. Especially strange given that wakuRelay is stopped explicitly, just never started.

[sinkingsugar]

As a side note, if nim had the option, GossipSub would be marked final, inheriting from it was not really intended as a feature.

[jm-clius]

Interesting, @sinkingsugar. Do you foresee any major issues if we continue in this way? I understand the js and go implementations are not as easily extensible.

[sinkingsugar]

Interesting, @sinkingsugar. Do you foresee any major issues if we continue in this way? I understand the js and go implementations are not as easily extensible.

Not for now but who knows, it is definitely something we did not expect from users tho. Sadly the language just make it implicitly possible.

[oskarth]

@sinkingsugar At least previously, the way the code was structured in nim-libp2p the inheritance chain was quite clear with GossipSub < FloodSub < PubSub, hence following that pattern. This is also true on a spec level.

If this becomes an in issue, we can take the existing logic and have it in nim-waku instead, but seems a bit silly to not do code re-use here

[oskarth]

It also used to be the case that we had WakuRelay being able to choose between GossipSub/FloodSub, the idea being that this could later be extended to various forms of Kademlia routing etc... Hiding implementation details and focusing on interface/privacy aspects for WakuRelay.

[sinkingsugar]

I understand that yes but I mean more in a broad sense it's not a good practice to inherit from external modules unless the modules explicitly allow it or state it.

In our case it's easy cos we have control over everything as organization and as long as we pay attention anything is possible.
But libp2p any time could change the layout and break it. In fact recent refractors went pretty close to it.

Of course the language did not help and the lack of proper documentation as well 😃

[dryajov]

The other part to inheriting and one reason why js and go make it difficult is because gossipsub is a "fine tuned and self contained" algorithm that's designed to perform within parameters as long as it's internals aren't "tweaked". You can think of it as a warranty seal that if broken the vendor cannot continue providing reasonable guarantees.

In the case of wako, the tweaks are minor and reasonable and there shouldn't be any major issues and as @sinkingsugar mentioned, for the most part the status org controls the stack, so we should be fine. That said, I would keep the tweaks to a minimum and if any major changes are required, then I would indeed fork/copy.

For this particular issue, I remember that the start/stop methods were overridden, which should be fine as long as the base method is called on gossipsub thru procCall GossipSub(w).start()/procCall GossipSub(w).stop(). However, Nim has numerous bugs/subtleties related to procs and methods overrides, where missing a * on a method will make it a completely different method call and break the dynamic dispatch chain, so I'd be careful with making sure that the signatures and exports all match. An even better approach would be to not add the additional indirection and simply not override the start/stop methods in waku - by reducing the surface of changes we also decrease the potential for bugs.