feat(quic): Add draft-29 support #3151

elenaf9 · 2022-11-21T15:51:14Z

Description

Add support for QUIC draft-29 / the quic codepoint. This enables both dialing and listening on quic addresses.
The motivation for adding support is to allow users to connect to old go-libp2p nodes that don't support the quic-v1 codepoint.
Per default support is disabled (cc @marten-seemann).

Links to any relevant issues

See discussion in #3133.

Open Questions

We use the same codepoint when reporting our listening addresses as address had that the user called listen_on on.
If users want to report listening addresses for both codepoints they either have to manually duplicate and map each listening address, or create two listeners.
The alternative would be to directly report listening addresses for both codepoints if draft-29 is enabled. I don't have a strong opinion, but I am leaning towards the current implementation to avoid reporting draft-29 addresses unless it is explicitly wanted by the user.

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

thomaseizinger

Not too invasive so this looks okay to add. We may want to document that this will be removed in the future.

elenaf9 · 2022-11-23T17:31:49Z

Not too invasive so this looks okay to add. We may want to document that this will be removed in the future.

Another option would be to deprecate Protocol::Quic in https://github.com/multiformats/rust-multiaddr.

…t-29

elenaf9 · 2022-11-25T01:21:07Z

Would love to get this into #3163 if it gets a review in time and there are no major change requests, but we don't have to block the release on it.

mxinden · 2022-11-25T08:23:32Z

~~@elenaf9 this is a non-breaking change, right? Mind if we release this as libp2p-quic v0.1.1-alpha then?~~

Never mind. It adds a pub field to a pub struct.

mergify · 2022-11-25T09:06:47Z

This pull request has merge conflicts. Could you please resolve them @elenaf9? 🙏

elenaf9 · 2022-11-25T09:14:05Z

~~@elenaf9 this is a non-breaking change, right? Mind if we release this as libp2p-quic v0.1.1-alpha then?~~

Never mind. It adds a pub field to a pub struct.

I am not 100% sure, but is it really a breaking change adding a pub field to a pub struct if that struct also has other private fields anyway (in our case server_tls_config and client_tls_config)? Deconstructions like let quic::Config { handshake_timeout, .. } = config are non-exhaustive anyway due to inaccessible fields I think.

mxinden

This looks good to me. Two suggestions, none of them blocking.

transports/quic/src/endpoint.rs

transports/quic/src/transport.rs

mxinden · 2022-11-25T15:37:03Z

~~@elenaf9 this is a non-breaking change, right? Mind if we release this as libp2p-quic v0.1.1-alpha then?~~
Never mind. It adds a pub field to a pub struct.

I am not 100% sure, but is it really a breaking change adding a pub field to a pub struct if that struct also has other private fields anyway (in our case server_tls_config and client_tls_config)? Deconstructions like let quic::Config { handshake_timeout, .. } = config are non-exhaustive anyway due to inaccessible fields I think.

Good point. I suggest following whatever cargo semver-checks on our CI says.

As far as I can tell, this is only time critical for hole punching month in December. I am fine with either of:

Releasing this as a breaking change.
Releasing this as a non-breaking change (see cargo semver-checks comment above).
Using an unreleased version in the Rust punchr client for hole punching month.

…t-29

thomaseizinger

LGTM

Only a few nits!

transports/quic/src/transport.rs

thomaseizinger · 2022-11-29T23:44:58Z

transports/quic/tests/smoke.rs

+        let _ = d_transport.poll_next_unpin(cx);
+        Poll::Pending
+    });
+    select! {


@mxinden converted me to prefer the select fn. Can we convert you too? :)

Can you point me to the discussion about the reasoning behind this? Right now I have a slight preference for the macro because of the readability.

Basically the rationale is:

Plays better with IDEs

Much easier to understand

Clear type-safety in how to handle the various outcomes

No fuse needed

No magic syntax to learn

I don't know where exactly we had this discussion, so can't link you to it unfortunately.

While I very much believe in the cause @thomaseizinger, I suggest we delay further select-evangelizing towards @elenaf9 and others to future pull request. In other words, I don't think we need to block here. (Also gives us more time to print t-shirts and stickers.)

thomaseizinger · 2022-11-29T23:47:12Z

transports/quic/tests/smoke.rs

+    let a_quic_mapped_addr = a_quic_addr
+        .clone()
+        .into_iter()
+        .map(|p| {
+            if matches!(p, Protocol::QuicV1) {
+                Protocol::Quic
+            } else {
+                p
+            }
+        })
+        .collect();


This snippet is repeated 3 times in the test. Can we make a little swap_protocol utility so these are one-liners?

Wdyt of 4cd494d?

transports/quic/src/transport.rs

thomaseizinger · 2022-11-29T23:53:13Z

~~@elenaf9 this is a non-breaking change, right? Mind if we release this as libp2p-quic v0.1.1-alpha then?~~

Never mind. It adds a pub field to a pub struct.

I am not 100% sure, but is it really a breaking change adding a pub field to a pub struct if that struct also has other private fields anyway (in our case server_tls_config and client_tls_config)? Deconstructions like let quic::Config { handshake_timeout, .. } = config are non-exhaustive anyway due to inaccessible fields I think.

We are currently on an "alpha" version. Anything is allowed there, i.e. a user doesn't get any guarantees (which is probably why we shouldn't export it by default from the root libp2p).

thomaseizinger · 2022-11-29T23:55:49Z

transports/quic/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "libp2p-quic"
-version = "0.7.0-alpha"
+version = "0.7.1-alpha"


I think the appropriate bump would be:

Suggested change

version = "0.7.1-alpha"

version = "0.7.0-alpha.2"

0.7.1-alpha suggests that a 0.7.0 exists and we are pre-releasing a patch version (although the non-breaking aspect of said patch version is not guaranteed, hence the alpha).

We haven't even released 0.7.0 so we may as well push more alpha's out until we are happy with it.

Ah yes you are right! Fixed in 2a83d3c. This also means that we don't need to bump the version in the libp2p crate.

Ah yes you are right! Fixed in 2a83d3c. This also means that we don't need to bump the version in the libp2p crate.

I think you will have to, I don't think alpha versions update automatically. AFAIK, they are basically major version bumps but with an "unstable" co-notation.

I could be wrong though, please test it :)

thomaseizinger · 2022-11-29T23:56:46Z

~~@elenaf9 this is a non-breaking change, right? Mind if we release this as libp2p-quic v0.1.1-alpha then?~~

Never mind. It adds a pub field to a pub struct.

I am not 100% sure, but is it really a breaking change adding a pub field to a pub struct if that struct also has other private fields anyway (in our case server_tls_config and client_tls_config)? Deconstructions like let quic::Config { handshake_timeout, .. } = config are non-exhaustive anyway due to inaccessible fields I think.

We are currently on an "alpha" version. Anything is allowed there, i.e. a user doesn't get any guarantees (which is probably why we shouldn't export it by default from the root libp2p).

Note that the breaking/non-breaking aspect still matters for the re-exported interface in libp2p.

Change version to 0.7.0-alpha.2 rather than 0.7.1-alpha. The latter wrongly implies that we are pre-releasing 0.7.1. Revert version bump and changelog entry in `libp2p` crate.

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

elenaf9 · 2022-11-30T12:41:22Z

The semver check in our CI passes.
Also, the Rust API evolution RFC only defines adding a public field as major (= breaking) change if no private fields exist on the struct: https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md#major-change-adding-a-public-field-when-no-private-field-exists.
Thus I think we can release this as a new alpha release 0.7.0-alpha.2

mxinden

Changes and version handling looks good to me.

mxinden · 2022-12-02T14:09:26Z

Interpreting @thomaseizinger approval as still valid.

mxinden · 2022-12-02T19:42:09Z

libp2p-dcutr and libp2p-relay failures are unrelated and handled in #2647 (comment). Moving forward here.

feat(quic): Add draft-29 support

122f370

mxinden modified the milestone: v0.50.0 Nov 22, 2022

thomaseizinger reviewed Nov 22, 2022

View reviewed changes

elenaf9 mentioned this pull request Nov 23, 2022

quic: Add support for quic codepoint , interpreted as QUIC version draft-29 #3133

Closed

elenaf9 marked this pull request as ready for review November 23, 2022 17:28

elenaf9 added 2 commits November 25, 2022 00:36

docs(quic): add note on draft-29 support removal

bbed9c0

Merge branch 'master' of github.com:libp2p/rust-libp2p into quic/draf…

415777d

…t-29

elenaf9 requested a review from thomaseizinger November 24, 2022 23:38

mxinden approved these changes Nov 25, 2022

View reviewed changes

transports/quic/src/endpoint.rs Outdated Show resolved Hide resolved

transports/quic/src/transport.rs Show resolved Hide resolved

elenaf9 added 4 commits November 28, 2022 18:50

Merge branch 'master' of github.com:libp2p/rust-libp2p into quic/draf…

14b4cb4

…t-29

chore: bump version, add changelog

85bafc1

feat(quic): add QuicVersion enum

1fc87da

doc(quic): link to issue about quic codepoint

fa39fc6

elenaf9 mentioned this pull request Nov 29, 2022

rust-client: Add support for QUIC transport libp2p/punchr#52

Merged

thomaseizinger approved these changes Nov 29, 2022

View reviewed changes

thomaseizinger reviewed Nov 29, 2022

View reviewed changes

elenaf9 added 5 commits November 30, 2022 12:44

fix(quic): fix copy-pase errors in smoke test

4ebb4f6

feat(quic): add swap_proto macro in smoke test

4cd494d

refactor(quic): rename QuicVersion => ProtocolVersion

8f05e09

chore(quic): relase patch as 0.7.0-alpha.2

2a83d3c

Change version to 0.7.0-alpha.2 rather than 0.7.1-alpha. The latter wrongly implies that we are pre-releasing 0.7.1. Revert version bump and changelog entry in `libp2p` crate.

fix(quic): fix version in CHANGELOG entry

b0dab3b

doc(quic): apply suggestions from review

885bbca

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

Merge branch 'master' into quic/draft-29

e09a860

elenaf9 requested review from thomaseizinger and mxinden November 30, 2022 12:41

mxinden approved these changes Dec 2, 2022

View reviewed changes

mxinden added the send-it label Dec 2, 2022

Merge branch 'master' into quic/draft-29

2204f79

mxinden merged commit dfc5ec8 into libp2p:master Dec 2, 2022

elenaf9 deleted the quic/draft-29 branch December 3, 2022 00:06

mxinden mentioned this pull request Dec 28, 2022

fix(quic): Identify /quic as QUIC address #3288

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(quic): Add draft-29 support #3151

feat(quic): Add draft-29 support #3151

elenaf9 commented Nov 21, 2022 •

edited

Loading

thomaseizinger left a comment

elenaf9 commented Nov 23, 2022

elenaf9 commented Nov 25, 2022

mxinden commented Nov 25, 2022 •

edited

Loading

mergify bot commented Nov 25, 2022

elenaf9 commented Nov 25, 2022 •

edited

Loading

mxinden left a comment

mxinden commented Nov 25, 2022

thomaseizinger left a comment

thomaseizinger Nov 29, 2022

elenaf9 Nov 30, 2022 •

edited

Loading

thomaseizinger Nov 30, 2022

mxinden Dec 2, 2022

thomaseizinger Nov 29, 2022

elenaf9 Nov 30, 2022

thomaseizinger commented Nov 29, 2022

thomaseizinger Nov 29, 2022 •

edited

Loading

elenaf9 Nov 30, 2022

thomaseizinger Nov 30, 2022

thomaseizinger Nov 30, 2022

thomaseizinger commented Nov 29, 2022

elenaf9 commented Nov 30, 2022

mxinden left a comment

mxinden commented Dec 2, 2022

mxinden commented Dec 2, 2022

feat(quic): Add draft-29 support #3151

feat(quic): Add draft-29 support #3151

Conversation

elenaf9 commented Nov 21, 2022 • edited Loading

Description

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

elenaf9 commented Nov 23, 2022

elenaf9 commented Nov 25, 2022

mxinden commented Nov 25, 2022 • edited Loading

mergify bot commented Nov 25, 2022

elenaf9 commented Nov 25, 2022 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Nov 25, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elenaf9 Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Nov 29, 2022

thomaseizinger Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Nov 29, 2022

elenaf9 commented Nov 30, 2022

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Dec 2, 2022

mxinden commented Dec 2, 2022

elenaf9 commented Nov 21, 2022 •

edited

Loading

mxinden commented Nov 25, 2022 •

edited

Loading

elenaf9 commented Nov 25, 2022 •

edited

Loading

elenaf9 Nov 30, 2022 •

edited

Loading

thomaseizinger Nov 29, 2022 •

edited

Loading