Per-connection transport configuration - #8
Merged
Merged
Conversation
Inert values only — nothing encodes them yet, so the bind payload is unchanged and both ends still agree. Every field is nullable and null means "I did not say", which leaves the six defaults iroh applies in a private constructor that every reachable config starts from.
Give MtuDiscovery's and AckFrequency's seven sub-fields their own KDoc, each drawn from the setter noq-proto documents for it (the previous claim that no such setter exists was wrong). Name congestionController's null default as CongestionController.Cubic, per noq-proto's TransportConfig::default. Correct maxConcurrentMultipathPaths's stated threshold: iroh's guard rejects anything below 9 (MAX_MULTIPATH_PATHS+1, where MAX_MULTIPATH_PATHS is 8), not below 8 as previously documented, and note that upstream's own doc comment on that setter claims 13 instead, which does not match the code it documents. Apply the same threshold fix to the plan's worked KDoc example so it stops feeding the wrong number to anyone who reads it later.
Both ends in one commit: the bind payload is positional at its outer level, so a drift between encoder and decoder compiles cleanly and fails at runtime. Sparse and tagged rather than positional, because a caller sets two knobs out of twenty-nine. Its own tag family, deliberately not addr.rs's or Discovery.kt's, and an unknown tag is an error rather than a skipped entry. One deliberate departure from the plan: maxIdleTimeout crosses as milliseconds, not nanoseconds like every other duration here. It is the only field that becomes a VarInt (noq_proto::IdleTimeout, 62 bits of milliseconds) rather than a plain Duration, and a nanosecond count can never be large enough, once divided back down to milliseconds, to exceed that varint's ~4.6e18 range -- i64::MAX nanoseconds is only ~1e13 milliseconds. Encoding it as nanoseconds would have made the "too large for the wire" case this module has to guard against unreachable from Kotlin; milliseconds is also upstream's own unit, so nothing is lost converting through it. The refusal test's duration literal was adjusted to one that actually clears the (very large) threshold under this encoding.
…iable overflow test maxIdleTimeout was the one Duration field encoded as milliseconds instead of nanoseconds, kept that way only to make an overflow test satisfiable. Restore the uniform i64-nanoseconds wire format on both sides; Rust hands the decoded Duration straight to IdleTimeout::try_from, which does its own nanoseconds-to-milliseconds conversion and varint bound check. The overflow test could never actually fail: an i64 nanosecond count tops out around 292 years, about six orders of magnitude below the varint's 62-bit millisecond bound, so no Duration a caller can construct can trip it. Replace it with a body that pins two reachable behaviours instead: Duration.INFINITE (the largest value a caller can express) binds successfully, and a negative duration is refused with InvalidArgument. The comment says plainly that the Rust range check is defence in depth, not a reachable path, so nobody reintroduces the unsatisfiable version. Also updates the Task 2 plan doc, which specified nanoseconds throughout from the start — the milliseconds exception was a deviation introduced during implementation, not the original design.
- Cargo.toml: declare noq-proto with default-features = false, matching how iroh itself depends on it and how iroh-relay/iroh-services are already declared here. Without this, Cargo's feature unification turned noq-proto's default `bloom` feature on for iroh's own copy of the crate too, pulling fastbloom/libm/siphasher into every artifact. Rebuilding drops all three from Cargo.lock. - TransportConfig.kt: fix the tag-family cross-reference comment, which named a nonexistent `Addr.kt` `ADDR_TAG_*` — the real constants there are `TAG_*`; `ADDR_TAG_*` is the private copy several other files keep. - transport.rs: give `timeThreshold` a `read_time_threshold` helper like every sibling numeric field, rejecting NaN and negative values instead of letting them reach upstream unvalidated — a NaN there silently disables time-based loss detection with no error anywhere. - TransportConfig.kt: extract the repeated counted-sequence preamble (count, tagged entries, splice) into one `writeCountedEntries` helper used by all three writers, documenting why the inner `BinaryWriter` receiver shadowing inside `entry`'s `write` lambda is what makes the scheme correct. - CommonEndpointTests.kt: narrow the "largest duration is accepted" comment to what the test actually shows for `maxIdleTimeout` — the one duration field with a real upstream bound check — instead of implying it for all nine duration fields; the other seven only get a sign check.
startConnect gains a defaulted third parameter, source- and binary-compatible. connect() deliberately does not: upstream offers ConnectOptions on connect_with_opts alone, which is what startConnect maps onto. The test caught the design being wrong about its own observable. The plan asserted that a client refusing datagrams leaves the server's maxDatagramSize reporting null; it reports 0. The difference is upstream's and worth keeping: null means the peer's transport does not carry datagrams at all, 0 means it does and the peer will accept none of them. Both the test and the KDoc that made the claim now say the true thing. 222 / 222 / 228 green, clippy and fmt clean.
accept_with needs a ServerConfig, which is built from the endpoint, so Incoming now carries the Endpoint it came from and the export takes both handles. The ALPN list is an explicit parameter because ServerConfigBuilder has no ALPN setter — the list is fixed when the builder is created, and one that omits the negotiated protocol accepts a connection that agrees on nothing. Inferring it would mean guessing, and guessing here fails silently.
The suite covers the value types, the encoding, a bind, a connect and an accept with a configuration set, and the one setting whose effect the other end can observe directly. It does not show that any of the other twenty-eight values changed anything on the wire — that needs traffic analysis the suite does not do. Said so rather than letting a green suite imply otherwise. STATUS.md's 0-RTT-and-transport-config entry splits, because one half now exists. Test-body counts (223 shared per facade, 446 across the two tested facades, 229 on Android, 675 host tests) recounted against the JUnit XML after a fresh run and updated everywhere they appeared.
Closes the coverage gap STATUS.md and README.md overclaimed: only 8 of the 29 top-level TransportConfig tags (plus one nested field each) were ever written by a test, so a wrong tag constant for any of the other 21 would compile cleanly and pass silently. Adds one shared body, `every transport configuration tag round-trips through the codec`, that sets all 29 top-level fields and every field of both nested records at once and binds an endpoint with it — a successful bind is only possible if every tag agrees between TransportConfig.kt and transport.rs. Delegated to all three runners; test counts in README.md, AGENTS.md and STATUS.md updated (217->218 delegated bodies, 223->224 shared per facade, 675->678 host tests), and the STATUS.md/README.md prose now says what is actually shown: every tag round-trips, one knob's effect is observable at the far end, the rest are not. Also: rewrites start_connect's doc comment, which still claimed no transport configuration was wired up; documents InvalidArgument and the "replaces rather than merges" behaviour on Incoming.acceptWith, and adds it to Incoming's class KDoc, accept's consumes-list and close's note; adds InvalidArgument and Discovery to Endpoint.bind's error-code list; fixes endpoint.rs's read_mdns/read_discovery claiming to be last on the wire, now that the transport record follows them; extends connection.rs's module-header codec block with the two new Kotlin->Rust payloads; and rewrites five comments that pointed at a process-scratch task/document not in this repository to instead describe the ordering or thing itself.
- Add Bbr3 as a third CongestionController (appended, ordinal 2), matched in transport.rs; correct the false "no BBR" claim and document upstream's own "Experimental! Use at your own risk" characterisation. - Add TransportConfig.overriddenBy(), the explicit merge none of the three attach points do on their own, with nested records (MtuDiscovery, AckFrequency) replaced wholesale rather than field-merged. - Empirically establish acceptWith's real ALPN failure modes: a non-overlapping list fails loudly from acceptWith itself (Code.Accept), not silently and not from Accepting.connect(); an empty list fails the same way on every call rather than disabling the check, so it is now refused up front with InvalidArgument before the incoming connection is touched. Correct the "actually negotiated" phrasing to reflect that nothing has been negotiated yet at the Incoming stage. - Document that acceptWith's null transportConfig uses the endpoint's default, matching startConnect's documented behaviour. - Document the hard upper bound (256, InvalidArgument) on maxRemoteNatTraversalAddresses alongside its existing soft lower bound. - Note in AGENTS.md to re-read the four hardcoded guard thresholds in upstream's endpoint/quic.rs on every iroh version bump. Test counts: 227 shared bodies per facade (was 224), 454 across jvmTest and macosArm64Test, 233 on Android, 687 host tests in all — recounted and cross-checked against the JUnit XML, and updated in README.md, AGENTS.md and STATUS.md.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Exposes iroh's QUIC transport configuration at three attach points:
EndpointConfig.transportConfig(the endpoint's default), atransportConfigparameter onEndpoint.startConnect(one outgoing connection), andIncoming.acceptWith(alpns, transportConfig)(one incoming one).All twenty-nine knobs cross as a sparse tagged payload rather than a positional record — a caller typically sets two of twenty-nine, and a knob added later is a new tag rather than a change of layout.
TransportConfig.ktencodes,transport.rsdecodes and replays ontoQuicTransportConfig::builder(). No new handle, no new async operation, no new error code.0-RTT is deliberately not here. Upstream models a 0-RTT connection as a distinct type with a smaller method surface, which collides with the
TypeId-tagged handles,stream.rs's typing and the path watchers. It needs its own design.Decisions worth finding in the code rather than the diff
nullmeans "I did not say", never "off". That matters more here than usual: iroh applies six hole-punching and multipath overrides in a private constructor that every reachable config starts from, so anullkeeps iroh's choice, not the QUIC library's.TransportConfig's class KDoc lists all six.A per-connection config replaces the endpoint's outright. Upstream's behaviour, and the opposite of what most people expect, so all three attach points say so — and
TransportConfig.overriddenByexists to make the merge the API deliberately does not do explicit at the call site.Four knobs are silently ignored by upstream when out of range, and this mirrors that silence rather than rejecting. Byte-for-byte parity with the same call from Rust, paid for in documentation and pinned by a test so nobody "fixes" it later. Rejecting would also be forward-hostile: a relaxed guard upstream would meet a binding that still refuses.
The congestion controller crosses as an ordinal despite being a trait object upstream — the choice of algorithm is data even when the algorithm is not. All three of
noq's are exposed,Cubic,NewRenoandBbr3.Three things measurement contradicted documentation
Each would have shipped had the claims not been tested.
maxDatagramSize()on a peer that refused datagrams reports 0, not null. The distinction is upstream's: null means the peer's transport does not carry datagrams at all.The planned overflow test was unsatisfiable: no Kotlin
Durationcan exceed the varint bound in nanoseconds — the gap is six orders of magnitude. Replaced with what is reachable, and the Rust check documented as defence in depth.acceptWithwas documented as silently accepting a connection when the ALPN list omits the negotiated protocol. It does not: it fails loudly fromacceptWith()itself, becauseaccept_withnegotiates synchronously off the received ClientHello. AndemptyList()behaves identically rather than disabling the check — ALPN overlap is a set intersection. The empty list is now refused eagerly, before theIncomingis consumed, because it can never succeed.Review found three defects tests could not
noq-prototook default features while iroh declaresdefault-features = false; Cargo unified them and turnedbloomon for iroh itself, adding three crates to every artifact including Android and iOS.time_thresholdreached upstream unvalidated — a NaN does not panic, it makes every time-threshold comparison false and silently disables time-based loss detection.Only 8 of 29 tags were exercised by any test, while
STATUS.mdclaimed 28. Closed by a body that sets every field rather than by softening the sentence; it passed first time, so the contract was right and only the evidence was missing.Verification
cargo fmt --checkandclippy -D warningsclean. 227 / 227 / 233, zero failures.STATUS.mdstates plainly that twenty-eight of the twenty-nine knobs are shown only to round-trip, and thatdatagramReceiveBufferSizeis the one whose effect the far end observes.