Delay creating outbound ChannelMonitor until funding_signed rcpt

Previously, we created the initial ChannelMonitor on outbound
channels when we generated the funding_created message. This was
somewhat unnecessary as, at that time, we hadn't yet received
clearance to broadcast our initial funding transaction, and thus
there should never be any use for a ChannelMonitor. It also
complicated ChannelMonitor a bit as, at this point, we didn't have
an initial local commitment transaction.

By moving the creation of the initial ChannelMonitor to when we
receive our counterparty's funding_signed, we can ensure that any
ChannelMonitor will always have both a latest remote commitment tx
and a latest local commitment tx for broadcast.

This also fixes a strange API where we would close a channel
unceremoniously on peer-disconnection if we hadn't yet received the
funding_signed, but we'd already have a ChannelMonitor for that
channel. While it isn't strictly a bug (some potential DoS issues
aside), it is strange that these two definitions of a channel being
open were not in sync.

Author: TheBlueMatt

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1629,12 +1629,9 @@ fn monitor_update_claim_fail_no_response() {
 	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
-// Note that restore_between_fails with !fail_on_generate is useless
-// Also note that !fail_on_generate && !fail_on_signed is useless
-// Finally, note that !fail_on_signed is not possible with fail_on_generate && !restore_between_fails
 // confirm_a_first and restore_b_before_conf are wholly unrelated to earlier bools and
 // restore_b_before_conf has no meaning if !confirm_a_first
-fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails: bool, fail_on_signed: bool, confirm_a_first: bool, restore_b_before_conf: bool) {
+fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf: bool) {
 	// Test that if the monitor update generated by funding_transaction_generated fails we continue
 	// the channel setup happily after the update is restored.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
@@ -1648,52 +1645,25 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 	let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43);
 
-	if fail_on_generate {
-		*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	}
 	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
-	check_added_monitors!(nodes[0], 1);
+	check_added_monitors!(nodes[0], 0);
 
 	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
 	let channel_id = OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
 	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
 	check_added_monitors!(nodes[1], 1);
 
-	if restore_between_fails {
-		assert!(fail_on_generate);
-		*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
-		let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
-		nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
-		check_added_monitors!(nodes[0], 0);
-		assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
-		assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-	}
-
-	if fail_on_signed {
-		*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	} else {
-		assert!(restore_between_fails || !fail_on_generate); // We can't switch to good now (there's no monitor update)
-		assert!(fail_on_generate); // Somebody has to fail
-	}
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
-	if fail_on_signed || !restore_between_fails {
-		assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-		if fail_on_generate && !restore_between_fails {
-			nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Previous monitor update failure prevented funding_signed from allowing funding broadcast".to_string(), 1);
-			check_added_monitors!(nodes[0], 1);
-		} else {
-			nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
-			check_added_monitors!(nodes[0], 1);
-		}
-		assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
-		*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
-		let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
-		nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
-		check_added_monitors!(nodes[0], 0);
-	} else {
-		check_added_monitors!(nodes[0], 1);
-	}
+	assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+	nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Failed to update ChannelMonitor".to_string(), 1);
+	check_added_monitors!(nodes[0], 1);
+	assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
+	*nodes[0].chan_monitor.update_ret.lock().unwrap() = Ok(());
+	let (outpoint, latest_update) = nodes[0].chan_monitor.latest_monitor_update_id.lock().unwrap().get(&channel_id).unwrap().clone();
+	nodes[0].node.channel_monitor_updated(&outpoint, latest_update);
+	check_added_monitors!(nodes[0], 0);
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
@@ -1757,8 +1727,7 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 
 #[test]
 fn during_funding_monitor_fail() {
-	do_during_funding_monitor_fail(false, false, true, true, true);
-	do_during_funding_monitor_fail(true, false, true, false, false);
-	do_during_funding_monitor_fail(true, true, true, true, false);
-	do_during_funding_monitor_fail(true, true, false, false, false);
+	do_during_funding_monitor_fail(true, true);
+	do_during_funding_monitor_fail(true, false);
+	do_during_funding_monitor_fail(false, false);
 }

lightning/src/ln/channel.rs

@@ -1539,50 +1539,65 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 	/// Handles a funding_signed message from the remote end.
 	/// If this call is successful, broadcast the funding transaction (and not before!)
-	pub fn funding_signed(&mut self, msg: &msgs::FundingSigned) -> Result<ChannelMonitorUpdate, (Option<ChannelMonitorUpdate>, ChannelError)> {
+	pub fn funding_signed(&mut self, msg: &msgs::FundingSigned) -> Result<ChannelMonitor<ChanSigner>, ChannelError> {
 		if !self.channel_outbound {
-			return Err((None, ChannelError::Close("Received funding_signed for an inbound channel?")));
+			return Err(ChannelError::Close("Received funding_signed for an inbound channel?"));
 		}
 		if self.channel_state & !(ChannelState::MonitorUpdateFailed as u32) != ChannelState::FundingCreated as u32 {
-			return Err((None, ChannelError::Close("Received funding_signed in strange state!")));
+			return Err(ChannelError::Close("Received funding_signed in strange state!"));
 		}
 		if self.commitment_secrets.get_min_seen_secret() != (1 << 48) ||
-				self.cur_remote_commitment_transaction_number != INITIAL_COMMITMENT_NUMBER - 1 ||
+				self.cur_remote_commitment_transaction_number != INITIAL_COMMITMENT_NUMBER ||
 				self.cur_local_commitment_transaction_number != INITIAL_COMMITMENT_NUMBER {
 			panic!("Should not have advanced channel commitment tx numbers prior to funding_created");
 		}
 
 		let funding_script = self.get_funding_redeemscript();
 
-		let local_keys = self.build_local_transaction_keys(self.cur_local_commitment_transaction_number).map_err(|e| (None, e))?;
+		let remote_keys = self.build_remote_transaction_keys()?;
+		let remote_initial_commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, false, self.feerate_per_kw).0;
+
+		let local_keys = self.build_local_transaction_keys(self.cur_local_commitment_transaction_number)?;
 		let local_initial_commitment_tx = self.build_commitment_transaction(self.cur_local_commitment_transaction_number, &local_keys, true, false, self.feerate_per_kw).0;
 		let local_sighash = hash_to_message!(&bip143::SighashComponents::new(&local_initial_commitment_tx).sighash_all(&local_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]);
 
 		let their_funding_pubkey = &self.their_pubkeys.as_ref().unwrap().funding_pubkey;
 
 		// They sign the "local" commitment transaction, allowing us to broadcast the tx if we wish.
 		if let Err(_) = self.secp_ctx.verify(&local_sighash, &msg.signature, their_funding_pubkey) {
-			return Err((None, ChannelError::Close("Invalid funding_signed signature from peer")));
+			return Err(ChannelError::Close("Invalid funding_signed signature from peer"));
 		}
 
-		self.latest_monitor_update_id += 1;
-		let monitor_update = ChannelMonitorUpdate {
-			update_id: self.latest_monitor_update_id,
-			updates: vec![ChannelMonitorUpdateStep::LatestLocalCommitmentTXInfo {
-				commitment_tx: LocalCommitmentTransaction::new_missing_local_sig(local_initial_commitment_tx, &msg.signature, &PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()), their_funding_pubkey, local_keys, self.feerate_per_kw, Vec::new()),
-				htlc_outputs: Vec::new(),
-			}]
-		};
-		self.channel_monitor.as_mut().unwrap().update_monitor_ooo(monitor_update.clone()).unwrap();
-		self.channel_state = ChannelState::FundingSent as u32 | (self.channel_state & (ChannelState::MonitorUpdateFailed as u32));
-		self.cur_local_commitment_transaction_number -= 1;
+		let their_pubkeys = self.their_pubkeys.as_ref().unwrap();
+		let funding_redeemscript = self.get_funding_redeemscript();
+		let funding_txo = self.funding_txo.as_ref().unwrap();
+		let funding_txo_script = funding_redeemscript.to_v0_p2wsh();
+		macro_rules! create_monitor {
+			() => { {
+				let mut channel_monitor = ChannelMonitor::new(self.local_keys.clone(),
+				                                              &self.shutdown_pubkey, self.our_to_self_delay,
+				                                              &self.destination_script, (funding_txo.clone(), funding_txo_script.clone()),
+				                                              &their_pubkeys.htlc_basepoint, &their_pubkeys.delayed_payment_basepoint,
+				                                              self.their_to_self_delay, funding_redeemscript.clone(), self.channel_value_satoshis,
+				                                              self.get_commitment_transaction_number_obscure_factor(),
+				                                              self.logger.clone());
 
-		if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
-			Ok(monitor_update)
-		} else {
-			Err((Some(monitor_update),
-				ChannelError::Ignore("Previous monitor update failure prevented funding_signed from allowing funding broadcast")))
+				channel_monitor.provide_latest_remote_commitment_tx_info(&remote_initial_commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap());
+				channel_monitor.provide_latest_local_commitment_tx_info(LocalCommitmentTransaction::new_missing_local_sig(local_initial_commitment_tx.clone(), &msg.signature, &PublicKey::from_secret_key(&self.secp_ctx, self.local_keys.funding_key()), their_funding_pubkey, local_keys.clone(), self.feerate_per_kw, Vec::new()), Vec::new()).unwrap();
+
+				channel_monitor
+			} }
 		}
+
+		self.channel_monitor = Some(create_monitor!());
+		let channel_monitor = create_monitor!();
+
+		assert_eq!(self.channel_state & (ChannelState::MonitorUpdateFailed as u32), 0); // We have no had any monitor(s) yet to fail update!
+		self.channel_state = ChannelState::FundingSent as u32;
+		self.cur_local_commitment_transaction_number -= 1;
+		self.cur_remote_commitment_transaction_number -= 1;
+
+		Ok(channel_monitor)
 	}
 
 	pub fn funding_locked(&mut self, msg: &msgs::FundingLocked) -> Result<(), ChannelError> {
@@ -2951,7 +2966,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 	/// May only be called after funding has been initiated (ie is_funding_initiated() is true)
 	pub fn channel_monitor(&mut self) -> &mut ChannelMonitor<ChanSigner> {
-		if self.channel_state < ChannelState::FundingCreated as u32 {
+		if self.channel_state < ChannelState::FundingSent as u32 {
 			panic!("Can't get a channel monitor until funding has been created");
 		}
 		self.channel_monitor.as_mut().unwrap()
@@ -3105,7 +3120,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 	/// Returns true if funding_created was sent/received.
 	pub fn is_funding_initiated(&self) -> bool {
-		self.channel_state >= ChannelState::FundingCreated as u32
+		self.channel_state >= ChannelState::FundingSent as u32
 	}
 
 	/// Returns true if this channel is fully shut down. True here implies that no further actions
@@ -3337,11 +3352,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	}
 
 	/// If an Err is returned, it is a ChannelError::Close (for get_outbound_funding_created)
-	fn get_outbound_funding_created_signature(&mut self) -> Result<(Signature, Transaction), ChannelError> {
+	fn get_outbound_funding_created_signature(&mut self) -> Result<Signature, ChannelError> {
 		let remote_keys = self.build_remote_transaction_keys()?;
 		let remote_initial_commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, false, self.feerate_per_kw).0;
-		Ok((self.local_keys.sign_remote_commitment(self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), self.our_to_self_delay, &self.secp_ctx)
-				.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed"))?.0, remote_initial_commitment_tx))
+		Ok(self.local_keys.sign_remote_commitment(self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), self.our_to_self_delay, &self.secp_ctx)
+				.map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed"))?.0)
 	}
 
 	/// Updates channel state with knowledge of the funding transaction's txid/index, and generates
@@ -3351,7 +3366,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	/// Note that channel_id changes during this call!
 	/// Do NOT broadcast the funding transaction until after a successful funding_signed call!
 	/// If an Err is returned, it is a ChannelError::Close.
-	pub fn get_outbound_funding_created(&mut self, funding_txo: OutPoint) -> Result<(msgs::FundingCreated, ChannelMonitor<ChanSigner>), ChannelError> {
+	pub fn get_outbound_funding_created(&mut self, funding_txo: OutPoint) -> Result<msgs::FundingCreated, ChannelError> {
 		if !self.channel_outbound {
 			panic!("Tried to create outbound funding_created message on an inbound channel!");
 		}
@@ -3365,7 +3380,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		}
 
 		self.funding_txo = Some(funding_txo.clone());
-		let (our_signature, commitment_tx) = match self.get_outbound_funding_created_signature() {
+		let our_signature = match self.get_outbound_funding_created_signature() {
 			Ok(res) => res,
 			Err(e) => {
 				log_error!(self, "Got bad signatures: {:?}!", e);
@@ -3378,37 +3393,15 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 		// Now that we're past error-generating stuff, update our local state:
 
-		let their_pubkeys = self.their_pubkeys.as_ref().unwrap();
-		let funding_redeemscript = self.get_funding_redeemscript();
-		let funding_txo_script = funding_redeemscript.to_v0_p2wsh();
-		macro_rules! create_monitor {
-			() => { {
-				let mut channel_monitor = ChannelMonitor::new(self.local_keys.clone(),
-				                                              &self.shutdown_pubkey, self.our_to_self_delay,
-				                                              &self.destination_script, (funding_txo, funding_txo_script.clone()),
-				                                              &their_pubkeys.htlc_basepoint, &their_pubkeys.delayed_payment_basepoint,
-				                                              self.their_to_self_delay, funding_redeemscript.clone(), self.channel_value_satoshis,
-				                                              self.get_commitment_transaction_number_obscure_factor(),
-				                                              self.logger.clone());
-
-				channel_monitor.provide_latest_remote_commitment_tx_info(&commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap());
-				channel_monitor
-			} }
-		}
-
-		self.channel_monitor = Some(create_monitor!());
-		let channel_monitor = create_monitor!();
-
 		self.channel_state = ChannelState::FundingCreated as u32;
 		self.channel_id = funding_txo.to_channel_id();
-		self.cur_remote_commitment_transaction_number -= 1;
 
-		Ok((msgs::FundingCreated {
-			temporary_channel_id: temporary_channel_id,
+		Ok(msgs::FundingCreated {
+			temporary_channel_id,
 			funding_txid: funding_txo.txid,
 			funding_output_index: funding_txo.index,
 			signature: our_signature
-		}, channel_monitor))
+		})
 	}
 
 	/// Gets an UnsignedChannelAnnouncement, as well as a signature covering it using our
@@ -4385,7 +4378,7 @@ mod tests {
 			value: 10000000, script_pubkey: output_script.clone(),
 		}]};
 		let funding_outpoint = OutPoint::new(tx.txid(), 0);
-		let (funding_created_msg, _) = node_a_chan.get_outbound_funding_created(funding_outpoint).unwrap();
+		let funding_created_msg = node_a_chan.get_outbound_funding_created(funding_outpoint).unwrap();
 		let (funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg).unwrap();
 
 		// Node B --> Node A: funding signed

lightning/src/ln/channelmanager.rs

@@ -1412,7 +1412,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 	pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_txo: OutPoint) {
 		let _ = self.total_consistency_lock.read().unwrap();
 
-		let (mut chan, msg, chan_monitor) = {
+		let (chan, msg) = {
 			let (res, chan) = match self.channel_state.lock().unwrap().by_id.remove(temporary_channel_id) {
 				Some(mut chan) => {
 					(chan.get_outbound_funding_created(funding_txo)
@@ -1425,30 +1425,11 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 			};
 			match handle_error!(self, res, chan.get_their_node_id()) {
 				Ok(funding_msg) => {
-					(chan, funding_msg.0, funding_msg.1)
+					(chan, funding_msg)
 				},
 				Err(_) => { return; }
 			}
 		};
-		// Because we have exclusive ownership of the channel here we can release the channel_state
-		// lock before add_monitor
-		if let Err(e) = self.monitor.add_monitor(chan_monitor.get_funding_txo(), chan_monitor) {
-			match e {
-				ChannelMonitorUpdateErr::PermanentFailure => {
-					match handle_error!(self, Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure", *temporary_channel_id, chan.force_shutdown(true), None)), chan.get_their_node_id()) {
-						Err(_) => { return; },
-						Ok(()) => unreachable!(),
-					}
-				},
-				ChannelMonitorUpdateErr::TemporaryFailure => {
-					// Its completely fine to continue with a FundingCreated until the monitor
-					// update is persisted, as long as we don't generate the FundingBroadcastSafe
-					// until the monitor has been safely persisted (as funding broadcast is not,
-					// in fact, safe).
-					chan.monitor_update_failed(false, false, Vec::new(), Vec::new());
-				},
-			}
-		}
 
 		let mut channel_state = self.channel_state.lock().unwrap();
 		channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingCreated {
@@ -2337,17 +2318,11 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 					if chan.get().get_their_node_id() != *their_node_id {
 						return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!", msg.channel_id));
 					}
-					let monitor_update = match chan.get_mut().funding_signed(&msg) {
-						Err((None, e)) => try_chan_entry!(self, Err(e), channel_state, chan),
-						Err((Some(monitor_update), e)) => {
-							assert!(chan.get().is_awaiting_monitor_update());
-							let _ = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update);
-							try_chan_entry!(self, Err(e), channel_state, chan);
-							unreachable!();
-						},
+					let monitor = match chan.get_mut().funding_signed(&msg) {
 						Ok(update) => update,
+						Err(e) => try_chan_entry!(self, Err(e), channel_state, chan),
 					};
-					if let Err(e) = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update) {
+					if let Err(e) = self.monitor.add_monitor(chan.get().get_funding_txo().unwrap(), monitor) {
 						return_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::RevokeAndACKFirst, false, false);
 					}
 					(chan.get().get_funding_txo().unwrap(), chan.get().get_user_id())