Quinn mistakenly applied the beta multiplier to the newly adjusted `W_max` instead of the current window.
This double-reduced the window to 59.5% instead of the intended 70%.
This fixes the math by snapshotting the original window for the multiplicative decrease.
Unlike the `ZeroRttAccepted` future, this is guaranteed to reliably indicate
whether a specific stream was rejected, even if it was opened concurrently
with completion of the handshake.
## Description
Continuing perusing stats, `path_stats` and `stats` both require `&mut
self`, which is unintuitive and actually not even needed.
- `stats` doesn't even need a change beside updating the api, so no
change for this function, just noted here for completeness.
- `path_stats` was taking the stats mutably, updating them and then
returning.
There is no point in the update, as this is never read. The next call to
`path_stats` overwrites whatever is in there. Instead of updating the
stored
value with the snapshot fields, it updates the returned stats. This
allows
us to polish the public API and remove the unexpected `&mut self` in
favor
of the more natural `&self`.
## Breaking Changes
n/a
## Notes & open questions
n/a
## Change checklist
<!-- Remove any that are not relevant. -->
- [x] Self-review.
## Description
Fixes a panic when a stale coalesced datagram is delivered for a path
after that path’s state has already been discarded. The packet receive
path now early-discards packets whose `path_id` is no longer in `paths`
but is known in `abandoned_paths`, before coalesced packet handling can
assume the path still exists.
## Change checklist
- [x] Self-review.
- [x] Tests if relevant.
## Description
Closes#727
Deprecates `ios` in `UdpStats`. It's a bit of a shame that `UdpStats` is
shared by so many places, path's sending stats, receiving stats,
connection-wide send and recv stats. There is no field that captures
something like this and makes sense across all of them, so adding it
would make for a structural change.
This field was clearly added to test gso batching: a connection-wide
sending stat. This is the only place where this is meaningful, so to be
able to continue testing this, a tet only field is added to the
connection stats.
## Breaking Changes
- Deprecates `UdpStats::ios` without replacement. See #727 If you do
need to count io operations, please do so by incorporating counting
operations in your `AsyncUdpSocket` implementation
## Notes & open questions
If anyone seriously cares about this, we should know thanks to the
deprecation warning, and would prompt us to add something that better
fits with the rest of the stats.
Whatever we do won't just be a undo of this PR as counting doesn't make
sense in the rest of places, and the name is misleading. Basically, the
deprecation warning should allow us to decide whether a larger change is
required to get the `gso_batches` (what the field actually tried to
measure, as noted by the tests) or if it can be cleanly removed. I'm
tempted to say the latter, as I don't think users care too much about
this.
Now, if they _do_ want a count of `io` operations, that's an entirely
different approach that requires interaction with the `AsyncUdpSocket`.
## Change checklist
<!-- Remove any that are not relevant. -->
- [x] Self-review.
- [x] Breaking changes documented.
<!--
tip:
Run `cargo make` in the workspace root to check many light-weight CI
steps locally.
-->
## Description
Today's cargo-deny gift
## Breaking Changes
none
## Notes & open questions
Not updating Cargo.toml because there's no API issue with those
versions for us. It's not our job to make sure users use sound
versions I guess.
## Change checklist
- [x] Self-review.
## Description
`PathStats` has a lot of values that are cumulative counters and so,
those are
updated as their respective actions are performed. Some other values,
like rtt,
are snapshots: views of the current state, and so, are only updated on
demand.
When a path is discarded, it _used_ to call `Connection::path_stats`
which
performs the update, but it was lost in a refactor. As a consequence,
those
values were no longer being updated before the final stats were returned
in the
`Discarded` event, despite the comment explaining the need to do so
remaining
in the code.
This fixes the issue by updating the stats and adds a test. Yes, it's a
stats
only test, but the fact this was properly done at some point and
regressed
later is evidence this is needed.
Small consequence is that the return type of `PathStatsMap::discard` is
no
longer needed/useful.
## Breaking Changes
n/a
## Notes & open questions
n/a
## Change checklist
<!-- Remove any that are not relevant. -->
- [x] Self-review.
- [x] Tests if relevant.
<!--
tip:
Run `cargo make` in the workspace root to check many light-weight CI
steps locally.
-->
## Description
Supersedes #674Fixes#329
Fixes a couple of issues with scheduling and sending OBSERVED_ADDR
frames:
- instead of sending the frame with path challenge and path response,
queue it, to keep a single place where sending occurs.
- remove `may_send_data` as a condition to send observed address frames.
This,
according to the docs, is about sending `Data` data, but this is a path
specific frame, not part of the broader Data space. The check is not
needed
and it's also not really correct. But this being path-specific became
clear
only recently, so it explains why this might have been added.
## Breaking Changes
n/a
## Notes & open questions
n/a
## 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.
<!--
tip:
Run `cargo make` in the workspace root to check many light-weight CI
steps locally.
-->
## Description
When receiving a PATH_RESPONSE for which the 4-tuple of the challenge
matches the local network path the path should be marked as
validated. But it should not itself update the PathData::network_path
of that path generation. This is already being correctly handled,
taking packet numbers and probing packets into account, at the end of
Connection::process_payload.
If the local IP *did* somehow change after the challenge was sent but
before the response was received, the PathData::network_path will no
longer match the path the challenge was sent on and the response will
be ignored. This means the timers would remain set and a new challenge
would be sent.
## Breaking Changes
none
## Notes & open questions
none
## Change checklist
- [x] Self-review.
## Description
- Adds `PathRetransmits` to track data that must be sent over a specific
path.
- Fixes re-transmission of `OBSERVED_ADDR` frames. This was not done
correctly
because the loss of a packet containing these frames would set a global
variable to be re-sent, that was then picked up by the first path. Think
a
path "stealing" re-transmission of the frame from another path.
- Naming standards: Replaces `observed_addr_sent` for
`pending_observed_addr` to
use the naming convention in the codebase.
Partially replaces #674
## Breaking Changes
## Notes & open questions
Much simpler and elegant this way.
As I mentioned in #674 a test is not really possible because of the
multiple
"safeguards" that prevents this from happening
## 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.
## Description
Fixes an underflow panic on decrementing `active_connections` in the noq
endpoint state.
This occurred when the API was used in this order:
1. client connects to server
2. server calls `Endpoint::accept` and captures the `Incoming`
3. server endpoint closes using `Endpoint::close`
4. server side calls `Incoming::accept` on the captured incoming
5. (EndpointDriver will panic)
The reason for this was that incrementing and decrementing the
`active_connections` was asymmetrical.
We would increment `active_connections` only if the endpoint wasn't
closed yet. However, we would decrement unconditionally, eventually
underflowing.
This PR removes the condition for incrementing. This preserves the
behavior of being able to accept `Incoming`s after calling
`Endpoint::close`. This is somewhat unlike what the iroh API expects
(because in iroh you can't do anything with the endpoint *after*
`Endpoint::close`), but noq's API is just different.
This also adds a regression test that fails without the fix. That needs
a `PanicPropagatingRuntime` implementation and some test helpers to
allow for configuring that though...
## Breaking Changes
None
## Notes & open questions
Disclosure: Heavily assisted with GLM 5.2.
## Change checklist
- [x] Self-review.
- [x] Tests if relevant.
- [x] All breaking changes documented.
## Description
Please merge this after #710
Prior to this change, retransmissions of PATHS_BLOCKED and
PATH_CIDS_BLOCKED frames would use "up to date" information for the
known maximum remote path ID and CID queue expected next seq number
respectively.
This is weird, because these frames are informational and meant for
debugging, and either we don't send them when they're not needed anymore
(e.g. between originally trying to send them and retransmission the
maximum remote path ID increased or the CID queue active seq number
increased. In both of these cases we wouldn't actually be blocked
anymore.), or we just retransmit them with with the "outdated" values
that we sent them with on the first transmit.
This PR decides to go for the latter. This provides the appropriate
debugging information to the remote that we were out of path CIDs *at
some point in time*, even if that is now obsolete.
---
Apart from that this PR also fixes the fact that we never actually
queued PATHS_BLOCKED frames in the first place...
## Breaking Changes
None.
## Notes & open questions
Arguably, we should not send any of these frames and I personally hate
them, they haven't really helped me at all and only caused problems so
far.
The only good reason I can find to keep sending them is that the spec
forces the server side to correctly *handle* them (and produce protocol
violations if they're sent incorrectly), and without actually sending
them, we would never exercise these code paths and not make sure that
said server side checks are correct. (Yes I think that's just
unnecessary busywork and wasted bytes/datagrams.)
I've also added a test to check that we transmit & retransmit
PATHS_BLOCKED frames. Unfortunately it's quite hard to check that the
data that is actually sent is the same PATHS_BLOCKED frame as originally
constructed. Instead I did a manual check by looking at the logs. Please
trust me bro.
## Change checklist
- [x] Self-review.
- [x] Tests if relevant.
## Description
When receiving a `PATH_CIDS_BLOCKED` frame we used to check the
`next_seq` value for protocol compliance like so:
```rs
if next_seq.0
> self
.local_cid_state
.get(&path_id)
.map(|cid_state| cid_state.active_seq().1 + 1)
.unwrap_or_default()
{
return Err(TransportError::PROTOCOL_VIOLATION(
"PATH_CIDS_BLOCKED next sequence number larger than in local state",
));
}
```
But the `.unwrap_or_default()` would fail if we don't actually store
path state for the given path. This can occur when the path has already
been abandoned and discarded.
Usually, we wouldn't hit this case because the most likely value for
`next_seq` for PATH_CIDS_BLOCKED is 0, *unless* it is lost and
retransmitted.
The added regression test simulates all of these rare circumstances at
once:
- All PATH_NEW_CONNECTION_ID frames from server to client are delayed
such that opening a path on the client side generates a
PATH_CIDS_BLOCKED frame
- The PATH_CIDS_BLOCKED frame is lost as well to ensure it is resent
with a newer `next_seq` number *after* the delayed
PATH_NEW_CONNECTION_ID frames have come in
- The path is abadoned and discarded on both ends before we send the
delayed PATH_CIDS_BLOCKED frame triggering the bug
The fix is to correctly check for
`!self.abandoned_paths.contains(&path_id)` in the above condition.
## Breaking Changes
None
## Notes & open questions
This bug report is what originally prompted this work:
https://github.com/n0-computer/iroh/issues/4347
It should now be fixed.
## Change checklist
- [x] Self-review.
- [x] Tests if relevant.
- [x] All breaking changes documented.
Description
On Windows, the OS reports the fate of a previously sent UDP datagram
against the **next `recv`** on that socket. A send to a port with no
listener draws an ICMP port-unreachable, reported as `WSAECONNRESET`; a
send whose datagram has its TTL expire in transit draws an ICMP
time-exceeded, reported as `WSAENETRESET`. We already ignore
`WSAECONNRESET` on the recv path (the Rust standard library maps
`WSAECONNRESET` to `io::ErrorKind::ConnectionReset which we ignore in
the `EndpointDriver`).
Windows provides ioctls to suppress these notifications altogether,
which saves a syscall and has no downsides for us, as we never handle
recv errors related to a previous send anyway. This disables both
`SIO_UDP_CONNRESET` (port-unreachable / `WSAECONNRESET`) and
`SIO_UDP_NETRESET` (TTL-expired / `WSAENETRESET`) in
`UdpSocketState::new`.
## Notes & open questions
Comes with a regression test that I confirmed to fail on Windows without
the fix. It deterministically reproduces the port-unreachable
(`WSAECONNRESET`) path.
`SIO_UDP_CONNRESET` and `SIO_UDP_NETRESET` are ioctls, not socket
options, so they go through `WSAIoctl` rather than the existing
`set_socket_option`/`setsockopt` helper. `SIO_UDP_NETRESET` requires
Windows 8+, and neither is implemented under Wine; a missing ioctl is
logged and tolerated rather than treated as fatal.
## 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.
- [x] Tests if relevant.
- [x] All breaking changes documented.
---------
Co-authored-by: Floris Bruynooghe <flub@n0.computer>
## Description
A path's open status, whether the application has been informed of the
path being usable, does not belong to the 4-tuple state of the path
generation. It is something that is specific to the PacketNumberSpace
as it stay stable regardless of the 4-tuple the path migrates to.
Contributes to #591.
## Breaking Changes
none
## Notes & open questions
none
## 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.
## Description
This primarily moves pending path responses to the PacketNumberSpace
they should be sent on. There is nothing in a path response that
signals it is on- or off-path, every response must be sent on the
4-tuple it was received on. The only difference is at sending time: an
on-path PATH_RESPONSE could be combined with other on-path data ready
to send.
## Breaking Changes
none
## Notes & open questions
This PR does a bunch of renames in independent commits. Best to review
commit-by-commit and also to merge without squashing.
## 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.
## Description
This moves the ADD_ADDRESS and REMOVE_ADDRESS frames to be the same
priority as REACH_OUT (they are only sent by the server while only the
client sends REACH_OUT frames). In particular they need to be sent
before any user data and before new connection IDs. Otherwise
PATH_NEW_CONNECTION_ID frames being issued for other paths are
delaying the ADD_ADDRESS frames which can already be used by QNT.
## Breaking Changes
n/a
## Notes & open questions
This is part of my notes from investigating #4247.
## Change checklist
- [x] Self-review.
This means the merge queue queue will trigger starting the patchbay
checks. And those checks are currently required to merge, so without
it the merge queue can not merge PRs.
## Description
This now includes the error_code that is part of the frame.
## Breaking Changes
none
## Notes & open questions
A small thing i still had lying around in a stash. Might as well
submit this separately.
## Change checklist
- [x] Self-review.
---------
Co-authored-by: Diva Martínez <26765164+divagant-martian@users.noreply.github.com>
## Description
We really can not afford these panics for broken
invariants. Especially not in auxiliary code like qlog.
## Breaking Changes
none
## Notes & open questions
none
## Change checklist
- [x] Self-review.
## Description
When the server sends a tail-loss probe in the Initial space it was not
padded to 1200-bytes. However a tail-loss probe is by-definition
ack-eliciting, so it MUST be padded to 1200 bytes.
Because of how space_can_send works it does not give us accurate
information for tail-loss probes. Which is why this bug occurred. We do
want to eventually get rid of space_can_send in #507 which would be a
more systematic fix, hence I'm fine with just the simple fix of catering
for this exception here.
## Breaking Changes
none
## Notes & open questions
I believe this will effectively fix#690. This is the reason that a
packet of an invalid size gets produced.
That issue has a secondary problem that the data scheduled for the 2nd
tail-loss probe than does not fit all in the 2nd tail-loss probe. This
will also be fixed as a side effect of the first tail-loss probe being
padded so the 2nd bug won't trigger.
That 2nd bug of not tracking the exact size needs is also an already
known bug that so far doesn't cause too much real-life trouble. It's
other symptom is tail-loss probes sending an entire GSO batch of empty
packets.
If you inspect the output of the test carefully there are still some
inefficient and minor bugs happening after "continue connection
establishment". Though none of them seem to be very critical right away.
## Change checklist
- [x] Self-review.
- [x] Tests if relevant.
## Description
This updates `n0-qlog` to `0.2.0`, which merges in changes from upstream
`qlog` and brings us up-to-date to the latest draft standards. See
https://github.com/n0-computer/n0-qlog/pull/16 for all changes.
## Change checklist
- [x] Self-review.
## Description
The fix in #695 was only gating emitting the event to the
application. However the entire frame should be ignored: the timers
are already all cancelled when the path is
abandoned (Connection::abandon_path). And this branch *could*
accidentally set a new timer if there are still other path challenges
without a response on the path. Which would then lead to spurious
firing of timers for an abandoned path.
## Breaking Changes
none
## Notes & open questions
I'm choosing to do this in the match branches so that the received
challenge is removed from the `PathData::on_path_challenges_unconfirmed`
field. This probably no longer matters, but it seems tidier than
entirely ignoring the frame.
Follow up to
https://github.com/n0-computer/noq/pull/695/files#r3386765693
I'm hoping the existing test is sufficient. Testing this particular
anomaly would be very tricky as we can't yet inject a custom packet
when we want it.
## Change checklist
- [x] Self-review.
## Description
Philipp's monkeys generated a case in which path challenges are sent on
the
second datagram of a batch, where the first one is smaller than the
required
`MIN_INITIAL_SIZE`. This causes a misalignment of the batch since these
frames
must be sent on expanded datagrams.
## Breaking Changes
n/a
## Notes & open questions
Regression test is in #694
## Change checklist
- [x] Self-review.
## Description
This change makes sure that we never emit a `PathEvent::Established` for
a path that has already been abandoned.
It may happen that a PATH_RESPONSE frame which validates a path is
received after a PATH_ABANDON frame was received for the same path. In
this case, before this fix we used to emit a `PathEvent::Established`
*after* a `PathEvent::Abandoned`. This ordering makes no sense for
applications, and it makes state tracking e.g. for iroh difficult and
error-prone.
This PR fixes this by not emitting `PathEvent::Established` for paths
that are already abandoned. Comes with a regression test that fails
without the fix.
## 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.
- [x] Tests if relevant.
---------
Co-authored-by: Diva Martínez <26765164+divagant-martian@users.noreply.github.com>
## Description
This is simply a LOC reduction PR. This is a change the quinn
maintainers
wanted and I finally got to do it for when we decide to upstream this.
Simple
refactor, nothing fancy. Just less lines of code.
## Breaking Changes
## Notes & open questions
## 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.
## Description
This stores the abandoned paths in an ArrayRangeSet, which results
that the memory is bounded by the number of allowed concurrent
multipath paths.
## Breaking Changes
## Notes & open questions
Mainly for comparison with #688.
- ~~AbandonedPaths::len is kind of sad, needing to construct a full
iter. Could be optimised to iterate over the ranges only and doing
the sums of each range end - start. That's probably reasonable.~~
done now.
- ~~All the casts to u32 are a bit unfortunate, but they are correct I
think.~~ They are gone now that the ArrayRangeSet can contain a u32.
- ~~The ARRAY_RANGE_SET_INLINE_CAPACITY (2) is not really a suitable
value for this use. It'll almost always allocate in practice. This
could probably be fixed by making it a const generic so we can pick
a different value for the use here.
Doing this might give this version an advantage?~~
This is now done.
- ~~Making the ArrayRangeSet generic over u32 (or PathId) or u64 would
save space in the inline vec. But some traits range depends on are
not public or unstable and it's a bit tricky.~~
done
Question is, which version would we prefer? I've not benchmarked these
against each other, it's hard to say which would be better and
probably doesn't really matter. Memory and allocation wise I don't
think it makes much difference, I think only CPU might be interesting
but suspect it doesn't matter much.
It is kind of nice that the compaction only needs to exist once. I
think this version is more complex but also it's already written. So
I'm undecided.
I'll still add tests, both versions need them anyway.
## 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.
- [x] Tests if relevant.
---------
Co-authored-by: Diva Martínez <26765164+divagant-martian@users.noreply.github.com>
## Description
It seems this was missed when this was ported from the upstream
PR. This is different from #657 which was doing this unconditionally,
regardless when the packet needed to be accounted for congestion
control or whether it was ack-eliciting.
I did not continue the tests. It's something extremely specific and
artificial that is being tested. It's like testing everything with a
mock, I'm not sure the value it provides is worth all the stuff that
is setup. Ideally we figure out a better way to test that congestion
controllers actually behave correctly sometime.
## Breaking Changes
none
## Notes & open questions
Replaces #657, I considered taking it over. But this way I can't
approve my own PR.
## Change checklist
- [x] Self-review.
## Description
We should be able to traverse NATs with a combination of a "very easy"
and "hard" NAT. Adjust the SimpleFirewallRouting to allow emulating
that behaviour.
Verified by reverting c20a684349 and
then test_hard_nat_server_opens_path fails.
## Breaking Changes
none
## Notes & open questions
Look how simple this was. #675 was a red herring.
## 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.
- [x] Tests if relevant.
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
## Description
This introduces a ConnPairBuilder to construct the ConnPair and
switches everything over to use it. The individual ConnPair
construction methods were becoming unwieldy.
There are probably a few style arguments to be had, but the current
code doesn't look to bad. We can keep tweaking this.
## Breaking Changes
none
## Notes & open questions
split off from #675 and taken a bit further.
## 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.
---------
Co-authored-by: Diva Martínez <26765164+divagant-martian@users.noreply.github.com>
## Description
This really surprised me that this wasn't calling me out on dead
code. I don't think this is very helpful.
ConnPair has loads of dead code, I assume if we'd convert all the Pair
usage to ConnPair they would go away so that is kind of intentional?
So I allowed it just there for now.
## Breaking Changes
none
## Notes & open questions
none
## Change checklist
- [x] Self-review.
## Description
This removes all the star-imports from the tests, making all the
imports much more explicit.
I guess tests/mod.rs' `use super::*` is the equivalent of the usual
test layout, but them not being in the same file makes it really hard
to follow imports. And implicitly relying on imports of 3rd party
crates via star imports is not very nice. So I think it being in a
separate file justifies not using the `use super::*`.
## Breaking Changes
none
## Notes & open questions
I know it is opiniated. I stumbled upon this because I was looking why
dead code was not being flagged in tests. Stand by for another PR...
## Change checklist
- [x] Self-review.
## Description
This implements a simple backoff algorithm for resending the
PATH_CHALLENGEs that are sent on-path, very similar to the backoff
mechanism in `pto_time_and_space`.
Fixes the proptest failure in #609
## Notes & open questions
This is one way to "fix" on-path path challenges.
I did look at what a version would look like that perhaps tried to use
`Retransmits` or the `LossDetection` timer, but (1) `Retransmits` is
used across all paths and so far only used for data that can be
transmitted over any valid path (speaking of the data space-kind only),
and the `LossDetection` timer is specifically about detecting loss via
acknowledgements, not about detecting loss from missing
`PATH_RESPONSE`s. It's possible that a `PATH_CHALLENGE` is ACKed, but
needs to be retransmitted, because the ACK was sent over a different
path than the `PATH_RESPONSE`, and the `PATH_RESPONSE` got lost.
We *could* look into a system that resends both `PATH_CHALLENGE` as well
as `PATH_RESPONSE`, but then you'd need to retransmit the
`PATH_RESPONSE` using the same token, which would invalidate RTT
estimation based on path challenges.
All in all, keeping our own timer and roughly duplicating the backoff
algorithm from `pto_time_and_space` seems like the most reasonable move
forward.
## 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.
- [x] Tests if relevant.
- [x] All breaking changes documented.
## Description
This changes the `ManyToManyRouting::route_client_to_server` and
`ManyToManyRouting::route_server_to_client` functions to take
`Transmit::src_ip` into account.
`Transmit::src_ip` is set when noq-proto wants to pin the traffic sent
to a specific interface. In that case, `src_ip` will be the IP address
of the local interface that's to be used to send to the remote.
If the routing table changes in between noq-proto assigning a local IP
for a path (and thus choosing a `src_ip`) and sending again - the
`src_ip` may be different from what the `ManyToManyRouting` table would
use otherwise.
In those cases, `src_ip` should force staying on the same 4-tuple.
We also check that the route even exists. It's possible that the
`ManyToManyRouting` table is modified in a way that will cut the network
path on a certain 4-tuple. In that case, we need to return
`RoutingDecision::Drop`.
Thus the logic is the following:
- Look for all local interfaces that are connected to given destination
socket address (this may be multiple)
- Filter all these local interfaces for the interfaces with an IP
address matching our `src_ip`.
- Choose the first local interface among those interfaces as our route.
That local interface address will then be the remote that the other side
will see for the incoming `Transmit`.
This is just some effort in making the `ManyToManyRouting` table a
little less weird. Otherwise it would represent a weird OS that
completely ignores the `src_ip` setting.
## Change checklist
<!-- Remove any that are not relevant. -->
- [x] Self-review.
## Description
When we receive a successful probe response from an unknown remote
that only means the remote managed to challenge us from that
remote. It is not because this remote was not advertised in an
ADD_ADDRESS frame that it should be ignored.
This now successfully opens paths if the server is behind a
Desitnation Endpoint Dependent NAT.
Replaces #647
## Breaking Changes
n/a
## Notes & open questions
I *really* wanted to have tests for this in proto, but they will come
later. In the meantime I'll point the patchbay tests from
https://github.com/n0-computer/iroh/pull/4254 to this PR which will
test this.
## 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.
- [x] Tests if relevant.
## Description
The RoutingTable is in charge of which addresses a TestEndpoint
has. So now all decisions are made there remove the addr field and
clean up any remaining usages. This should avoid confusion in the
future.
## Breaking Changes
n/a
## Notes & open questions
Maybe I'm finally done with cleaning up existing test infra. I'm
medium enthusiastic about this whole thing. I think it's an
improvement but there's definitely some things that look more complex
now. And some of the tests still do just plain weird stuff. Like when
opening a 2nd path we should now probably have a router that has a 2nd
path. But it was originally written without any router so cheated by
disconnecting the first path and relying on not reaching the
keep-alive interval or idle-timeout of the first path. So those are
still further improvements. But I need to work towards writing my
test, so I'll stop here.
## Change checklist
- [x] Self-review.
- [ ] Tests if relevant.
## Description
Based on #643
Removes `identity_hash` from the public API of `noq_proto`.
## Breaking Changes
* changed: `noq_proto::PathId` no longer
implements`identity_hash::IdentityHashable`
## Notes & open questions
Not sure if it's worth it?
## Change checklist
- [x] Self-review.
- [x] All breaking changes documented.
---------
Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
## Description
This is for future-proofing the API.
Closes#642
## Breaking Changes
- `enum PathEvent` and all of its cases are now marked as
`#[non_exhaustive]`
## Notes & open questions
I'm ignoring the `_ =>` case, as anything else would mean we would "do
something" in case e.g. noq is bound against a newer noq-proto.
In `#[cfg(test)]` however, I panic, as in that case the version we
depend on must be up to date.
## Change checklist
<!-- Remove any that are not relevant. -->
- [x] Self-review.
Vladimir Petrzhikovskii