Gossipsub Parameters are Not Easily Configurable

[nisdas]

Currently all the gossipsub specific parameters are package globals which are initialized at startup.Ex:

	GossipSubD = 6
	GossipSubDlo = 5
	GossipSubDhi = 12
	GossipSubDscore = 4
	GossipSubDout = 2
        .
        .
        .

However for our usecase we need the ability to modify a few parameters, before creating the gossipsub router. The current method would be to set the package variable to the value we want and the instantiate the router. However this method seems potentially brittle as it could easily conflict if we instantiate another router with different parameters. Currently there is no way to provide specific gossipsub parameters for one router instance.

We could instead set all the parameters using a GossipSubParams config and just call the values from there instead of referring to the global parameters. That way each instance of the gossip-sub router refers to the parameters in the config instead of the globally set parameters.

[raulk]

Yes, this seems like a trivial change to implement. We would encapsulate all these parameters in a struct, as you say, and have the gossipsub logic be driven by a variable of that type that's set on the router.

We'd provide a WithGossipsubParams option. If the constructor finds that this option is not set, and therefore the struct on the router is nil it would fall back to populating it by using the global defaults.

@vyzo?

[vyzo]

Sure let's do that. Care for a pr? It should be straightforward.

[nisdas]

Sure thing, I can open up a PR for this 👍

[nisdas]

I have ran into a bit of a problem with implementing this, while it is easy enough to create a new Config Struct with all the gossipsub parameters, using that config throughout the router brings about some issues. A lot of the methods in the router using the global defaults directly, if we were to change to check for the set parameter config. It would look like this:

if ps.params != nil {
// Use value from params
} else {
// use global defaults
}

This would add a lot of boiler-plate code to an already large file, and it would make the code harder to read as the global defaults are used everywhere.

Alternatively we could instead set a default config, which would be used whenever the pubsub option isnt set , and just fallback to that. So instead of

GossipSubD 

we use

ps.params.GossipSubD

this would be much cleaner to read and parse compared to the first option. The downside would be that this would remove all the global defaults and would involve all instances of global defaults across the repo to be replaced and instead we refer to the params as shown above.

[vyzo]

The router uses the reified parameters already (thre are D* members in the struct).
What you can do is simply pass a struct in the option and set the reified variables from it (using the defaults at construction).
Alternatively, you can group them in a struct, with the default initializer.
Don't bother with unset and nil checks, always initialize the router parameters from the defaults at construction, and let the option override.

[nisdas]

I was referring to parameters such as this
https://github.com/libp2p/go-libp2p-pubsub/blob/master/gossipsub.go#L779
https://github.com/libp2p/go-libp2p-pubsub/blob/master/gossipsub.go#L542
https://github.com/libp2p/go-libp2p-pubsub/blob/master/gossipsub.go#L1203

None of these params are set in the struct, so there is no easy way to set it and use it other than setting a param config in the router and accessing it from there. These parameters are not initialized at construction instead always using the global defaults.

[vyzo]

ok, but why is that a problem? Just reify them too.

[nisdas]

Ok great, we are in agreement then. I misunderstood your earlier point, one last question do we still need to keep the package global defaults around for any reason or is it fine to just remove them completely.

[vyzo]

Yes, please keep them -- these are the values that will initialize the parameters per instance, unless there is an option override.