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

Multiaddr duplicated p2p protocol in relay reservation (#2696) #2701

Merged

Conversation

stormshield-pj50
Copy link
Contributor

Description

In protocols/relay/src/v2/relay.rs do not append P2p protocol if already present.

Now the built multiaddr does not contain a dupped P2p protocol. Logs are OK:

[2022-06-07T08:38:16Z INFO  client] Relay accepted our reservation request.
[2022-06-07T08:38:16Z INFO  client] Listening on "/ip4/$RELAY_SERVER_IP/tcp/4001/p2p/12D3KooWDpJ7As7BWAwRMfu1VU2WCqNjvq387JEYKDBj4kx6nXTN/p2p-circuit/p2p/12D3KooWPjceQrSwdWXPyLLeABRXmuqt69Rg3sBYbU1Nft9HyQ6X"

Open Questions

Change checklist

  • [ X] I have performed a self-review of my own code
  • 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
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.

Thanks for tackling this.

Small suggestions. What do you think of the diff below?

diff --git a/protocols/relay/src/v2/relay.rs b/protocols/relay/src/v2/relay.rs
index 8d8bb75c..ed5fe6ca 100644
--- a/protocols/relay/src/v2/relay.rs
+++ b/protocols/relay/src/v2/relay.rs
@@ -756,9 +756,13 @@ impl Action {
                     inbound_reservation_req,
                     addrs: poll_parameters
                         .external_addresses()
-                        .map(|a| {
-                            a.addr
-                                .with(Protocol::P2p((*poll_parameters.local_peer_id()).into()))
+                        .map(|a| a.addr)
+                        // Add local peer ID in case it isn't present yet.
+                        .filter_map(|a| match a.iter().last()? {
+                            Protocol::P2p(_) => Some(a),
+                            _ => Some(
+                                a.with(Protocol::P2p(*poll_parameters.local_peer_id().as_ref())),
+                            ),
                         })
                         .collect(),
                 }),

Also, would you mind including a changelog entry and a libp2p-relay patch version bump?

protocols/relay/src/v2/relay.rs Outdated Show resolved Hide resolved
- Minor fixes after review
- Bump libp2p-relay Cargo.toml version to 0.9.2
- Add entry in Changelog for upcoming 0.9.2 version
@stormshield-pj50 stormshield-pj50 force-pushed the issue_2696_multiaddr_dup_p2p_protocol branch from a67aaf1 to b3f3b59 Compare June 15, 2022 07:58
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.

Sorry for the delay here. Thanks @Pj50.

I am planning to cut a new release soonish. In case you need this earlier, let me know.

@mxinden mxinden merged commit f814b21 into libp2p:master Jun 27, 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.

2 participants