## Description
Now depends on #590 merging first so the added test coverage actually
passes.
This replaces three different proptest functions (`random_interaction`,
`random_interaction_with_multipath_simple_routing` and
`random_interaction_with_multipath_complex_routing`) with a single
`random_interaction` function with a `PairSetup` proptest parameter that
effectively generates the different proptest cases these functions
generate (and more!).
This also:
- fixes a bug where we were never generating a transport config that
would allow testing any QNT operations ever
- fixes a bug in the `Arbitrary` impl for `ArrayRangeSet` that required
filtering out empty `ArrayRangeSet`s after the fact instead of
generating them without them possibly being empty in the first place
- improves the cargo-make scripts to enable optimizations for
longer-running proptests, reducing runtime together with compliation
time overall
## Change checklist
<!-- Remove any that are not relevant. -->
- [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.
## Description
This fixes some bugs found via #585.
When a connection doesn't support multipath, or it receives a malicious
or misconstructed PATH_ACK frame that has e.g. a `path_id` of `1`, which
the current connection may not actually have any state for, this would
previously cause us to panic.
With this change we exit out with a PROTOCOL_VIOLATION connection close
instead.
## Notes & open questions
I don't have a regression test for this yet. It is generated in #585 and
uses the test setup from #589, which both aren't merged yet.
I'll make sure to re-generate the regression test and add it in #585
once that's ready.
## Change checklist
<!-- Remove any that are not relevant. -->
- [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.
## Description
This fixes a bug uncovered in #589
When we initiate a NAT traversal round with an address we already have a
path for, we re-use that path, but re-trigger sending an on-path
challenge for that path. Once we eventually send the PATH_CHALLENGE, we
arm a `PathChallengeLost` timer to allow re-sending it. This will then
loop indefinitely, but usually only until the `PathChallengeLost` timer
and challenge is cleared by either the challenge arriving or by the
`PathOpenFailed` timer triggering.
However, the `PathOpenFailed` timer is not armed in the NAT traversal
case for revalidation, as we're not opening the path, we're just
re-validating it.
So unless revalidation works, this path will be broken forever and keep
looping with path challenges until it receives a response.
This fixes revalidation by also arming the `PathOpenFailed` timer when
we re-validate.
To ensure we correctly handle that timer, this adds a
`OpenStatus::Revalidation` case.
I also renamed `PathOpenFailed` to `PathTimer::AbandonFromValidation`
because it now serves a slightly different purpose.
## Notes & open questions
A test for this exists in #589, but it relies on a fixed proptest setup,
which that PR addresses. I wanted to pull out the bugfix to its own PR
though.
## Change checklist
<!-- Remove any that are not relevant. -->
- [x] Self-review.
## Description
The careful reader will notice that fields are in the wrong
struct. This is natural because both of these are identified by PathId
and it is very confusing. Cleaning this up comes later.
## Breaking Changes
Not, yet.
## Notes & open questions
We have a lot of state in the wrong place:
- `PathData::off_path_challenges_unconfirmed` (the reason I'm here)
- `PathData::status`
- `PathData::open_status`
- `PathData::draining`
- `PacketNumberSpace::ecn_counters`
- `PacketNumberSpace::ecn_feedback`
I suggest we file issues for this, because I tried to move
`PathData::status` and it ends up moving so much code. End we get to
grapple with naming issues because e.g. `enum PathStatus` would then
be defined in a file called `spaces.rs`.
## Change checklist
<!-- Remove any that are not relevant. -->
- [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.
## Description
Currently `handle_network_change` only checks `is_path_recoverable()` on
the client side. The server side always assumed paths are recoverable,
based on the assumption that servers have stable addresses.
This assumption breaks when the server's uplink changes (e.g., mobile
device acting as server replugs to a different network). The dead direct
path would linger until the 3*PTO idle timeout instead of being
immediately abandoned.
Now both sides use the hint when provided. When no hint is given,
clients default to non-recoverable (existing behavior) and servers
default to recoverable (existing behavior).
## Breaking Changes
<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->
## Notes & open questions
<!-- Any notes, remarks or open questions you have to make about the PR.
-->
---------
Co-authored-by: Floris Bruynooghe <flub@n0.computer>
## Description
- More cargo-make targets:
- Disable all the built-in targets
- Add steps that will otherwise fail on CI
- Add checklist to PR template
- Also point to cargo-make
## Breaking Changes
n/a
## Notes & open questions
I'm not sure if the pointer to cargo-make is clear enough. I
considered making it a checkbox but that probably also gets annoying
for frequent contributors.
## Description
- Does what the PR title says
- Also some small cleanups in `poll_transmit_path_space`, if I may.
Context for what `app_limited` is:
> When bytes in flight is smaller than the congestion window and sending
is not pacing limited, the congestion window is underutilized. This can
happen due to insufficient application data or flow control limits. When
this occurs, the congestion window SHOULD NOT be increased in either
slow start or congestion avoidance.
https://www.rfc-editor.org/rfc/rfc9002.html#section-7.8
I.e. when `app_limited` is `true`, the congestion controller doesn't
increase the congestion window.
You can see this in `Cubic::on_ack` where it outright immediately bails,
doing nothing, when `app_limited` is `true`.
Because all paths have their own congestion window, they need to track
whether they're application limited separately.
Otherwise it's possible that there's a situation where one path is never
exercised fully (e.g. a backup path), while another path is saturated
with application data. Previously, the application data path would set
the connection-wide `app_limited` to `false`, causing the backup path
(that never experiences congestion) to inflate its congestion window.
With this change, the backup path would know it's not application
limited and pass that correct information to the congestion controller.
## Description
I got really annoyed at the weird variables trying to convey they are
representing the packet number in my last PR. Any many times
before. Finally fix this lazy naming.
## Breaking Changes
n/a
## Notes & open questions
If there is some logic behind the previous naming I failed to grasp
feel free to push back.
## Description
- Adds a regression test that used to fail and even produced a tight
infinite loop in `Pair::step`
- Adds a check to only schedule the loss detection timer for tail loss
probes when the current path is not yet abandoned.
The regression test derived from the proptest failure contains an
extensive comment explaining the problem.
Essentially it boils down to the `LossDetection` timer never being
cleared because the abandoned path was never polled for transmitting
tail loss probes, because it was abandoned.
## Description
The idle timer is kept per path, the permit_idle_reset field also
needs to be per path. It is now documented better as well.
The on_packet_authenticated needs to be careful about what it does for
probing packets. Take care of this.
One footgun still around is that if we keep selecting a broken path
for off-path nat traversal probes we're probably keeping that path
alive for longer. But I don't expect this to have much influence in
the grand scheme of things, because we need to have our packet
scheduling already avoid a path that's not really working.
## Breaking Changes
n/a
## Notes & open questions
Originally spotted by @Frando
## Description
This changes the server-side of the NAT traversal to always send path
challenges using the active CID. This means it does not skip probes
when there are no more CIDs (currently still limited to 5).
It does means the paths are linkable, and this is a violation of a
MUST in RFC9000. But this is the direction we want to take right now.
## Breaking Changes
Paths are now linkable.
## Notes & open questions
This is on the path towards #567.
Closes#574.
An attempt to replace #571 with something much simpler and without
design problems. If combined with increasing MAX_MULTIPATH_PATHS on
the iroh side I expect it to be similarly effective.
## Description
The PacketBuilder gets is_ack_eliciting from the SendableFrames, but
that is empty if data had to be queued especially for the probe. This
would mean that the PacketBuilder::finish_and_track did not note the
correct number of in-flight bytes for the congestion controller and
also did not update the
PacketNumberSpace::time_of_last_ack_eliciting_packet. That field in
turn is used by the PTO calculation as time from which to arm the next
PTO timeout.
To fix this we add information about all frames on whether they are
ack-eliciting or not. And if any ack-eliciting frame is written by the
PacketBuilder it keeps track of this internally.
Tail-loss probes should also not happen for the Data spaces as long as
the handshake has not yet been confirmed. During this time the Initial
and Handshake spaces' tail-loss probes are the ones that drive the
connection forward. And sending tail-loss probes on the Data space
already
consumes congestion controller window and pacing tokens. Also you
don't even know if the peer has keys to decrypt them.
## Breaking Changes
n/a
## Notes & open questions
n/a
## Description
Off-path NAT traversal probes were fire-and-forget: sent once per
address per round with no retry. This broke simultaneous-open NAT
traversal because the first probe is typically dropped (the peer's NAT
mapping doesn't exist yet when the probe arrives).
Now probes are retransmitted up to 10 times at initial-RTT PTO
intervals, with fresh CIDs reserved for each attempt.
Fixes#410, relates to #376
## Changes
- `ServerState` tracks per-probe attempt count via `ProbeState`
- New `NatTraversalProbeRetry` connection timer fires at initial
PTO-base intervals to re-queue probes
- Each probe reserves a fresh CID (no cross-path CID reuse)
- On new round, stale off-path challenges are cleared
## References
- [draft-seemann-quic-nat-traversal-02
§4](https://datatracker.ietf.org/doc/html/draft-seemann-quic-nat-traversal-02#section-4):
Off-path challenges for NAT traversal
- [picoquic challenge retry in
paths.c](https://github.com/private-octopus/picoquic/blob/master/picoquic/paths.c#L51):
picoquic retries challenges up to `PICOQUIC_CHALLENGE_REPEAT_MAX` (3)
attempts
## Notes
- If no reserved CIDs are available, the probe is skipped (not sent with
the active CID)
- Retry timer uses PTO-base from initial RTT without max_ack_delay,
since PATH_RESPONSE must be sent immediately
## Description
Remove poll_read_buf from public api.
We can always make it pub later if somebody needs it.
## Breaking Changes
<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->
## Notes & open questions
<!-- Any notes, remarks or open questions you have to make about the PR.
-->
## Summary
Fixes an overflow in `Endpoint::cids_exhausted()` on 32-bit targets when
CID length is 4.
The previous check used `usize` exponentiation:
- `2usize.pow(cid_len * 8)` overflows for `cid_len == 4` on 32-bit
platforms (`2^32` does not fit in `usize`).
This change is already merged into quinn:
https://github.com/quinn-rs/quinn/pull/2533
## Changes
- Replaced the exhaustion arithmetic with an overflow-safe
implementation using `u64`.
## Notes
- Kept the same exhaustion threshold semantics (>3/4 used).
- Preserved the early return behavior for `cid_len == 0` and `cid_len >
4`.
- No behavior change is intended on 64-bit targets.
- Arithmetic is now architecture-independent.
## Description
These fields probably also make sense to track connection-wide.
Fixes#559.
## Breaking Changes
n/a
## Notes & open questions
I'm not sure adding mtu probes makes sense, so leaving that out for now.
## Description
`maybe_queue_probe` was only called in one place and always had its
precondition (`loss_probes != 0`) hold because the place it was called
already checked this.
Thus, there's no need for the precondition check. We can drop the
`maybe_` prefix, too, and use the extra space to add a more descriptive
function name.
## Description
This is more in line with what `is_idle` is doing with `ConnIdle` as
well, and in the future once we handle #518 this is even more important,
as the tests expect that calling `pair.drive()` doesn't run the
connection till the idle timeout.
## Breaking Changes
Test changes only.
## Description
The auto-generated display was wrong and did not clearly show what
frame this was when it was being built or received. This shows up
better in the logs.
## Breaking Changes
our display names are not part of the api??
(i say, knowing that won't be true, can't be true. sadness)
## Notes & open questions
n/a
## Description
Adds a test that check if we close correctly during a connection
migration.
Test adapted from #390, but as a more constructive test than a
regression test. (Hot take: regression tests are technical debt in
less than 3 refactors)
## Breaking Changes
n/a
## Notes & open questions
Closing during a migration has many more sharp edges that aren't yet
covered or tested. But that's not too high on our priority list right
now.
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
## Description
The STREAM frames can easily take up the entire remainder of a
datagram. We need to make sure that ADD_ADDRESS and REMOVE_ADDRESS
frames are sent before we start building STREAM frames. They are also
higher priority than application data.
## Breaking Changes
n/a
## Notes & open questions
Replaces #526.
## Description
Reset `pto_count` and re-arm the loss detection timer for recoverable
paths in `handle_network_change`.
Moved before CID rotation so it runs even when the CID pool is
exhausted. Congestion controller and RTT estimates are preserved.
Split from #525.
Relates to #376
---------
Co-authored-by: Floris Bruynooghe <flub@n0.computer>
## Description
This adds the test for sending the PATH_ABANDON frame on the path
itself, which was merged earlier. At the time we did not have the
per-path frame stats so we could not write the test. Now we can.
## Breaking Changes
n/a
## Notes & open questions
n/a
Based on #540
Trying to make it less easy to forget this, I think it's neat, but maybe
not?
This does wake in some more cases now, specifically on error branches,
but I don't think that really matters. But please tell me if you think
it does
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
## Description
Hide the 2 usages of broadcast::Receiver behind a newtype wrapper that
implements Stream.
Hide the 1 usage of watch::Receiver behind a newtype wrapper that
implements Stream.
## Breaking Changes
noq::Connection::path_events: now returns a PathEvents struct
noq::Connection:: nat_traversal_updates: now returns a
NatTraversalUpdates struct
noq::Connection:: observed_external_addr: now returns a
ObservedExternalAddr struct
## Notes & open questions
Note: I did a very simple wrapper for watch::Receiver<SocketAddr> since
SocketAddr is Copy, and the main complexity of watch::Receiver is to
allow to borrow the underlying value. We don't need that. The only fn is
get which allows you to sync get the current value or none if none
present yet.
For the two broadcast receiver wrappers the public API is just Stream.
You get notified if you miss events with a Lagged error, and you can
always grab a new one if you want to start from now.
I did not try to abstract over the 2 different broadcast wrappers.
Better to have distinct types.
---------
Co-authored-by: Floris Bruynooghe <flub@n0.computer>
## Description
Immediately re-arm or stop the PathIdle timer when
`set_path_max_idle_timeout` is called, accounting for already-elapsed
idle time. Previously the running timer continued with the old deadline,
so extending the timeout during a network change had no effect until the
next received packet.
Setting `None` now stops the timer.
Split from #525.
Relates to #376
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
## Description
`handle_network_change` cleared `local_ip` before the hint callback
could check it, causing all paths to be marked recoverable regardless of
interface state. Read the network path first so the hint sees the
correct `local_ip`.
Split from #525.
Relates to #376
## Description
I keep fixing this locally, but then diagnose test issues forever
without submitting this fix as a PR.
The log line had a copy-and-paste error where it said "from server to
client" in both directions.
## Description
While investigating a bug in iroh involving custom transports, I noticed
that the information exchange between two endpoints connected via
localhost sometimes takes surprisingly long.
It seems that under very specific conditions, it can happen that frames
for hole punching are emitted but not sent immediately. Eventually
something else will happen on the connection, and they will be sent.
This change adds a conn.wake() after all noq-proto fns that can emit
frames to make sure that emitted frames are actually sent immediately.
The test fails before the change but passes after the change.
## Breaking Changes
None
## Notes & open questions
Note: maybe there is a reason we don't call wake after these calls. If
so, please enlighten me and ignore the PR.
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Description
When `initiate_nat_traversal_round` finds an existing path to a remote
address via `open_path_ensure`, trigger a new PATH_CHALLENGE to verify
the path is still alive. Previously the path was silently reused even if
broken (e.g. after a link outage), causing the connection to hang.
---------
Co-authored-by: Diva Martínez <git@divma.net>
## Description
This fixes an issue that #523 uncovered:
Proptests can get us into a situation where one among two paths is
broken, but only one path is validated.
Tail loss probes will be sent forever, as long as the broken path
exists.
Because the proptests didn't have a path idle timeout set, this would
happen forever, all while the other path would still be active, thus the
connection idle timeout wouldn't trigger.
We fix this by properly setting an idle timeout for paths in proptests.
## Breaking Changes
None.
## Notes & open questions
We should consider updating the default for path idle timeout.
## Description
Accept remote PATH_ABANDON for the last remaining path instead of
dropping it. When the remote abandons our last path, a NoViablePath
grace timer (3 × initial PTO) allows the application to open a new path
before connection closure.
Local `close_path()` still returns `LastOpenPath` for the last path —
use `Connection::close()` to end the connection instead.
Fixes#397, #398, #400
## Changes
- `close_path_inner` blocks last-path close for local reasons, allows
`RemoteAbandoned`
- `abandon_path()` extracted for the actual abandon logic
- `NoViablePath` grace timer set to `3 * initial_PTO` (accounts for
packet loss on replacement path)
- Timer cancelled in `ensure_path` when a new path appears
- `current_mtu()` falls back to `INITIAL_MTU` when all paths abandoned
## References
- [§3.4 para
4](https://www.ietf.org/archive/id/draft-ietf-quic-multipath-21.html#section-3.4-4):
MUST accept remote PATH_ABANDON and send reciprocal
- [§3.4 para
8](https://www.ietf.org/archive/id/draft-ietf-quic-multipath-21.html#section-3.4-8):
MAY wait for a short time before CONNECTION_CLOSE
- [picoquic
`picoquic_decode_path_abandon_frame()`](https://github.com/private-octopus/picoquic/blob/d6ba0415d5b2/picoquic/frames.c#L5745):
reference implementation accepts last-path abandon
---------
Co-authored-by: Diva Martínez <git@divma.net>
## Description
This moves the FrameStats into the PathStats, allowing us to check
what frames where sent on which paths. It then makes the
ConnectionStats always be computed to be the sum of all the PathStats.
This has as a nice side effect that some stats no longer need to be
recorded twice. Which was also error-prone.
The PathStats are now boxed in the PathEvent because that variant was
now way bigger than any of the other variants.
It also fixes a few missed fields in the stats code and uses more
defensive code so that won't happen again.
## Breaking Changes
Probably none? I'm only adding some fields to PathStats on the public
API, while the fields on ConnectionStats remain the same.
## Notes & open questions
I need this to be able to write tests that assert frames on specific
paths. But I also think that generally this makes more sense. The
previous version of the stats was thrown together without much thought
I think (by me).
I've cleaned up some cargo.toml mistakes that accumulated by using
tooling that doesn't follow the sorting/and locations of where things
live.
## Description
This was some kind of hack to be able to use wireshark on tests in
proto. But we have qlog now, which is probably much better for this.
Also, we have tests, e.g. in #522 and proptests, that would not work
with this because they use multiple fake addresses but we only ever
bind one socket.
## Breaking Changes
n/a
## Notes & open questions
n/a
## Description
Off-path NAT traversal probes (PATH_CHALLENGE) and off-path responses
(PATH_RESPONSE) are sent to a different destination than path_id's
network path. ACKs for these packet numbers won't arrive on path_id, so
tracking them in path_id's in_flight permanently inflates the congestion
state.
This caused the relay path to become congestion-blocked after repeated
NAT traversal probing, preventing REACH_OUT and ADD_ADDRESS frames from
being sent — breaking holepunching after network changes.
Use builder.finish() (no tracking) instead of finish_and_track(),
matching the existing send_prev_path_challenge() which already does this
correctly.
## Notes
- Is this the right solution?
- This does fix actual tests, the full combined fixes now pass the
`switch_uplink_ipv6` and `switch_uplink` tests from patchbay
## Description
We need to accept in-flight acks from an abandoned path, otherwise we
will be retransmitting data that we know the peer already received.
Only ignore the ACKs if we already discarded the path state, in this
case we'll already have marked all the data for retransmission when
the path state was discarded.
## Breaking Changes
n/a
## Notes & open questions
n/a
When `sendmsg` returns ENETUNREACH, EHOSTUNREACH, EADDRNOTAVAIL, or
ENETDOWN, propagate the error as `NetworkUnreachable` instead of
silently returning `Ok(())`.
Previously, all non-WouldBlock send errors were swallowed. This caused
the connection driver to believe packets were sent successfully, leading
to a PTO retransmit busy-loop: the driver would generate loss probes,
"send" them (error swallowed), rearm PTO, and immediately poll again
with `keep_going = true`. On a `current_thread` tokio runtime this
starved all other tasks — timers, application futures, and relay
connections all froze.
The fix follows picoquic's approach: network-unreachable errors are
classified separately from transient errors, propagated to the QUIC
stack, and the connection driver drops the transmit and stops spinning.
The QUIC timer system handles path idle timeout and demotion naturally.
Other send errors (ECONNREFUSED, EPERM, etc.) continue to be swallowed
as before, since QUIC retransmission handles recovery for those cases.
## Description
I keep getting mildly confused about the timer names. This tries to
name them consistently: they all say what happend when the timer
expires. The remaining timer names are from the specs itself AFAIK, so
I left them.
## Breaking Changes
n/a
## Notes & open questions
This is entirely opinionated, feel free to push back.
Because of that review request is an AND.
## Description
This updates noq and its dependencies to rand 0.10 and getrandom 0.4.
## Breaking Changes
I don't think there are any of rand's APIs exposed in noq or noq-proto.
So no breaking changes.
## Notes & open questions
~~Blocked on https://github.com/tomtomwombat/fastbloom/pull/27 being
released.~~ We can disable the rand feature for fastbloom instead.
`proptest` still depends on rand 0.9:
```
$ cargo tree -i rand@0.9
rand v0.9.2
└── proptest v1.9.0
[dev-dependencies]
└── noq-proto v0.16.0 (/home/philipp/program/work/noq/noq-proto)
├── noq v0.17.0 (/home/philipp/program/work/noq/noq)
│ ├── bench v0.2.0 (/home/philipp/program/work/noq/bench)
│ └── perf v0.2.0 (/home/philipp/program/work/noq/perf)
└── perf v0.2.0 (/home/philipp/program/work/noq/perf)
```
But it's only a dev-dependency, so won't get pulled in for people
depending on noq.
## Description
In favor of having a more stable API for noq, the definition of a
network path
needs some changes.
The changes, in short, are that the flow label is always cleaned, and
the scope
id is kept in cases where it might be necessary.
This is possibly the best compromise between RFC9000's address
definition
(§1.2) and the reality of requiring some ipv6 extra fields in the
address for
adecuate routing for the future goal of being able to handle link-local
inclusive topologies, while ensuring comparison between network paths is
meaningful. This last part is necessary to properly handle connection
wide path
and address validation.
## Breaking Changes
`FourTuple`'s fields are no longer public, use `FourTuple::from_remote`
and
`FourTuple::new` to create a `FourTuple` and the `FourTuple::remote` and
`FourTuple::local_ip` accessors instead of direct field access.
## Notes & open questions
n/a
## Description
If there is no other validated path then we allow sending PATH_ABANDON
on the path to be abandoned itself.
Closes#399.
## Breaking Changes
n/a
## Notes & open questions
Please review the logic changes carefully, I had to modify all
conditions and we need to be sure I only added a condition without
modifying previous logic.
I have removed the test because this is blocking other work. I will
PR the test separately. The test succeeds by checking the logs
manually, so the change is correct. There are however changes needed
to be able to assert that in the test.
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
## Description
On multipath we can not use the is-validating check to know if the
packet belongs to this 4-tuple. However the path generation was
introduced exactly to identify the 4-tuple a packet was sent on, so
use that instead. As a bonus it is much more explicit in intent.
## Breaking Changes
n/a
## Notes & open questions
n/a
* it's the weekend, this is useful in my head
so let's check in totally broken code.
* more
* wip
* wip
* wip
* try more stuff
* moar
* more wip
* can't send anything when there's nothing to send
This stopped us from going to the next space to coalesce the Initial
with the Handshake packet on the server-side. And probably lots of
other stuff
* wip
* and check for loss probes that need to be sent
* some doc updates
* remove SendableFrames::validation, it was redundant
Turns out this was exactly the same case as
SendableFrames::space_id_only.
* Add a test that checks we send on an available path
* simplify the packet scheduling logic a little
* Simplify, immediate close during handshake is broken though
* wip
* Guess we do need a new field for this
Slightly sad to have to make an explicit exception for this, at least
currently.
* clippy
* may_send_data has been made smarter, don't need this
* wip
* tweak logging
* proptests should maybe log errors?
* drive to idle for a 1000 iterations, 100 is a bit small
* turns out we don't need this and i find it a bit confusing
* remove stuff split off into #494
* Remove changes split off into 495
* typo
* style
* ww
* remove this old thing
* tweak
* needs_loss_probe is a variable above, it's not that bad
* collapse these two cases. it's a hard call though
* well doh, the doc comment says why
* wordsmithing, because 90% of packet scheduling is about that
* put this back, split off into another pr
* fix doc comment
* Typos from code review
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
* Do not allocate and move scheduling info to separate function
Two bits of PR review:
- Move the scheduling to a separate function.
- Avoid an allocation, but this has some tradeoffs.
- I now have two loops that look for the PathId by doing `next_path_id
= self.path.keys().find(|i| **i > path_id).copied();`. It might be
possible to fold the MTU discovery in the main poll loop, MTU packets
would get a slightly higher priority but probably not really harmful
overall.
- I now need to do the computation for
`have_validated_status_available_space many more times.
I'm not sure how much the compiler manages to remove all of that. Is
it smart enough to figure out that
`have_validate_status_available_space` won't change between the calls
and does it move it out? Does it make the iteration as fast as the
previous version?
On the other hand, we now have some situations where we don't have to
compute the scheduling information, and no longer need to compute it
for all paths if we don't send on the last path.
What do you think, which version is better (though I also adopted
@matheus23's feedback about splitting it off to a function, but that
doesn't affect this really. It does make the diff a little bit more
though)?
As an aside, in working out of how scheduling should work it was
really helpful to have to extremely explicit as a bunch of data that's
computed up-front. But it's fair that now we know this is how it
should work that we can implement it in the most optimal way.
* Small attempt at making this clearer
* fix docs
* refactor(proto): ordering of REACH_OUT and OBSERVED_ADDR building
The ordering of these frames was a bit too eager.
REACH_OUT is important timing-wise. But it is not more important than
HANDSHAKE_DONE, PING, IMMEDIATE_ACK, ACK, ACK_FREQUENCY and should
anyway not be sent on a path that also needs
PATH_CHALLENGE. PATH_RESPONSE could be the one exception but it is
also small.
If one of those frames do end up in the same packet as REACH_OUT there
will still be place for the REACH_OUT frame. The CRYPTO frame is left
after it, because after the handshake that is only carrying auxiliarry
non-time-sensitive information, and REACH_OUT is also only possible in
the data space kind.
OBSERVED_ADDRESS is definitely not that high priority, it might need
to move back even further.
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
* it's the weekend, this is useful in my head
so let's check in totally broken code.
* more
* wip
* wip
* wip
* try more stuff
* moar
* more wip
* can't send anything when there's nothing to send
This stopped us from going to the next space to coalesce the Initial
with the Handshake packet on the server-side. And probably lots of
other stuff
* wip
* and check for loss probes that need to be sent
* some doc updates
* remove SendableFrames::validation, it was redundant
Turns out this was exactly the same case as
SendableFrames::space_id_only.
* Add a test that checks we send on an available path
* simplify the packet scheduling logic a little
* Simplify, immediate close during handshake is broken though
* wip
* Guess we do need a new field for this
Slightly sad to have to make an explicit exception for this, at least
currently.
* clippy
* may_send_data has been made smarter, don't need this
* wip
* tweak logging
* proptests should maybe log errors?
* drive to idle for a 1000 iterations, 100 is a bit small
* turns out we don't need this and i find it a bit confusing
* remove stuff split off into #494
* Remove changes split off into 495
* typo
* style
* ww
* remove this old thing
* tweak
* needs_loss_probe is a variable above, it's not that bad
* collapse these two cases. it's a hard call though
* well doh, the doc comment says why
* wordsmithing, because 90% of packet scheduling is about that
* put this back, split off into another pr
* fix doc comment
* Typos from code review
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
* Do not allocate and move scheduling info to separate function
Two bits of PR review:
- Move the scheduling to a separate function.
- Avoid an allocation, but this has some tradeoffs.
- I now have two loops that look for the PathId by doing `next_path_id
= self.path.keys().find(|i| **i > path_id).copied();`. It might be
possible to fold the MTU discovery in the main poll loop, MTU packets
would get a slightly higher priority but probably not really harmful
overall.
- I now need to do the computation for
`have_validated_status_available_space many more times.
I'm not sure how much the compiler manages to remove all of that. Is
it smart enough to figure out that
`have_validate_status_available_space` won't change between the calls
and does it move it out? Does it make the iteration as fast as the
previous version?
On the other hand, we now have some situations where we don't have to
compute the scheduling information, and no longer need to compute it
for all paths if we don't send on the last path.
What do you think, which version is better (though I also adopted
@matheus23's feedback about splitting it off to a function, but that
doesn't affect this really. It does make the diff a little bit more
though)?
As an aside, in working out of how scheduling should work it was
really helpful to have to extremely explicit as a bunch of data that's
computed up-front. But it's fair that now we know this is how it
should work that we can implement it in the most optimal way.
* Small attempt at making this clearer
* fix docs
* Rename field based on more feedback
* fix docs
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
Ruediger Klaehn