Lock outbound channels at 0conf if the peer indicates support for it

TheBlueMatt · TheBlueMatt · commit b5147daae741 · 2022-04-02T23:24:42.000Z
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
@@ -1941,12 +1941,6 @@ impl<Signer: Sign> Channel<Signer> {
 		if msg.minimum_depth > peer_limits.max_minimum_depth {
 			return Err(ChannelError::Close(format!("We consider the minimum depth to be unreasonably large. Expected minimum: ({}). Actual: ({})", peer_limits.max_minimum_depth, msg.minimum_depth)));
 		}
-		if msg.minimum_depth == 0 {
-			// Note that if this changes we should update the serialization minimum version to
-			// indicate to older clients that they don't understand some features of the current
-			// channel.
-			return Err(ChannelError::Close("Minimum confirmation depth must be at least 1".to_owned()));
-		}
 
 		if let Some(ty) = &msg.channel_type {
 			if *ty != self.channel_type {
@@ -1983,7 +1977,12 @@ impl<Signer: Sign> Channel<Signer> {
 		self.counterparty_selected_channel_reserve_satoshis = Some(msg.channel_reserve_satoshis);
 		self.counterparty_htlc_minimum_msat = msg.htlc_minimum_msat;
 		self.counterparty_max_accepted_htlcs = msg.max_accepted_htlcs;
-		self.minimum_depth = Some(msg.minimum_depth);
+
+		if peer_limits.trust_own_funding_0conf {
+			self.minimum_depth = Some(msg.minimum_depth);
+		} else {
+			self.minimum_depth = Some(cmp::max(1, msg.minimum_depth));
+		}
 
 		let counterparty_pubkeys = ChannelPublicKeys {
 			funding_pubkey: msg.funding_pubkey,
@@ -2123,7 +2122,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, best_block: BestBlock, logger: &L) -> Result<(ChannelMonitor<Signer>, Transaction), ChannelError> where L::Target: Logger {
+	pub fn funding_signed<L: Deref>(&mut self, msg: &msgs::FundingSigned, best_block: BestBlock, logger: &L) -> Result<(ChannelMonitor<Signer>, Transaction, Option<msgs::FundingLocked>), ChannelError> where L::Target: Logger {
 		if !self.is_outbound() {
 			return Err(ChannelError::Close("Received funding_signed for an inbound channel?".to_owned()));
 		}
@@ -2192,7 +2191,7 @@ impl<Signer: Sign> Channel<Signer> {
 
 		log_info!(logger, "Received funding_signed from peer for channel {}", log_bytes!(self.channel_id()));
 
-		Ok((channel_monitor, self.funding_transaction.as_ref().cloned().unwrap()))
+		Ok((channel_monitor, self.funding_transaction.as_ref().cloned().unwrap(), self.check_get_funding_locked(0)))
 	}
 
 	/// Handles a funding_locked message from our peer. If we've already sent our funding_locked
@@ -3495,12 +3494,13 @@ impl<Signer: Sign> Channel<Signer> {
 	/// monitor update failure must *not* have been sent to the remote end, and must instead
 	/// have been dropped. They will be regenerated when monitor_updating_restored is called.
 	pub fn monitor_update_failed(&mut self, resend_raa: bool, resend_commitment: bool,
-		mut pending_forwards: Vec<(PendingHTLCInfo, u64)>,
+		resend_funding_locked: bool, mut pending_forwards: Vec<(PendingHTLCInfo, u64)>,
 		mut pending_fails: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
 		mut pending_finalized_claimed_htlcs: Vec<HTLCSource>
 	) {
 		self.monitor_pending_revoke_and_ack |= resend_raa;
 		self.monitor_pending_commitment_signed |= resend_commitment;
+		self.monitor_pending_funding_locked |= resend_funding_locked;
 		self.monitor_pending_forwards.append(&mut pending_forwards);
 		self.monitor_pending_failures.append(&mut pending_fails);
 		self.monitor_pending_finalized_fulfills.append(&mut pending_finalized_claimed_htlcs);
@@ -3514,17 +3514,27 @@ impl<Signer: Sign> Channel<Signer> {
 		assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, ChannelState::MonitorUpdateFailed as u32);
 		self.channel_state &= !(ChannelState::MonitorUpdateFailed as u32);
 
-		let funding_broadcastable = if self.channel_state & (ChannelState::FundingSent as u32) != 0 && self.is_outbound() {
-			self.funding_transaction.take()
-		} else { None };
+		// If we're past (or at) the FundingSent stage on an outbound channel, try to
+		// (re-)broadcast the funding transaction as we may have declined to broadcast it when we
+		// first received the funding_signed.
+		let mut funding_broadcastable =
+			if self.is_outbound() && self.channel_state & !MULTI_STATE_FLAGS >= ChannelState::FundingSent as u32 {
+				self.funding_transaction.take()
+			} else { None };
+		// That said, if the funding transaction is already confirmed (ie we're active with a
+		// minimum_depth over 0, don't bother re-broadcasting the confirmed funding tx.
+		if self.channel_state & !MULTI_STATE_FLAGS >= ChannelState::ChannelFunded as u32 && self.minimum_depth != Some(0) {
+			funding_broadcastable = None;
+		}
 
 		// 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 by the local client before it should have - LDK didn't do it!");
+			assert!(!self.is_outbound() || self.minimum_depth == Some(0),
+				"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 {
@@ -4476,6 +4486,11 @@ impl<Signer: Sign> Channel<Signer> {
 		self.channel_state >= ChannelState::FundingSent as u32
 	}
 
+	/// Returns true if our funding_locked has been sent
+	pub fn is_our_funding_locked(&self) -> bool {
+		(self.channel_state & ChannelState::OurFundingLocked as u32) != 0 || self.channel_state >= ChannelState::ChannelFunded as u32
+	}
+
 	/// Returns true if our peer has either initiated or agreed to shut down the channel.
 	pub fn received_shutdown(&self) -> bool {
 		(self.channel_state & ChannelState::RemoteShutdownSent as u32) != 0
@@ -4506,7 +4521,7 @@ impl<Signer: Sign> Channel<Signer> {
 	}
 
 	fn check_get_funding_locked(&mut self, height: u32) -> Option<msgs::FundingLocked> {
-		if self.funding_tx_confirmation_height == 0 {
+		if self.funding_tx_confirmation_height == 0 && self.minimum_depth != Some(0) {
 			return None;
 		}
 
@@ -4682,6 +4697,10 @@ impl<Signer: Sign> Channel<Signer> {
 			// the funding transaction's confirmation count has dipped below minimum_depth / 2,
 			// close the channel and hope we can get the latest state on chain (because presumably
 			// the funding transaction is at least still in the mempool of most nodes).
+			//
+			// Note that ideally we wouldn't force-close if we see *any* reorg on a 1-conf or
+			// 0-conf funding channel, but doing not doing so may lead to the
+			// `ChannelManager::short_to_id` map being inconsistent, so we currently have to.
 			if funding_tx_confirmations < self.minimum_depth.unwrap() as i64 / 2 {
 				let err_reason = format!("Funding transaction was un-confirmed. Locked at {} confs, now have {} confs.",
 					self.minimum_depth.unwrap(), funding_tx_confirmations);
@@ -5540,7 +5559,7 @@ impl<Signer: Sign> Channel<Signer> {
 }
 
 const SERIALIZATION_VERSION: u8 = 2;
-const MIN_SERIALIZATION_VERSION: u8 = 1;
+const MIN_SERIALIZATION_VERSION: u8 = 2;
 
 impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
 	(0, FailRelay),
@@ -5605,12 +5624,10 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 
 		self.user_id.write(writer)?;
 
-		// Write out the old serialization for the config object. This is read by version-1
-		// deserializers, but we will read the version in the TLV at the end instead.
-		self.config.forwarding_fee_proportional_millionths.write(writer)?;
-		self.config.cltv_expiry_delta.write(writer)?;
-		self.config.announced_channel.write(writer)?;
-		self.config.commit_upfront_shutdown_pubkey.write(writer)?;
+		// Version 1 deserializers expected to read parts of the config object here. Version 2
+		// deserializers (0.0.99) now read config through TLVs, and as we now require them for
+		// `minimum_depth` we simply write dummy values here.
+		writer.write_all(&[0; 8])?;
 
 		self.channel_id.write(writer)?;
 		(self.channel_state | ChannelState::PeerDisconnected as u32).write(writer)?;
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -1526,7 +1526,7 @@ macro_rules! remove_channel {
 }
 
 macro_rules! handle_monitor_err {
-	($self: ident, $err: expr, $short_to_id: expr, $chan: expr, $action_type: path, $resend_raa: expr, $resend_commitment: expr, $failed_forwards: expr, $failed_fails: expr, $failed_finalized_fulfills: expr, $chan_id: expr) => {
+	($self: ident, $err: expr, $short_to_id: expr, $chan: expr, $action_type: path, $resend_raa: expr, $resend_commitment: expr, $resend_funding_locked: expr, $failed_forwards: expr, $failed_fails: expr, $failed_finalized_fulfills: expr, $chan_id: expr) => {
 		match $err {
 			ChannelMonitorUpdateErr::PermanentFailure => {
 				log_error!($self.logger, "Closing channel {} due to monitor update ChannelMonitorUpdateErr::PermanentFailure", log_bytes!($chan_id[..]));
@@ -1564,30 +1564,33 @@ macro_rules! handle_monitor_err {
 				if !$resend_raa {
 					debug_assert!($action_type == RAACommitmentOrder::CommitmentFirst || !$resend_commitment);
 				}
-				$chan.monitor_update_failed($resend_raa, $resend_commitment, $failed_forwards, $failed_fails, $failed_finalized_fulfills);
+				$chan.monitor_update_failed($resend_raa, $resend_commitment, $resend_funding_locked, $failed_forwards, $failed_fails, $failed_finalized_fulfills);
 				(Err(MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore("Failed to update ChannelMonitor".to_owned()), *$chan_id)), false)
 			},
 		}
 	};
-	($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $resend_raa: expr, $resend_commitment: expr, $failed_forwards: expr, $failed_fails: expr, $failed_finalized_fulfills: expr) => { {
-		let (res, drop) = handle_monitor_err!($self, $err, $channel_state.short_to_id, $entry.get_mut(), $action_type, $resend_raa, $resend_commitment, $failed_forwards, $failed_fails, $failed_finalized_fulfills, $entry.key());
+	($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $resend_raa: expr, $resend_commitment: expr, $resend_funding_locked: expr, $failed_forwards: expr, $failed_fails: expr, $failed_finalized_fulfills: expr) => { {
+		let (res, drop) = handle_monitor_err!($self, $err, $channel_state.short_to_id, $entry.get_mut(), $action_type, $resend_raa, $resend_commitment, $resend_funding_locked, $failed_forwards, $failed_fails, $failed_finalized_fulfills, $entry.key());
 		if drop {
 			$entry.remove_entry();
 		}
 		res
 	} };
 	($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $chan_id: expr, COMMITMENT_UPDATE_ONLY) => { {
 		debug_assert!($action_type == RAACommitmentOrder::CommitmentFirst);
-		handle_monitor_err!($self, $err, $channel_state, $entry, $action_type, false, true, Vec::new(), Vec::new(), Vec::new(), $chan_id)
+		handle_monitor_err!($self, $err, $channel_state, $entry, $action_type, false, true, false, Vec::new(), Vec::new(), Vec::new(), $chan_id)
 	} };
 	($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $chan_id: expr, NO_UPDATE) => {
-		handle_monitor_err!($self, $err, $channel_state, $entry, $action_type, false, false, Vec::new(), Vec::new(), Vec::new(), $chan_id)
+		handle_monitor_err!($self, $err, $channel_state, $entry, $action_type, false, false, false, Vec::new(), Vec::new(), Vec::new(), $chan_id)
+	};
+	($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $resend_funding_locked: expr, OPTIONALLY_RESEND_FUNDING_LOCKED) => {
+		handle_monitor_err!($self, $err, $channel_state, $entry, $action_type, false, false, $resend_funding_locked, Vec::new(), Vec::new(), Vec::new())
 	};
 	($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $resend_raa: expr, $resend_commitment: expr) => {
-		handle_monitor_err!($self, $err, $channel_state, $entry, $action_type, $resend_raa, $resend_commitment, Vec::new(), Vec::new(), Vec::new())
+		handle_monitor_err!($self, $err, $channel_state, $entry, $action_type, $resend_raa, $resend_commitment, false, Vec::new(), Vec::new(), Vec::new())
 	};
 	($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $resend_raa: expr, $resend_commitment: expr, $failed_forwards: expr, $failed_fails: expr) => {
-		handle_monitor_err!($self, $err, $channel_state, $entry, $action_type, $resend_raa, $resend_commitment, $failed_forwards, $failed_fails, Vec::new())
+		handle_monitor_err!($self, $err, $channel_state, $entry, $action_type, $resend_raa, $resend_commitment, false, $failed_forwards, $failed_fails, Vec::new())
 	};
 }
 
@@ -4495,7 +4498,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					// hasn't persisted to disk yet - we can't lose money on a transaction that we haven't
 					// accepted payment from yet. We do, however, need to wait to send our funding_locked
 					// until we have persisted our monitor.
-					chan.monitor_update_failed(false, false, Vec::new(), Vec::new(), Vec::new());
+					chan.monitor_update_failed(false, false, false, Vec::new(), Vec::new(), Vec::new());
 				},
 			}
 		}
@@ -4526,12 +4529,12 @@ 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, funding_tx) = match chan.get_mut().funding_signed(&msg, best_block, &self.logger) {
+					let (monitor, funding_tx, funding_locked) = match chan.get_mut().funding_signed(&msg, best_block, &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) {
-						let mut res = handle_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::RevokeAndACKFirst, false, false);
+						let mut res = handle_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::RevokeAndACKFirst, funding_locked.is_some(), OPTIONALLY_RESEND_FUNDING_LOCKED);
 						if let Err(MsgHandleErrInternal { ref mut shutdown_finish, .. }) = res {
 							// We weren't able to watch the channel to begin with, so no updates should be made on
 							// it. Previously, full_stack_target found an (unreachable) panic when the
@@ -4542,6 +4545,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						}
 						return res
 					}
+					if let Some(msg) = funding_locked {
+						send_funding_locked!(channel_state.short_to_id, channel_state.pending_msg_events, chan.get(), msg);
+					}
 					funding_tx
 				},
 				hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
@@ -4892,7 +4898,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						} else {
 							if let Err(e) = handle_monitor_err!(self, e, channel_state, chan,
 									RAACommitmentOrder::CommitmentFirst, false,
-									raa_updates.commitment_update.is_some(),
+									raa_updates.commitment_update.is_some(), false,
 									raa_updates.accepted_htlcs, raa_updates.failed_htlcs,
 									raa_updates.finalized_claimed_htlcs) {
 								break Err(e);
@@ -5746,6 +5752,18 @@ where
 							}
 						}
 					}
+					if channel.is_our_funding_locked() {
+						if let Some(real_scid) = channel.get_short_channel_id() {
+							// If we sent a 0conf funding_locked, and now have an SCID, we add it
+							// to the short_to_id map here. Note that we check whether we can relay
+							// using the real SCID at relay-time, and if the funding tx is ever
+							// un-confirmed we force-close the channel, ensuring short_to_id is
+							// always consistent.
+							let scid_insert = short_to_id.insert(real_scid, channel.channel_id());
+							assert!(scid_insert.is_none() || scid_insert.unwrap() == channel.channel_id(),
+								"SCIDs should never collide - ensure you weren't behind by a full XXX days when creating channels");
+						}
+					}
 				} else if let Err(reason) = res {
 					update_maps_on_chan_removal!(self, short_to_id, channel);
 					// It looks like our counterparty went on-chain or funding transaction was
diff --git a/lightning/src/ln/priv_short_conf_tests.rs b/lightning/src/ln/priv_short_conf_tests.rs
@@ -541,3 +541,37 @@ fn test_scid_alias_returned() {
 		PaymentFailedConditions::new().blamed_scid(last_hop[0].inbound_scid_alias.unwrap())
 			.blamed_chan_closed(false).expected_htlc_error_data(0x1000|12, &err_data));
 }
+
+#[test]
+fn test_simple_0_conf_channel() {
+	// If our peer tells us they will accept our channel with 0 confs, and we funded the channel,
+	// we should trust the funding won't be double-spent (assuming `trust_own_funding_0conf` is
+	// set)!
+
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap();
+	let open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+
+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_channel);
+	let mut accept_channel = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
+	accept_channel.minimum_depth = 0;
+	nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &accept_channel);
+
+	let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[0], 100000, 42);
+	nodes[0].node.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap();
+	let funding_created = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
+
+	nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created);
+	check_added_monitors!(nodes[1], 1);
+	let funding_signed = get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id());
+
+	nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &funding_signed);
+	check_added_monitors!(nodes[0], 1);
+
+	let as_funding_locked = get_event_msg!(nodes[0], MessageSendEvent::SendFundingLocked, nodes[1].node.get_our_node_id());
+	nodes[1].node.handle_funding_locked(&nodes[0].node.get_our_node_id(), &as_funding_locked);
+}
diff --git a/lightning/src/util/config.rs b/lightning/src/util/config.rs
