Log and panic when fallen behind remote commitment number

[AnthonyRonning]

Pretty sure this is the source of many of our force closes. The if-else going on here does not consider when we have fallen behind, and the last else state assumes that the other peer is the one that is fallen behind, which is not accurate in all of our revoked transaction cases. When we get that log, it always ends with our users losing funds.

I just copied and pasted the identical text from above where some of the state is checked, not sure what exactly it should say.

[codecov-commenter]

Codecov Report

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

Comparison is base (1e25979) 88.80% compared to head (585bb07) 88.81%.
Report is 2 commits behind head on main.

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2717   +/-   ##
=======================================
  Coverage   88.80%   88.81%           
=======================================
  Files         113      113           
  Lines       89170    89179    +9     
  Branches    89170    89179    +9     
=======================================
+ Hits        79191    79205   +14     
+ Misses       7735     7726    -9     
- Partials     2244     2248    +4     

Files	Coverage Δ
lightning/src/ln/channel.rs	88.52% <0.00%> (-0.14%)	⬇️

... and 7 files with indirect coverage changes

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

[G8XSU]

Even if we can't panic (as matt suggests).
I think it would be helpful to at-least add a log.
I was thinking about it earlier when you posted to channel, currently we assume if commitment numbers don't match, peer must be stale. It could be helpful to distinguish b/w < and > cases here.

[TheBlueMatt]

I need to write some tests, but I do think there's an off-by-one in the original panic, can you try applying this and seeing if it fixes the issue:

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 1b5d6cfa7..b6a13bcd3 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -4159,7 +4159,7 @@ impl<SP: Deref> Channel<SP> where
                        if expected_point != PublicKey::from_secret_key(&self.context.secp_ctx, &given_secret) {
                                return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned()));
                        }
-                       if msg.next_remote_commitment_number > INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number {
+                       if msg.next_remote_commitment_number >= INITIAL_COMMITMENT_NUMBER - self.context.cur_holder_commitment_transaction_number {
                                macro_rules! log_and_panic {
                                        ($err_msg: expr) => {
                                                log_error!(logger, $err_msg, &self.context.channel_id, log_pubkey!(self.context.counterparty_node_id));

[TheBlueMatt]

Closing in favor of #2721 (which is the above patch ++)