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(identify): only report observed address once per connection #4721

Merged
merged 21 commits into from
Oct 31, 2023

Conversation

thomaseizinger
Copy link
Contributor

Description

At the moment, libp2p-identify reports an observed address repeatedly as a new external address candidate even if it is the same address from the same connection. Unless the underlying transport supports roaming, a connection does not change its observed address.

We change the behaviour of libp2p-identify to remember the observed address for a particular connection and not re-emit the NewExternalAddrCandidate event for it. This allows users to probabilistically promote a candidate to an external address based on its report frequency. If an address is reported twice, it means we have two connections where the remote observed this address. Chances are, we have port-reuse enabled for this connection and it might thus be dialable or at least a good candidate for hole-punching.

Related: #4688.

Notes & open questions

cc @altonen

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

@mergify

This comment was marked as resolved.

@mergify

This comment was marked as resolved.

@thomaseizinger thomaseizinger added this to the v0.53.0 milestone Oct 27, 2023
@thomaseizinger thomaseizinger added the internal-change Pull requests that make internal changes to crates and thus don't need to include a changelog entry. label Oct 27, 2023
@thomaseizinger
Copy link
Contributor Author

Applying internal-change because we just adjust the tests.

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.

Good to go from my end. Thanks.

misc/metrics/CHANGELOG.md Show resolved Hide resolved
protocols/identify/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/identify/src/behaviour.rs Show resolved Hide resolved
protocols/identify/tests/smoke.rs Show resolved Hide resolved
@thomaseizinger thomaseizinger added send-it and removed internal-change Pull requests that make internal changes to crates and thus don't need to include a changelog entry. labels Oct 30, 2023
@thomaseizinger
Copy link
Contributor Author

@mxinden I updated some docs as well.

@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.

Docs read well to me.

@mxinden
Copy link
Member

mxinden commented Oct 31, 2023

Retriggered CI with "spurious network failure".

@mergify mergify bot merged commit 57c2100 into master Oct 31, 2023
71 checks passed
@mergify mergify bot deleted the fix/identify-no-repeated-reporting branch October 31, 2023 13:19
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.

2 participants