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

chore: integrate on-demand DNS discovery and implement discoverAndConnectPeers #6000

Conversation

gabrielmer
Copy link

@gabrielmer gabrielmer commented Oct 28, 2024

Integrating libwaku's on-demand DNS discovery functionality and implementing discoverAndConnectPeers

Notice that for discoverAndConnectPeers, we have to do it sequentially and not with goroutines, as libwaku doesn't work properly with goroutines for now (because of goroutines spinning up multiple threads and Nim being set up only in one).

This PR will work properly once waku-org/nwaku#3155 is merged, will update the nwaku submodule here once the fix is in master

Important changes:

  • integrated libwaku's waku_dns_discovery procedure
  • added test for DNS discovery
  • implemented discoverAndConnectPeers

Issue waku-org/nwaku#3076

richard-ramos and others added 12 commits October 15, 2024 18:22
- some minor progress to add nwaku in status-go
- nwaku.go: GetNumConnectedPeers controls when passed pubsub is empty
- waku_test.go: adapt TestWakuV2Store
- add missing shard.go
- feat_: build nwaku with nix and use build tags to choose between go-waku and nwaku (#5896)
- chore_: update nwaku
- nwaku bump (#5911)
- bump: nwaku
- chore: add USE_NWAKU env flag
- fix: build libwaku only if needed
- feat: testing discovery and dialing with nwaku integration (#5940)
@gabrielmer gabrielmer self-assigned this Oct 28, 2024
Copy link

We require commits to follow the Conventional Commits, but with _ for non-breaking changes.
Please fix these commit messages:

implement discoverandConnectPeers
adding dns discovery test
test test
initial dns discovery integration

}
}
var addrsToConnect []multiaddr.Multiaddr
nameserver := "1.1.1.1"
Copy link
Author

Choose a reason for hiding this comment

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

What should I add here? For sure there's a better option than hardcoding this, but want to know how is it currently done :))

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say for now just add TODO-nwaku ;P
Jokes apart (or not) I think the best would be to avoid passing the nameserver parameter and instead, have the complexity of picking up the appropriate dns-server within libwaku.

time.Sleep(1 * time.Second)

sampleEnrTree := "enrtree://AMOJVZX4V6EXP7NTJPMAYJYST2QP6AJXYW76IU6VGJS7UVSNDYZG4@boot.prod.status.nodes.status.im"
nameserver := "1.1.1.1"
Copy link
Author

Choose a reason for hiding this comment

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

same here

Copy link
Contributor

@Ivansete-status Ivansete-status Oct 29, 2024

Choose a reason for hiding this comment

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

As mentioned above, I think we can avoid passing the nameserver and leave that responsability to libwaku.
Something to be considered once the following is completed: waku-org/nwaku#1490

@gabrielmer gabrielmer marked this pull request as ready for review October 28, 2024 16:43
@status-im-auto
Copy link
Member

status-im-auto commented Oct 28, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ 3a03c44 #1 2024-10-28 16:43:08 ~1 min tests 📄log
✔️ 3a03c44 #1 2024-10-28 16:46:18 ~4 min ios 📦zip
✔️ 3a03c44 #1 2024-10-28 16:46:31 ~5 min linux 📦zip
✔️ 3a03c44 #1 2024-10-28 16:46:47 ~5 min android 📦aar
✖️ 3a03c44 #1 2024-10-28 16:48:07 ~6 min tests-rpc 📄log

Copy link

codecov bot commented Oct 28, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
23 1 22 0
View the top 1 failed tests by shortest run time
tests.test_router.TestTransactionFromRoute test_tx_from_route
Stack Traces | 10.6s run time
tests/test_router.py:114: in test_tx_from_route
    assert tx_details["to"] == user_2.address
E   AssertionError: assert '0x70997970C5...50e0d17dc79C8' == '0x70997970c5...50e0d17dc79c8'
E     - 0x70997970c51812dc3a010c7d01b50e0d17dc79c8
E     ?           ^        ^   ^                ^
E     + 0x70997970C51812dc3A010C7d01b50e0d17dc79C8
E     ?           ^        ^   ^                ^

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

Copy link
Contributor

@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! 🙌 🥳

}
}
var addrsToConnect []multiaddr.Multiaddr
nameserver := "1.1.1.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say for now just add TODO-nwaku ;P
Jokes apart (or not) I think the best would be to avoid passing the nameserver parameter and instead, have the complexity of picking up the appropriate dns-server within libwaku.

time.Sleep(1 * time.Second)

sampleEnrTree := "enrtree://AMOJVZX4V6EXP7NTJPMAYJYST2QP6AJXYW76IU6VGJS7UVSNDYZG4@boot.prod.status.nodes.status.im"
nameserver := "1.1.1.1"
Copy link
Contributor

@Ivansete-status Ivansete-status Oct 29, 2024

Choose a reason for hiding this comment

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

As mentioned above, I think we can avoid passing the nameserver and leave that responsability to libwaku.
Something to be considered once the following is completed: waku-org/nwaku#1490

@gabrielmer
Copy link
Author

Closing this PR in favor of #6017

@gabrielmer gabrielmer closed this Oct 31, 2024
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.

4 participants