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

fix(swarm): don't emit NewExternalAddrCandidate on clients #4886

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nazar-pc
Copy link
Contributor

@nazar-pc nazar-pc commented Nov 17, 2023

Description

Nodes without active listen addresses (aka "clients") still produce external address candidates via protocols such as identify through the observed address. Those are however guaranteed to be useless as not having any listeners means no other node can reach us.

Notes & open questions

There are a few more issues that this PR didn't fix (UDP: I now see that transports take care of this in their translation implementation):

  • address translation is flawed, it doesn't take protocol into consideration, so it is possible to receive observed TCP address from identify and end up with UDP translated address (which in most cases will still be correct, but strictly speaking doesn't have to)
  • this PR doesn't check that listener addresses which are present are in fact of the same protocol, mirroring address translation "behavior"

This PR could be potentially incorrect in case there are protocols where it is possible to have observed address without listener addresses, but I am not aware of such protocols and it would still be possible to work around that with user-level code.

Change checklist

  • 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
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! This makes sense. I've left two comments.

swarm/src/lib.rs Outdated Show resolved Hide resolved
@nazar-pc nazar-pc force-pushed the ignore-ephemeral-ports branch from a0022d4 to 8659c50 Compare November 20, 2023 13:51
swarm/CHANGELOG.md Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title fix(swarm): do not generate NewExternalAddrCandidate events if there are no listeners fix(swarm): do not emit NewExternalAddrCandidate if there are no listeners Nov 20, 2023
@thomaseizinger thomaseizinger changed the title fix(swarm): do not emit NewExternalAddrCandidate if there are no listeners fix(swarm): don't emit NewExternalAddrCandidate if there are no listeners Nov 20, 2023
@thomaseizinger thomaseizinger changed the title fix(swarm): don't emit NewExternalAddrCandidate if there are no listeners fix(swarm): don't emit NewExternalAddrCandidate on clients Nov 20, 2023
thomaseizinger
thomaseizinger previously approved these changes Nov 20, 2023
@thomaseizinger
Copy link
Contributor

@nazar-pc It seems there is a test-failure in libp2p-identify now.

Copy link
Contributor

mergify bot commented Dec 1, 2023

This pull request has merge conflicts. Could you please resolve them @nazar-pc? 🙏

mxinden
mxinden previously approved these changes Dec 3, 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

@mxinden
Copy link
Member

mxinden commented Dec 3, 2023

@nazar-pc can you resolve the merge conflicts?

@mergify mergify bot dismissed stale reviews from mxinden and thomaseizinger December 3, 2023 22:47

Approvals have been dismissed because the PR was updated after the send-it label was applied.

Copy link
Contributor

mergify bot commented Dec 3, 2023

This pull request has merge conflicts. Could you please resolve them @nazar-pc? 🙏

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Dec 4, 2023

only_emits_address_candidate_once_per_connection test failed, but I'm not sure why would it from the code, looks like listen address should be present by then.

@drHuangMHT
Copy link
Contributor

@thomaseizinger Stalled PR here.

@thomaseizinger
Copy link
Contributor

only_emits_address_candidate_once_per_connection test failed, but I'm not sure why would it from the code, looks like listen address should be present by then.

The test is failing because swarm1 doesn't actually listen on any address. That is the exact check this PR implements :)

We will need to make swarm1 listen on something to bypass the if you are adding here!

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.

4 participants