Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add experimental support for QUIC #11514

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Add experimental support for QUIC #11514

wants to merge 26 commits into from

Conversation

kpp
Copy link
Contributor

@kpp kpp commented May 24, 2022 •
edited by dmitry-markin
Loading

This is a follow-up of #6366

From the point of view of Substrate:

  • We now open a UDP socket only if --experimental-quic CLI option is set to true (with default value = false).
  • This UDP socket is used both to accept incoming QUIC connections but also to establish connections to the respective UDP socket of other nodes. In other words, all communications go through this single socket.
  • At the moment, since support is experimental, we refuse all incoming QUIC connections by default. One can listen on incoming QUIC connections by passing --listen-addr /ip4/0.0.0.0/udp/30333/quic-v1. Note that multiaddr should listen on quic-v1 not on quic, otherwise the listening will fail.
  • There is a small caveat at the moment: if you spawn two nodes on the same machine the same UDP port, one of them will fail with "Address already in use". Contrary to TCP/IP, the consequence is that the node which failed to open the UDP port will also be unable to dial the QUIC addresses of other nodes. This is pretty annoying, and I'd be in favour of automatically trying multiple ports if the default one fails to open, but this needs to be brainstormed because of the interaction with --listen-addr.

Before we can consider merging, we need to:

So far I managed to connect two nodes through QUIC:

  • cargo r --bin node-template -- --no-mdns --experimental-quic --listen-addr /ip6/::1/tcp/8081 /ip6/::1/udp/8081/quic-v1 --alice --tmp
  • cargo r --bin node-template -- --no-mdns --experimental-quic --listen-addr /ip4/0.0.0.0/udp/8080/quic-v1 --bootnodes /ip6/::1/udp/8081/quic-v1/p2p/12D3KooWRRNUr42JBC79TuiWSg3BnAcsNwjT6zasJsCrcB5bgaxK --bob --tmp

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label May 24, 2022
@kpp kpp mentioned this pull request May 24, 2022
Open
10 tasks
@kpp @maxwase
Review fixes: ipv6 :: addr
4d61feb 
Co-authored-by: Max Wase <max.vvase@gmail.com>
@cla-bot-2021
Copy link

cla-bot-2021 bot commented May 26, 2022

User @maxwase, please sign the CLA here.

@kpp kpp added B0-silent Changes should not be mentioned in any release notes D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels May 26, 2022
@stale
Copy link

stale bot commented Jun 25, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jun 25, 2022
@kpp
Copy link
Contributor Author

kpp commented Jun 25, 2022

I am working on this.

@stale stale bot closed this Jul 9, 2022
@kpp kpp removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 12, 2022
@kpp kpp reopened this Jul 12, 2022
@stale
Copy link

stale bot commented Aug 31, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 31, 2022
@kpp
Copy link
Contributor Author

kpp commented Aug 31, 2022

I am working on this. This is a very important issue.

@kpp kpp removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 31, 2022
@crystalin
Copy link
Contributor

@kpp can you give us an update? I agree this is important specially considering the issues with networking/parachains at the moment

@kpp
Copy link
Contributor Author

kpp commented Aug 31, 2022

So we got 2 working implementations: libp2p/rust-libp2p#2289 and libp2p/rust-libp2p#2801. Both of them are compatible with the go implementation but none of them are merged.

@stale
Copy link

stale bot commented Sep 30, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 30, 2022
@kpp
Copy link
Contributor Author

kpp commented Oct 1, 2022

I am working on this issue

@kpp kpp removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 1, 2022
@kpp kpp added the C1-low PR touches the given topic and has a low impact on builders. label Feb 1, 2023
dmitry-markin
dmitry-markin approved these changes Feb 3, 2023
View reviewed changes
Copy link
Contributor

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

client/network/src/transport.rs Outdated Show resolved Hide resolved
client/network/src/transport.rs
.timeout(Duration::from_secs(20))
.timeout(Duration::from_secs(20));

let quic_transport = if enable_quic {
Copy link
Contributor

@dmitry-markin dmitry-markin Feb 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the either way is OK.

@the-right-joyce the-right-joyce added B1-note_worthy Changes should be noted in the release notes T0-node This PR/Issue is related to the topic “node”. and removed B5-clientnoteworthy labels Feb 13, 2023
@stale
Copy link

stale bot commented Mar 16, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Mar 16, 2023
melekes
melekes reviewed Mar 16, 2023
View reviewed changes
client/network/src/transport.rs Outdated Show resolved Hide resolved
client/network/src/transport.rs Outdated Show resolved Hide resolved
@stale stale bot removed A3-stale labels Mar 16, 2023
@stale
Copy link

stale bot commented Apr 15, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Apr 15, 2023
@kpp @melekes
Update client/network/src/transport.rs
fcc4b51 
Co-authored-by: Anton <anton.kalyaev@gmail.com>
@kamilsa kamilsa mentioned this pull request May 3, 2023
Open
1 task
@stale
Copy link

stale bot commented May 25, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label May 25, 2023
@stale stale bot closed this Jun 9, 2023
@crystalin
Copy link
Contributor

:(

@kpp kpp reopened this Jun 14, 2023
@stale stale bot removed the A3-stale label Jun 14, 2023
@stale
Copy link

stale bot commented Jul 14, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Jul 14, 2023
@stale
Copy link

stale bot commented Aug 21, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Aug 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maxwase maxwase maxwase left review comments

@melekes melekes melekes left review comments

@dmitry-markin dmitry-markin dmitry-markin approved these changes

@bkchr bkchr Awaiting requested review from bkchr

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T0-node This PR/Issue is related to the topic “node”.
Projects
Networking
Status: Blocked ⛔️
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kpp @crystalin @dmitry-markin @melekes @maxwase @altonen @the-right-joyce