-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat: making dns discovery async #3175
Conversation
You can find the image built from this PR at
Built from a5588af |
8f8d588
to
5551809
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM !
There was a problem hiding this 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! 💯
I just added a nitpick comment
waku/factory/waku.nim
Outdated
@@ -400,3 +439,6 @@ proc stop*(waku: Waku): Future[void] {.async: (raises: [Exception]).} = | |||
|
|||
if not waku.node.isNil(): | |||
await waku.node.stop() | |||
|
|||
if waku.dnsRetryLoopHandle.isSome(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be a little bit simpler by just checking if dnsRetryLoopHandle
is not nil. I'd suggest to avoid using Option as much as possible but is just a personal nitpick, not a big difference though ;P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superr. good point! Updated it - I agree that it's nicer and simpler without the Option :)
Thank you!
This reverts commit f35ded4.
6db2aab
to
b9d8ddc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The only observation i have is that in status-go it is possible to specify more than one enrtree. We don't use that tho since we have a single one, but i thought that it would be a good idea to mention it.
Good work, sir!
Description
Instead of exiting if DNS Discovery fails at startup, we now create a loop that attempts to perform DNS Discovery every 30 seconds until it succeeds.
Changes
wakunode2
nim-dnsdisc
submoduleHow to test
Run
wakunode2
configuring discv5 and dns-discovery with the internet connection off, then turn it on after some time and see that DNS Discovery is performed and Discv5 finds peersIssue
#3076