rust-lightning icon indicating copy to clipboard operation
rust-lightning copied to clipboard

Implement accepting dual-funded channels without contributing

Open dunxen opened this issue 1 year ago • 11 comments
trafficstars

We split this out from #2302 for easier review and to address the common non-public API parts of the V2 channel establishment implementation.

This will allow the holder to be an acceptor, but not initiator of V2 channels. We also don't expose an API for contributing to an inbound channel.

The functionality to initiate V2 channels and fund inbound channels forms part of #2302.

dunxen avatar Jun 20 '24 11:06 dunxen

Codecov Report

Attention: Patch coverage is 60.06849% with 583 lines in your changes missing coverage. Please review.

Project coverage is 89.29%. Comparing base (4322b19) to head (8b0f4b5).

Files with missing lines Patch % Lines
lightning/src/ln/channelmanager.rs 47.67% 227 Missing and 21 partials :warning:
lightning/src/ln/channel.rs 59.73% 162 Missing and 18 partials :warning:
lightning/src/ln/interactivetxs.rs 49.16% 150 Missing and 3 partials :warning:
lightning/src/ln/dual_funding_tests.rs 99.02% 2 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3137      +/-   ##
==========================================
- Coverage   89.69%   89.29%   -0.41%     
==========================================
  Files         129      130       +1     
  Lines      105437   106910    +1473     
  Branches   105437   106910    +1473     
==========================================
+ Hits        94573    95464     +891     
- Misses       8117     8665     +548     
- Partials     2747     2781      +34     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jun 24 '24 17:06 codecov-commenter

As I see, the dual_funding cfg flag has been removed (not a problem)

optout21 avatar Jun 26 '24 12:06 optout21

Looks to me that #2989 is in fact included in this PR (good!)

optout21 avatar Jun 26 '24 12:06 optout21

Still working on remaining initial feedback.

dunxen avatar Jul 10 '24 16:07 dunxen

No rush, let me know when you want another pass.

TheBlueMatt avatar Jul 15 '24 15:07 TheBlueMatt

I realise I haven't addressed https://github.com/lightningdevkit/rust-lightning/pull/3137#discussion_r1667860073 yet.

dunxen avatar Aug 29 '24 09:08 dunxen

I see there is still a lot of potential DRYing up to do here.

dunxen avatar Sep 03 '24 14:09 dunxen

Needs rebase now :(

TheBlueMatt avatar Sep 17 '24 19:09 TheBlueMatt

Needs rebase now :(

Sorry, catching up on everything now! Been mostly offline for a week.

dunxen avatar Sep 18 '24 10:09 dunxen

Just rebased for now while I'm still addressing past week's feedback.

dunxen avatar Sep 18 '24 11:09 dunxen

Rebased and working through drying up and continuing to address: https://github.com/lightningdevkit/rust-lightning/pull/3137#discussion_r1784955635

dunxen avatar Oct 04 '24 14:10 dunxen

LMK when you want another round of review here. I think it was mostly there so kinda waiting for you to tell me you've DRY'd things up and will take a look.

TheBlueMatt avatar Oct 07 '24 13:10 TheBlueMatt

DRY'd up new initial commitment in latest push. Now moving over some tests from #2302 and modifying them to have manual V2 open channel messages so we can at least test them here.

dunxen avatar Oct 08 '24 12:10 dunxen

Pushed the DRYing up. Migrating the appropriate tests from 2302 is taking some time as more manual helper methods are needed. Almost there, though.

dunxen avatar Oct 10 '24 13:10 dunxen

Git history is weird here looks like some commits from upstream got cherry-picked, can just rebase to fix it.

TheBlueMatt avatar Oct 15 '24 21:10 TheBlueMatt

Git history is weird here looks like some commits from upstream got cherry-picked, can just rebase to fix it.

Yeah, I see now. Fixing.

Will address outstanding feedback from today.

dunxen avatar Oct 16 '24 10:10 dunxen

Now depends on #3370, so have rebased on that and will continue to address outstanding feedback.

dunxen avatar Oct 17 '24 14:10 dunxen

Ah looks like the last round of comments wasn't fully addressed yet either, sorry got excited :)

Ah sorry. I didn't actually send the comment. Addressing your feedback this morning.

dunxen avatar Oct 23 '24 05:10 dunxen

Another round of fix-ups. The review comments not marked as resolved, I'm still working on. Just quite a bit to get through.

dunxen avatar Oct 24 '24 13:10 dunxen

Thanks for the feedback, @jkczyz. Addressing it today.

dunxen avatar Oct 30 '24 08:10 dunxen

@TheBlueMatt this ready for another pass when you get a gap :pray:

dunxen avatar Nov 06 '24 07:11 dunxen

Mostly looks good. Let me know if there's anything else outstanding that needs to be addressed still.

Async persist test mentioned in https://github.com/lightningdevkit/rust-lightning/pull/3137#discussion_r1832969732 still needs to be added.

dunxen avatar Nov 13 '24 11:11 dunxen

Few more comments, got through the bulk of it. Basically the only issue(s) I think need fixing are the persistence of Channels before we have a ChannelMonitor which is gonna cause issues on restart. It can be fixed in a followup, though. Still need to review the tests but I can do that after this lands.

Thanks! Going to address a few more things you brought up throughout tonight and tomorrow morning for me and then I'll open an issue with remaining follow-ups. So unless something major prevents this from landing, we could land it tomorrow?

dunxen avatar Nov 19 '24 15:11 dunxen

I've pushed up a few fixes. There are some nits that could be fixed here, but I'll include them in a followup to start unblocking other PRs.

@jkczyz, if CI and you are happy, we can go ahead and get this in when you're ready. Then I'll create the follow-up issue and tag it to make sure it's a blocker for next release.

dunxen avatar Nov 20 '24 12:11 dunxen

I've pushed up a few fixes. There are some nits that could be fixed here, but I'll include them in a followup to start unblocking other PRs.

@jkczyz, if CI and you are happy, we can go ahead and get this in when you're ready. Then I'll create the follow-up issue and tag it to make sure it's a blocker for next release.

Sure, let's land this now and address the rest in follow-ups. Incremental mutants has been running for five hours, but no need to wait on it.

jkczyz avatar Nov 20 '24 17:11 jkczyz