lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.15k stars 363 forks source link

Handle fallible per commitment point in channel establishment #3109

Open alecchendev opened 3 months ago

alecchendev commented 3 months ago

Handles fallible get_per_commitment_point signer method (except for channel reestablish).

We make get_per_commitment_point return a result type so that in the Err case, we (LDK) can resume operation while an async signer fetches the commitment point. This will typically block sending out a message, but otherwise most state updates will occur. When the signature arrives, the user is expected to call ChannelManager::signer_unblocked and we will retry get_per_commitment_point however this time the signer will return the commitment point with Ok, and we'll send out our message.

This PR implements this flow for channel establishment, i.e. open_channel, accept_channel, and channel_ready.

Rough outline of how our HolderCommitmentPoint advances and gets new signatures:

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 82.91815% with 48 lines in your changes missing coverage. Please review.

Project coverage is 89.92%. Comparing base (6662c5c) to head (ab345cd). Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channel.rs 84.23% 36 Missing and 2 partials :warning:
lightning/src/ln/channelmanager.rs 87.50% 0 Missing and 4 partials :warning:
lightning/src/ln/functional_test_utils.rs 0.00% 3 Missing :warning:
lightning/src/util/test_utils.rs 40.00% 0 Missing and 3 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3109 +/- ## ========================================== + Coverage 89.65% 89.92% +0.27% ========================================== Files 126 126 Lines 102546 105339 +2793 Branches 102546 105339 +2793 ========================================== + Hits 91935 94725 +2790 - Misses 7890 7974 +84 + Partials 2721 2640 -81 ```

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

alecchendev commented 3 months ago

the commits from #3115 are second from the end, will rebase on top later this week, but generally most of the stuff here is in place

TheBlueMatt commented 2 months ago

When we get back to this, please address the comments from https://github.com/lightningdevkit/rust-lightning/pull/3152#pullrequestreview-2183958862

TheBlueMatt commented 1 month ago

This also needs rebase now.

alecchendev commented 2 weeks ago

Will probably squash some of these commits together at some point but just kept them separate for now

alecchendev commented 2 weeks ago

rebased and force pushed with the new approach. Previous branch is still here if it's helpful to see

TheBlueMatt commented 2 weeks ago

Also CI is very sad

TheBlueMatt commented 2 weeks ago

Finally, if you get a chance, can you flesh out your commit messages more? Describe why we're doing each commit, what considerations went into the approach, and what risks we might have in the design we took, even just a few sentences may be helpful in the future when someone is looking at old code for what we were thinking when we made changes.

TheBlueMatt commented 23 hours ago

CI still looks to be failing.