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

kad: emit ToSwarm::NewExternalAddrOfPeer #5103

Closed
thomaseizinger opened this issue Jan 21, 2024 · 15 comments
Closed

kad: emit ToSwarm::NewExternalAddrOfPeer #5103

thomaseizinger opened this issue Jan 21, 2024 · 15 comments
Labels
difficulty:easy help wanted

Comments

@thomaseizinger
Copy link
Contributor

Description

With #4371 merged, we have the foundation of #4302 implemented.

libp2p-kad needs to be extended to emit this new event whenever it discovers a new address through the DHT.

Motivation

Other behaviours should be able to learn the addresses discovered by kademlia.

Current Implementation

Only kademlia knows about its discovered addresses.

Are you planning to do it yourself in a pull request ?

No
@1010adigupta
Copy link

would like to work on this

@1010adigupta
Copy link

would work on mDNS and DHT simultaneously, you can assign me this as well

@thomaseizinger
Copy link
Contributor Author

would work on mDNS and DHT simultaneously, you can assign me this as well

Make them two PRs please! :)

@justcode740
Copy link

unsure whether this one is taken but seem prev one has made such claim and not submit pattern in some other repo couple times..

@thomaseizinger
Copy link
Contributor Author

You are welcome to work on this too!

@thomaseizinger thomaseizinger mentioned this issue Apr 15, 2024
Open
@blacktoast
Copy link

Hi, do you mind if I try to do this?

I know a little bit of Rust, but this is my first time contributing to a p2p related library, so if you don't mind, is there any code or documentation that would help me solve this issue?

@dariusc93
Copy link
Member

@blacktoast are you still interested in tackling this issue?

@PanGan21 PanGan21 mentioned this issue Aug 11, 2024
Merged
4 tasks
@PanGan21
Copy link
Contributor

Hi guys, I saw that this ticket was stale for some time so I gave it a try at #5549 . Let me know what you think 🙂

mergify bot pushed a commit that referenced this issue Aug 27, 2024
@PanGan21 @guillaumemichel
feat(kad): emit ToSwarm::NewExternalAddrOfPeer (#5549)
de8cba9 
## 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: #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]>
@guillaumemichel
Copy link
Contributor

Following up the discussion from the merged PR #5549.

With #5549 a NewExternalAddrOfPeer event is only emitted when a new peer is added to the routing table, and not when a new peer is discovered.

I agree with @Wiezzel that it would be best to emit the event when the address is discovered, so that an event is emitted for all (newly) discovered peers.

Though as we don't have a peerstore, it means that there may be many events emitted for the same node (and same address) every time the node is included a kad response. We could keep a state per query, which would already limit the number of duplicates, and not generate an event for peers that are already in the routing table, but even then these event will be noisy.

@dariusc93
Copy link
Member

I agree with @Wiezzel that it would be best to emit the event when the address is discovered, so that an event is emitted for all (newly) discovered peers.

I kind of agree that we should probably emit on newly discovered external addresses and not just when adding to the routing table (which is the purpose of the event anyway).

@PanGan21
Copy link
Contributor

Agreed. I will check what is the best place to move this, so I can do a fix pr if you agree. Any pointers are welcome!

TimTinkers pushed a commit to unattended-backpack/rust-libp2p that referenced this issue 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]>
@drHuangMHT
Copy link
Contributor

Is this done? #5549 has been merged.

@elenaf9
Copy link
Contributor

elenaf9 commented Dec 7, 2024 •
edited
Loading

Is this done? #5549 has been merged.

👍 yes, thanks.

@elenaf9 elenaf9 closed this as completed Dec 7, 2024
@dariusc93
Copy link
Member

Is this done? #5549 has been merged.

Is it but it looks like discussion was going to continue to determine if we should emit only when adding to the kbucket or upon discovery of any peers regardless of their position in the kbucket. Not sure that been resolved, has it?

@elenaf9
Copy link
Contributor

elenaf9 commented Dec 7, 2024

Is it but it looks like discussion was going to continue to determine if we should emit only when adding to the kbucket or upon discovery of any peers regardless of their position in the kbucket. Not sure that been resolved, has it?

You're right, I should have scrolled down more on that PR 😄. Still, I would consider the current issue to be resolved, kademlia does emit a NewExternalAddrOfPeer event when discovering new addresses. If the current implementation of it should be changed / adapted to a specific use-case, a new issue should be opened IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
difficulty:easy help wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@dariusc93 @thomaseizinger @blacktoast @guillaumemichel @PanGan21 @elenaf9 @drHuangMHT @1010adigupta @justcode740