Skip to content
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

[testing] Enable QUIC support #5229

Closed
wants to merge 13 commits into from
Closed

[testing] Enable QUIC support #5229

wants to merge 13 commits into from

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Nov 6, 2023

Motivation

Testing QUIC support for better NAT hole punching performance

Changes

Test Plan

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan
  • Update documentation as needed
  • Update changelog as needed

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Attention: 111 lines in your changes are missing coverage. Please review.

Comparison is base (d424b78) 77.5% compared to head (2193d38) 77.4%.

Files Patch % Lines
p2p/host.go 44.9% 28 Missing and 10 partials ⚠️
p2p/handshake/handshake.go 83.1% 19 Missing and 7 partials ⚠️
p2p/dhtdiscovery/discovery.go 73.5% 20 Missing and 3 partials ⚠️
p2p/ping.go 76.4% 11 Missing and 5 partials ⚠️
p2p/upgrade.go 52.9% 5 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           develop   #5229     +/-   ##
=========================================
- Coverage     77.5%   77.4%   -0.1%     
=========================================
  Files          252     254      +2     
  Lines        29650   29989    +339     
=========================================
+ Hits         22983   23239    +256     
- Misses        5209    5273     +64     
- Partials      1458    1477     +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

p2p/host.go Outdated
@@ -206,6 +211,7 @@ func New(
return tcp.NewTCPTransport(upgrader, rcmgr, opts...)
},
),
libp2p.Transport(quic.NewTransport),
Copy link
Contributor

Choose a reason for hiding this comment

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

we are using noise prologue with tcp transport so that peers from different networks can't establish connection with each other. it will fail with obsure error that secure handshake failed if they are using different genesis value.

i noticed that quic.NewTransport accepts PSK that can be used with the same purpose, but implementation doesn't support that yet. we will need to find out what are other options. we can also do genesis check after establishing connection, but it is slightly annoying.

Copy link
Contributor Author

@ivan4th ivan4th Nov 13, 2023

Choose a reason for hiding this comment

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

From what I see implementing PSK for QUIC in go-libp2p is a serious issue: libp2p/go-libp2p#1432
There's a simpler trick we could use, given that we're speaking about a safeguard against users shooting themselves in the foot (IIUC). QUIC uses TLS certs and libp2p injects its own extension into the cert: https://github.com/libp2p/go-libp2p/blob/master/p2p/security/tls/crypto.go#L233-L238
https://github.com/libp2p/go-libp2p/blob/master/p2p/security/tls/crypto.go#L200
When a client connects, the extension is verified to be present in the peer's cert
https://github.com/libp2p/go-libp2p/blob/master/p2p/security/tls/crypto.go#L152-L168

We could "customize" the extension, or add another one including the byte sequence used as Noise prologue for TCP connections, and check that the byte sequence matches the expected sequence.

The problem with this approach is that there appear to be no proper extension points in go-libp2p to do this. We could solve that by either

  1. "Forking" libp2p QUIC transport package, which is not that big, and relevant parts of the helper tls package in libp2p, copying that code into go-spacemesh project, and adding the feature here, or
  2. Suggesting this feature in go-libp2p upstream and posting a PR there, but in this case, I'm not fully sure they will want it

Copy link
Contributor Author

@ivan4th ivan4th Nov 13, 2023

Choose a reason for hiding this comment

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

If we go with (2), then maybe we may already be able to provide the cert extension in the cert template, so we just need to add a certificate chain verification callback. Such a change can be accepted by the go-libp2p maintainers I think. Will check it further

Copy link
Contributor

Choose a reason for hiding this comment

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

in the past we had this https://github.com/spacemeshos/go-spacemesh/blob/v0.3.0-beta.0/p2p/handshake/handshake.go

the main downside is that we accidentally can connect another network, and some protocols will be doing useless work until we disconnect such peer. we had such issues in the past.

@ivan4th ivan4th force-pushed the testing-quic branch 2 times, most recently from 4dae952 to 042b420 Compare November 13, 2023 09:05
}

func (d *Discovery) Start() {
if len(d.pingPeers) != 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a quick temporary hack to get 2 peers to talk to each other for debugging. To be removed.

@ivan4th ivan4th force-pushed the testing-quic branch 2 times, most recently from be56ce0 to 9e96cb9 Compare November 17, 2023 00:37
@ivan4th
Copy link
Contributor Author

ivan4th commented Dec 5, 2023

Closing in favor of #5329

@ivan4th ivan4th closed this Dec 5, 2023
@fasmat fasmat deleted the testing-quic branch June 11, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants