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: setting up node with modified config #3036

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

gabrielmer
Copy link
Contributor

@gabrielmer gabrielmer commented Sep 12, 2024

Description

We currently modify a copy of our configurations when initializing a node in

nwaku/waku/factory/waku.nim

Lines 104 to 106 in 1713f56

proc init*(T: type Waku, conf: WakuNodeConf): Result[Waku, string] =
var confCopy = conf
let rng = crypto.newRng()

However, we don't use this modified configuration when setting up the protocols and initializing the node, which causes mismatches between them.

Added a fix to use the same version of the configuration at thewakunode2 application layer.

Changes

  • using modified config copy in wakunode2

Copy link

github-actions bot commented Sep 12, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3036

Built from f081254

@gabrielmer gabrielmer force-pushed the fix-overriding-shards-if-pubsbtopic-is-configured branch from 17f18b9 to 23d6a77 Compare September 13, 2024 06:53
@gabrielmer gabrielmer marked this pull request as ready for review September 13, 2024 11:59
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

Hm, nice catch! Thank you for it!

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.

Thanks for it! Just added a question

Comment on lines -105 to +104
var confCopy = conf
proc init*(T: type Waku, confCopy: var WakuNodeConf): Result[Waku, string] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually think these are the only needed changes.
Did you try to see what happens with just that?

Copy link
Contributor Author

@gabrielmer gabrielmer Sep 16, 2024

Choose a reason for hiding this comment

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

Yes, so the need of each change is the following:

  1. In this file, apart from this line, changed the name to refer that we are using a copy of the original configuration and that we're not modifying the user's input

  2. In apps/wakunode2/wakunode2.nim, we create a copy of the original configuration and pass it to all the procs (so we don't modify the original configuration passed by the user)

  3. In tests/wakunode2/test_app.nim, had to change from let to var for the configurations passed to init() now that we modify the object in the proc

Let me know if there's one of these points you think we could spare

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we use/re-use the original user configuration afterward? Just curious...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we use/re-use the original user configuration afterward? Just curious...

After that point we actually don't use the original configuration, but did it just to not override the original user configuration which generally isn't a good pattern. In principle we can avoid doing the copy and change the original but idk, it doesn't feel right haha

@gabrielmer gabrielmer merged commit 8f28992 into master Sep 16, 2024
9 of 11 checks passed
@gabrielmer gabrielmer deleted the fix-overriding-shards-if-pubsbtopic-is-configured branch September 16, 2024 13:30
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