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 report NewExternalAddrCandidate if already confirmed #5582

Conversation

stormshield-frb
Copy link
Contributor

@stormshield-frb stormshield-frb commented Aug 30, 2024

Description

Currently, NewExternalAddrCandidate events are emitted for every connections. However, we continue to get this event even when autonat has already confirmed that this address is external. So we should not continue to advertise the "candidate" event.

Notes & open questions

We have made the changes in the swarm instead of identify because it does not make it necessary to duplicate the ConfirmedExternalAddr vector in the identify Behaviour. Moreover, if any future Behaviour emit NewExternalAddrCandidate, the same rule will also be applied.

I had to edit the autonat_v2 tests which were always expecting a NewExternalAddrCandidate but the address was already confirmed.

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
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi François, this LGTM! CC @umgefahren wdyt?

@drHuangMHT
Copy link
Contributor

This PR is not marked send-it but ready to merge(branch out-of-date aside), any problem here? @jxs

@drHuangMHT
Copy link
Contributor

drHuangMHT commented Sep 19, 2024

cargo deny failed due to #5596; clippy(beta) failed due to too_long_first_doc_paragraph here:

/// [`Codec`] implements [`Encoder`] and [`Decoder`], uses [`unsigned_varint`]
/// to prefix messages with their length and uses [`quick_protobuf`] and a provided
/// `struct` implementing [`MessageRead`] and [`MessageWrite`] to do the encoding.

@drHuangMHT
Copy link
Contributor

@dariusc93 May you get the checks running again and merge this?

@drHuangMHT
Copy link
Contributor

send-it lable must be applied in order to merge this automatically 🙏

@mergify mergify bot merged commit 8ceadaa into libp2p:master Sep 26, 2024
69 checks passed
sdbondi added a commit to sdbondi/rust-libp2p that referenced this pull request Nov 1, 2024
* master: (36 commits)
  chore: refactor ping tests (libp2p#5655)
  feat: refactor distributed-key-value-store example (libp2p#5652)
  chore(ci): address clippy beta lints (libp2p#5649)
  feat(gossipsub): apply `max_transmit_size` to the published message (libp2p#5642)
  feat(kad): add `Behavior::find_closest_local_peers()` (libp2p#5645)
  fix(swarm-test): set proper version (libp2p#5648)
  deps(ci): update cargo-semver-checks (libp2p#5647)
  chore: fix typo in comment (libp2p#5643)
  feat: make runtime features optional in swarm-test (libp2p#5551)
  deps: bump Swatinem/rust-cache from 2.7.3 to 2.7.5 (libp2p#5633)
  chore: update igd-next to 0.15.1 (libp2p#5625)
  fix(server): removing dependency on libp2p-lookup (libp2p#5610)
  refactor(examples): use tokio instead of async-std in relay-server (libp2p#5600)
  deps: update metrics example dependencies (libp2p#5617)
  chore: update interop test run condition (libp2p#5611)
  chore(autonat-v2): fix dial_back_to_non_libp2p test (libp2p#5621)
  fix(swarm): don't report `NewExternalAddrCandidate` if already confirmed (libp2p#5582)
  chore(ci): address beta clippy lints (libp2p#5606)
  fix(ci): address cargo-deny advisories (libp2p#5596)
  chore(ci): only run interop tests on commits to master (libp2p#5604)
  ...
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.

5 participants