Merge pull request #491 from TheBlueMatt/2020-02-one-conf-lock

Fix one-confirmation funding_locked generation

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)
 	}
 

lightning/src/ln/channelmanager.rs

@@ -1268,7 +1268,10 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 	}
 
 	fn get_announcement_sigs(&self, chan: &Channel<ChanSigner>) -> Option<msgs::AnnouncementSignatures> {
-		if !chan.should_announce() { return None }
+		if !chan.should_announce() {
+			log_trace!(self, "Can't send announcement_signatures for private channel {}", log_bytes!(chan.channel_id()));
+			return None
+		}
 
 		let (announcement, our_bitcoin_sig) = match chan.get_channel_announcement(self.get_our_node_id(), self.genesis_hash.clone()) {
 			Ok(res) => res,
@@ -1984,6 +1987,7 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 				}
 				try_chan_entry!(self, chan.get_mut().funding_locked(&msg), channel_state, chan);
 				if let Some(announcement_sigs) = self.get_announcement_sigs(chan.get()) {
+					log_trace!(self, "Sending announcement_signatures for {} in response to funding_locked", log_bytes!(chan.get().channel_id()));
 					// If we see locking block before receiving remote funding_locked, we broadcast our
 					// announcement_sigs at remote funding_locked reception. If we receive remote
 					// funding_locked before seeing locking block, we broadcast our announcement_sigs at locking
@@ -2578,10 +2582,13 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send> ChainListener for ChannelM
 						msg: funding_locked,
 					});
 					if let Some(announcement_sigs) = self.get_announcement_sigs(channel) {
+						log_trace!(self, "Sending funding_locked and announcement_signatures for {}", log_bytes!(channel.channel_id()));
 						pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures {
 							node_id: channel.get_their_node_id(),
 							msg: announcement_sigs,
 						});
+					} else {
+						log_trace!(self, "Sending funding_locked WITHOUT announcement_signatures for {}", log_bytes!(channel.channel_id()));
 					}
 					short_to_id.insert(channel.get_short_channel_id().unwrap(), channel.channel_id());
 				} else if let Err(e) = chan_res {

lightning/src/ln/functional_tests.rs

@@ -378,6 +378,41 @@ fn test_multi_flight_update_fee() {
 	check_added_monitors!(nodes[1], 1);
 }
 
+#[test]
+fn test_1_conf_open() {
+	// Previously, if the minium_depth config was set to 1, we'd never send a funding_locked. This
+	// tests that we properly send one in that case.
+	let mut alice_config = UserConfig::default();
+	alice_config.own_channel_config.minimum_depth = 1;
+	alice_config.channel_options.announced_channel = true;
+	alice_config.peer_channel_config_limits.force_announced_channel_preference = false;
+	let mut bob_config = UserConfig::default();
+	bob_config.own_channel_config.minimum_depth = 1;
+	bob_config.channel_options.announced_channel = true;
+	bob_config.peer_channel_config_limits.force_announced_channel_preference = false;
+	let node_cfgs = create_node_cfgs(2);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(alice_config), Some(bob_config)]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let tx = create_chan_between_nodes_with_value_init(&nodes[0], &nodes[1], 100000, 10001, InitFeatures::supported(), InitFeatures::supported());
+	assert!(nodes[0].chain_monitor.does_match_tx(&tx));
+	assert!(nodes[1].chain_monitor.does_match_tx(&tx));
+
+	let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+	nodes[1].block_notifier.block_connected_checked(&header, 1, &[&tx; 1], &[tx.version; 1]);
+	nodes[0].node.handle_funding_locked(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingLocked, nodes[0].node.get_our_node_id()));
+
+	nodes[0].block_notifier.block_connected_checked(&header, 1, &[&tx; 1], &[tx.version; 1]);
+	let (funding_locked, _) = create_chan_between_nodes_with_value_confirm_second(&nodes[1], &nodes[0]);
+	let (announcement, as_update, bs_update) = create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_locked);
+
+	for node in nodes {
+		assert!(node.router.handle_channel_announcement(&announcement).unwrap());
+		node.router.handle_channel_update(&as_update).unwrap();
+		node.router.handle_channel_update(&bs_update).unwrap();
+	}
+}
+
 #[test]
 fn test_update_fee_vanilla() {
 	let node_cfgs = create_node_cfgs(2);