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.

Sign up for GitHub

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

Jump to bottom

Reintroduce cfg(dual_funding) for handling of open_channel2 messages #3485

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Reintroduce cfg(dual_funding) for handling of open_channel2 messages #3485

merged 2 commits into from
Dec 16, 2024

Conversation

dunxen
Copy link
Contributor

@dunxen dunxen commented Dec 13, 2024

There is still some work to be done here (like #3423) and it probably makes sense to release this (around v0.2) with the ability to open V2 channels too. This would also make our functional tests for dual-funding more complete.

@dunxen dunxen added this to the 0.1 milestone Dec 13, 2024
@dunxen dunxen marked this pull request as draft December 13, 2024 09:14
@dunxen dunxen force-pushed the 2024-12-cfgflagdualfunding branch from 490b2ac to d67c00a Compare December 13, 2024 09:23
@codecov Codecov
Copy link

codecov bot commented Dec 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 61.90476% with 8 lines in your changes missing coverage. Please review.

Project coverage is 90.17%. Comparing base (6e85a0d) to head (76608f7).
Report is 30 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channel.rs 25.00% 3 Missing ⚠️
lightning/src/ln/channelmanager.rs 78.57% 3 Missing ⚠️
lightning/src/ln/peer_handler.rs 33.33% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3485      +/-   ##
==========================================
+ Coverage   89.71%   90.17%   +0.45%     
==========================================
  Files         130      129       -1     
  Lines      107625   114866    +7241     
  Branches   107625   114866    +7241     
==========================================
+ Hits        96553   103576    +7023     
- Misses       8672     8998     +326     
+ Partials     2400     2292     -108

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

@dunxen dunxen force-pushed the 2024-12-cfgflagdualfunding branch from d67c00a to e774784 Compare December 13, 2024 09:56
@dunxen dunxen marked this pull request as ready for review December 13, 2024 10:01
@dunxen dunxen force-pushed the 2024-12-cfgflagdualfunding branch from e774784 to e8cf37e Compare December 13, 2024 17:39
@jkczyz jkczyz self-requested a review December 13, 2024 17:45
@dunxen dunxen force-pushed the 2024-12-cfgflagdualfunding branch from e8cf37e to e5f6e4e Compare December 14, 2024 06:32
@optout21
Copy link
Contributor

Note: previously splicing branches used cfg(any(dual_funding, splicing)) for the dual funding code; if at that time dual_funding flag still exist, this will have to be done in #3444 .

TheBlueMatt
TheBlueMatt reviewed Dec 16, 2024
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM except we need to remove features.set_dual_fund_optional(); in provided_init_features too.

@dunxen
Reintroduce cfg(dual_funding) for handling of open_channel2 messages
6f8328e
@dunxen
Modify release notes for PR 3137
76608f7 
We will not support accepting V2 channels in the v0.1 release, but
we do need to document the API change for push_msats -> channel_negotiation_type.
@dunxen dunxen force-pushed the 2024-12-cfgflagdualfunding branch from e5f6e4e to 76608f7 Compare December 16, 2024 04:08
@dunxen
Copy link
Contributor Author

dunxen commented Dec 16, 2024

Note: previously splicing branches used cfg(any(dual_funding, splicing)) for the dual funding code; if at that time dual_funding flag still exist, this will have to be done in #3444 .

So here I've been quite conservative and have only cfg flagged the parts (message handling) that are definitely just for dual-funding, and which splicing shouldn't be using. If that's not the case then would you mind introducing the any logic again?

@TheBlueMatt TheBlueMatt linked an issue Dec 16, 2024 that may be closed by this pull request
(Temporary) Disable v2 opens #3451
Closed
TheBlueMatt
TheBlueMatt approved these changes Dec 16, 2024
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks.

lightning/src/ln/peer_handler.rs
@@ -1820,8 +1821,8 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
wire::Message::OpenChannel(msg) => {
self.message_handler.chan_handler.handle_open_channel(their_node_id, &msg);
},
wire::Message::OpenChannelV2(msg) => {
self.message_handler.chan_handler.handle_open_channel_v2(their_node_id, &msg);
wire::Message::OpenChannelV2(_msg) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ha, this doesn't seem required :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yup. Ended up removing the cfg flag in the branch...

@TheBlueMatt TheBlueMatt merged commit f53a09d into lightningdevkit:main Dec 16, 2024
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@jkczyz jkczyz Awaiting requested review from jkczyz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.1
Development

Successfully merging this pull request may close these issues.

(Temporary) Disable v2 opens
3 participants
@dunxen @optout21 @TheBlueMatt