Fix panic on receiving `channel_ready` after 1st commitment update

`cur_counterparty_commitment_transaction_number` starts at
`INITIAL_COMMITMENT_NUMBER`, gets decremented once when the initial
`channel_ready` message is received, and gets decremented a second
time when the first `revoke_and_ack` is received, revoking the
first counterparty commitment point.

At this point, `counterparty_prev_commitment_point` points to the
non-revoked second commitment point.

If we then process a second `channel_ready`, we check the
`cur_counterparty_commitment_transaction_number` number, see that
if is `INITIAL_COMMITMENT_NUMBER - 2` (i.e. not `- 1`) and assume
that the *second* commitment number has been revoked (by
`expect`ing `CounterpartyCommitmentSecrets::get_secret` with
`INITIAL_COMMITMENT_NUMBER - 1`). This `expect` panic's.

As the second commitment point has not yet been revoked, we should
fetch it from `counterparty_prev_commitment_point`, which we do
here, adding a test which failed on the previous code as well.

Found by the `full_stack_target` fuzzer.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 76dc15065c05cd298b532247de377b757ce5491f..ec2810d14cbd8cce512956cb8f7d930f55a8d30f 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -2426,6 +2426,11 @@ impl<Signer: Sign> Channel<Signer> {
                                        // If they haven't ever sent an updated point, the point they send should match
                                        // the current one.
                                        self.counterparty_cur_commitment_point
+                               } else if self.cur_counterparty_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 2 {
+                                       // If we've advanced the commitment number once, the second commitment point is
+                                       // at `counterparty_prev_commitment_point`, which is not yet revoked.
+                                       debug_assert!(self.counterparty_prev_commitment_point.is_some());
+                                       self.counterparty_prev_commitment_point
                                } else {
                                        // If they have sent updated points, channel_ready is always supposed to match
                                        // their "first" point, which we re-derive here.

diff --git a/lightning/src/ln/priv_short_conf_tests.rs b/lightning/src/ln/priv_short_conf_tests.rs
index bac542996e7e75df4dde7acceb9f224914047940..44d4cd9cd569176f60045610431a577c950a3a78 100644 (file)
--- a/lightning/src/ln/priv_short_conf_tests.rs
+++ b/lightning/src/ln/priv_short_conf_tests.rs
@@ -245,6 +245,13 @@ fn test_routed_scid_alias() {
        check_added_monitors!(nodes[0], 1);
 
        pass_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], 100_000, payment_hash, payment_secret);
+
+       as_channel_ready.short_channel_id_alias = Some(0xeadbeef);
+       nodes[2].node.handle_channel_ready(&nodes[1].node.get_our_node_id(), &as_channel_ready);
+       // Note that we always respond to a channel_ready with a channel_update. Not a lot of reason
+       // to bother updating that code, so just drop the message here.
+       get_event_msg!(nodes[2], MessageSendEvent::SendChannelUpdate, nodes[1].node.get_our_node_id());
+
        claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage);
 
        // Now test that if a peer sends us a second channel_ready after the channel is operational we