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

Add websocket to NetAddr #2231

Conversation

benthecarman
Copy link
Contributor

@benthecarman benthecarman commented Apr 26, 2023

I think (?) this is all that is needed for LDK to support the websockets as the rest of networking it is handled by the user.

lightning/bolts#891

Implements an updated version of using websockets for transport lightning/bolts#1068.

@benthecarman benthecarman force-pushed the add-websocket-to-netaddr branch from 6241f6b to ee0aaac Compare April 26, 2023 02:53
@TheBlueMatt
Copy link
Collaborator

A few things - (a) I think we should actually implement websockets in our net code, apparently it's not too complicated so we should do it for the user. (B) I don't actually think this announcement design works, given we also need to be able to announce wss, so the bolt pr probably needs to be reworked.

@benthecarman benthecarman marked this pull request as draft April 26, 2023 07:27
@benthecarman benthecarman force-pushed the add-websocket-to-netaddr branch from ee0aaac to 5a5c044 Compare April 26, 2023 07:28
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2023

Codecov Report

Patch coverage: 76.92% and project coverage change: -0.02 ⚠️

Comparison is base (c182567) 91.56% compared to head (5a5c044) 91.54%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2231      +/-   ##
==========================================
- Coverage   91.56%   91.54%   -0.02%     
==========================================
  Files         104      104              
  Lines       51749    51763      +14     
  Branches    51749    51763      +14     
==========================================
+ Hits        47384    47388       +4     
- Misses       4365     4375      +10     
Impacted Files Coverage Δ
lightning/src/ln/msgs.rs 86.82% <76.92%> (-0.34%) ⬇️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@benthecarman
Copy link
Contributor Author

A few things - (a) I think we should actually implement websockets in our net code, apparently it's not too complicated so we should do it for the user. (B) I don't actually think this announcement design works, given we also need to be able to announce wss, so the bolt pr probably needs to be reworked.

The PR hadn't been touched in 2 years so I made a proposal for an alternate (we'll see how that goes).

Can work on implementing it into the networking code. I assume just using tokio-tungstenite would be fine here?

@TheBlueMatt
Copy link
Collaborator

I don't think we need to pull in a dependency, I haven't checked but Rusty had claimed that its a super trivial thing to implement (apparently he did it in CLN) - basically a new "header" on the protocol that you can trivially check for when reading the first bytes and then we switch to normal operation thereafter.

@ariard
Copy link

ariard commented Jul 12, 2023

(B) I don't actually think this announcement design works, given we also need to be able to announce wss, so the bolt pr probably needs to be reworked.

100% to have an update of the bolt gossips to have wss support, already have downstream LDK-based software implementing this. Let me know if there is a need for review on the lightning-rfc side here.

@TheBlueMatt
Copy link
Collaborator

Not just review, no one has written it, someone needs to write that spec :)

@ariard
Copy link

ariard commented Jul 15, 2023

Not just review, no one has written it, someone needs to write that spec :)

That someone can feel free to tag me on the proposed spec changes when it does happen :p

@TheBlueMatt
Copy link
Collaborator

Closing as stalled-on-spec. Once there's a spec proposal for announcing wss endpoint hostnames we can reopen this here.

@benthecarman benthecarman deleted the add-websocket-to-netaddr branch January 10, 2024 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants