rust-lightning
rust-lightning copied to clipboard
Implement accepting dual-funded channels without contributing
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.
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).
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.
As I see, the dual_funding cfg flag has been removed (not a problem)
Looks to me that #2989 is in fact included in this PR (good!)
Still working on remaining initial feedback.
No rush, let me know when you want another pass.
I realise I haven't addressed https://github.com/lightningdevkit/rust-lightning/pull/3137#discussion_r1667860073 yet.
I see there is still a lot of potential DRYing up to do here.
Needs rebase now :(
Needs rebase now :(
Sorry, catching up on everything now! Been mostly offline for a week.
Just rebased for now while I'm still addressing past week's feedback.
Rebased and working through drying up and continuing to address: https://github.com/lightningdevkit/rust-lightning/pull/3137#discussion_r1784955635
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.
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.
Pushed the DRYing up. Migrating the appropriate tests from 2302 is taking some time as more manual helper methods are needed. Almost there, though.
Git history is weird here looks like some commits from upstream got cherry-picked, can just rebase to fix it.
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.
Now depends on #3370, so have rebased on that and will continue to address outstanding feedback.
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.
Another round of fix-ups. The review comments not marked as resolved, I'm still working on. Just quite a bit to get through.
Thanks for the feedback, @jkczyz. Addressing it today.
@TheBlueMatt this ready for another pass when you get a gap :pray:
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.
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 aChannelMonitorwhich 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?
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.
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.