Take the full funding transaction from the user on generation

Instead of relying on the user to ensure the funding transaction is
correct (and panicing when it is confirmed), we should check it is
correct when it is generated. By taking the full funding transaciton
from the user on generation, we can also handle broadcasting for
them instead of doing so via an event.

Author: TheBlueMatt

background-processor/src/lib.rs

@@ -203,7 +203,7 @@ mod tests {
 				_ => panic!("Unexpected event"),
 			};
 
-			$node_a.node.funding_transaction_generated(&temporary_channel_id, funding_output);
+			$node_a.node.funding_transaction_generated(&temporary_channel_id, tx.clone(), funding_output.index).unwrap();
 			$node_b.node.handle_funding_created(&$node_a.node.get_our_node_id(), &get_event_msg!($node_a, MessageSendEvent::SendFundingCreated, $node_b.node.get_our_node_id()));
 			$node_a.node.handle_funding_signed(&$node_b.node.get_our_node_id(), &get_event_msg!($node_b, MessageSendEvent::SendFundingSigned, $node_a.node.get_our_node_id()));
 			tx

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1825,7 +1825,7 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf:
 
 	let (temporary_channel_id, funding_tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 43);
 
-	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
+	nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_tx.clone(), funding_output.index).unwrap();
 	check_added_monitors!(nodes[0], 0);
 
 	*nodes[1].chain_monitor.update_ret.lock().unwrap() = Some(Err(ChannelMonitorUpdateErr::TemporaryFailure));
@@ -1846,14 +1846,9 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf:
 	check_added_monitors!(nodes[0], 0);
 
 	let events = nodes[0].node.get_and_clear_pending_events();
-	assert_eq!(events.len(), 1);
-	match events[0] {
-		Event::FundingBroadcastSafe { ref funding_txo, user_channel_id } => {
-			assert_eq!(user_channel_id, 43);
-			assert_eq!(*funding_txo, funding_output);
-		},
-		_ => panic!("Unexpected event"),
-	};
+	assert_eq!(events.len(), 0);
+	assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1);
+	assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0)[0].txid(), funding_output.txid);
 
 	if confirm_a_first {
 		confirm_transaction(&nodes[0], &funding_tx);

lightning/src/ln/channel.rs

@@ -409,9 +409,9 @@ pub(super) struct Channel<Signer: Sign> {
 	counterparty_forwarding_info: Option<CounterpartyForwardingInfo>,
 
 	pub(crate) channel_transaction_parameters: ChannelTransactionParameters,
+	funding_transaction: Option<Transaction>,
 
 	counterparty_cur_commitment_point: Option<PublicKey>,
-
 	counterparty_prev_commitment_point: Option<PublicKey>,
 	counterparty_node_id: PublicKey,
 
@@ -603,8 +603,9 @@ impl<Signer: Sign> Channel<Signer> {
 				counterparty_parameters: None,
 				funding_outpoint: None
 			},
-			counterparty_cur_commitment_point: None,
+			funding_transaction: None,
 
+			counterparty_cur_commitment_point: None,
 			counterparty_prev_commitment_point: None,
 			counterparty_node_id,
 
@@ -844,8 +845,9 @@ impl<Signer: Sign> Channel<Signer> {
 				}),
 				funding_outpoint: None
 			},
-			counterparty_cur_commitment_point: Some(msg.first_per_commitment_point),
+			funding_transaction: None,
 
+			counterparty_cur_commitment_point: Some(msg.first_per_commitment_point),
 			counterparty_prev_commitment_point: None,
 			counterparty_node_id,
 
@@ -1608,7 +1610,7 @@ impl<Signer: Sign> Channel<Signer> {
 
 	/// 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<L: Deref>(&mut self, msg: &msgs::FundingSigned, last_block_hash: BlockHash, logger: &L) -> Result<ChannelMonitor<Signer>, ChannelError> where L::Target: Logger {
+	pub fn funding_signed<L: Deref>(&mut self, msg: &msgs::FundingSigned, last_block_hash: BlockHash, logger: &L) -> Result<(ChannelMonitor<Signer>, Transaction), ChannelError> where L::Target: Logger {
 		if !self.is_outbound() {
 			return Err(ChannelError::Close("Received funding_signed for an inbound channel?".to_owned()));
 		}
@@ -1670,7 +1672,7 @@ impl<Signer: Sign> Channel<Signer> {
 		self.cur_holder_commitment_transaction_number -= 1;
 		self.cur_counterparty_commitment_transaction_number -= 1;
 
-		Ok(channel_monitor)
+		Ok((channel_monitor, self.funding_transaction.as_ref().cloned().unwrap()))
 	}
 
 	pub fn funding_locked(&mut self, msg: &msgs::FundingLocked) -> Result<(), ChannelError> {
@@ -2771,11 +2773,13 @@ impl<Signer: Sign> Channel<Signer> {
 	/// Indicates that the latest ChannelMonitor update has been committed by the client
 	/// successfully and we should restore normal operation. Returns messages which should be sent
 	/// to the remote side.
-	pub fn monitor_updating_restored<L: Deref>(&mut self, logger: &L) -> (Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, bool, Option<msgs::FundingLocked>) where L::Target: Logger {
+	pub fn monitor_updating_restored<L: Deref>(&mut self, logger: &L) -> (Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, Option<Transaction>, Option<msgs::FundingLocked>) where L::Target: Logger {
 		assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, ChannelState::MonitorUpdateFailed as u32);
 		self.channel_state &= !(ChannelState::MonitorUpdateFailed as u32);
 
-		let needs_broadcast_safe = self.channel_state & (ChannelState::FundingSent as u32) != 0 && self.is_outbound();
+		let funding_broadcastable = if self.channel_state & (ChannelState::FundingSent as u32) != 0 && self.is_outbound() {
+			self.funding_transaction.take()
+		} else { None };
 
 		// Because we will never generate a FundingBroadcastSafe event when we're in
 		// MonitorUpdateFailed, if we assume the user only broadcast the funding transaction when
@@ -2801,7 +2805,7 @@ impl<Signer: Sign> Channel<Signer> {
 		if self.channel_state & (ChannelState::PeerDisconnected as u32) != 0 {
 			self.monitor_pending_revoke_and_ack = false;
 			self.monitor_pending_commitment_signed = false;
-			return (None, None, RAACommitmentOrder::RevokeAndACKFirst, forwards, failures, needs_broadcast_safe, funding_locked);
+			return (None, None, RAACommitmentOrder::RevokeAndACKFirst, forwards, failures, funding_broadcastable, funding_locked);
 		}
 
 		let raa = if self.monitor_pending_revoke_and_ack {
@@ -2815,11 +2819,11 @@ impl<Signer: Sign> Channel<Signer> {
 		self.monitor_pending_commitment_signed = false;
 		let order = self.resend_order.clone();
 		log_trace!(logger, "Restored monitor updating resulting in {}{} commitment update and {} RAA, with {} first",
-			if needs_broadcast_safe { "a funding broadcast safe, " } else { "" },
+			if funding_broadcastable.is_some() { "a funding broadcast safe, " } else { "" },
 			if commitment_update.is_some() { "a" } else { "no" },
 			if raa.is_some() { "an" } else { "no" },
 			match order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"});
-		(raa, commitment_update, order, forwards, failures, needs_broadcast_safe, funding_locked)
+		(raa, commitment_update, order, forwards, failures, funding_broadcastable, funding_locked)
 	}
 
 	pub fn update_fee<F: Deref>(&mut self, fee_estimator: &F, msg: &msgs::UpdateFee) -> Result<(), ChannelError>
@@ -3734,7 +3738,7 @@ impl<Signer: Sign> Channel<Signer> {
 	/// 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<L: Deref>(&mut self, funding_txo: OutPoint, logger: &L) -> Result<msgs::FundingCreated, ChannelError> where L::Target: Logger {
+	pub fn get_outbound_funding_created<L: Deref>(&mut self, funding_transaction: Transaction, funding_txo: OutPoint, logger: &L) -> Result<msgs::FundingCreated, ChannelError> where L::Target: Logger {
 		if !self.is_outbound() {
 			panic!("Tried to create outbound funding_created message on an inbound channel!");
 		}
@@ -3765,6 +3769,7 @@ impl<Signer: Sign> Channel<Signer> {
 
 		self.channel_state = ChannelState::FundingCreated as u32;
 		self.channel_id = funding_txo.to_channel_id();
+		self.funding_transaction = Some(funding_transaction);
 
 		Ok(msgs::FundingCreated {
 			temporary_channel_id,
@@ -4489,8 +4494,9 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 		}
 
 		self.channel_transaction_parameters.write(writer)?;
-		self.counterparty_cur_commitment_point.write(writer)?;
+		self.funding_transaction.write(writer)?;
 
+		self.counterparty_cur_commitment_point.write(writer)?;
 		self.counterparty_prev_commitment_point.write(writer)?;
 		self.counterparty_node_id.write(writer)?;
 
@@ -4659,6 +4665,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 		};
 
 		let channel_parameters = Readable::read(reader)?;
+		let funding_transaction = Readable::read(reader)?;
+
 		let counterparty_cur_commitment_point = Readable::read(reader)?;
 
 		let counterparty_prev_commitment_point = Readable::read(reader)?;
@@ -4731,8 +4739,9 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 			counterparty_forwarding_info,
 
 			channel_transaction_parameters: channel_parameters,
-			counterparty_cur_commitment_point,
+			funding_transaction,
 
+			counterparty_cur_commitment_point,
 			counterparty_prev_commitment_point,
 			counterparty_node_id,
 
@@ -5000,7 +5009,7 @@ mod tests {
 			value: 10000000, script_pubkey: output_script.clone(),
 		}]};
 		let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 };
-		let funding_created_msg = node_a_chan.get_outbound_funding_created(funding_outpoint, &&logger).unwrap();
+		let funding_created_msg = node_a_chan.get_outbound_funding_created(tx.clone(), funding_outpoint, &&logger).unwrap();
 		let (funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg, last_block_hash, &&logger).unwrap();
 
 		// Node B --> Node A: funding signed

lightning/src/ln/channelmanager.rs

@@ -19,6 +19,7 @@
 //!
 
 use bitcoin::blockdata::block::{Block, BlockHeader};
+use bitcoin::blockdata::transaction::Transaction;
 use bitcoin::blockdata::constants::genesis_block;
 use bitcoin::network::constants::Network;
 
@@ -1528,29 +1529,48 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// Note that ALL inputs in the transaction pointed to by funding_txo MUST spend SegWit outputs
 	/// or your counterparty can steal your funds!
 	///
+	/// Returns an [`APIError::APIMisuseError`] if the funding_transaction spent non-SegWit outputs
+	/// or the output didn't match the parameters in [`Event::FundingGenerationReady`].
+	///
 	/// Panics if a funding transaction has already been provided for this channel.
 	///
 	/// May panic if the funding_txo is duplicative with some other channel (note that this should
 	/// be trivially prevented by using unique funding transaction keys per-channel).
-	pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_txo: OutPoint) {
+	pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_transaction: Transaction, output_index: u16) -> Result<(), APIError> {
 		let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 
+		for inp in funding_transaction.input.iter() {
+			if inp.witness.is_empty() {
+				return Err(APIError::APIMisuseError {
+					err: "Funding transaction must be fully signed and spend Segwit outputs".to_owned()
+				});
+			}
+		}
+		let funding_txo = OutPoint { txid: funding_transaction.txid(), index: output_index };
+
 		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, &self.logger)
+					if output_index as usize >= funding_transaction.output.len() ||
+							funding_transaction.output[output_index as usize].script_pubkey != chan.get_funding_redeemscript().to_v0_p2wsh() ||
+							funding_transaction.output[output_index as usize].value != chan.get_value_satoshis() {
+						return Err(APIError::APIMisuseError {
+							err: "Funding Transaction Output did not match the script_pubkey or value in the FundingGenerationReady event".to_owned()
+						});
+					}
+					(chan.get_outbound_funding_created(funding_transaction, funding_txo, &self.logger)
 						.map_err(|e| if let ChannelError::Close(msg) = e {
 							MsgHandleErrInternal::from_finish_shutdown(msg, chan.channel_id(), chan.force_shutdown(true), None)
 						} else { unreachable!(); })
 					, chan)
 				},
-				None => return
+				None => { return Err(APIError::ChannelUnavailable { err: "No such channel".to_owned() }) },
 			};
 			match handle_error!(self, res, chan.get_counterparty_node_id()) {
 				Ok(funding_msg) => {
 					(chan, funding_msg)
 				},
-				Err(_) => { return; }
+				Err(_) => { return Err(APIError::ChannelUnavailable { err: "Error deriving keys - either our RNG or our counterparty's RNG is broken".to_owned() }) },
 			}
 		};
 
@@ -1567,6 +1587,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				e.insert(chan);
 			}
 		}
+		Ok(())
 	}
 
 	fn get_announcement_sigs(&self, chan: &Channel<Signer>) -> Option<msgs::AnnouncementSignatures> {
@@ -2359,7 +2380,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				return;
 			}
 
-			let (raa, commitment_update, order, pending_forwards, mut pending_failures, needs_broadcast_safe, funding_locked) = channel.monitor_updating_restored(&self.logger);
+			let (raa, commitment_update, order, pending_forwards, mut pending_failures, funding_broadcastable, funding_locked) = channel.monitor_updating_restored(&self.logger);
 			if !pending_forwards.is_empty() {
 				htlc_forwards.push((channel.get_short_channel_id().expect("We can't have pending forwards before funding confirmation"), funding_txo.clone(), pending_forwards));
 			}
@@ -2391,11 +2412,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					handle_cs!();
 				},
 			}
-			if needs_broadcast_safe {
-				pending_events.push(events::Event::FundingBroadcastSafe {
-					funding_txo: channel.get_funding_txo().unwrap(),
-					user_channel_id: channel.get_user_id(),
-				});
+			if let Some(tx) = funding_broadcastable {
+				self.tx_broadcaster.broadcast_transaction(&tx);
 			}
 			if let Some(msg) = funding_locked {
 				pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
@@ -2529,7 +2547,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	}
 
 	fn internal_funding_signed(&self, counterparty_node_id: &PublicKey, msg: &msgs::FundingSigned) -> Result<(), MsgHandleErrInternal> {
-		let (funding_txo, user_id) = {
+		let funding_tx = {
 			let last_block_hash = *self.last_block_hash.read().unwrap();
 			let mut channel_lock = self.channel_state.lock().unwrap();
 			let channel_state = &mut *channel_lock;
@@ -2538,23 +2556,19 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					if chan.get().get_counterparty_node_id() != *counterparty_node_id {
 						return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
 					}
-					let monitor = match chan.get_mut().funding_signed(&msg, last_block_hash, &self.logger) {
+					let (monitor, funding_tx) = match chan.get_mut().funding_signed(&msg, last_block_hash, &self.logger) {
 						Ok(update) => update,
 						Err(e) => try_chan_entry!(self, Err(e), channel_state, chan),
 					};
 					if let Err(e) = self.chain_monitor.watch_channel(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())
+					funding_tx
 				},
 				hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
 			}
 		};
-		let mut pending_events = self.pending_events.lock().unwrap();
-		pending_events.push(events::Event::FundingBroadcastSafe {
-			funding_txo,
-			user_channel_id: user_id,
-		});
+		self.tx_broadcaster.broadcast_transaction(&funding_tx);
 		Ok(())
 	}
 

lightning/src/ln/functional_test_utils.rs

@@ -421,7 +421,7 @@ pub fn create_chan_between_nodes_with_value_init<'a, 'b, 'c>(node_a: &Node<'a, '
 
 	let (temporary_channel_id, tx, funding_output) = create_funding_transaction(node_a, channel_value, 42);
 
-	node_a.node.funding_transaction_generated(&temporary_channel_id, funding_output);
+	node_a.node.funding_transaction_generated(&temporary_channel_id, tx.clone(), funding_output.index).unwrap();
 	check_added_monitors!(node_a, 0);
 
 	node_b.node.handle_funding_created(&node_a.node.get_our_node_id(), &get_event_msg!(node_a, MessageSendEvent::SendFundingCreated, node_b.node.get_our_node_id()));
@@ -441,14 +441,11 @@ pub fn create_chan_between_nodes_with_value_init<'a, 'b, 'c>(node_a: &Node<'a, '
 	}
 
 	let events_4 = node_a.node.get_and_clear_pending_events();
-	assert_eq!(events_4.len(), 1);
-	match events_4[0] {
-		Event::FundingBroadcastSafe { ref funding_txo, user_channel_id } => {
-			assert_eq!(user_channel_id, 42);
-			assert_eq!(*funding_txo, funding_output);
-		},
-		_ => panic!("Unexpected event"),
-	};
+	assert_eq!(events_4.len(), 0);
+
+	assert_eq!(node_a.tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1);
+	assert_eq!(node_a.tx_broadcaster.txn_broadcasted.lock().unwrap()[0], tx);
+	node_a.tx_broadcaster.txn_broadcasted.lock().unwrap().clear();
 
 	tx
 }