Handle fallible per commitment point for RAA #3150

alecchendev · 2024-06-30T23:51:35Z

Builds on ~~#3149~~.

This is the first PR to handle async get_per_commitment_point, and only handles the Err cases for retrieving revoke_and_ack. We still need to handle this during funding (#3109) and channel reestablish (no PR yet) in upcoming PRs.

For all async signing, we try to go about our normal business, and when we fail to get a signature from our signer, we pause our channel, and only unpause upon the user calling ChannelManager::signer_unblocked. With the signer_pending_revoke_and_ack flag being added in the prereq PR, we simply set the flag and allow get_last_revoke_and_ack to return an Option. We also make sure that in cases where we must send RAA then CS, that we defer sending a CS (even if it's available) until the signature for the RAA is unblocked.

codecov-commenter · 2024-06-30T23:59:09Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 79.48718% with 24 lines in your changes missing coverage. Please review.

Project coverage is 90.02%. Comparing base (6035c83) to head (45c0a0f).
Report is 20 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/channel.rs	77.66%	20 Missing and 3 partials ⚠️
lightning/src/util/test_channel_signer.rs	80.00%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3150      +/-   ##
==========================================
+ Coverage   89.74%   90.02%   +0.27%     
==========================================
  Files         121      121              
  Lines       99858   103150    +3292     
  Branches    99858   103150    +3292     
==========================================
+ Hits        89622    92863    +3241     
- Misses       7561     7646      +85     
+ Partials     2675     2641      -34

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

alecchendev · 2024-07-03T20:16:58Z

(rebased after changes to #3149)

TheBlueMatt · 2024-07-09T13:51:45Z

Looks like the test code doesn't currently compile on this one.

lightning/src/ln/channel.rs

lightning/src/sign/mod.rs

TheBlueMatt

LGTM, feel free to squash we'll need to get a second review to glance.

TheBlueMatt · 2024-07-10T20:21:14Z

lightning/src/ln/channel.rs

+				log_trace!(logger, "Last revoke-and-ack pending in channel {} for sequence {} because the next per-commitment point is not available",
+					&self.context.channel_id(), self.context.holder_commitment_point.transaction_number());
+				self.context.signer_pending_revoke_and_ack = true;
+				None


It might be worth mentioning in a comment why we don't want to send an RAA here. If we have HolderCommitmentPoint::PendingNext we still have current and can in theory build a RevokeAndACK, but if we send one immediately in response to a commitment_signed we'll just get another commitment_signed and be out of points anyway and unable to send a RevokeAndACK, and avoiding sending the RAA is a neat way to avoid having to deal with ever actually not having the current point available. Obviously once we make release_commitment_secret async we'll have to block on the signer anyway, so blocking here on a different signer operation doesnt add latency and simplifies our code by making our peer be blocked waiting on our signer, not just us.

squashed + added a comment for this

lightning/src/ln/async_signer_tests.rs

lightning/src/sign/mod.rs

lightning/src/ln/channel.rs

valentinewallace · 2024-07-11T20:25:27Z

lightning/src/ln/channel.rs

+			// in revoke_and_ack, which requires unblocking our signer and completing
+			// the advance to the next point. So if we get here, either we've messed
+			// up, or our counterparty has.
+			debug_assert!(false, "We should be ready to advance our commitment point by the time we receive commitment_signed");


To be clear, is this reachable with a misbehaving counterparty? Because we're currently closing the channel after advancing some state above like the latest monitor update id, may be worth adding some test coverage for this (maybe in follow-up)

It shouldn't be. Without our commitment point provided in the last RAA our counterparty shouldn't be able to give us a CS that passes our signature checks.

hmm, i think maybe a way to handle this is to check self.holder_commitment_point.is_available() with our checks above before we start updating state, then when we get here it's easier to know that this is unreachable?

or maybe just move this to be the first thing we update?

I don't feel strongly since Matt clarified that this should be unreachable. Could also update the comment to be more explicit about that, or what you suggested, up to you!

just updated the last sentence of the comment to clarify this should be unreachable

lightning/src/ln/channel.rs

valentinewallace · 2024-07-11T20:41:37Z

lightning/src/ln/channel.rs

 			self.get_last_commitment_update_for_send(logger).ok()
 		} else { None };
 		if self.context.resend_order == RAACommitmentOrder::CommitmentFirst
 			&& self.context.signer_pending_commitment_update && raa.is_some() {
 			self.context.signer_pending_revoke_and_ack = true;
 			raa = None;
 		}
+		if self.context.resend_order == RAACommitmentOrder::RevokeAndACKFirst
+			&& self.context.signer_pending_revoke_and_ack {
+			self.context.signer_pending_commitment_update = true;


Tests pass with this and the line below commented out

added some test cases to cover here + the similar condition added in channel reestablish

Hmm, tests still pass with these lines commented out. I don't wanna hold up the PR, though, so feel free to save this for follow-up.

hmm, are you running with RUSTFLAGS="--cfg=async_signing"? they seem to fail when i comment out locally for me

nvm you're right specifically commenting out the self.context.signer_pending_commitment_update = true; still passes, will add coverage for this in a followup

Oh you were right, I was using the cfg flag last review but not this one lol. But yeah sounds good!

valentinewallace

Need to revisit the last non-test commit tomorrow and I'm still somewhat playing catch-up with the past signer work but this should be close. Thanks for tackling async signing!

lightning/src/ln/channel.rs

valentinewallace

LGTM after squash. There are a few places missing test coverage but I'm good to save them for follow-up.

valentinewallace · 2024-07-15T15:17:35Z

lightning/src/ln/channel.rs

 			self.get_last_commitment_update_for_send(logger).ok()
 		} else { None };
 		if self.context.resend_order == RAACommitmentOrder::CommitmentFirst
 			&& self.context.signer_pending_commitment_update && raa.is_some() {
 			self.context.signer_pending_revoke_and_ack = true;
 			raa = None;
 		}
+		if self.context.resend_order == RAACommitmentOrder::RevokeAndACKFirst
+			&& self.context.signer_pending_revoke_and_ack {
+			self.context.signer_pending_commitment_update = true;


Hmm, tests still pass with these lines commented out. I don't wanna hold up the PR, though, so feel free to save this for follow-up.

valentinewallace · 2024-07-15T15:53:23Z

lightning/src/ln/channel.rs

+				let commitment_update = if self.context.resend_order == RAACommitmentOrder::RevokeAndACKFirst
+					&& self.context.signer_pending_revoke_and_ack {
+					log_trace!(logger, "Reconnected channel {} with lost outbound RAA and lost remote commitment tx, but unable to send due to resend order, waiting on signer for revoke and ack", &self.context.channel_id());
+					self.context.signer_pending_commitment_update = true;


Missing coverage here

alecchendev · 2024-07-15T18:43:25Z

squashed with one small change updating the comment for this. Planning to add more test coverage in a follow up

valentinewallace

LGTM if CI's happy ✨

TheBlueMatt

Lets land it! We can do followups or whatever.

lightning/src/ln/channel.rs

lightning/src/sign/mod.rs

Includes simple changes to test util signers and tests, as well as handling the error case for get_per_commitment_point in HolderCommitmentPoint. This leaves a couple `.expect`s in places that will be addressed in a separate PR for handling funding.

Note: this does not test the CS -> RAA resend ordering, because this requires handling async get_per_commitment_point for channel reestablishment, which will be addressed in a follow up PR.

alecchendev · 2024-07-15T21:29:11Z

failed rustfmt so i just fixed the nits

TheBlueMatt · 2024-07-16T13:30:36Z

Changes since @valentinewallace's ACK are pretty trivial, worst case we can hash them out in a followup:

$ git diff-tree -U1  daa9baf 45c0a0f
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 2bed16bc5..be9d2d3ea 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -5499,5 +5499,5 @@ impl<SP: Deref> Channel<SP> where

-	fn get_last_revoke_and_ack<L: Deref>(&mut self, _logger: &L) -> Option<msgs::RevokeAndACK> where L::Target: Logger {
+	fn get_last_revoke_and_ack<L: Deref>(&mut self, logger: &L) -> Option<msgs::RevokeAndACK> where L::Target: Logger {
 		debug_assert!(self.context.holder_commitment_point.transaction_number() <= INITIAL_COMMITMENT_NUMBER - 2);
-		self.context.holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, _logger);
+		self.context.holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger);
 		let per_commitment_secret = self.context.holder_signer.as_ref().release_commitment_secret(self.context.holder_commitment_point.transaction_number() + 2);
@@ -5523,3 +5523,3 @@ impl<SP: Deref> Channel<SP> where
 				// we're only ever waiting on one commitment point at a time.
-				log_trace!(_logger, "Last revoke-and-ack pending in channel {} for sequence {} because the next per-commitment point is not available",
+				log_trace!(logger, "Last revoke-and-ack pending in channel {} for sequence {} because the next per-commitment point is not available",
 					&self.context.channel_id(), self.context.holder_commitment_point.transaction_number());
diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs
index 49f62a5bd..d684b4fe6 100644
--- a/lightning/src/sign/mod.rs
+++ b/lightning/src/sign/mod.rs
@@ -1358,4 +1358,5 @@ impl ChannelSigner for InMemorySigner {
 		let commitment_secret =
-			SecretKey::from_slice(&chan_utils::build_commitment_secret(&self.commitment_seed, idx));
-		commitment_secret.map(|secret| PublicKey::from_secret_key(secp_ctx, &secret)).map_err(|_| ())
+			SecretKey::from_slice(&chan_utils::build_commitment_secret(&self.commitment_seed, idx))
+				.unwrap();
+		Ok(PublicKey::from_secret_key(secp_ctx, &commitment_secret))
 	}
$

alecchendev mentioned this pull request Jun 30, 2024

Handle fallible per commitment point in channel establishment #3109

Open

alecchendev mentioned this pull request Jul 2, 2024

Handle fallible commitment secret #3152

Merged

alecchendev force-pushed the 2024-06-async-commit-point-raa branch from a079267 to 71cb518 Compare July 3, 2024 20:16

TheBlueMatt reviewed Jul 9, 2024

View reviewed changes

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

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

lightning/src/sign/mod.rs Outdated Show resolved Hide resolved

alecchendev force-pushed the 2024-06-async-commit-point-raa branch 3 times, most recently from 6bf0b5a to 28f4678 Compare July 10, 2024 01:25

TheBlueMatt previously approved these changes Jul 10, 2024

View reviewed changes

alecchendev dismissed TheBlueMatt’s stale review via eb30c39 July 11, 2024 18:03

alecchendev force-pushed the 2024-06-async-commit-point-raa branch from 28f4678 to eb30c39 Compare July 11, 2024 18:03

valentinewallace self-requested a review July 11, 2024 18:05

valentinewallace reviewed Jul 11, 2024

View reviewed changes

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

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

alecchendev force-pushed the 2024-06-async-commit-point-raa branch 2 times, most recently from ce899fa to 39da43c Compare July 12, 2024 20:39

valentinewallace reviewed Jul 15, 2024

View reviewed changes

alecchendev added 3 commits July 15, 2024 11:30

Split HolderCommitmentPoint::advance off into separate function

516e5e6

Return an error if we fail to advance our commitment number

9c2a050

Add logger as parameter in creating ChannelContext

1fa67d9

alecchendev force-pushed the 2024-06-async-commit-point-raa branch from 39da43c to daa9baf Compare July 15, 2024 18:41

valentinewallace previously approved these changes Jul 15, 2024

View reviewed changes

TheBlueMatt previously approved these changes Jul 15, 2024

View reviewed changes

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

lightning/src/sign/mod.rs Outdated Show resolved Hide resolved

alecchendev added 3 commits July 15, 2024 14:28

Allow failing revoke_and_ack if commitment point is not available

614da40

Test async get per commitment point for revoke_and_ack

45c0a0f

Note: this does not test the CS -> RAA resend ordering, because this requires handling async get_per_commitment_point for channel reestablishment, which will be addressed in a follow up PR.

alecchendev dismissed TheBlueMatt’s stale review via 45c0a0f July 15, 2024 21:28

alecchendev dismissed valentinewallace’s stale review via 45c0a0f July 15, 2024 21:28

alecchendev force-pushed the 2024-06-async-commit-point-raa branch from daa9baf to 45c0a0f Compare July 15, 2024 21:28

TheBlueMatt approved these changes Jul 16, 2024

View reviewed changes

TheBlueMatt merged commit 17d5baa into lightningdevkit:main Jul 16, 2024
15 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle fallible per commitment point for RAA #3150

Handle fallible per commitment point for RAA #3150

alecchendev commented Jun 30, 2024 •

edited

Loading

codecov-commenter commented Jun 30, 2024 •

edited

Loading

alecchendev commented Jul 3, 2024

TheBlueMatt commented Jul 9, 2024

TheBlueMatt left a comment

TheBlueMatt Jul 10, 2024

alecchendev Jul 11, 2024

valentinewallace Jul 11, 2024

TheBlueMatt Jul 11, 2024

alecchendev Jul 12, 2024

alecchendev Jul 12, 2024

valentinewallace Jul 15, 2024

alecchendev Jul 15, 2024

valentinewallace Jul 11, 2024 •

edited

Loading

alecchendev Jul 12, 2024

valentinewallace Jul 15, 2024

alecchendev Jul 15, 2024

alecchendev Jul 15, 2024

valentinewallace Jul 15, 2024

valentinewallace left a comment

valentinewallace left a comment

valentinewallace Jul 15, 2024

valentinewallace Jul 15, 2024

alecchendev commented Jul 15, 2024

valentinewallace left a comment

TheBlueMatt left a comment

alecchendev commented Jul 15, 2024

TheBlueMatt commented Jul 16, 2024

Handle fallible per commitment point for RAA #3150

Handle fallible per commitment point for RAA #3150

Conversation

alecchendev commented Jun 30, 2024 • edited Loading

codecov-commenter commented Jun 30, 2024 • edited Loading

Codecov Report

alecchendev commented Jul 3, 2024

TheBlueMatt commented Jul 9, 2024

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

valentinewallace Jul 11, 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

valentinewallace left a 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

alecchendev commented Jul 15, 2024

valentinewallace left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

alecchendev commented Jul 15, 2024

TheBlueMatt commented Jul 16, 2024

alecchendev commented Jun 30, 2024 •

edited

Loading

codecov-commenter commented Jun 30, 2024 •

edited

Loading

valentinewallace Jul 11, 2024 •

edited

Loading