-
Notifications
You must be signed in to change notification settings - Fork 993
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
refactor(quic): rewrite quic using quinn #3454
Merged
Merged
Changes from 64 commits
Commits
Show all changes
83 commits
Select commit
Hold shift + click to select a range
8bd903d
Init (it compiles)
kpp 8bce874
Implement fn dial
kpp 2fa3408
Handle quic version
kpp 22b7944
Implement tokio/async-std switch
kpp 3c77392
Fix one error handling
kpp 729ebf9
Revisit todo
kpp ff2dc4b
Fix one error handling
kpp a288c97
Reduce the number of warnings
kpp a5493a3
Remove a lot of commented code
kpp b781c87
Merge branch 'master' into quic_quinn
kpp ce2403d
Remove quinn-proto dep
kpp b1560dd
Remove redundant Arcs
kpp 8171862
Add more comments
kpp dd394db
Merge branch 'master' into quic_quinn
kpp a1789af
More comments
kpp b46feb7
More comments
kpp b947b12
Remove unused commented code
kpp 0bce473
Fix old tests to match new structs
kpp 26ac662
Merge branch 'master' into quic_quinn
kpp 7e788b6
Rename endpoint.rs -> config.rs
kpp 953ce83
Fix doc links
kpp df0f789
cargo fmt
kpp e8cbd5a
Merge branch 'master' into quic_quinn
kpp ee7c2cd
Merge branch 'master' into quic_quinn
kpp ab1c047
Review fixes
kpp 54606bf
Review fixes
kpp bb5b0f9
Merge branch 'master' into quic_quinn
kpp 32f0c08
Merge branch 'master' of https://github.com/libp2p/rust-libp2p into q…
mxinden 808a6b4
Make incoming and outgoing futures in connection an Option<_>
kpp ce30bcb
Update transports/quic/src/connection.rs
kpp 6e9be7b
Merge branch 'master' into quic_quinn
kpp 11bf960
Refactor poll_close for Substream
kpp 45f7222
Merge branch 'master' into quic_quinn
kpp 2f6e719
cargo fmt
kpp 2a93d21
Remove quinn structs from public API
kpp 2826b7c
Merge branch 'master' into quic_quinn
kpp ffc7767
Merge branch 'master' into quic_quinn
thomaseizinger c357f73
Remove quinn structs from public Runtime API
kpp 2a1948c
Remove thiserror::from impl for error types
kpp 9158f48
Merge branch 'master' into quic_quinn
kpp 9cd937b
Merge branch 'master' of https://github.com/libp2p/rust-libp2p into q…
mxinden 34d15cf
Merge branch 'master' into quic_quinn
kpp ae0b427
Fix merge
kpp e507e44
Implement hole punching with SO_REUSEPORT
kpp c48b8e6
fix clippy
kpp af684ce
Use try_clone UdpSocket instead of PORT_REUSE
kpp 608e94d
Update transports/quic/src/config.rs
kpp 78f3e3c
Explain why Listener::socket_addr cannot fail
kpp be97ffe
Simplify calls to .poll/.poll_unpin
kpp d4de2d5
Rename Substream -> Stream
kpp 5ee2e1f
Merge branch 'master' into quic_quinn
kpp 686ec3f
Fix merge
kpp 0b49010
Remove duplicate code
kpp 18e7661
Merge branch 'master' into quic_quinn
kpp dcd1eda
Fix clippy
kpp 49a9e27
Fix fmt
kpp e90556d
Revert changes in eligible_listener to match master
kpp 769c6c1
Remove param need_if_watcher from Listener::new
kpp d4b8a58
Remove unused variable need_if_watcher
kpp 4a0d9bc
Fix poll_read/poll_close on a closed stream
kpp 038d00c
Fix dcutr example to bind to 127.0.0.1 instead of 0.0.0.0
kpp 08f58f9
Fix test read_after_peer_dropped_stream
kpp 33869b3
Merge branch 'master' into quic_quinn
kpp fd0187b
cargo fmt
kpp 459dedd
fix(quic): use Provider::send_to for UDP datagram
mxinden bae4983
Clone ErrorKind in Stream::poll_close
kpp dd619ca
cargo clippy
kpp f31da2e
Simplify and hide dependency types
mxinden 29c575e
Merge pull request #28 from mxinden/quic-quinn-send-to
kpp f019c7a
Merge branch 'master' into quic_quinn
kpp bde3252
fix(quic/stream): return error on read
mxinden 7505c65
refactor(quic/provider): remove Provider::spawn
mxinden d977cb5
fix(Cargo.lock): udpate
mxinden 16aa52b
Merge branch 'master' into quic_quinn
kpp 19ff4d9
Merge pull request #29 from mxinden/quic_quinn
kpp 8b65105
fix(quic/connection): await connection.closed
mxinden d275e85
Merge pull request #30 from mxinden/quinn-closed
kpp 0b08770
Revert back to 0.0.0.0 listen_on in dcutr example
mxinden b6bd51f
Bump version
mxinden 0a842b8
Merge branch 'master' of https://github.com/libp2p/rust-libp2p into q…
mxinden fccd979
Try hiding Provider trait
mxinden cfd147e
Fix doc comment
mxinden a92954a
Revert hiding Provider trait
mxinden File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
// Copyright 2017-2020 Parity Technologies (UK) Ltd. | ||
// | ||
// Permission is hereby granted, free of charge, to any person obtaining a | ||
// copy of this software and associated documentation files (the "Software"), | ||
// to deal in the Software without restriction, including without limitation | ||
// the rights to use, copy, modify, merge, publish, distribute, sublicense, | ||
// and/or sell copies of the Software, and to permit persons to whom the | ||
// Software is furnished to do so, subject to the following conditions: | ||
// | ||
// The above copyright notice and this permission notice shall be included in | ||
// all copies or substantial portions of the Software. | ||
// | ||
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS | ||
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING | ||
// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER | ||
// DEALINGS IN THE SOFTWARE. | ||
|
||
use quinn::VarInt; | ||
use std::{sync::Arc, time::Duration}; | ||
|
||
/// Config for the transport. | ||
#[derive(Clone)] | ||
pub struct Config { | ||
/// Timeout for the initial handshake when establishing a connection. | ||
/// The actual timeout is the minimum of this and the [`Config::max_idle_timeout`]. | ||
pub handshake_timeout: Duration, | ||
/// Maximum duration of inactivity in ms to accept before timing out the connection. | ||
pub max_idle_timeout: u32, | ||
/// Period of inactivity before sending a keep-alive packet. | ||
/// Must be set lower than the idle_timeout of both | ||
/// peers to be effective. | ||
/// | ||
/// See [`quinn::TransportConfig::keep_alive_interval`] for more | ||
/// info. | ||
pub keep_alive_interval: Duration, | ||
/// Maximum number of incoming bidirectional streams that may be open | ||
/// concurrently by the remote peer. | ||
pub max_concurrent_stream_limit: u32, | ||
|
||
/// Max unacknowledged data in bytes that may be send on a single stream. | ||
pub max_stream_data: u32, | ||
|
||
/// Max unacknowledged data in bytes that may be send in total on all streams | ||
/// of a connection. | ||
pub max_connection_data: u32, | ||
|
||
/// Support QUIC version draft-29 for dialing and listening. | ||
/// | ||
/// Per default only QUIC Version 1 / [`libp2p_core::multiaddr::Protocol::QuicV1`] | ||
/// is supported. | ||
/// | ||
/// If support for draft-29 is enabled servers support draft-29 and version 1 on all | ||
/// QUIC listening addresses. | ||
/// As client the version is chosen based on the remote's address. | ||
pub support_draft_29: bool, | ||
|
||
/// TLS client config for the inner [`quinn::ClientConfig`]. | ||
client_tls_config: Arc<rustls::ClientConfig>, | ||
/// TLS server config for the inner [`quinn::ServerConfig`]. | ||
server_tls_config: Arc<rustls::ServerConfig>, | ||
} | ||
|
||
impl Config { | ||
/// Creates a new configuration object with default values. | ||
pub fn new(keypair: &libp2p_identity::Keypair) -> Self { | ||
let client_tls_config = Arc::new(libp2p_tls::make_client_config(keypair, None).unwrap()); | ||
let server_tls_config = Arc::new(libp2p_tls::make_server_config(keypair).unwrap()); | ||
Self { | ||
client_tls_config, | ||
server_tls_config, | ||
support_draft_29: false, | ||
handshake_timeout: Duration::from_secs(5), | ||
max_idle_timeout: 30 * 1000, | ||
max_concurrent_stream_limit: 256, | ||
keep_alive_interval: Duration::from_secs(15), | ||
max_connection_data: 15_000_000, | ||
|
||
// Ensure that one stream is not consuming the whole connection. | ||
max_stream_data: 10_000_000, | ||
} | ||
} | ||
} | ||
|
||
/// Represents the inner configuration for [`quinn`]. | ||
#[derive(Debug, Clone)] | ||
pub(crate) struct QuinnConfig { | ||
pub(crate) client_config: quinn::ClientConfig, | ||
pub(crate) server_config: quinn::ServerConfig, | ||
pub(crate) endpoint_config: quinn::EndpointConfig, | ||
} | ||
|
||
impl From<Config> for QuinnConfig { | ||
fn from(config: Config) -> QuinnConfig { | ||
let Config { | ||
client_tls_config, | ||
server_tls_config, | ||
max_idle_timeout, | ||
max_concurrent_stream_limit, | ||
keep_alive_interval, | ||
max_connection_data, | ||
max_stream_data, | ||
support_draft_29, | ||
handshake_timeout: _, | ||
} = config; | ||
let mut transport = quinn::TransportConfig::default(); | ||
// Disable uni-directional streams. | ||
transport.max_concurrent_uni_streams(0u32.into()); | ||
transport.max_concurrent_bidi_streams(max_concurrent_stream_limit.into()); | ||
// Disable datagrams. | ||
transport.datagram_receive_buffer_size(None); | ||
transport.keep_alive_interval(Some(keep_alive_interval)); | ||
transport.max_idle_timeout(Some(VarInt::from_u32(max_idle_timeout).into())); | ||
transport.allow_spin(false); | ||
transport.stream_receive_window(max_stream_data.into()); | ||
transport.receive_window(max_connection_data.into()); | ||
let transport = Arc::new(transport); | ||
|
||
let mut server_config = quinn::ServerConfig::with_crypto(server_tls_config); | ||
server_config.transport = Arc::clone(&transport); | ||
// Disables connection migration. | ||
// Long-term this should be enabled, however we then need to handle address change | ||
// on connections in the `Connection`. | ||
server_config.migration(false); | ||
|
||
let mut client_config = quinn::ClientConfig::new(client_tls_config); | ||
client_config.transport_config(transport); | ||
|
||
let mut endpoint_config = quinn::EndpointConfig::default(); | ||
if !support_draft_29 { | ||
endpoint_config.supported_versions(vec![1]); | ||
} | ||
|
||
QuinnConfig { | ||
client_config, | ||
server_config, | ||
endpoint_config, | ||
} | ||
} | ||
} |
Oops, something went wrong.
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.
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.
You said this is only because of how we implemented
eligible_listener
right?But if you only listen on localhost, then this example is sort of use-less. Hole-punching on localhost doesn't make much sense and if we don't listen on all interfaces, you can't actually use this example to test hole-punching.
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.
But it does not work on 0.0.0.0 either because of how
eligible_listener
is implemented.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.
In such case the
eligible_listener
implementation needs to be changed, no?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.
Frankly speaking I don't know what purpose serves that
listen_addr.ip().is_loopback() == socket_addr.ip().is_loopback()
. It was introduced in 1a4b8ac#diff-b1dcda9367774dd8a742457ff79bc528d030a8cc38d6791efefc78b30f3f2e03R141There 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.
My guess is that it forces that the local addr of an "outgoing" packet with destination localhost uses a localhost listening socket, if present.
I believe @elenaf9 originally implemented this, maybe she can shed some light on this :)
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.
@mxinden That's weird we can hole punch
!is_loopback()
remote addrs with0.0.0.0
but we can't punch127.0.0.1
addrs. Maybe we should remove thislisten_addr.ip().is_loopback() == socket_addr.ip().is_loopback()
check?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.
Why would that be weird?
127.0.0.1
is a localhost only address. If the node is listening on localhost only, how should a remote node establish a connection to it, let alone establish a connection to it through a NAT or firewall?I don't understand the issue at hand. Nor do I understand how removing this line would fix something. Can you expand @kpp? If we want to dial a node on loopback, i.e. on localhost (
socket_addr.ip().is_loopback()
istrue
), then we want to choose a listener that is listening on loopback, i.e. localhost (listen_addr.ip().is_loopback()
istrue
).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 issue is simple, we have have the following setup:
And it does not work without my patch, it fails with "no listeners" error. First of all, this code does not work on master and I am pretty surprised. Do you thing the setup above is wrong? If yes, lets fix my setup. If it is a correct setup then how shall we make the code work?
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.
Thanks for being persistent here @kpp. This indeed seems to be a bug.
In TCP we track the specific listen addresses, even if the user calls
listen_on
with an unspecified address (e.g.0.0.0.0
). If the user callslisten_on
with a specific IP address i.e. not a wildcard like0.0.0.0
but e.g.127.0.0.1
we track (here register) the specific address right away.rust-libp2p/transports/tcp/src/lib.rs
Line 394 in f10f1a2
In case where the user calls
listen_on
with an unspecified address (e.g.0.0.0.0
) we depend onif-watch
, more specificallypoll_if_watch
to provide us with the concrete addresses:rust-libp2p/transports/tcp/src/lib.rs
Lines 666 to 682 in f10f1a2
Either way, we end up tracking specific listen addresses with port-reuse enabled and can thus choose the right listener in
local_dial_addr
:rust-libp2p/transports/tcp/src/lib.rs
Lines 122 to 151 in f10f1a2
In
libp2p-quic
we only track the initial address that the user provided on thelisten_on
call.rust-libp2p/transports/quic/src/endpoint.rs
Lines 170 to 178 in f10f1a2
This might be a wildcard address. Later on, when dialing a
127.0.0.1
address, the checklisten_addr.ip().is_loopback() == socket_addr.ip().is_loopback()
discards the listener with the unspecified listen address, thus a new dial-only socket is used. Using a new socket for dialing makes hole punching from the dcutr dialer to the dcutr listener impossible.How to move forward?
I am not sure simply removing the
listen_addr.ip().is_loopback() == socket_addr.ip().is_loopback()
check is the way to go. This check is still helpful. E.g. say we have two listeners, one on localhost, one not on localhost. Ideally we would use the former when dialing.We could mirror what
libp2p-tcp
does. That is, track the specified listen addresses reported by if-watch. Later, when dialing, choose the listener with the right address.Ideally I would like to have the operating system choose. It has the most information about all available routes to each interface.
For now, I suggest we go with the
libp2p-tcp
approach.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.
Thinking about it some more, this is not a bug introduced in this pull request, but instead an already existing bug in our
libp2p-quic
implementation. With that in mind, I don't think this pull request needs to block on a fix for it.I documented it in #4259 instead. Let's revert the
dcutr/main.rs
change in this pull request and tackle the root issue in a follow-up pull request.