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

feat(modp2p): websocket transport with TLS #3560

Merged
merged 14 commits into from
Aug 6, 2024

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Jul 11, 2024

Added support of the TLS certificates to satisfy conditions for the websocket connection. MinTLS version was set to 1.2 as a default version for now.(used lower versions but linter returned errors)

Screenshot 2024-07-11 at 12 01 05

@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 43.63636% with 31 lines in your changes missing coverage. Please review.

Project coverage is 45.46%. Comparing base (2469e7a) to head (5861eac).
Report is 150 commits behind head on main.

Files Patch % Lines
nodebuilder/p2p/tls.go 39.13% 14 Missing ⚠️
nodebuilder/p2p/config.go 0.00% 11 Missing ⚠️
nodebuilder/p2p/host.go 63.63% 2 Missing and 2 partials ⚠️
nodebuilder/p2p/pubsub.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3560      +/-   ##
==========================================
+ Coverage   44.83%   45.46%   +0.63%     
==========================================
  Files         265      281      +16     
  Lines       14620    16002    +1382     
==========================================
+ Hits         6555     7276     +721     
- Misses       7313     7887     +574     
- Partials      752      839      +87     

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

ramin
ramin previously approved these changes Jul 11, 2024
@renaynay
Copy link
Member

@vgonkivs can you please convert to draft and wait for this to be tested? Let's wait to confirm that this actually makes lumina work on safari

@vgonkivs vgonkivs marked this pull request as draft July 17, 2024 14:18
Wondertan
Wondertan previously approved these changes Jul 31, 2024
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Approving as we need this ASAP.

But generally I think this PR does more than needed. We don't need to store TLS config and we don't need a separate config for "tls" addresses. We only need a simple switch to enable WSS with the path to certificate and key, which could be an ENVVAR.

@Wondertan Wondertan marked this pull request as ready for review July 31, 2024 20:23
nodebuilder/p2p/tls.go Outdated Show resolved Hide resolved
nodebuilder/p2p/tls.go Outdated Show resolved Hide resolved
nodebuilder/p2p/tls.go Outdated Show resolved Hide resolved
renaynay
renaynay previously approved these changes Aug 5, 2024
state/core_access.go Outdated Show resolved Hide resolved
nodebuilder/p2p/tls.go Outdated Show resolved Hide resolved
nodebuilder/p2p/host.go Outdated Show resolved Hide resolved
@renaynay renaynay added v0.15.0 Intended for v0.15.0 release and removed needs:triage labels Aug 5, 2024
state/core_access.go Outdated Show resolved Hide resolved
state/core_access.go Outdated Show resolved Hide resolved
nodebuilder/p2p/tls.go Outdated Show resolved Hide resolved
@Wondertan Wondertan added kind:feat Attached to feature PRs and removed kind:chore labels Aug 5, 2024
@Wondertan Wondertan changed the title chore: enable websocket support feat(modp2p): websocket transport with TLS Aug 5, 2024
@renaynay renaynay enabled auto-merge (squash) August 6, 2024 12:56
@renaynay renaynay merged commit 2ff58bf into celestiaorg:main Aug 6, 2024
23 of 25 checks passed
Wondertan added a commit that referenced this pull request Aug 6, 2024
We mainly need the bump for:
* https://github.com/quic-go/quic-go/releases/tag/v0.45.2 which fixes the leak that could affect us
* WebRTC becoming non-experimental

We enable WebRTC by default to improve connectivity in some cases for Lumina. There are known issues on that front, but at least we unblock eiger team from waiting on us to enable the transport, once those issues are resolved.

Waiting on #3560 to be merged first
sebasti810 pushed a commit to sebasti810/celestia-node that referenced this pull request Aug 14, 2024
sebasti810 pushed a commit to sebasti810/celestia-node that referenced this pull request Aug 14, 2024
We mainly need the bump for:
* https://github.com/quic-go/quic-go/releases/tag/v0.45.2 which fixes the leak that could affect us
* WebRTC becoming non-experimental

We enable WebRTC by default to improve connectivity in some cases for Lumina. There are known issues on that front, but at least we unblock eiger team from waiting on us to enable the transport, once those issues are resolved.

Waiting on celestiaorg#3560 to be merged first
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feat Attached to feature PRs v0.15.0 Intended for v0.15.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants