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

feat(wakunode2): enable libp2p rendezvous protocol by default #1770

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

vpavlin
Copy link
Member

@vpavlin vpavlin commented May 31, 2023

Description

Rendezvous Discovery protocol allows peers to advertise their addresses at a rendezvous point and other peers to query the rendezvous point to discover new peers.

This will be particularly useful for peers using circuit relay as they can then easily advertise their circuit relay multiaddress and be discovered by other network participants.

This PR uses libp2p Rendezvous implementation and mounts the protocol by default when relay protocol is enabled.

Changes

  • mount Rendezvous protocol when relay is mounted
  • add simple test of libp2p rendezvous protocol to waku_switch test suite
  • add a check to wakunode2 app tests that rendezvous is enabled along with relay

Issue

closes #1605

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! Just a little comment


await allFutures(wakuSwitch.start(), sourceSwitch.start(), destSwitch.start())

# Connect clients to the rendezvous pint
Copy link
Collaborator

Choose a reason for hiding this comment

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

small typo

Suggested change
# Connect clients to the rendezvous pint
# Connect clients to the rendezvous point

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM! All my comments below are nitpicks (except perhaps moving the test to a different suite), so feel free to address/ignore as you see fit.

@@ -100,3 +111,37 @@ suite "Waku Switch":

## Teardown
await allFutures(wakuSwitch.stop(), sourceSwitch.stop(), destSwitch.stop())

asyncTest "Waku Switch uses Rendezvous":
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't include this in the switch tests. Since this is a separate protocol I suggest creating a new test_waku_rendezvous suite (even if it contains only the one test for now).

@@ -9,7 +9,7 @@ import
chronos, chronicles,
eth/keys,
libp2p/crypto/crypto,
libp2p/protocols/pubsub/gossipsub,
libp2p/protocols/[pubsub/gossipsub, rendezvous],
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally have imports fully qualified and in separate lines (collective imports like this are difficult to "find and replace" at later stage). See coding guidelines

@@ -21,6 +21,7 @@ import
libp2p/protocols/connectivity/autonat/service,
libp2p/nameresolving/nameresolver,
libp2p/builders,
libp2p/protocols/rendezvous,
Copy link
Contributor

Choose a reason for hiding this comment

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

Very much a nitpick: while builders above clearly doesn't follow this practice, we tend to sort imports more or less alphabetically (with the exception of common imports (std chronos metrics etc.) which are usually top-level). Not a strict rule, but would be great for this import to fit in with the other libp2p/protocols above :)

@jm-clius
Copy link
Contributor

Will be good to do interop test with go-waku next to see if it works. Does the underlying implementation contain any metrics which we can add to our monitoring dashboards?

@vpavlin
Copy link
Member Author

vpavlin commented Jun 1, 2023

Fixed all the issues in comments.

Will reach out to go-waku for the interop test

Sadly, no metrics are exposed in the libp2p implementation, might be worth openning a PR on nim-libp2p to add some?

@richard-ramos
Copy link
Member

Can confirm that go-waku can use nwaku as a rendezvous point in this PR! 🚀

@vpavlin vpavlin merged commit 835a409 into master Jun 1, 2023
13 checks passed
@vpavlin vpavlin deleted the feat/rendezvous branch June 1, 2023 19:43
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.

feat: integrate libp2p rendezvous discovery
5 participants