kad: emit `ToSwarm::NewExternalAddrOfPeer`

libp2p / rust-libp2p

The Rust Implementation of the libp2p networking stack.

https://libp2p.io

MIT License

4.62k stars 962 forks source link

kad: emit `ToSwarm::NewExternalAddrOfPeer` #5103

Open thomaseizinger opened 10 months ago

thomaseizinger commented 10 months ago

Description

With https://github.com/libp2p/rust-libp2p/pull/4371 merged, we have the foundation of https://github.com/libp2p/rust-libp2p/issues/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 ?

1010adigupta commented 10 months ago

would like to work on this

1010adigupta commented 10 months ago

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

thomaseizinger commented 10 months ago

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

Make them two PRs please! :)

justcode740 commented 9 months ago

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 commented 9 months ago

You are welcome to work on this too!

blacktoast commented 7 months ago

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 commented 6 months ago

@blacktoast are you still interested in tackling this issue?

PanGan21 commented 3 months ago

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 🙂

guillaumemichel commented 2 months ago

Following up the discussion from the merged PR https://github.com/libp2p/rust-libp2p/pull/5549.

With https://github.com/libp2p/rust-libp2p/pull/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 commented 2 months ago

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 commented 2 months ago

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!