-
Notifications
You must be signed in to change notification settings - Fork 3
Add multipath #28
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
base: iroh-0.11.x
Are you sure you want to change the base?
Add multipath #28
Conversation
6f37612
to
f5485c2
Compare
6fa6b81
to
f3f1a44
Compare
Hi there! |
@MikeRomaniuk This is still very early right now, maybe like the first 5% if that. We are still in the process of restructuring things for the basic building blocks for multipath to be there. If you want to help you can take a look at the code so far, but it is currently still rather difficult to point at anything. It will probably be easier to jump in once we have the most basic 2-path connection working. But if you're keen to really help, right now it is mostly a case of trying to understand what would be a useful next step based on the specs and code. And then try and implement some of it. With just two of us this seems to move us forwards so far. |
@MikeRomaniuk Can I inquire as to what your usecase is? |
@flub, thanks for answering. |
@MikeRomaniuk The usecase does kind of matter, because we have a very specific usecase in mind (documented in my fosdem talk, though the recording is terrible - i should re-record that sometime). We want to do the absolute minimum we can do for our usecase, and this keeps the number of public APIs that will be needed small as well. The multipath spec does give you enough to make a largely interoperable protocol, but also still leaves a lot of things as essentially further research topics. The largest one is probably packet scheduling, also things like how to interoperate with the ack-frequency spec and probably a bunch more that escape me right now. So what you want to get out of this does matter in that regard. |
@flub, I might have understood your speech in the video wrong, so feel free to correct me. |
@MikeRomaniuk Yes, more or less. Our "typical" case is to establish the connection via a relayed path, we'll use an IPv6 private address space IP to identify paths via the relay and handle that in our impl of Quinn's AsyncUdpSockt trait. This path will be set as PATH_BACKUP. Then combine draft-seeman-quic-nat-traversal with multipath to open another path using real IP addresses and mark that path as PATH_AVAIALBE. This will give us very simple packet scheduling semantics, I think the multipath part of this it's about the simplest version of multipath you can build. Once we have this working stable and reliable we will probably start looking at more advanced packet scheduling, there are a lot of possible things, as you say maximise bandwith by using multiple paths, or interactions with lost packets etc. But that is much further down the line for us, we really want to have the simplest multipath first. |
@flub, thanks for clarification. |
@MikeRomaniuk We would like to get to a stage where we can schedule packets on multiple paths at the same time to increase bandwidth. But as I explained for us that's not a high priority and not yet on our roadmap. But that doesn't mean we wouldn't love it if someone else figured out what good behaviour and a nice API is there. So we'd totally accept contributions. (Though at this stage packet scheduling is still some way away.) I'm not sure what your goal is here, because tquic already has multipath and claims to care about performance (I have never tried it). Quinn is not the highest performant QUIC stack around so might be worth to benchmark even just single-path connections to see if they meet your goals. We also had other considerations like being able to use the Lastly I'd like to repeat what this repo says in the readme: we intent to upstream anything that's not a gross hack and would discourage anyone from depending on us directly because we may make changes not very nice to users. Just be aware of that. |
@flub I am very grateful for your explanations.
Honestly, I would rather use quinn over tquic, since it is widely used in the community and I believe will be a better choice in the longrun. I will need to talk to the team to take into account their opinion and I will let you if we would decide to proceed our way with you. |
@MikeRomaniuk Hello, I'm also working on an MPQUIC-based project (specifically for my bachelor's final year project), focusing on developing custom MPQUIC schedulers. I've found that the event-based You might be interested in @qdeconinck's fork of I'm curious about which direction you're planning to take! Would love to collaborate on this problem if you're interested. |
@shirok1 thanks for the recommendation! I am very glad that you are interested in QUIC and the problem I am about to tackle. However, I cannot invite you to participate, since this is not my personal project. |
We don't care about those platforms for now. We likely don't do platform-specific changes anyway.
merging main again, in a PR to check CI will be happy
This TransmitBuf is a wrapper around the buffer in which datagrams are being created. It keeps track of the state required to know the boundaries of the datagrams in the buffer.
This removes the extra arguments from Packetbuilder::new that tell the builder about datagram boundaries in favour of using the state of TransmitBuf directly.
This moves the logic of updating the buffer for new datagrams into a function on the TransmitBuf. This centralises all the manipulation of these variables into one logical place, ensuring all the invariants are upheld.
This helps encapsulation by ensuring that no outside users can mutate these fields. Reducing the amount of logic that needs to be reasoned about.
We want to hide the actual buffer implementation from most of these locations. Currently it is a TransmitBuf but it likely will become something else in the future. So make this generic.
This allows making TransmitBuf::buf private at last. Now all the logic it handles is fully encapsulated.
This re-arranges the loop in poll_transmit to always finish the packet before going to the next iteration. This primarily enables to mutably borrow the TransmitBuf into a packet-specific buffer while the packet is being built. But this is not yet utilised in this commit. It does however remove the need of the mutable builder_storage Option, which makes reasoning over packet building slightly easier. - The logic to know on which packet space to send next, or whether there is no longer anything to send, has been moved to the next_send_space method. - The logic to decide whether to pad a packet before finishing it is moved to the end of the loop. - The logic to check the congestion controller and pacing is kept at the start of the loop. Before a new packet is started. Starting a new datagram also stays there.
Now that the packet is always finished at the end of the loop we no longer need to carry around the mutable SentFrames. Reducing further the number of things to keep track of.
The PacketBuilder::finish_and_track function took a SentFrames argument as an Option. However this was an artifact of the poll_transmit loop tracking it in an Option before, and it not being clear this was always Some when things were going correctly. Now all the callers clearly always pass this in so we can remove the Option.
Now the lifetimes allow for this the PacketBuilder can own the TransmitBuf. This is gives it more control over the buffer into which the packet can be written. This commit itself does nothing interesting with this yet. It merely moves the buffer ownership in a mechanical way. However, this enables future changes to reduce the use of offsets in so many places.
This comment being in the wrong place makes reading this code a bit confusing.
The number of lost packets is already tracked as path stats, there is no need to duplicate it on the connection struct itself. While access was only ever in tests, the field would still be present in release builds I think.
Bumps [serde_json](https://github.com/serde-rs/json) from 1.0.140 to 1.0.141. - [Release notes](https://github.com/serde-rs/json/releases) - [Commits](serde-rs/json@v1.0.140...v1.0.141) --- updated-dependencies: - dependency-name: serde_json dependency-version: 1.0.141 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [async-io](https://github.com/smol-rs/async-io) from 2.4.1 to 2.5.0. - [Release notes](https://github.com/smol-rs/async-io/releases) - [Changelog](https://github.com/smol-rs/async-io/blob/master/CHANGELOG.md) - [Commits](smol-rs/async-io@v2.4.1...v2.5.0) --- updated-dependencies: - dependency-name: async-io dependency-version: 2.5.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [rcgen](https://github.com/rustls/rcgen) from 0.14.2 to 0.14.3. - [Release notes](https://github.com/rustls/rcgen/releases) - [Commits](rustls/rcgen@v0.14.2...v0.14.3) --- updated-dependencies: - dependency-name: rcgen dependency-version: 0.14.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
Necessary for qlog streams shared between concurrent connections to be intelligible.
draft-ietf-quic-qlog-quic-events-11 specifies bits per second.
* Allow opening path only if it does not yet exists * Hook this up to futures All this is terrible. * clippy
* Immediately send ACKs for abandoned paths This is required by § 3.4.3 from QUIC-MULTIPATH. * Account for lost packets when dropping the path state
* relly on existing paths for validation * still send the challenge * check the server gets the event too now that they validate the address as well * remove path from frame spacn This is already part of the spans that contain this one * Set validation timer for new paths * make sure validation failure removes the path * check server events for failed validation test * add client side failure test * fix comment * better docs * add docs for the blackhole step * set the previous log behaviour as default
…o multipath-quinn-0.11.x
actually merge quinn main into an old commit of the multipath branch. Then merge that old commit into the current head of the multipath branch. Life is messy, and git doubly so.
* set timer when setting the path challenge, still set the timer even if the path is validated * modify comments about path challenge timeouts * actually stop timer
Once we have validated the server's address during the handshake we should no longer allow the serers's address to migrate. Before that we're just accepting any migrations. This also adds the logic to consider the server's address validated in a retry packet if possible.
To support sending initial packets to many destinations and accepting the first one received back, we need to allow the server address to migrate. Once we have validated the server's address during the handshake we no longer allow the server's address to migrate. This also adds the logic to consider the server's address validated in a retry packet if possible.
No description provided.