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.

Sign up for GitHub

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

Jump to bottom

feat(kad): emit ToSwarm::NewExternalAddrOfPeer #5549

Merged
merged 11 commits into from
Aug 27, 2024
Merged

feat(kad): emit ToSwarm::NewExternalAddrOfPeer #5549

merged 11 commits into from
Aug 27, 2024

Conversation

PanGan21
Copy link
Contributor

@PanGan21 PanGan21 commented Aug 11, 2024 •
edited
Loading

Description

Updates libp2p-kad to emit new event ToSwarm::NewExternalAddrOfPeer whenever it discovers a new address through the DHT.
Related: #5103

Notes & open questions

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

@PanGan21 PanGan21 mentioned this pull request Aug 11, 2024
Open
guillaumemichel
guillaumemichel approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@guillaumemichel guillaumemichel left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @PanGan21 !

@thomaseizinger thomaseizinger requested review from jxs and removed request for thomaseizinger August 15, 2024 01:20
@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 15, 2024

This pull request has merge conflicts. Could you please resolve them @PanGan21? 🙏

@mergify mergify bot merged commit de8cba9 into libp2p:master Aug 27, 2024
72 checks passed
@Wiezzel
Copy link
Contributor

Wiezzel commented Aug 27, 2024

@PanGan21 Will this emit NewExternalAddrOfPeer even when the new peer address is not added to the routing table?

@PanGan21
Copy link
Contributor Author

NewExternalAddrOfPeer

Hi @Wiezzel , I believe it will not. In which cases would this be required?

@Wiezzel
Copy link
Contributor

Wiezzel commented Aug 28, 2024

Hi @Wiezzel , I believe it will not. In which cases would this be required?

Let's say you would like to discover an addr of a peer (e.g. to establish a connection). So, you start a getClosestPeers query. As the peer is found, it turns out that the appropriate bucket is full, so it will not be added. In such case, despite having a successful query, and actually discovering the peer's address, we won't receive the event. This seems like a quite common and reasonable case to me.

@guillaumemichel
Copy link
Contributor

Let's continue the discussion at #5103

TimTinkers pushed a commit to unattended-backpack/rust-libp2p that referenced this pull request Sep 14, 2024
@PanGan21 @guillaumemichel @TimTinkers
feat(kad): emit ToSwarm::NewExternalAddrOfPeer (libp2p#5549)
50af7fe 
## Description

<!--
Please write a summary of your changes and why you made them.
This section will appear as the commit message after merging.
Please craft it accordingly.
For a quick primer on good commit messages, check out this blog post:
https://cbea.ms/git-commit/

Please include any relevant issues in here, for example:

Related https://github.com/libp2p/rust-libp2p/issues/ABCD.
Fixes https://github.com/libp2p/rust-libp2p/issues/XYZ.
-->
Updates `libp2p-kad` to emit new event `ToSwarm::NewExternalAddrOfPeer`
whenever it discovers a new address through the DHT.
Related: libp2p#5103 

## Notes & open questions

<!--
Any notes, remarks or open questions you have to make about the PR which
don't need to go into the final commit message.
-->

## Change checklist

<!-- Please add a Changelog entry in the appropriate crates and bump the
crate versions if needed. See
<https://github.com/libp2p/rust-libp2p/blob/master/docs/release.md#development-between-releases>-->

- [X] I have performed a self-review of my own code
- [ ] I have made corresponding changes to the documentation
- [X] 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

---------

Co-authored-by: Guillaume Michel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dariusc93 dariusc93 dariusc93 left review comments

@guillaumemichel guillaumemichel guillaumemichel approved these changes

@jxs jxs Awaiting requested review from jxs

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PanGan21 @Wiezzel @guillaumemichel @dariusc93