Skip to content

feat(iroh)!: encapsulate the quinn::TransportConfig to enforce certain minimums when used with multipath#3721

Merged
ramfox merged 6 commits intofeat-multipathfrom
ramfox/transport-params
Dec 4, 2025
Merged

feat(iroh)!: encapsulate the quinn::TransportConfig to enforce certain minimums when used with multipath#3721
ramfox merged 6 commits intofeat-multipathfrom
ramfox/transport-params

Conversation

@ramfox
Copy link
Copy Markdown
Member

@ramfox ramfox commented Dec 2, 2025

Description

This PR encapsulates the quinn::TransportConfig in a new struct called QuicTransportConfig. It has all of the same methods as the quinn::TransportConfig, but the follow methods will log warnings if the user given values make iroh + multipath sub-optimal:

  • default_path_keep_alive_interval() - should be at most HEARTBEAT_INTERVALms
  • default_path_max_idle_timeout() - should be at most PATH_MAX_IDLE_TIMEOUTms
  • max_concurrent_multipath_paths() - should be at least MAX_MULTIPATH + 1
  • set_max_remote_nat_traversal_addresses() - should be at least MAX_MULTIPATH

These values are also set properly by default when creating a QuicTransportConfig.

quinn encapsulation/co-location

Created a new mod quic and co-located all the quinn exports there.

This would be the mod where we do any future encapsulation.

Note: the StaticConfig struct is a private struct that still uses quinn::TransportConfig directly.

Second note: the ConnectConfig was Clone, and no longer is. This is due to a weird tension in our encapsulation of quinn::TransportConfig. The quinn APIs typically expect an Arc<quinn::TransportConfig>, however if we store an Arc of the quinn::TransportConfig in QuicTransportConfig, then we can't actually mutate any fields inside it, unless we also put it in a Mutex. The "slickest" way to ensure that we did not need to store an Arc<quinn::TransportConfig>, was to ensure that we didn't have APIs that required cloning or Arc-ing the QuicTransportConfig itself (quinn::TransportConfig is also not clone).

It's possible this is terribly annoying for our users, so if someone else has an ergonomic way of handling this, please let me know.

waiting for PR n0-computer/noq#213 to merge, then this can be merged merged

third note: I adjusted the HEARTBEAT_INTERVAL and PATH_MAX_IDLE_TIMEOUT to be numerical values rather than Durations, so they can be read-able in the documentation.

closes #3635

Breaking Changes

  • iroh
    • changes
      • QuinnTransportConfig renamed to QuicTransportConfig & is now Clone
      • ConnectOptions::transport_config: Option<Arc<quinn::TransportConfig>> -> `ConnectOptions::transport_config: Option
      • ConnectOptions::with_transport_config(mut self, transport_config: Arc<quinn::TransportConfig>) -> `ConnectOptions::with_transport_config(mut self, transport_config: QuicTransportConfig)

Open Questions

@ramfox ramfox changed the base branch from main to feat-multipath December 2, 2025 02:02
@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 2, 2025

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

Last updated: 2025-12-04T15:02:31Z

@n0bot n0bot bot added this to iroh Dec 2, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Dec 2, 2025
@ramfox ramfox force-pushed the ramfox/transport-params branch from b38720a to 9ccd81c Compare December 3, 2025 00:58
@ramfox ramfox marked this pull request as ready for review December 3, 2025 01:20
@ramfox ramfox changed the title WIP feat(iroh)!: encapsulate the quinn::TransportConfig to enforce certain minimums when used with multipath feat(iroh)!: encapsulate the quinn::TransportConfig to enforce certain minimums when used with multipath Dec 3, 2025
@ramfox ramfox requested review from dignifiedquire and flub December 3, 2025 01:44
@ramfox ramfox self-assigned this Dec 3, 2025
@flub
Copy link
Copy Markdown
Contributor

flub commented Dec 3, 2025

Second note: the ConnectConfig was Clone, and no longer is. This is due to a weird tension in our encapsulation of quinn::TransportConfig. The quinn APIs typically expect an Arc<quinn::TransportConfig>, however if we store an Arc of the quinn::TransportConfig in QuicTransportConfig, then we can't actually mutate any fields inside it, unless we also put it in a Mutex. The "slickest" way to ensure that we did not need to store an Arc<quinn::TransportConfig>, was to ensure that we didn't have APIs that required cloning or Arc-ing the QuicTransportConfig itself (quinn::TransportConfig is also not clone).

It's possible this is terribly annoying for our users, so if someone else has an ergonomic way of handling this, please let me know.

I don't really know why quinn:TransportConfig isn't Clone? Everything inside it is Clone. Maybe we even could make all the fields pub and mark it #[non_exhaustive]. It has a Default impl anyway so you can then still construct it. Somehow I wonder if the current layout pre-dates the existence of non_exhaustive?

@ramfox ramfox force-pushed the ramfox/transport-params branch from 9ccd81c to 5e42916 Compare December 3, 2025 22:01
MAX_MULTIPATH_PATHS + 1
);
}
self.0.max_concurrent_multipath_paths(max_concurrent);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should ignore the user's setting if they set it too small. if max_concurrent < MAX_MULTIPATH_PATHS + 1 we print a warning saying we're ignoring their setting and actually set self.0.max_concurrent_multipath_paths(MAX_MULTIPATH_PATHS + 1).

Otherwise it is too easy to get hard-to-debug issues, where we'll only figure things out much too late. The issues we will get from ignoring the user-provided value will be much easier to understand and then we can figure out what the usecases are and how to support them.

Same for the others.

@ramfox ramfox requested a review from flub December 4, 2025 17:15
@ramfox ramfox merged commit cc932ef into feat-multipath Dec 4, 2025
22 of 28 checks passed
@ramfox ramfox deleted the ramfox/transport-params branch December 4, 2025 20:02
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in iroh Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

Improve setting TransportConfig parameters

2 participants