Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow customizing the quinn::TransportConfig in iroh::node::Builder #2592

Open
flub opened this issue Aug 5, 2024 · 6 comments
Open

Allow customizing the quinn::TransportConfig in iroh::node::Builder #2592

flub opened this issue Aug 5, 2024 · 6 comments
Assignees
Labels
bug Something isn't working c-iroh
Milestone

Comments

@flub
Copy link
Contributor

flub commented Aug 5, 2024

The iroh::node::Builder sets the max_concurrent_uni_streams to 0 (i.e. none) and max_concurrent_bidi_streams to 10 (i.e. barely any).

Furthermore it does not allow customising these values.

  • It should not set default values other than the Quinn defaults probably.
  • It should allow for a custom TransportConfig to be used.
@flub flub added bug Something isn't working c-iroh labels Aug 5, 2024
@flub flub changed the title iroh::node::Builder sets max_concurrent_uni_streams to none, does not allow customising TransportConfig iroh::node::Builder sets max_concurrent_uni_streams to none Aug 5, 2024
@dignifiedquire dignifiedquire added this to the v0.23.0 milestone Aug 5, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 5, 2024
## Description

This removes the max streams in the builder.  This makes little sense
when users are allowed to create their custom protocol.

right now both uni and bidirectional streams default to 100 max, which
is reasonable for now.  We should allow fully customising the
TransportConfig later.

## Breaking Changes

I don't think this counts as a breaking change.  If we want to lower
one of these later it would be, but we're probably fine with 100 by
default?

## Notes & open questions

See #2592

## Change checklist

- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [ ] Tests if relevant.
- [x] All breaking changes documented.
@matheus23
Copy link
Contributor

#2593 fixes the first point, but does not yet allow for a custom TransportConfig. Editing the title.

@matheus23 matheus23 changed the title iroh::node::Builder sets max_concurrent_uni_streams to none Allow customizing the quinn::TransportConfig in iroh::node::Builder Aug 6, 2024
@dignifiedquire dignifiedquire removed this from the v0.23.0 milestone Aug 14, 2024
@dignifiedquire dignifiedquire added this to the v0.24.0 milestone Aug 21, 2024
@divagant-martian divagant-martian self-assigned this Aug 23, 2024
@divagant-martian
Copy link
Contributor

We have

/// Sets a custom [`quinn::TransportConfig`] for this endpoint.
///
/// The transport config contains parameters governing the QUIC state machine.
///
/// If unset, the default config is used. Default values should be suitable for most
/// internet applications. Applications protocols which forbid remotely-initiated
/// streams should set `max_concurrent_bidi_streams` and `max_concurrent_uni_streams` to
/// zero.
pub fn transport_config(mut self, transport_config: quinn::TransportConfig) -> Self {
self.transport_config = Some(transport_config);
self
}
which has been around for awhile. You updated it's docs in #2334 and I thought it was added in that PR due to re-ordering but it was added in #1133, more than a year ago.

Is there something else missing?

@matheus23
Copy link
Contributor

Oh, I missed that. In that case we can probably close this, right?

@flub
Copy link
Contributor Author

flub commented Aug 26, 2024

We have

/// Sets a custom [`quinn::TransportConfig`] for this endpoint.
///
/// The transport config contains parameters governing the QUIC state machine.
///
/// If unset, the default config is used. Default values should be suitable for most
/// internet applications. Applications protocols which forbid remotely-initiated
/// streams should set `max_concurrent_bidi_streams` and `max_concurrent_uni_streams` to
/// zero.
pub fn transport_config(mut self, transport_config: quinn::TransportConfig) -> Self {
self.transport_config = Some(transport_config);
self
}

This is in iroh-net. The issue is about the iroh::node::Builder which I think is distinct from using iroh-net on it's own.

I'm not really sure about what should be in the iroh::node::Node, but I gather it's a larger system that build on iroh-net but still allows you to drop down to iroh-net and write your own additional protocols.

@divagant-martian
Copy link
Contributor

that makes sense, thanks!

@ramfox ramfox modified the milestones: v0.24.0, v0.25.0 Sep 2, 2024
@dignifiedquire dignifiedquire modified the milestones: v0.25.0, v0.26.0 Sep 16, 2024
@Frando
Copy link
Member

Frando commented Sep 16, 2024

We could also allow passing an iroh_net::Endpoint to iroh::node::Builder I think. We have some different defaults though atm (iroh sets discovery whereas iroh_net does not)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working c-iroh
Projects
Status: No status
Development

No branches or pull requests

6 participants