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

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

Open nazar-pc opened this issue 2 years ago • 8 comments

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

  • [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
  • [x] A changelog entry has been made in the appropriate crates

nazar-pc avatar Nov 17 '23 11:11 nazar-pc

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

thomaseizinger avatar Nov 22 '23 03:11 thomaseizinger

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

mergify[bot] avatar Dec 01 '23 23:12 mergify[bot]

@nazar-pc can you resolve the merge conflicts?

mxinden avatar Dec 03 '23 20:12 mxinden

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

mergify[bot] avatar Dec 03 '23 22:12 mergify[bot]

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.

nazar-pc avatar Dec 04 '23 14:12 nazar-pc

@thomaseizinger Stalled PR here.

drHuangMHT avatar Dec 07 '23 00:12 drHuangMHT

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!

thomaseizinger avatar Jan 15 '24 02:01 thomaseizinger