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 89.84375% with 39 lines in your changes missing coverage. Please review.

Project coverage is 90.65%. Comparing base (ba3d4ff) to head (20642a6).
Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	90.41%	27 Missing and 5 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 ⚠️
lightning/src/ln/channelmanager.rs	97.61%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3109      +/-   ##
==========================================
+ Coverage   89.25%   90.65%   +1.40%     
==========================================
  Files         130      130              
  Lines      106959   114214    +7255     
  Branches   106959   114214    +7255     
==========================================
+ Hits        95464   103545    +8081     
+ Misses       8706     8232     -474     
+ Partials     2789     2437     -352

☔ 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.

lightning/src/ln/channel.rs

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).

sounds good, will do in a followup

lightning/src/ln/channel.rs

lightning/src/ln/async_signer_tests.rs

valentinewallace

Going to give this another look next week but it looks good!

valentinewallace · 2024-12-06T18:42:51Z

lightning/src/ln/channel.rs

+	fn generate_accept_channel_message<L: Deref>(&mut self, _logger: &L) -> Option<msgs::AcceptChannel>
+	where L::Target: Logger
+	{


Could you format these method signatures per rustfmt, here and elsewhere in the PR:

Suggested change

fn generate_accept_channel_message<L: Deref>(&mut self, _logger: &L) -> Option<msgs::AcceptChannel>

where L::Target: Logger

{

fn generate_accept_channel_message<L: Deref>(

&mut self, _logger: &L

) -> Option<msgs::AcceptChannel> where L::Target: Logger {

Did a full scan of the function signatures changed in this PR and formatted them accordingly, lmk if i missed any

lightning/src/ln/async_signer_tests.rs

valentinewallace · 2024-12-06T19:05:42Z

lightning/src/ln/channel.rs

+		}
+		if let Some(ref mut point) = self.unfunded_context.holder_commitment_point {
+			if !point.is_available() {
+				point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger);


Ah would it be easy to add test coverage for this case?

Edit: looking at this again, this looks potentially redundant with HolderCommitmentPoint::new, which already tries to fetch the next point?

This case is for if they already got the first commitment point, but not the next. HolderCommitmentPoint::new will try to fetch both points, but if one is ready before the other, I think we still need this check.

Unfortunately, I don't think it's easy to add coverage for this. Right now our test utils for disabling signer ops mainly just disable all calls for a certain signing method on a signer, whereas for this we'd need to disable it for the second call and not the first. I have an idea for how to do this, i.e. we could hold a queue of if the next signer calls should be disabled/enabled, but my first impression is it'll be a little complicated just to get this case. Open to suggestions + I'm happy to give it a try it if you want, but yea

lightning/src/ln/channel.rs

valentinewallace · 2024-12-09T19:03:32Z

lightning/src/ln/channel.rs

+		let mut holder_commitment_point = match self.unfunded_context.holder_commitment_point {
+			Some(point) => point,
+			None => return Err((self, ChannelError::close("Received funding_created before our first commitment point was available".to_owned()))),
+		};


In funding_signed we'll expect that holder_commitment_point is Some, but here we'll error and close the channel. Is there reason for the difference in handling there?

Unintentional, just changed funding_signed to return an error!

lightning/src/ln/channel.rs

valentinewallace · 2024-12-09T19:29:35Z

lightning/src/ln/channel.rs

@@ -8298,7 +8304,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 			holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx),
 		};

-		let chan = Self { context, unfunded_context };
+		let chan = Self { context, unfunded_context, signer_pending_open_channel: false };


Not obvious to me why this is set to false, seems like whether it's set should depend on the result of HolderCommitmentPoint::new above?

Ah yea, we just initialize to false here, and leave the logic for setting the flag for where we actually try to generate the open_channel message. Added a comment, lmk if that works

valentinewallace · 2024-12-09T19:55:34Z

lightning/src/ln/channel.rs

+		let open_channel = match self.unfunded_context.holder_commitment_point {
+			Some(ref mut point) if point.is_available() && self.signer_pending_open_channel => {
+				log_trace!(logger, "Attempting to generate open_channel...");
+				self.get_open_channel(chain_hash, logger)
+			}
+			_ => None
+		};


I think this can be simplified due to the existing checks in get_open_channel?

let open_channel = if self.signer_pending_open_channel { log_trace!(logger, "Attempting to generate open_channel..."); self.get_open_channel(chain_hash, logger) } else { None };

We check here point.is_available and don't in get_open_channel. But it does seem simpler to just do that check in get_open_channel (and be consistent in both places) so I'll just move it there and simplify this. Same with accept channel.

lightning/src/ln/channel.rs

alecchendev · 2024-12-10T00:17:41Z

pushed some, still addressing more comments

alecchendev · 2024-12-11T21:25:16Z

almost there...

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.

We were only checking point.is_available() on the signer unblocked case, but should just check it whenever we're getting open_channel.

For all of our async signing logic in channel establishment v1, we set signer flags in the method where we create the raw lightning message object. To keep things consistent, this commit moves setting the signer flags to where we create funding_created, since this was being set elsewhere before. While we're doing this cleanup, this also slightly refactors our funding_signed method to move some code out of an indent, 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.

We were only checking point.is_available() on the signer unblocked case, but should just check it whenever we're getting accept_channel.

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.

alecchendev · 2024-12-11T22:20:12Z

Okay, I think I've addressed all the comments up to now. Lmk when/if I'm good to squash!

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 force-pushed the 2024-06-async-commit-point-funding branch from 8f49a7d to 6041e37 Compare December 6, 2024 00:00

valentinewallace reviewed Dec 6, 2024

View reviewed changes

valentinewallace reviewed Dec 9, 2024

View reviewed changes

alecchendev force-pushed the 2024-06-async-commit-point-funding branch from 6041e37 to 20642a6 Compare December 10, 2024 00:06

f - return error on funding signed instead of crash

aa2b957

alecchendev force-pushed the 2024-06-async-commit-point-funding branch from 20642a6 to ceae1c4 Compare December 11, 2024 21:24

alecchendev added 19 commits December 11, 2024 13:45

f - rustfmt function signature

08bc29d

f - remove TODO in comment

e45fb90

f - typo

6bab36d

f - consistent checks for open channel

01fd0e0

We were only checking point.is_available() on the signer unblocked case, but should just check it whenever we're getting open_channel.

f - clarify initialization of signer_pending_open_channel flag

5d51b25

f - rustfmt function signature

a217aba

f - move comment, redundant variable

95afaa1

f - consistent checks for accept channel

e72977d

We were only checking point.is_available() on the signer unblocked case, but should just check it whenever we're getting accept_channel.

f - rustfmt function signature

7d6449a

f - fix setting need_channel_ready in funding_created/signed

1bd47e7

f - rustfmt function signature

f8fbb03

Add test for async open and accept channel

d5b03d0

f - test coverage for updating maps after async chan close

4b12826

f - typo

221494c

f - remove dangling reference to uninitialized

4aae0ff

alecchendev force-pushed the 2024-06-async-commit-point-funding branch from ceae1c4 to 4aae0ff Compare December 11, 2024 22:01

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 Nov 26, 2024

valentinewallace left a comment

valentinewallace Dec 6, 2024

alecchendev Dec 11, 2024

valentinewallace Dec 6, 2024 •

edited

Loading

alecchendev Dec 11, 2024

valentinewallace Dec 9, 2024

alecchendev Dec 11, 2024

valentinewallace Dec 9, 2024

alecchendev Dec 11, 2024

valentinewallace Dec 9, 2024

alecchendev Dec 10, 2024

alecchendev commented Dec 10, 2024

alecchendev commented Dec 11, 2024

alecchendev commented Dec 11, 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

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Dec 6, 2024 • edited Loading

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

alecchendev commented Dec 10, 2024

alecchendev commented Dec 11, 2024

alecchendev commented Dec 11, 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

valentinewallace Dec 6, 2024 •

edited

Loading