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

doc: clarify max shard value on CLI args #2856

Closed
wants to merge 3 commits into from
Closed

Conversation

fryorcraken
Copy link
Collaborator

I started this PR because it bugged me that I could not simply start wakunode2 and connect to it via wakucanary because wakucanary wants shards:

▶ ./build/wakucanary -a=...
INF 2024-06-28 13:59:19.881+10:00 Cli flags                                  tid=2317631 file=wakucanary.nim:164 address=/ip4/0.0.0.0/tcp/60000/p2p/16Uiu2HAm6AKL8iczGNguEwwcJGmCmgAS2cpTRve4UkMExtwt45gA timeout=10s protocols=@[] logLevel=INFO
ERR 2024-06-28 13:59:19.883+10:00 Relay shards initialization failed         tid=2317631 file=wakucanary.nim:209 error="invalid shard count"
ERR 2024-06-28 13:59:19.883+10:00 The node has some problems (see logs)      tid=2317631 file=wakucanary.nim:283

In the good default value spirit, we should be able to run the binary with minimal config out of the box, without having to fiddle things.

The good default at the moment should either be:
1, TWN (aka RLN Relay protected network)
2. Status prod

There is value in doing either. However, for the sake of dogfooding and pushing RLN further, I believe (1) should be default. Until we can get RLN on Status network.

So, the proposal I'd like to make before I proceed forward is to change all default parameters to TWN, meaning that a user will have to pass a API address for eth client to play with nwaku.

This would mean something similar to nwaku-compose, but without postgres.

Cc @waku-org/nwaku-developers

cc @gabrielmer some suggestions in there in terms of handlng shards, in reference to #2163

Copy link

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

Unknown release type "doc" found in pull request title "doc: clarify max shard value on CLI args".

Available types:

  • chore
  • docs
  • feat
  • fix
  • refactor
  • style
  • test

Copy link

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2856

Built from c62a8f8

pubsubTopics.add($NsPubsubTopic.staticSharding(conf.clusterId, shard))

if conf.shards.len == 0:
# If shards are not configured but content topics are, then assume autosharding is expected
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am guessing this is incorrect as you'd need to have shards configured (ie 0 to 7 ) to apply an auto sharding logic as the result of the autosharding logic depends on the shard range available.

Copy link
Contributor

Choose a reason for hiding this comment

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

so currently we deduce the amount of shards based on the number of pubsub topic configured. In other words, the --pubsub-topic parameter shows all the possible shards that exist

To avoid ambiguity, in the PR deprecating the pubsub topic concept will add a new CLI flag which will indicate the number of shards in a network

debug "Shards created from content topics",
contentTopics = conf.contentTopics, shards = shards
var pubsubTopics: seq[string] = conf.pubsubTopics
for shard in conf.shards:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gabrielmer I think this is needed in #2723

@@ -423,7 +423,7 @@ proc mountRelay*(

node.switch.mount(node.wakuRelay, protocolMatcher(WakuRelayCodec))

info "relay mounted successfully"
info "relay mounted successfully", pubsubTopics
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Something useful @gabrielmer

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.

2 participants