From 8adf0cdc057b8aeb08625a77a40f11dd657785e2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 31 Oct 2018 13:09:38 -0400 Subject: [PATCH] Delay closing_signed until update_fee exchanges complete See https://github.com/lightningnetwork/lightning-rfc/issues/499 for a bit more about the ambiguity we're addressing here. Also drop holding cell update_fee the same way we drop holding cell update_add_htlcs when sending shutdown, resolving a bug. --- src/ln/channel.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/ln/channel.rs b/src/ln/channel.rs index 5ac52671741..d7bef3ff46a 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -1709,7 +1709,8 @@ impl Channel { let (msg, monitor) = self.send_commitment_no_status_check()?; (Some(msg), monitor, None) } else if !need_our_commitment && self.channel_state & (BOTH_SIDES_SHUTDOWN_MASK | ChannelState::AwaitingRemoteRevoke as u32) == BOTH_SIDES_SHUTDOWN_MASK && - self.channel_outbound && self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() && self.last_sent_closing_fee.is_none() { + self.channel_outbound && self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() && self.last_sent_closing_fee.is_none() && + self.cur_remote_commitment_transaction_number == self.cur_local_commitment_transaction_number { (None, self.channel_monitor.clone(), Some(self.propose_first_closing_signed(fee_estimator))) } else { (None, self.channel_monitor.clone(), None) }; @@ -1976,7 +1977,9 @@ impl Channel { commitment_signed }), to_forward_infos, revoked_htlcs, None, monitor_update)) } else { - if self.channel_state & BOTH_SIDES_SHUTDOWN_MASK == BOTH_SIDES_SHUTDOWN_MASK && self.channel_outbound && self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() && self.last_sent_closing_fee.is_none() { + if self.channel_state & BOTH_SIDES_SHUTDOWN_MASK == BOTH_SIDES_SHUTDOWN_MASK && self.channel_outbound && + self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() && self.last_sent_closing_fee.is_none() && + self.cur_remote_commitment_transaction_number == self.cur_local_commitment_transaction_number { Ok((None, to_forward_infos, revoked_htlcs, Some(self.propose_first_closing_signed(fee_estimator)), self.channel_monitor.clone())) } else { Ok((None, to_forward_infos, revoked_htlcs, None, self.channel_monitor.clone())) @@ -2149,6 +2152,9 @@ impl Channel { if self.channel_outbound { return Err(ChannelError::Close("Non-funding remote tried to update channel fee")); } + if (self.channel_state & (ChannelState::ChannelFunded as u32 | ChannelState::RemoteShutdownSent as u32)) != (ChannelState::ChannelFunded as u32) { + return Err(ChannelError::Close("Got update_fee message when channel was not in an operational state")); + } if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 { return Err(ChannelError::Close("Peer sent update_fee when we needed a channel_reestablish")); } @@ -2414,6 +2420,7 @@ impl Channel { // We can't send our shutdown until we've committed all of our pending HTLCs, but the // remote side is unlikely to accept any new HTLCs, so we go ahead and "free" any holding // cell HTLCs and return them to fail the payment. + self.holding_cell_update_fee = None; let mut dropped_outbound_htlcs = Vec::with_capacity(self.holding_cell_htlc_updates.len()); self.holding_cell_htlc_updates.retain(|htlc_update| { match htlc_update { @@ -2439,7 +2446,9 @@ impl Channel { self.channel_state |= ChannelState::LocalShutdownSent as u32; self.channel_update_count += 1; - if self.channel_outbound && self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() { + if self.channel_outbound && self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() && + self.cur_remote_commitment_transaction_number == self.cur_local_commitment_transaction_number && + self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32) == 0 { // There are no more HTLCs and we're the funder, this means we start the closing_signed // dance with an initial fee proposal! Ok((our_shutdown, Some(self.propose_first_closing_signed(fee_estimator)), dropped_outbound_htlcs)) @@ -3224,9 +3233,9 @@ impl Channel { } self.channel_update_count += 1; - // We can't send our shutdown until we've committed all of our pending HTLCs, but the - // remote side is unlikely to accept any new HTLCs, so we go ahead and "free" any holding - // cell HTLCs and return them to fail the payment. + // Go ahead and drop holding cell updates as we'd rather fail payments than wait to send + // our shutdown until we've committed all of the pending changes. + self.holding_cell_update_fee = None; let mut dropped_outbound_htlcs = Vec::with_capacity(self.holding_cell_htlc_updates.len()); self.holding_cell_htlc_updates.retain(|htlc_update| { match htlc_update {