feat(mdns): emit ToSwarm::NewExternalAddrOfPeer

[wlynxg]

Description

implement #5104

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

[jxs]

Hi, and thanks for this! Overal looks good too me, just left one comment.

[jxs]

why do you prefer having this here instead of after returning discovered addresses?
I feel it's more correct to do this after returning the addresses and maintaining the previous flow of the pool loop.

[wlynxg]

Thanks for your comments.

However, my idea here is that Event::Discovered and ToSwarm::NewExternalAddrOfPeer events are related.

After Event::Discovered is returned in

rust-libp2p/protocols/mdns/src/behaviour.rs

Line 354 in 9a45db3

return Poll::Ready(ToSwarm::GenerateEvent(event));

,
poll will return.
When poll is entered next time, the NewExternalAddrOfPeer event will be returned because there is a NewExternalAddrOfPeer event in the queue.

The result is similar to this:

receive mdns::Event::Discovered Event
mDNS discovered a new peer: 12D3KooWGkhG2wR1mtadqShqiVEBjLHo8xk48ULrbkdftMsBbttR
NewExternalAddrOfPeer: /ip4/192.168.175.1/udp/63808/quic-v1/p2p/12D3KooWGkhG2wR1mtadqShqiVEBjLHo8xk48ULrbkdftMsBbttR, 12D3KooWGkhG2wR1mtadqShqiVEBjLHo8xk48ULrbkdftMsBbttR

[jxs]

However, my idea here is that Event::Discovered and ToSwarm::NewExternalAddrOfPeer events are related.

yeah exactly, that's why if we discover multiple addresses we shouldn't return all the NewExternalAddrOfPeer events before the second and subsequent Event::Discovered(discovered) right?

[wlynxg]

Thank you for your detailed explanation.
I have placed the return time of the NewExternalAddrOfPeer event after the Event::Discovered event.
Please check whether it meets your expectations.

[jxs]

This is interesting, I just found what may be a bug, even if we discover multiple nodes and insert them in discovered we only return the first one added as discovered is a scope variable, right? So what I suggest is dropping discovered and just push everything to pending events, that way we'll have the ToSwarm::GenerateEvent(Event::Discovered(event)) ToSwarm::NewExternalAddrOfPeer events returned sequentially wdyt?

[wlynxg]

This is interesting, I just found what may be a bug, even if we discover multiple nodes and insert them in discovered we only return the first one added as discovered is a scope variable, right? So what I suggest is dropping discovered and just push everything to pending events, that way we'll have the ToSwarm::GenerateEvent(Event::Discovered(event)) ToSwarm::NewExternalAddrOfPeer events returned sequentially wdyt?

This is indeed a very good suggestion.