From b02ccbb92f5826e1292f39826bb1008d3b0ffd78 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 10 Feb 2020 10:27:23 +0100 Subject: [PATCH] Fix sending funding_locked with 1 conf. We previously tracked funding transaction confirmation by marking funding_tx_confirmations to 1 when we see it in a block and incrementing each block thereafter if its non-0. To avoid double-incrementing the first confirmation, we did the increment (and funding_locked check) after doing the first-confirmation checks. Thus, we'd never hit the funding_locked case during the first confirmation. To address this, we simply swap the order of the checks, though bumping the funding_tx_confirmations increment up to the top. Reported-by: Igor Cota --- lightning/src/ln/channel.rs | 86 +++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 41 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 231ee832b..58ade789f 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2987,49 +2987,8 @@ impl Channel { pub fn block_connected(&mut self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[u32]) -> Result, msgs::ErrorMessage> { let non_shutdown_state = self.channel_state & (!MULTI_STATE_FLAGS); if header.bitcoin_hash() != self.last_block_connected { - self.last_block_connected = header.bitcoin_hash(); - self.channel_monitor.last_block_hash = self.last_block_connected; if self.funding_tx_confirmations > 0 { self.funding_tx_confirmations += 1; - if self.funding_tx_confirmations == self.minimum_depth as u64 { - let need_commitment_update = if non_shutdown_state == ChannelState::FundingSent as u32 { - self.channel_state |= ChannelState::OurFundingLocked as u32; - true - } else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32) { - self.channel_state = ChannelState::ChannelFunded as u32 | (self.channel_state & MULTI_STATE_FLAGS); - self.channel_update_count += 1; - true - } else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) { - // We got a reorg but not enough to trigger a force close, just update - // funding_tx_confirmed_in and return. - false - } else if self.channel_state < ChannelState::ChannelFunded as u32 { - panic!("Started confirming a channel in a state pre-FundingSent?: {}", self.channel_state); - } else { - // We got a reorg but not enough to trigger a force close, just update - // funding_tx_confirmed_in and return. - false - }; - self.funding_tx_confirmed_in = Some(header.bitcoin_hash()); - - //TODO: Note that this must be a duplicate of the previous commitment point they sent us, - //as otherwise we will have a commitment transaction that they can't revoke (well, kinda, - //they can by sending two revoke_and_acks back-to-back, but not really). This appears to be - //a protocol oversight, but I assume I'm just missing something. - if need_commitment_update { - if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 { - let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number); - let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret); - return Ok(Some(msgs::FundingLocked { - channel_id: self.channel_id, - next_per_commitment_point: next_per_commitment_point, - })); - } else { - self.monitor_pending_funding_locked = true; - return Ok(None); - } - } - } } } if non_shutdown_state & !(ChannelState::TheirFundingLocked as u32) == ChannelState::FundingSent as u32 { @@ -3072,6 +3031,51 @@ impl Channel { } } } + if header.bitcoin_hash() != self.last_block_connected { + self.last_block_connected = header.bitcoin_hash(); + self.channel_monitor.last_block_hash = self.last_block_connected; + if self.funding_tx_confirmations > 0 { + if self.funding_tx_confirmations == self.minimum_depth as u64 { + let need_commitment_update = if non_shutdown_state == ChannelState::FundingSent as u32 { + self.channel_state |= ChannelState::OurFundingLocked as u32; + true + } else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32) { + self.channel_state = ChannelState::ChannelFunded as u32 | (self.channel_state & MULTI_STATE_FLAGS); + self.channel_update_count += 1; + true + } else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) { + // We got a reorg but not enough to trigger a force close, just update + // funding_tx_confirmed_in and return. + false + } else if self.channel_state < ChannelState::ChannelFunded as u32 { + panic!("Started confirming a channel in a state pre-FundingSent?: {}", self.channel_state); + } else { + // We got a reorg but not enough to trigger a force close, just update + // funding_tx_confirmed_in and return. + false + }; + self.funding_tx_confirmed_in = Some(header.bitcoin_hash()); + + //TODO: Note that this must be a duplicate of the previous commitment point they sent us, + //as otherwise we will have a commitment transaction that they can't revoke (well, kinda, + //they can by sending two revoke_and_acks back-to-back, but not really). This appears to be + //a protocol oversight, but I assume I'm just missing something. + if need_commitment_update { + if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 { + let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number); + let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret); + return Ok(Some(msgs::FundingLocked { + channel_id: self.channel_id, + next_per_commitment_point: next_per_commitment_point, + })); + } else { + self.monitor_pending_funding_locked = true; + return Ok(None); + } + } + } + } + } Ok(None) } -- 2.39.5