Handle fallible per commitment point in channel establishment #3109

alecchendev · 2024-06-06T23:49:41Z

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:

sending open_channel - creates new outbound channel, immediately requests the first commit point, waits to have the first 2 commit points to be unblocked to send the message
sending accept_channel - same ^
sending funding_created - no op regarding commit points
receiving funding_created - uses point to verify first commitment, then advances commitment, requesting next point
sending funding_signed - no op
receiving funding_signed - same as above, verifies, advances, requests next point
sending channel_ready - waits to have the next commit point ready, then once unblocked sends the current point in the channel_ready message

codecov-commenter · 2024-06-10T00:52:00Z

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 ⚠️
lightning/src/ln/channelmanager.rs	87.50%	0 Missing and 4 partials ⚠️
lightning/src/ln/functional_test_utils.rs	0.00%	3 Missing ⚠️
lightning/src/util/test_utils.rs	40.00%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            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

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

alecchendev · 2024-06-11T01:16:38Z

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

lightning/src/ln/channel.rs

TheBlueMatt · 2024-07-18T15:18:46Z

When we get back to this, please address the comments from #3152 (review)

lightning/src/ln/channel.rs

TheBlueMatt · 2024-08-28T20:25:46Z

This also needs rebase now.

alecchendev · 2024-09-16T23:58:07Z

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

alecchendev · 2024-09-16T23:59:50Z

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

alecchendev · 2024-09-17T00:02:48Z

lightning/src/ln/channel.rs

 		// `current_point` will become optional when async signing is implemented.
-		let cur_holder_commitment_point = Some(self.context.holder_commitment_point.current_point());
-		let next_holder_commitment_point = self.context.holder_commitment_point.next_point();
+		let cur_holder_commitment_point = Some(self.holder_commitment_point.current_point());
+		let next_holder_commitment_point = self.holder_commitment_point.next_point();


when reading, now that this will never be optional, can we unwrap/expect the value (or return a DecodeError on None)? what's the right way to handle this

TheBlueMatt

Nice, this is looking great. A few nits here and there but basically this LGTM.

TheBlueMatt · 2024-09-17T18:20:59Z

lightning/src/ln/channel.rs

 	{
 		self.context.maybe_downgrade_channel_features(fee_estimator)?;
-		Ok(self.get_open_channel(chain_hash))
+		self.get_open_channel(chain_hash, logger).ok_or(())


Sadly I think we need to duplicate this code in the v2 pipeline (get_open_channel_v2)

still need to get to this

TheBlueMatt · 2024-09-17T18:57:28Z

lightning/src/ln/channel.rs

+		} else {
+			log_debug!(logger, "Not producing channel_ready: the holder commitment point is not available.");
+			self.context.signer_pending_channel_ready = true;
+			None


I think after this we are missing an update to get_announcement_sigs to replay it (can come in a followup, but right now if you do an async signer it will always be forced to be a non-public channel).

TheBlueMatt · 2024-09-17T19:03:33Z

lightning/src/ln/channel.rs

+			// We make sure to set the channel ready flag here so that we try to
+			// generate a channel ready for 0conf channels once our signer unblocked
+			// for funding.
+			self.context.signer_pending_channel_ready = true;


This is a confusing way to update the need_channel_readys in funding_signed an funding_created :). Instead, why don't we just update those to check if we want a channel_ready whether we're ready to send one or not?

Sorta confused here, are you saying we could just drop this since we already set need_channel_ready in funding_created/signed?

In funding_signed and funding_created, need_channel_ready are set by calling check_get_channel_ready which is used to store whether we need to send a channel_ready in the 0conf case. We now can fail check_get_channel_ready even though we do need to send a channel_ready (when the monitor update persist finishes) but have to handle checking signer_pending_channel_ready now too, when we could instead just set need_channel_ready because we do need a channel_ready, and just possibly fail to send when we actually go to do the send.

TheBlueMatt · 2024-09-17T19:05:20Z

lightning/src/ln/async_signer_tests.rs

@@ -31,7 +31,59 @@ use crate::util::test_channel_signer::SignerOp;
 use crate::util::logger::Logger;

 #[test]
-fn test_async_commitment_signature_for_funding_created() {
+fn test_open_channel() {


Since it uses a different path, mind adding a test/variant that tests 0conf opens?

TheBlueMatt · 2024-09-17T19:08:11Z

lightning/src/ln/async_signer_tests.rs

@@ -82,7 +138,12 @@ fn test_async_commitment_signature_for_funding_created() {
 }

 #[test]
-fn test_async_commitment_signature_for_funding_signed() {
+fn test_funding_signed() {
+	do_test_funding_signed(vec![SignerOp::SignCounterpartyCommitment, SignerOp::GetPerCommitmentPoint]);


If we're gonna run the test twice to test enabling ops in different orders can we check that the message we're looking for is only generated after the op we expect to generate the message?

TheBlueMatt · 2024-09-17T19:08:45Z

Also CI is very sad

TheBlueMatt · 2024-09-17T19:40:26Z

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.

We are choosing to move the `HolderCommitmentPoint` (the struct that tracks commitment points retrieved from the signer + the commitment number) to handle channel establishment, where we have no commitment point at all. Previously we introduced this struct to track when we were pending a commitment point (because of an async signer) during normal channel operation, which assumed we always had a commitment point to start out with. Intiially we tried to add an `Uninitialized` variant that held no points, but that meant that we needed to handle that case everywhere which left a bunch of scary/unnecessary unwraps/expects. Instead, we just hold an optional `HolderCommitmentPoint` struct on our unfunded channels, and a non-optional `HolderCommitmentPoint` for funded channels. This commit starts that transition. A following commit will remove the holder commitment point from the current `ChannelContext`. This also makes small fixups to the comments on the HolderCommitmentPoint variants.

Following a previous commit adding `HolderCommitmentPoint` elsewhere, we make the transition to use those commitment points and remove the existing one.

In the event that a signer cannot provide a commitment point immediately, we set a flag to remember we're waiting for this before we can send `open_channel`. We make sure to get the first two commitment points, so when we advance commitments, we always have a commitment point available.

This also slightly refactors get_funding_signed_msg to match the logic more similarly, as well as removes a log to fix a nit from lightningdevkit#3152.

Similar to `open_channel`, if a signer cannot provide a commitment point immediately, we set a flag to remember we're waiting for a point to send `accept_channel`. We make sure to get the first two points before moving on, so when we advance our commitment we always have a point available.

Here we handle the case where our signer is pending the next commitment point when we try to send channel ready. We set a flag to remember to send this message when our signer is unblocked. This follows the same general pattern as everywhere else where we're waiting on a commitment point from the signer in order to send a message.

TheBlueMatt · 2024-09-30T21:20:46Z

CI still looks to be failing.

alecchendev mentioned this pull request Jun 6, 2024

Add HolderCommitmentPoint struct to track commitment points #3086

Merged

alecchendev force-pushed the 2024-06-async-commit-point-funding branch from 3fbf2e6 to 1e5b210 Compare June 10, 2024 00:44

alecchendev force-pushed the 2024-06-async-commit-point-funding branch from 1e5b210 to 07991fa Compare June 10, 2024 01:16

alecchendev force-pushed the 2024-06-async-commit-point-funding branch from 07991fa to 744f2ca Compare June 11, 2024 01:22

TheBlueMatt reviewed Jun 11, 2024

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

alecchendev force-pushed the 2024-06-async-commit-point-funding branch 3 times, most recently from 73583e7 to b798ffa Compare June 18, 2024 22:24

alecchendev marked this pull request as ready for review June 18, 2024 22:27

TheBlueMatt reviewed Jun 19, 2024

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

alecchendev mentioned this pull request Jun 30, 2024

Handle fallible per commitment point for RAA #3150

Merged

alecchendev force-pushed the 2024-06-async-commit-point-funding branch 2 times, most recently from 0bdd4cc to 0ac4ad8 Compare July 3, 2024 20:42

valentinewallace added the blocked on dependent pr label Jul 15, 2024

TheBlueMatt mentioned this pull request Jul 18, 2024

Handle fallible commitment secret #3152

Merged

valentinewallace removed the blocked on dependent pr label Jul 18, 2024

alecchendev force-pushed the 2024-06-async-commit-point-funding branch 2 times, most recently from 7ea6d3f to c109e18 Compare July 20, 2024 00:00

TheBlueMatt reviewed Aug 6, 2024

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

alecchendev force-pushed the 2024-06-async-commit-point-funding branch 2 times, most recently from df8902b to 50b1d88 Compare September 16, 2024 23:44

alecchendev commented Sep 17, 2024

View reviewed changes

alecchendev force-pushed the 2024-06-async-commit-point-funding branch from 50b1d88 to 760b055 Compare September 17, 2024 00:15

TheBlueMatt reviewed Sep 17, 2024

View reviewed changes

alecchendev added 9 commits September 18, 2024 13:45

Remove holder commitment point from channel context

35c1139

Following a previous commit adding `HolderCommitmentPoint` elsewhere, we make the transition to use those commitment points and remove the existing one.

f - unused param open_chan

5c62627

Move setting signer flags to get_funding_created_msg

7713e1e

This also slightly refactors get_funding_signed_msg to match the logic more similarly, as well as removes a log to fix a nit from lightningdevkit#3152.

f - unused param accept_chan

00f149e

Add test for async open and accept channel

cc68c27

alecchendev force-pushed the 2024-06-async-commit-point-funding branch from 760b055 to 140441e Compare September 18, 2024 20:49

f - test zero conf, assert after specific op

ab345cd

alecchendev force-pushed the 2024-06-async-commit-point-funding branch from 140441e to ab345cd Compare September 18, 2024 23:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle fallible per commitment point in channel establishment #3109

Handle fallible per commitment point in channel establishment #3109

alecchendev commented Jun 6, 2024 •

edited

Loading

codecov-commenter commented Jun 10, 2024 •

edited by codecov bot

Loading

alecchendev commented Jun 11, 2024 •

edited

Loading

TheBlueMatt commented Jul 18, 2024

TheBlueMatt commented Aug 28, 2024

alecchendev commented Sep 16, 2024

alecchendev commented Sep 16, 2024

alecchendev Sep 17, 2024

TheBlueMatt left a comment

TheBlueMatt Sep 17, 2024

alecchendev Sep 18, 2024

TheBlueMatt Sep 17, 2024

TheBlueMatt Sep 17, 2024

alecchendev Sep 18, 2024

TheBlueMatt Sep 26, 2024

TheBlueMatt Sep 17, 2024

alecchendev Sep 18, 2024

TheBlueMatt Sep 17, 2024

alecchendev Sep 18, 2024

TheBlueMatt commented Sep 17, 2024

TheBlueMatt commented Sep 17, 2024

TheBlueMatt commented Sep 30, 2024

Handle fallible per commitment point in channel establishment #3109

Are you sure you want to change the base?

Handle fallible per commitment point in channel establishment #3109

Conversation

alecchendev commented Jun 6, 2024 • edited Loading

codecov-commenter commented Jun 10, 2024 • edited by codecov bot Loading

Codecov Report

alecchendev commented Jun 11, 2024 • edited Loading

TheBlueMatt commented Jul 18, 2024

TheBlueMatt commented Aug 28, 2024

alecchendev commented Sep 16, 2024

alecchendev commented Sep 16, 2024

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 17, 2024

TheBlueMatt commented Sep 17, 2024

TheBlueMatt commented Sep 30, 2024

alecchendev commented Jun 6, 2024 •

edited

Loading

codecov-commenter commented Jun 10, 2024 •

edited by codecov bot

Loading

alecchendev commented Jun 11, 2024 •

edited

Loading