Merge pull request #856 from TheBlueMatt/2021-03-check-tx

Take the full funding transaction from the user on generation

Author: TheBlueMatt

background-processor/src/lib.rs

@@ -215,21 +215,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

@@ -52,7 +52,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]
@@ -117,9 +117,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)]
@@ -342,7 +346,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) });
@@ -372,7 +376,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] {
@@ -515,18 +518,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);
 				}
 			},
@@ -568,9 +572,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

@@ -405,9 +405,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,
 
@@ -595,8 +595,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,
 
@@ -836,8 +837,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,
 
@@ -1600,7 +1602,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()));
 		}
@@ -1662,7 +1664,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> {
@@ -2763,20 +2765,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 {
@@ -2793,7 +2796,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 {
@@ -2807,11 +2810,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>
@@ -3764,7 +3767,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!");
 		}
@@ -3795,6 +3798,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,
@@ -4519,8 +4523,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)?;
 
@@ -4689,6 +4694,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)?;
@@ -4761,8 +4768,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,
 
@@ -5030,7 +5038,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