fix(swarm): external address candidate only after address translation

[b-zee]

The NewExternalAddrCandidate event is yielded both before and after address translation. This will
cause, in the case of TCP, ephemeral ports to be added as candidate.

In turn, that will cause protocols like AutoNAT to fail as these candidates are not actually
reachable/external.

Whilst technically not wrong, yielding candidates that are for sure invalid is mere noise. Thus, we only yield the candidate if address translation did not yield any new candidates.

Fixes #4153.

[mxinden]

🚀 thank you @b-zee for reporting, debugging and fixing!

Will give @thomaseizinger some time as well in case we are missing something.

In the meantime @b-zee mind adding a changelog entry? See https://github.com/libp2p/rust-libp2p/blob/master/docs/release.md#make-changelog-entries for details. That way I can cut a patch release once this pull request is merged.

[thomaseizinger]

Hmm, this was actually on purpose but may have been a logic error!

First off, it should not be a problem to yield address candidates that are wrong. They are candidates for a reason!

Second, address translation will return None if the transport does not understand the address. Thus despite having listen addresses, this loop is not guaranteed to yield an event!

This will sound niche but consider the following:

• We listen on TCP locally
• We have some kind of overlay proxy network, like Tor
• We emit an external addr candidate for the Tor address

If we merge this PR, that would never get emitted.

One could argue that Tor addresses are always external and so one should not emit a candidate?

Perhaps we should first do address translation and if that does not yield anything for whatever reason, just emit the untouched address?

[thomaseizinger]

First off, it should not be a problem to yield address candidates that are wrong. They are candidates for a reason!

Reading your original issue, I'd say that in general, this isn't a bug. There may be all sorts of reasons why what we thought is an external address does not turn out to be one.

However, it does make sense to optimize our code so that bad guesses are avoided which is what this is doing :)

It does feel like not emitting the untouched address can lead to a different, subtle and more severe bug than emitting it so I am a bit torn.

I guess we can assume that address translation does not produce a garbage address? i.e. does the assumption hold that if we translate it, the translated candidate is always strictly better than the given one?

[b-zee]

First off, it should not be a problem to yield address candidates that are wrong. They are candidates for a reason!

That is true. I have been looking at this from the perspective of AutoNAT. If I remember correctly, AutoNAT v2 will fix the problem that rises from this issue, because one failed dial will not disturb the rest of the results.

Perhaps we should first do address translation and if that does not yield anything for whatever reason, just emit the untouched address?

This sounds reasonable as far as I can tell.

[b-zee]

I guess we can assume that address translation does not produce a garbage address? i.e. does the assumption hold that if we translate it, the translated candidate is always strictly better than the given one?

I think that 'address translation' should happen. If there is nothing to translate or the transport doesn't need to translate, then I expect the same address to come out. This was my assumption going into the PR. So yes, I think that sounds intuitive, the translated address(es) is always better than the input.

[thomaseizinger]

Okay, thanks for thinking it through with me :)

Are you okay to adjust the PR to the proposed solution?

[b-zee]

Made the changes. Let me know if this works for you! @thomaseizinger @mxinden

[thomaseizinger]

Thanks! One more thing to discuss.

[thomaseizinger]

Thank you!