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 <[email protected]>

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -2987,49 +2987,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	pub fn block_connected(&mut self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[u32]) -> Result<Option<msgs::FundingLocked>, 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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 				}
 			}
 		}
+		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)
 	}