feat(iroh-net)!: add subscribe method to Discovery trait

[ramfox]

Description

Adds a subscribe method to the Discovery trait that returns an Option<BoxStream>. The subscribe method will send DiscoveryItems each time the discovery service discovers a remote node.

The magicsock is now subscribed to the discovery service and updates the internal address book each time it receives a DiscoveryItem. The source is marked as Source::Discovery{ service: String } and the Instant that the magicsock received the information.

Users can now filter their list of RemoteInfos for sources.

Breaking Changes

• struct RemoteInfo now has field sources, which is a Vec of (iroh::net::endpoint::Source, Duration). The Source is how we heard about the remote, and the Duration is how long ago we heard about it. The sources field is ordered from the first time we heard about the remote node to the most recent time we learned about the remote node, in this session.

Change checklist

• Self-review.
• Documentation updates following the style guide, if relevant.
• Tests if relevant.
• All breaking changes documented.

[github-actions]

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2656/docs/iroh/

Last updated: 2024-09-27T17:06:25Z

[divagant-martian]

about the use of channels:

I generally prefer bounded channels, since they provide back pressure, but by spawning a task that does the send every time the back pressure is lost, making an unbounded channel preferable. In other channel, when sending the event back to the subscriber, there isn't this per-send spawn so it does provide back pressure, but I would argue here it's not what we want. Discovery should not be held up by an outsider reader not making room for another event. Also, a slow subscriber should not be able to held up other subscribers. I think this scenario is somewhat being negated by some other spawn somewhere up in the stack, but the point remains: If we do intend to be held a slow reader (this would make sense for example if we schedule something such that we only care about the last result), or if we are able to drop data when full, then a bounded channel is the best choice. If not, then unbounded channels are the way to go.

I another note, I checked out the cli output and was a bit confused to get the source for the node and no the address. I would think the "source of the node" is the source of the best address we use to communicate. Maybe it's just me, I'm open to leave it like this and understand it in another way, but would like to know what others think

[flub]

wrt to the bounded/unbounded channel. I only wanted to point out that spawning a task to send on a bounded channel is not a good pattern and is essentially worse than using an unbounded channel. It does always make sense to think about what backpressure is needed and whether something is possible or better without unbounded channels. @divagant-martian and @matheus23 were much more constructive in that regard :)

[ramfox]

After a discussion with dig last week, he suggested I just make the Discovery trait async, to avoid these gotchas.

[matheus23]

✅ as in "happy from my perspective". But ofc would welcome more ✅s (or comments on addressed feedback) from diva and flub :)

[flub]

lgtm

[divagant-martian]

LGTM 🎉