Skip to content

Conversation

@Frando
Copy link
Member

@Frando Frando commented Nov 11, 2025

Description

Fixes #3642

This moves discovery handling fully into the EndpointStateActor.
The pub(crate) interface to trigger discovery and get a EndpointMappedAddr is now Magicsock::resolve_remote, which sends the provided addresses to the EndpointStateActor. The actor starts discovery if it does not have a selected path and if discovery is not running. It returns either immediately if there are any known paths, or waits for discovery to produce at least one result or an error. Once this returns, resolve_remote returns either with a EndpointMappedAddr or with the discovery error.

This means the current behavior is kept: We only start quinn::Endpoint::connect once we have at least one transport address for the remote. If not, we return the discovery error immediately from iroh::Endpoint::connect.

This opens the door for us to easily tune when to run discovery in other siutations, e.g. when all available paths to a remote are closed. However, for now this PR still only starts discovery when Endpoint::connect is called and no path is selected at the moment.

Breaking Changes

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.
    • List all breaking changes in the above "Breaking Changes" section.
    • Open an issue or PR on any number0 repos that are affected by this breaking change. Give guidance on how the updates should be handled or do the actual updates themselves. The major ones are:

@github-actions
Copy link

github-actions bot commented Nov 11, 2025

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

Last updated: 2025-11-12T11:44:04Z

@n0bot n0bot bot added this to iroh Nov 11, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Nov 11, 2025
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

I guess most tests needs to be moved the the StaticProvider?

I don't quite follow the WantConnect stuff. Is this trying to make sure you can return an error straight from calling Endpoint::connect when you don't have any discovery results? As otherwise you'd start connecting and then the connection would time out?

Comment on lines 302 to 303
// Prune our own addreses from the endpoint address.
// TODO: Move this somewhere else?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please!

The EndpointStateActor should collect these addresses as usual but when choosing addresses to send to when handling SendDatagram it should filter out any that are itself. That would mean the possible remote addresses are not lost when you move network. The EndpointStateActor already has access to the local DirectAddrs so this is way more logical.

@Frando
Copy link
Member Author

Frando commented Nov 11, 2025

I don't quite follow the WantConnect stuff. Is this trying to make sure you can return an error straight from calling Endpoint::connect when you don't have any discovery results?

Yes, exactly. It mimics the current behavior that quinn::Endpoint::connect is only started once we have at least one dialable remote address, and if discovery errors or returns no results, we return straight from iroh::Endpoint::connect without touching quinn at all. The alternative would be that all connections time out if discovery yields nothing.

@Frando Frando force-pushed the Frando/mp-discovery branch from 6189b7f to 196d89a Compare November 11, 2025 16:22
@github-actions
Copy link

github-actions bot commented Nov 12, 2025

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: bdd7c68

@Frando Frando marked this pull request as ready for review November 12, 2025 10:04
@Frando Frando requested a review from flub November 12, 2025 10:04
@flub flub linked an issue Nov 13, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🏗 In progress

Development

Successfully merging this pull request may close these issues.

Improve discovery cooperation with EndpointStateActor

3 participants