-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
add QUIC support #5350
add QUIC support #5350
Conversation
da95d79
to
7c3fe23
Compare
|
||
- [ ] The IETF QUIC specification needs to be finalised. | ||
- [ ] Make sure QUIC connections work reliably | ||
- [ ] Make sure QUIC connection offer equal or better performance than TCP connections on real world networks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finalize libp2p-TLS handshake spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
7c3fe23
to
4e40f97
Compare
This is blocked, right? |
Yes, we need to merge a few gx publishes. |
4e40f97
to
01e460b
Compare
@Stebalien, @Kubuxu: This PR is ready for merging now. Please merge ipfs/go-ipfs-config#4 before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to get a sharness test or two, other than that looks good
520157a
to
adc5692
Compare
@magik6k: Thanks for the hint. I wasn't aware of the sharness tests before. This is exactly the kind of test we should have to test that we don't accidentally break QUIC support in the future. |
@marten-seemann looks like one of the tests is hanging. |
Actually, it would be kind of nice to have a "twonode" test. Just add a variant that uses quic to the twonode sharness test. |
test/sharness/t0190-quic-ping.sh
Outdated
' | ||
|
||
test_expect_success "test ping self" ' | ||
! ipfsi 0 ping -n2 -- "$PEERID_0" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The standard way to check that a command fails in sharness is test_must_fail ipfsi 0 ping -n2 -- "$PEERID_0"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. I just copied it from the ping test: https://github.com/ipfs/go-ipfs/blob/93c4f19169e0570adfb35fb6e9420d414c64a733/test/sharness/t0041-ping.sh#L26-L34
adc5692
to
6b45fc3
Compare
@Stebalien: I just added a copy of the two-node test that uses QUIC. PHAL. Note that this test is currently failing, which is due to this bug in quic-go: quic-go/quic-go#1499. I'll cherry-pick the fix for this issue once it is reviewed, and bubble up the fix through gx. |
ba249e9
to
6169394
Compare
I rebased everything to include the current version of quic-go. |
I'll do that once merge some stuff (keep needing to rebase PRs). |
License: MIT Signed-off-by: Marten Seemann <martenseemann@gmail.com>
License: MIT Signed-off-by: Marten Seemann <martenseemann@gmail.com>
6169394
to
35bef6f
Compare
@Kubuxu: Can you review this please? This should be ready for merge now. |
gx renders aside, this change looks good to me. |
test/sharness/t0126-twonode-quic.sh
Outdated
@@ -0,0 +1,109 @@ | |||
#!/usr/bin/env bash | |||
|
|||
test_description="Test two ipfs nodes transferring a file via QUIC" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just add a case to the existing twonode test. It's designed for multiple variants (just run run_advanced_test
again after configuring the nodes to use QUIC).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried about the test output. By putting it in one file, it's hard to see which test exactly is failing. I added some more logging, so that shouldn't be a problem anymore.
gx update bonanza done. |
e877fea
to
d85b4eb
Compare
I need to pull through a multistream update to get this to work. |
License: MIT Signed-off-by: Marten Seemann <martenseemann@gmail.com>
License: MIT Signed-off-by: Marten Seemann <martenseemann@gmail.com>
d85b4eb
to
f07816f
Compare
License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>
ci seems unhappy about deps |
It keeps complaining about not being able to fetch deps it really should be able to fetch. |
We have liftoff! 🎉 🚀 🎉 |
Fixes #5343.