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

examples/gossipsub-chat: Add mDNS peer discovery #2996

Merged
merged 6 commits into from
Oct 26, 2022
Merged

examples/gossipsub-chat: Add mDNS peer discovery #2996

merged 6 commits into from
Oct 26, 2022

Conversation

andrewdavidmackenzie
Copy link
Contributor

Description

This PR adds the discovery of other peers using mDNS to the gossipsub example, allowing each peer to not have to know the address of the first peer to connect to.

Links to any relevant issues

N/A

Open Questions

N/A

Change checklist

  • [ X ] I have performed a self-review of my own code
  • [ X ] I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Just one comment, otherwise LGTM!

examples/gossipsub-chat.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title Add mDNS peer discovery to the gossipsub-chat example examples/gossipsub-chat: Add mDNS peer discovery Oct 16, 2022
@andrewdavidmackenzie
Copy link
Contributor Author

andrewdavidmackenzie commented Oct 17, 2022

I synced my fork and pulled master and getting errors running cargo test, cargo build seems to be fine.

running 9 tests
test src/tutorials/hole_punching.rs - tutorials::hole_punching (line 165) ... ignored
test src/tutorials/hole_punching.rs - tutorials::hole_punching (line 173) ... ignored
test src/tutorials/hole_punching.rs - tutorials::hole_punching (line 181) ... ignored
test src/tutorials/ping.rs - tutorials::ping (line 176) ... FAILED
test src/tutorials/ping.rs - tutorials::ping (line 106) ... FAILED
test src/tutorials/ping.rs - tutorials::ping (line 272) - compile ... FAILED
test src/tutorials/ping.rs - tutorials::ping (line 228) ... FAILED
test src/tutorials/ping.rs - tutorials::ping (line 144) ... FAILED
test src/tutorials/ping.rs - tutorials::ping (line 72) ... ok

failures:

---- src/tutorials/ping.rs - tutorials::ping (line 176) stdout ----
error[E0432]: unresolved import `libp2p::ping`
 --> src/tutorials/ping.rs:177:32
  |
3 | use libp2p::{identity, PeerId, ping};
  |                                ^^^^ no `ping` in the root

error[E0425]: cannot find function `development_transport` in crate `libp2p`
  --> src/tutorials/ping.rs:187:29
   |
13 |     let transport = libp2p::development_transport(local_key).await?;
   |                             ^^^^^^^^^^^^^^^^^^^^^ not found in `libp2p`

Is master building OK?

Maybe some refactor needs updating of doc tests?
(do you run doc tests in CI?)

@thomaseizinger
Copy link
Contributor

You probably have to run it with --all-features. Unfortunately our CI currently runs on the entire workspace at once so this isn't checked.

@andrewdavidmackenzie
Copy link
Contributor Author

Removing #[behaviour(out_event = "MyBehaviourEvent")] to leave :

    #[derive(NetworkBehaviour)]
    struct MyBehaviour {
        gossipsub: Gossipsub,
        mdns: Mdns,
    }

you get

cargo run --example gossipsub-chat --features="full"
   Compiling libp2p v0.49.0 (/Users/andrew/workspace/rust-libp2p)
error[E0428]: the name `MyBehaviourEvent` is defined multiple times
  --> examples/gossipsub-chat.rs:83:5
   |
77 |     #[derive(NetworkBehaviour)]
   |              ---------------- previous definition of the type `MyBehaviourEvent` here
...
83 |     enum MyBehaviourEvent {
   |     ^^^^^^^^^^^^^^^^^^^^^ `MyBehaviourEvent` redefined here
   |
   = note: `MyBehaviourEvent` must be defined only once in the type namespace of this block

@thomaseizinger
Copy link
Contributor

Removing #[behaviour(out_event = "MyBehaviourEvent")] to leave :

    #[derive(NetworkBehaviour)]
    struct MyBehaviour {
        gossipsub: Gossipsub,
        mdns: Mdns,
    }

you get

cargo run --example gossipsub-chat --features="full"
   Compiling libp2p v0.49.0 (/Users/andrew/workspace/rust-libp2p)
error[E0428]: the name `MyBehaviourEvent` is defined multiple times
  --> examples/gossipsub-chat.rs:83:5
   |
77 |     #[derive(NetworkBehaviour)]
   |              ---------------- previous definition of the type `MyBehaviourEvent` here
...
83 |     enum MyBehaviourEvent {
   |     ^^^^^^^^^^^^^^^^^^^^^ `MyBehaviourEvent` redefined here
   |
   = note: `MyBehaviourEvent` must be defined only once in the type namespace of this block

Yep, it generates the event for you so you can delete the definition below :)

examples/gossipsub-chat.rs Outdated Show resolved Hide resolved
@andrewdavidmackenzie
Copy link
Contributor Author

Accepted suggested comment change.
Removed the struct definition that is not needed.

examples/gossipsub-chat.rs Outdated Show resolved Hide resolved
andrewdavidmackenzie and others added 6 commits October 25, 2022 13:29
- Add mDNS behaviour and combine with the Gossipsub behaviour
- Added a text prompt for user input.
- Modified code and comments slightly to be more similar to the distributed-key-value-store.rs example.
- Formatted code with rustfmt
…ers. Updated doc comment describing the example to match.
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Looks good to me. @thomaseizinger can you merge in case you have no more comments?

@thomaseizinger thomaseizinger merged commit 5bce6ed into libp2p:master Oct 26, 2022
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.

5 participants