Handle missing case in reestablish local commitment number checks #2721

TheBlueMatt · 2023-11-09T03:37:37Z

If we're behind exactly one commitment (which we've revoked), we'd
previously force-close the channel, guaranteeing we'll lose funds
as the counterparty has our latest local commitment state's
revocation secret.

While this shouldn't happen because users should never lose data,
sometimes issues happen, and we should ensure we always panic.

Further, test_data_loss_protect is updated to test this case.

The data loss protect test was panicking in a message assertion which should be passing, but because the test was marked only `#[should_panic]` it was being treated as a successful outcome. Instead, we use `catch_unwind` on exactly the line we expect to panic to ensure we are hitting the right one.

codecov-commenter · 2023-11-09T03:43:29Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (1e25979) 88.80% compared to head (ac3fd98) 90.09%.
Report is 68 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/reload_tests.rs	93.57%	7 Missing ⚠️
lightning/src/ln/channel.rs	88.88%	3 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2721      +/-   ##
==========================================
+ Coverage   88.80%   90.09%   +1.28%     
==========================================
  Files         113      114       +1     
  Lines       89170   101278   +12108     
  Branches    89170   101278   +12108     
==========================================
+ Hits        79191    91243   +12052     
+ Misses       7735     7647      -88     
- Partials     2244     2388     +144

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

TonyGiorgio · 2023-11-09T05:17:13Z

we should ensure we always panic

I dont think that the PR is doing that though, it just logs and closes the channel with a different case but it would still result in loss of funds from the looks of it.

benthecarman · 2023-11-09T05:46:21Z

I dont think that the PR is doing that though, it just logs and closes the channel with a different case but it would still result in loss of funds from the looks of it.

the off by one is fixed in the final commit

benthecarman · 2023-11-09T05:58:02Z

lightning/src/ln/channel.rs

 		} else {
-			Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction".to_owned()))
+			Err(ChannelError::Close(format!(
+				"Peer attempted to reestablish channel with a future local commitment transaction: {} (received) vs {} (expected)",


If they have a future commitment shouldn't we just send them an an error and have them close, so we don't close with a revoked state

In this case we won't be closing with a revoked state, it implies we have lost some state, but only a new commitment transaction, we haven't revoked the latest state we have, so its still safe to broadcast.

lightning/src/ln/channel.rs

benthecarman · 2023-11-09T06:44:28Z

lightning/src/ln/channel.rs

@@ -4295,8 +4302,18 @@ impl<SP: Deref> Channel<SP> where
 					order: self.context.resend_order.clone(),
 				})
 			}
+		} else if msg.next_local_commitment_number < next_counterparty_commitment_number {


the case before this is else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1 which would qualify for this as well. Should this be msg.next_local_commitment_number < next_counterparty_commitment_number - 1?

Its an else if and and the intent is to capture any "they're behind us" case (that we cannot recover from, which is handled in the above conditionals).

I just think it'd be more clear if this was msg.next_local_commitment_number < next_counterparty_commitment_number - 1 since the statement before is else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1, shouldn't have to rely on the -1 in the previous statement to make this one correct.

Oh, oh, okay so that's where the disagreement is. So the statement in the error message (even if not closing in response, or the "very" part of the comment) is correct with or without the - 1. Given the else clause is "future", it seems better to use "very old" in the "one step old" case.

shaavan

Concept ACK!

This pull request effectively addresses the scenario where the next_remote_commitment_number is equal to our_commitment_transaction + 1. Previously, this specific case was not accounted for, leading to the triggering of the else statement and resulting in a final close and hence loss of funds. Now, with the recent changes, this situation is properly handled under the 'We have fallen behind case…'

I'm planning to review the test soon and will share my complete review shortly!

TonyGiorgio · 2023-11-20T18:32:15Z

FYI, since we started panic-ing here instead of allowing it to force close when running behind, we have had a lot less force closes and none that have resulted in justice transactions. And no reports of errors on the wallet side yet.

wpaulino

LGTM after squash

lightning/src/ln/channel.rs

When we reestablish there are generally always 4 conditions for both local and remote commitment transactions: * we're stale and have possibly lost data * we're ahead and the peer has lost data * we're caught up * we're nearly caught up and need to retransmit one update. In especially the local commitment case we had a mess of different comparisons, which is improved here. Further, the error messages are clarified and include more information.

If we're behind exactly one commitment (which we've revoked), we'd previously force-close the channel, guaranteeing we'll lose funds as the counterparty has our latest local commitment state's revocation secret. While this shouldn't happen because users should never lose data, sometimes issues happen, and we should ensure we always panic. Further, `test_data_loss_protect` is updated to test this case.

TheBlueMatt · 2023-11-28T00:02:06Z

Squashed without any changes -

$ git diff-tree -U1 1e4c520a ac3fd98e
$

arik-so · 2023-11-29T06:53:34Z

lightning/src/ln/channel.rs

@@ -4152,14 +4152,15 @@ impl<SP: Deref> Channel<SP> where
 			return Err(ChannelError::Close("Peer sent an invalid channel_reestablish to force close in a non-standard way".to_owned()));
 		}

+		let our_commitment_transaction = INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number - 1;


somewhat poor choice for a variable name that's a number/index, and not an actual tx

TheBlueMatt mentioned this pull request Nov 9, 2023

Log and panic when fallen behind remote commitment number #2717

Closed

TheBlueMatt force-pushed the 2023-11-log-forward-peer branch from 4a5d7f7 to cc441dc Compare November 9, 2023 04:14

benthecarman reviewed Nov 9, 2023

View reviewed changes

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

benthecarman reviewed Nov 9, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-11-log-forward-peer branch from cc441dc to c4e9da4 Compare November 9, 2023 21:32

shaavan reviewed Nov 20, 2023

View reviewed changes

wpaulino reviewed Nov 22, 2023

View reviewed changes

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

TheBlueMatt added 2 commits November 27, 2023 23:59

TheBlueMatt force-pushed the 2023-11-log-forward-peer branch from 1e4c520 to ac3fd98 Compare November 27, 2023 23:59

wpaulino approved these changes Nov 28, 2023

View reviewed changes

TheBlueMatt added this to the 0.0.119 milestone Nov 28, 2023

arik-so approved these changes Nov 29, 2023

View reviewed changes

wpaulino merged commit c2bbfff into lightningdevkit:main Nov 29, 2023
14 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle missing case in reestablish local commitment number checks #2721

Handle missing case in reestablish local commitment number checks #2721

TheBlueMatt commented Nov 9, 2023

codecov-commenter commented Nov 9, 2023 •

edited

Loading

TonyGiorgio commented Nov 9, 2023

benthecarman commented Nov 9, 2023

benthecarman Nov 9, 2023

TheBlueMatt Nov 9, 2023

benthecarman Nov 9, 2023

TheBlueMatt Nov 9, 2023

benthecarman Nov 9, 2023

TheBlueMatt Nov 13, 2023

shaavan left a comment

TonyGiorgio commented Nov 20, 2023

wpaulino left a comment

TheBlueMatt commented Nov 28, 2023

arik-so Nov 29, 2023

Handle missing case in reestablish local commitment number checks #2721

Handle missing case in reestablish local commitment number checks #2721

Conversation

TheBlueMatt commented Nov 9, 2023

codecov-commenter commented Nov 9, 2023 • edited Loading

Codecov Report

TonyGiorgio commented Nov 9, 2023

benthecarman commented Nov 9, 2023

benthecarman Nov 9, 2023

Choose a reason for hiding this comment

TheBlueMatt Nov 9, 2023

Choose a reason for hiding this comment

benthecarman Nov 9, 2023

Choose a reason for hiding this comment

TheBlueMatt Nov 9, 2023

Choose a reason for hiding this comment

benthecarman Nov 9, 2023

Choose a reason for hiding this comment

TheBlueMatt Nov 13, 2023

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

TonyGiorgio commented Nov 20, 2023

wpaulino left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Nov 28, 2023

arik-so Nov 29, 2023

Choose a reason for hiding this comment

codecov-commenter commented Nov 9, 2023 •

edited

Loading