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

@@ -189,21 +189,20 @@ mod tests {
 			$node_a.node.handle_accept_channel(&$node_b.node.get_our_node_id(), InitFeatures::known(), &get_event_msg!($node_b, MessageSendEvent::SendAcceptChannel, $node_a.node.get_our_node_id()));
 			let events = $node_a.node.get_and_clear_pending_events();
 			assert_eq!(events.len(), 1);
-			let (temporary_channel_id, tx, funding_output) = match events[0] {
+			let (temporary_channel_id, tx) = match events[0] {
 				Event::FundingGenerationReady { ref temporary_channel_id, ref channel_value_satoshis, ref output_script, user_channel_id } => {
 					assert_eq!(*channel_value_satoshis, $channel_value);
 					assert_eq!(user_channel_id, 42);
 
 					let tx = Transaction { version: 1 as i32, lock_time: 0, input: Vec::new(), output: vec![TxOut {
 						value: *channel_value_satoshis, script_pubkey: output_script.clone(),
 					}]};
-					let funding_outpoint = OutPoint { txid: tx.txid(), index: 0 };
-					(*temporary_channel_id, tx, funding_outpoint)
+					(*temporary_channel_id, tx)
 				},
 				_ => 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()).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

fuzz/src/chanmon_consistency.rs

@@ -397,7 +397,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 						value: *channel_value_satoshis, script_pubkey: output_script.clone(),
 					}]};
 					funding_output = OutPoint { txid: tx.txid(), index: 0 };
-					$source.funding_transaction_generated(&temporary_channel_id, funding_output);
+					$source.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap();
 					channel_txn.push(tx);
 				} else { panic!("Wrong event type"); }
 			}
@@ -420,12 +420,6 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 			};
 			$source.handle_funding_signed(&$dest.get_our_node_id(), &funding_signed);
 
-			{
-				let events = $source.get_and_clear_pending_events();
-				assert_eq!(events.len(), 1);
-				if let events::Event::FundingBroadcastSafe { .. } = events[0] {
-				} else { panic!("Wrong event type"); }
-			}
 			funding_output
 		} }
 	}

fuzz/src/full_stack.rs

@@ -51,7 +51,7 @@ use bitcoin::secp256k1::Secp256k1;
 use std::cell::RefCell;
 use std::collections::{HashMap, hash_map};
 use std::cmp;
-use std::sync::Arc;
+use std::sync::{Arc, Mutex};
 use std::sync::atomic::{AtomicU64,AtomicUsize,Ordering};
 
 #[inline]
@@ -116,9 +116,13 @@ impl FeeEstimator for FuzzEstimator {
 	}
 }
 
-struct TestBroadcaster {}
+struct TestBroadcaster {
+	txn_broadcasted: Mutex<Vec<Transaction>>,
+}
 impl BroadcasterInterface for TestBroadcaster {
-	fn broadcast_transaction(&self, _tx: &Transaction) {}
+	fn broadcast_transaction(&self, tx: &Transaction) {
+		self.txn_broadcasted.lock().unwrap().push(tx.clone());
+	}
 }
 
 #[derive(Clone)]
@@ -340,7 +344,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 		Err(_) => return,
 	};
 
-	let broadcast = Arc::new(TestBroadcaster{});
+	let broadcast = Arc::new(TestBroadcaster{ txn_broadcasted: Mutex::new(Vec::new()) });
 	let monitor = Arc::new(chainmonitor::ChainMonitor::new(None, broadcast.clone(), Arc::clone(&logger), fee_est.clone(), Arc::new(TestPersister{})));
 
 	let keys_manager = Arc::new(KeyProvider { node_secret: our_network_key.clone(), counter: AtomicU64::new(0) });
@@ -370,7 +374,6 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 	let mut payments_sent = 0;
 	let mut pending_funding_generation: Vec<([u8; 32], u64, Script)> = Vec::new();
 	let mut pending_funding_signatures = HashMap::new();
-	let mut pending_funding_relay = Vec::new();
 
 	loop {
 		match get_slice!(1)[0] {
@@ -513,18 +516,19 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 							continue 'outer_loop;
 						}
 					};
-					channelmanager.funding_transaction_generated(&funding_generation.0, funding_output.clone());
+					channelmanager.funding_transaction_generated(&funding_generation.0, tx.clone()).unwrap();
 					pending_funding_signatures.insert(funding_output, tx);
 				}
 			},
 			11 => {
-				if !pending_funding_relay.is_empty() {
-					loss_detector.connect_block(&pending_funding_relay[..]);
+				let mut txn = broadcast.txn_broadcasted.lock().unwrap();
+				if !txn.is_empty() {
+					loss_detector.connect_block(&txn[..]);
 					for _ in 2..100 {
 						loss_detector.connect_block(&[]);
 					}
 				}
-				for tx in pending_funding_relay.drain(..) {
+				for tx in txn.drain(..) {
 					loss_detector.funding_txn.push(tx);
 				}
 			},
@@ -566,9 +570,6 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 				Event::FundingGenerationReady { temporary_channel_id, channel_value_satoshis, output_script, .. } => {
 					pending_funding_generation.push((temporary_channel_id, channel_value_satoshis, output_script));
 				},
-				Event::FundingBroadcastSafe { funding_txo, .. } => {
-					pending_funding_relay.push(pending_funding_signatures.remove(&funding_txo).unwrap());
-				},
 				Event::PaymentReceived { payment_hash, payment_secret, amt } => {
 					//TODO: enhance by fetching random amounts from fuzz input?
 					payments_received.push((payment_hash, payment_secret, amt));

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()).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,20 +2773,21 @@ 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
-		// they received the FundingBroadcastSafe event, we can only ever hit
-		// monitor_pending_funding_locked when we're an inbound channel which failed to persist the
-		// monitor on funding_created, and we even got the funding transaction confirmed before the
-		// monitor was persisted.
+		// We will never broadcast the funding transaction when we're in MonitorUpdateFailed (and
+		// we assume the user never directly broadcasts the funding transaction and waits for us to
+		// do it). Thus, we can only ever hit monitor_pending_funding_locked when we're an inbound
+		// channel which failed to persist the monitor on funding_created, and we got the funding
+		// transaction confirmed before the monitor was persisted.
 		let funding_locked = if self.monitor_pending_funding_locked {
-			assert!(!self.is_outbound(), "Funding transaction broadcast without FundingBroadcastSafe!");
+			assert!(!self.is_outbound(), "Funding transaction broadcast by the local client before it should have - LDK didn't do it!");
 			self.monitor_pending_funding_locked = false;
 			let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
 			Some(msgs::FundingLocked {
@@ -2801,7 +2804,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 +2818,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 broadcastable, " } 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 +3737,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 +3768,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 +4493,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 +4664,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 +4738,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 +5008,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