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.

Sign up for GitHub

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

Jump to bottom

chore: remove rln epoch hardcoding #2483

Merged
merged 5 commits into from
Feb 28, 2024
Merged

chore: remove rln epoch hardcoding #2483

merged 5 commits into from
Feb 28, 2024

Conversation

alrevuelta
Copy link
Contributor

@alrevuelta alrevuelta commented Feb 27, 2024 •
edited

  • Remove hardcoded EpochUnitSeconds and allows configuring it via rln-relay-epoch-sec.
  • It also sets TheWakuNetwork default epoch value in here as per spec
  • This allows anyone to deploy another network with their own RLN configuration.

@github-actions GitHub Actions
Copy link

This PR may contain changes to configuration options of one of the apps.

If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed.

Please also make sure the label release-notes is added to make sure any changes to the user interface are properly announced in changelog and release notes.

alrevuelta
alrevuelta commented Feb 27, 2024
View reviewed changes
apps/networkmonitor/networkmonitor.nim
@@ -546,6 +548,7 @@ when isMainModule:
rlnRelayCredPath: "",
rlnRelayCredPassword: "",
rlnRelayTreePath: conf.rlnRelayTreePath,
rlnEpochSizeSec: conf.rlnEpochSizeSec
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vpavlin if you can have a look just in case.

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 27, 2024 •
edited

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2483

Built from 3e691c7

rymnc
rymnc reviewed Feb 28, 2024
View reviewed changes
Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

Not sure if this is a required feature, are there any applications who want to use a larger epoch size?

apps/benchmarks/benchmarks.nim Show resolved Hide resolved
apps/wakunode2/external_config.nim Show resolved Hide resolved
@alrevuelta
Copy link
Contributor Author

Not sure if this is a required feature, are there any applications who want to use a larger epoch size?

@rymnc well, at some point us? With rln v2 epoch size would have to be greater than 1. And if someone else wants to deploy a network with a different configuration, then this would be interesting. Adding it for completeness as well, because right now you can configure the rlnv2 message limit but not the epoch size.

gabrielmer
gabrielmer reviewed Feb 28, 2024
View reviewed changes
Copy link
Contributor

@gabrielmer gabrielmer left a comment •
edited

Choose a reason for hiding this comment

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

Looks great! Thanks so much!

Added minor comments. We should probably address the failed test. Taking a look at it

apps/chat2/config_chat2.nim Outdated
@@ -272,6 +272,11 @@ type
defaultValue: 1,
name: "rln-relay-user-message-limit" .}: uint64

rlnEpochSizeSec* {.
desc: "Epoch size in second used to rate limit RLN memberships. Default is 1 second.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Super mega nitpick

Suggested change
desc: "Epoch size in second used to rate limit RLN memberships. Default is 1 second.",
desc: "Epoch size in seconds used to rate limit RLN memberships. Default is 1 second.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! fixed 857810d

apps/networkmonitor/networkmonitor.nim Outdated
@@ -494,6 +494,8 @@ when isMainModule:
conf.pubsubTopics = twnClusterConf.pubsubTopics
conf.rlnRelayDynamic = twnClusterConf.rlnRelayDynamic
conf.rlnRelayEthContractAddress = twnClusterConf.rlnRelayEthContractAddress
conf.rlnEpochSizeSec = twnClusterConf.rlnEpochSizeSec
conf.rlnRelayUserMessageLimit: twnClusterConf.rlnRelayUserMessageLimit
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be like this?

Suggested change
conf.rlnRelayUserMessageLimit: twnClusterConf.rlnRelayUserMessageLimit
conf.rlnRelayUserMessageLimit = twnClusterConf.rlnRelayUserMessageLimit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks fixed! 3932fe2

@gabrielmer
Copy link
Contributor

Ok so the failed test is because we need to add rlnEpochSizeSec and rlnRelayUserMessageLimit to networkmonitor_config.nim I think

@gabrielmer gabrielmer self-requested a review February 28, 2024 12:19
@alrevuelta alrevuelta merged commit 3f4f6d7 into master Feb 28, 2024
9 of 10 checks passed
@alrevuelta alrevuelta deleted the epoch-parameter branch February 28, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rymnc rymnc rymnc left review comments

@jm-clius jm-clius jm-clius approved these changes

@gabrielmer gabrielmer gabrielmer approved these changes

@Ivansete-status Ivansete-status Awaiting requested review from Ivansete-status

Assignees

@alrevuelta alrevuelta

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alrevuelta @gabrielmer @rymnc @jm-clius