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

fix: cluster-id 0 discv5 issue #2562

Merged
merged 1 commit into from
Apr 4, 2024
Merged

fix: cluster-id 0 discv5 issue #2562

merged 1 commit into from
Apr 4, 2024

Conversation

alrevuelta
Copy link
Contributor

Description

  • This PR deprecated topics but introduced a regression.
  • Since topics flag default value /waku/2/default-waku/proto was being used when constructing the ENR, after removing that flag, we no longer had a default value.
  • This means that eg wakunode2 --cluster-id=0 ... discv5 was not working properly, since the node had no default pubsub topic configured.
  • This PR fixes it, by setting the default pubsub topic for cluster-id=0.
  • Other unknown clusters, would need to provide their desired pubsub-topic.

let clusterZeroConf = ClusterConf.ClusterZeroConf()
conf.pubsubTopics = clusterZeroConf.pubsubTopics
# TODO: Write some template to "merge" the configs
# cluster-id=1 (aka The Waku Network)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no real change from here. just idendation.

Copy link

github-actions bot commented Apr 2, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2562-rln-v2-false

Built from 12f693a

@alrevuelta alrevuelta changed the title fix: cluster-id 0 disc5 issue fix: cluster-id 0 discv5 issue Apr 2, 2024
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for such a quick fix!

conf.discv5BootstrapNodes & twnClusterConf.discv5BootstrapNodes
conf.rlnEpochSizeSec = twnClusterConf.rlnEpochSizeSec
conf.rlnRelayUserMessageLimit = twnClusterConf.rlnRelayUserMessageLimit
discard
Copy link
Collaborator

Choose a reason for hiding this comment

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

We discard at this point because we are not willing to set any particular configuration when either cluster_id == 0 or 1, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct. anyone can define their own clusterId eg =999 and there is no config we set in there.

of 0:
let clusterZeroConf = ClusterConf.ClusterZeroConf()
conf.pubsubTopics = clusterZeroConf.pubsubTopics
# TODO: Write some template to "merge" the configs
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean by "merge the configs" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

imagine

type A:
  field1
  field2

typeB:
  field1
  field2
  field3
  field4

which is similar to WakuConf and ClusterZeroConf. Note that typeB is a superset of typeA.

Right now we are "merging" boh manually.

let a = TypeA(...)
let b = TypeB(...)

# we want a config to override b
b.field1 = a.field1
b.field2 = a.field2

this is i) error prone, ii) if number of parameters increase, we have to assign them manually.

so I'm suggesting with some templating, writing a merge() function so that.

let a = TypeA(...)
let b = TypeB(...)

merge(a, b)

or something like this. with this we no longer need all this

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Thanks!

@alrevuelta alrevuelta merged commit a76c958 into master Apr 4, 2024
13 of 15 checks passed
@alrevuelta alrevuelta deleted the fix-cluster-zero branch April 4, 2024 06:19
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.

3 participants