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(transport): Add GlobalOnly Transport implementation #3814

Merged
merged 27 commits into from
May 10, 2023

Conversation

tcoratger
Copy link
Contributor

@tcoratger tcoratger commented Apr 23, 2023

Description

Upstreams the GlobalOnly transport from @mxinden's kademlia-exporter to rust-libp2p.

Source: https://github.com/mxinden/kademlia-exporter/blob/master/src/exporter/client/global_only.rs.

Resolves: #3669.

Notes & open questions

The Rust is_global() function associated to Ipv4Addr and Ipv6Addr used in this implementation is unstable.

The method chosen here is to include the implementation of is_global() directly to the library to secure its implementation while waiting for the Rust issue to be resolved. The is_global() is then constructed to refer to all addresses that are not contained in the following Special-Purpose Address lists:

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • A changelog entry has been made in the appropriate crates

@tcoratger tcoratger changed the title feat(Transpor): Add GlobalOnly Transport implementation blocking dials to private IPs feat(Transport): Add GlobalOnly Transport implementation blocking dials to private IPs Apr 23, 2023
@tcoratger tcoratger changed the title feat(Transport): Add GlobalOnly Transport implementation blocking dials to private IPs feat(transport): Add GlobalOnly Transport implementation blocking dials to private IPs Apr 23, 2023
@tcoratger tcoratger marked this pull request as ready for review April 24, 2023 22:07
@tcoratger tcoratger changed the title feat(transport): Add GlobalOnly Transport implementation blocking dials to private IPs feat(transport): Add GlobalOnly Transport implementation Apr 24, 2023
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.

Thanks! I left some comments.

core/src/transport/global_only.rs Outdated Show resolved Hide resolved
core/src/transport/global_only.rs Outdated Show resolved Hide resolved
core/src/transport/global_only.rs Outdated Show resolved Hide resolved
core/src/transport/global_only.rs Outdated Show resolved Hide resolved
core/src/transport/global_only.rs Outdated Show resolved Hide resolved
core/src/transport/global_only.rs Outdated Show resolved Hide resolved
core/src/transport/global_only.rs Outdated Show resolved Hide resolved
@mergify

This comment was marked as resolved.

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.

Thank you for the work here @tcoratger!

core/src/transport/global_only.rs Show resolved Hide resolved
core/src/transport/global_only.rs Outdated Show resolved Hide resolved
@mergify

This comment was marked as resolved.

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.

Apart from one unresolved conversation, this looks good to me!

core/CHANGELOG.md Outdated Show resolved Hide resolved
core/src/transport/global_only.rs Outdated Show resolved Hide resolved
core/src/transport/global_only.rs Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

Can you address the clippy and doc test failures? I think then we can merge this 🥳

@tcoratger
Copy link
Contributor Author

Can you address the clippy and doc test failures? I think then we can merge this 🥳

@thomaseizinger Done

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.

Thank you!

@mergify mergify bot merged commit 3fde132 into libp2p:master May 10, 2023
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 @tcoratger. Upgraded downstream user mxinden/kademlia-exporter#292.

@tcoratger tcoratger deleted the GlobalOnlyTransport branch May 11, 2023 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GlobalOnly Transport implementation blocking dials to private IPs
3 participants