feat: quic,webtransport: enable both quic-draft29 and quic-v1 addrs on quic. only quic-v1 on webtransport #1881
Conversation
There was a problem hiding this comment.
This turned out to be a much larger change than I expected. Good work here.
Contrary to what the PR title suggests, we're not advertising draft-29 for WebTransport, right? This is the reasonable thing to do. We don't need draft-29 to support WebTransport (it's unclear if it's even RFC-compliant to run WebTransport over draft versions).
| rcmgr network.ResourceManager | ||
| privKey ic.PrivKey | ||
| localPeer peer.ID | ||
| localMultiaddrs map[quic.VersionNumber]ma.Multiaddr |
There was a problem hiding this comment.
Does a map make sense here? Alternatively, we could just have 2 member variables (localMultiaddrDraft29 and localMultiaddrQUICv1).
I don't expect us to add any more QUIC versions here any time soon (the opposite actually, we'll be dropping draft-29 long before we'll have a new incompatible QUIC version).
There was a problem hiding this comment.
It's a bit cleaner when we access them: https://github.com/libp2p/go-libp2p/blob/marco/quic-v1/p2p/transport/quic/listener.go#L143
Otherwise we'd have to branch and update a variable. Not really a problem, just a bit less tidy imo. I don't have a strong opinion here so if you feel strongly we can use two vars.
There was a problem hiding this comment.
No strong opinion either, but using two vars would make the return order of LocalMultiAddr deterministic, which would be nice (see #1881 (comment)).
| } | ||
|
|
||
| func fromQuicMultiaddr(addr ma.Multiaddr) (net.Addr, error) { | ||
| return manet.ToNetAddr(addr.Decapsulate(quicMA)) | ||
| func fromQuicMultiaddr(addr ma.Multiaddr) (net.Addr, quic.VersionNumber, error) { |
There was a problem hiding this comment.
Alternatively (might loop through the multiaddr twice, but easier to read):
func fromQuicMultiaddr(addr ma.Multiaddr) (net.Addr, quic.VersionNumber, error) {
if _, err := addr.ValueForProtocol(ma.P_QUIC); err == nil {
return manet.ToNetAddr(addr.Decapsulate(quicV1MA))
}
if _, err := addr.ValueForProtocol(ma.P_QUIC_V1); err == nil {
return manet.ToNetAddr(addr.Decapsulate(quicDraft29MA))
}
return errors.New("not a QUIC multiaddr")
}There was a problem hiding this comment.
Preference for keeping this such that we iterate the multiaddr fewer times. (I've even made a change to join the multiaddrs once at the end instead of each step).
Do you feel strongly here?
There was a problem hiding this comment.
In general, it feels "wrong" to do that much multiaddr magic in libp2p packages. Maybe the API we expose in multiaddr is insufficient? Or are we over-optimizing things here?
Anyway, no strong feelings, I'm ok with keeping this as is, just something to think about.
There was a problem hiding this comment.
Maybe the API we expose in multiaddr is insufficient
I think so. I think there are some things we can add that would replace 80% of our uses of ForEach.
MarcoPolo
changed the title
MarcoPolo
force-pushed
the
marco/quic-v1
branch
from
5099616
to
1ace1e9
Compare
| ConnsInbound: 4, | ||
| ConnsOutbound: 8, | ||
| ConnsInbound: 6, | ||
| ConnsOutbound: 6, |
There was a problem hiding this comment.
Why are you modifying the limits here?
There was a problem hiding this comment.
A bit more info in the commit: a969c6d
We see flakiness in CI because a client will dial with concurrency 8 but the limit on the server is 4 so we get rcmgr failures. We notice this when we have a lot of addrs to dial
| // can optionally implement if they want to filter the multiaddrs. | ||
| // | ||
| // This mutates the input | ||
| func maybeRemoveQUICDraft29(addrs []ma.Multiaddr) []ma.Multiaddr { |
|
@MarcoPolo I just released quic-go v0.31.0. Let's get #1882 merged and then rebase this PR? |
MarcoPolo
force-pushed
the
marco/quic-v1
branch
from
655117d
to
1a51d4b
Compare
Co-authored-by: Marten Seemann <[email protected]>
Co-authored-by: Marten Seemann <[email protected]>
Co-authored-by: Marten Seemann <[email protected]>
MarcoPolo
force-pushed
the
marco/quic-v1
branch
from
1a51d4b
to
8e45102
Compare
Fixes #1841 see that issue for more context.
This lets transports advertise multiple multiaddrs. Allows the quic transport to advertise both QUIC versions draft29 and 1. Changes webtransport to only support QUIC version 1.
Before merge: