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: avoid sending relay callbacks if relay is disabled #3276

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

gabrielmer
Copy link
Contributor

@gabrielmer gabrielmer commented Feb 5, 2025

Description

When we attempt to start a node with Relay disabled using libwaku, the node creation fails because we attempt to set Relay app callbacks while having Relay disabled.

We should not attempt to configure Relay related callbacks if Relay is disabled.
Added a fix to send nil for the Relay related callbacks in case Relay is disabled.

Notice that I override the callbacks instead of configuring them to nil in the first place, and that's because when we set the callbacks, we still don't have the configuration parsed and it's not trivial to parse. I also attempted to set the callbacks after parsing the configuration but I got into a circular dependency error.

Therefore, I'm just adjusting which callbacks to actually send the moment we have the configuration parsed.

Changes

  • setting Relay callbacks to nil in case Relay is disabled

Issue

#3076

Copy link

github-actions bot commented Feb 5, 2025

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3276

Built from a3924ff

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 it! 💯
Should we do that in "factory" side instead? That way wakunode2 also benefits from it.

@gabrielmer
Copy link
Contributor Author

gabrielmer commented Feb 5, 2025

Should we do that in "factory" side instead? That way wakunode2 also benefits from it.

So in "factory", if an app callback for Relay is configured without having Relay mounted, we return an error.

nwaku/waku/factory/waku.nim

Lines 158 to 160 in 81a19c3

if not appCallbacks.relayHandler.isNil():
if node.wakuRelay.isNil():
return err("Cannot configure relayHandler callback without Relay mounted")

I'm more keen on not sending wrong configurations from libwaku than accepting wrong configurations in nwaku, does it make sense?

@gabrielmer gabrielmer merged commit 8a7e602 into master Feb 5, 2025
12 of 15 checks passed
@gabrielmer gabrielmer deleted the fix-disabling-relay-on-libwaku branch February 5, 2025 16:16
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