rust-libp2p icon indicating copy to clipboard operation
rust-libp2p copied to clipboard

fix(swarm): Showcase impossibility of dialing self by ip.

Open vnermolaev opened this issue 1 year ago • 4 comments

Description

This PR addresses issue #4460.

However, the issue described in #4460 is not actually an issue, and, as the update test showcases, errors were already raised when swarm tried to dial self. The updated test adds ::1 and 127.0.0.1 version of self.

Change checklist

I do not believe there must be any changelog items as the code did not change at all.

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

vnermolaev avatar Jan 25 '24 11:01 vnermolaev

This pull request has merge conflicts. Could you please resolve them @vnermolaev? 🙏

mergify[bot] avatar Jan 25 '24 11:01 mergify[bot]

I am getting a failure with Cargo.lock; however, my changes do not concern it.

vnermolaev avatar Jan 26 '24 10:01 vnermolaev

What is the actual error that is returned here? I think to correctly fix #4460, we should catch this case explicitly and not return on the kernel to fail dialing 0.0.0.0.

thomaseizinger avatar Jan 30 '24 02:01 thomaseizinger

The error returned is

SwarmEvent::OutgoingConnectionError {
   peer_id,
   // multiaddr,
   error: DialError::Transport(errors),
    ..
}

It is already the case that libp2p fails on dialling 0.0.0.0 as shown by the original test. However, this test did not list ::1 as an erroneous case, which already is. I just added an ipv6 address to showcase that.

#4460 requests to disallow dialling self, which is already the case. As I see it, there are two options:

  • keep things intact because libp2p won't dial self. This requires the small changes I introduced,
  • filter the localhost versions from the list of addresses to dial with a warning, then ofc no dial error will be returned.

I believe the former is better because it already works and produces expected errors. In the latter case, imagine the list of addresses consists of only the localhost versions, which will be filtered out, then one shall receive an error stating that no addresses were present. Such an error is confusing and requires a log investigation.

vnermolaev avatar Feb 01 '24 09:02 vnermolaev