Holding cell: if we fail to free an HTLC, fail it backwards

valentinewallace · valentinewallace · commit 38b44f5a1c2b · 2020-06-24T15:48:00.000-04:00
Plus add a test.
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
@@ -2115,7 +2115,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 	/// Used to fulfill holding_cell_htlcs when we get a remote ack (or implicitly get it by them
 	/// fulfilling or failing the last pending HTLC)
-	fn free_holding_cell_htlcs<L: Deref>(&mut self, logger: &L) -> Result<Option<(msgs::CommitmentUpdate, ChannelMonitorUpdate)>, ChannelError> where L::Target: Logger {
+	fn free_holding_cell_htlcs<L: Deref>(&mut self, logger: &L) -> Result<Option<(msgs::CommitmentUpdate, ChannelMonitorUpdate, Vec<(HTLCSource, PaymentHash)>)>, ChannelError> where L::Target: Logger {
 		assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, 0);
 		if self.holding_cell_htlc_updates.len() != 0 || self.holding_cell_update_fee.is_some() {
 			log_trace!(logger, "Freeing holding cell with {} HTLC updates{}", self.holding_cell_htlc_updates.len(), if self.holding_cell_update_fee.is_some() { " and a fee update" } else { "" });
@@ -2130,6 +2130,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			let mut update_add_htlcs = Vec::with_capacity(htlc_updates.len());
 			let mut update_fulfill_htlcs = Vec::with_capacity(htlc_updates.len());
 			let mut update_fail_htlcs = Vec::with_capacity(htlc_updates.len());
+			let mut htlcs_to_fail = Vec::new();
 			let mut err = None;
 			for htlc_update in htlc_updates.drain(..) {
 				// Note that this *can* fail, though it should be due to rather-rare conditions on
@@ -2148,6 +2149,17 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 									match e {
 										ChannelError::Ignore(ref msg) => {
 											log_info!(logger, "Failed to send HTLC with payment_hash {} due to {}", log_bytes!(payment_hash.0), msg);
+											// If we fail to send here, then
+											// this HTLC should be failed
+											// backwards. Failing to send here
+											// indicates that this HTLC may keep
+											// being put back into the holding
+											// cell without ever being
+											// successfully
+											// forwarded/failed/fulfilled,
+											// causing our counterparty to
+											// eventually close on us.
+											htlcs_to_fail.push((source.clone(), *payment_hash));
 										},
 										_ => {
 											log_info!(logger, "Failed to send HTLC with payment_hash {} resulting in a channel closure during holding_cell freeing", log_bytes!(payment_hash.0));
@@ -2199,10 +2211,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			//fail it back the route, if it's a temporary issue we can ignore it...
 			match err {
 				None => {
-					if update_add_htlcs.is_empty() && update_fulfill_htlcs.is_empty() && update_fail_htlcs.is_empty() && self.holding_cell_update_fee.is_none() {
-						// This should never actually happen and indicates we got some Errs back
-						// from update_fulfill_htlc/update_fail_htlc, but we handle it anyway in
-						// case there is some strange way to hit duplicate HTLC removes.
+					if update_add_htlcs.is_empty() && update_fulfill_htlcs.is_empty() && update_fail_htlcs.is_empty() && self.holding_cell_update_fee.is_none() && htlcs_to_fail.is_empty() {
+						// Hitting this case indicates that we got some Errs back from update_fulfill_htlc
+						// or update_fail_htlc.
+						log_warn!(logger, "Attempted to fulfill or fail an HTLC that was already removed");
 						return Ok(None);
 					}
 					let update_fee = if let Some(feerate) = self.holding_cell_update_fee {
@@ -2228,7 +2240,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 						update_fail_malformed_htlcs: Vec::new(),
 						update_fee: update_fee,
 						commitment_signed,
-					}, monitor_update)))
+					}, monitor_update, htlcs_to_fail)))
 				},
 				Some(e) => Err(e)
 			}
@@ -2242,7 +2254,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	/// waiting on this revoke_and_ack. The generation of this new commitment_signed may also fail,
 	/// generating an appropriate error *after* the channel state has been updated based on the
 	/// revoke_and_ack message.
-	pub fn revoke_and_ack<F: Deref, L: Deref>(&mut self, msg: &msgs::RevokeAndACK, fee_estimator: &F, logger: &L) -> Result<(Option<msgs::CommitmentUpdate>, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, Option<msgs::ClosingSigned>, ChannelMonitorUpdate), ChannelError>
+	pub fn revoke_and_ack<F: Deref, L: Deref>(&mut self, msg: &msgs::RevokeAndACK, fee_estimator: &F, logger: &L) -> Result<(Option<msgs::CommitmentUpdate>, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, Option<msgs::ClosingSigned>, ChannelMonitorUpdate, Vec<(HTLCSource, PaymentHash)>), ChannelError>
 		where F::Target: FeeEstimator,
 					L::Target: Logger,
 	{
@@ -2417,11 +2429,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			}
 			self.monitor_pending_forwards.append(&mut to_forward_infos);
 			self.monitor_pending_failures.append(&mut revoked_htlcs);
-			return Ok((None, Vec::new(), Vec::new(), None, monitor_update))
+			return Ok((None, Vec::new(), Vec::new(), None, monitor_update, Vec::new()))
 		}
 
 		match self.free_holding_cell_htlcs(logger)? {
-			Some((mut commitment_update, mut additional_update)) => {
+			Some((mut commitment_update, mut additional_update, htlcs_to_fail)) => {
 				commitment_update.update_fail_htlcs.reserve(update_fail_htlcs.len());
 				for fail_msg in update_fail_htlcs.drain(..) {
 					commitment_update.update_fail_htlcs.push(fail_msg);
@@ -2436,7 +2448,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 				self.latest_monitor_update_id = monitor_update.update_id;
 				monitor_update.updates.append(&mut additional_update.updates);
 
-				Ok((Some(commitment_update), to_forward_infos, revoked_htlcs, None, monitor_update))
+				Ok((Some(commitment_update), to_forward_infos, revoked_htlcs, None, monitor_update, htlcs_to_fail))
 			},
 			None => {
 				if require_commitment {
@@ -2454,9 +2466,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 						update_fail_malformed_htlcs,
 						update_fee: None,
 						commitment_signed
-					}), to_forward_infos, revoked_htlcs, None, monitor_update))
+					}), to_forward_infos, revoked_htlcs, None, monitor_update, Vec::new()))
 				} else {
-					Ok((None, to_forward_infos, revoked_htlcs, self.maybe_propose_first_closing_signed(fee_estimator), monitor_update))
+					Ok((None, to_forward_infos, revoked_htlcs, self.maybe_propose_first_closing_signed(fee_estimator), monitor_update, Vec::new()))
 				}
 			}
 		}
@@ -2726,7 +2738,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 	/// May panic if some calls other than message-handling calls (which will all Err immediately)
 	/// have been called between remove_uncommitted_htlcs_and_mark_paused and this call.
-	pub fn channel_reestablish<L: Deref>(&mut self, msg: &msgs::ChannelReestablish, logger: &L) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitorUpdate>, RAACommitmentOrder, Option<msgs::Shutdown>), ChannelError> where L::Target: Logger {
+	pub fn channel_reestablish<L: Deref>(&mut self, msg: &msgs::ChannelReestablish, logger: &L) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitorUpdate>, Option<Vec<(HTLCSource, PaymentHash)>>, RAACommitmentOrder, Option<msgs::Shutdown>), ChannelError> where L::Target: Logger {
 		if self.channel_state & (ChannelState::PeerDisconnected as u32) == 0 {
 			// While BOLT 2 doesn't indicate explicitly we should error this channel here, it
 			// almost certainly indicates we are going to end up out-of-sync in some way, so we
@@ -2774,7 +2786,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 					return Err(ChannelError::Close("Peer claimed they saw a revoke_and_ack but we haven't sent funding_locked yet"));
 				}
 				// Short circuit the whole handler as there is nothing we can resend them
-				return Ok((None, None, None, None, RAACommitmentOrder::CommitmentFirst, shutdown_msg));
+				return Ok((None, None, None, None, None, RAACommitmentOrder::CommitmentFirst, shutdown_msg));
 			}
 
 			// We have OurFundingLocked set!
@@ -2783,7 +2795,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			return Ok((Some(msgs::FundingLocked {
 				channel_id: self.channel_id(),
 				next_per_commitment_point: next_per_commitment_point,
-			}), None, None, None, RAACommitmentOrder::CommitmentFirst, shutdown_msg));
+			}), None, None, None, None, RAACommitmentOrder::CommitmentFirst, shutdown_msg));
 		}
 
 		let required_revoke = if msg.next_remote_commitment_number + 1 == INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number {
@@ -2832,11 +2844,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 				match self.free_holding_cell_htlcs(logger) {
 					Err(ChannelError::Close(msg)) => return Err(ChannelError::Close(msg)),
 					Err(ChannelError::Ignore(_)) | Err(ChannelError::CloseDelayBroadcast(_)) => panic!("Got non-channel-failing result from free_holding_cell_htlcs"),
-					Ok(Some((commitment_update, monitor_update))) => return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(monitor_update), self.resend_order.clone(), shutdown_msg)),
-					Ok(None) => return Ok((resend_funding_locked, required_revoke, None, None, self.resend_order.clone(), shutdown_msg)),
+					Ok(Some((commitment_update, monitor_update, htlcs_to_fail))) => return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(monitor_update), Some(htlcs_to_fail), self.resend_order.clone(), shutdown_msg)),
+					Ok(None) => return Ok((resend_funding_locked, required_revoke, None, None, None, self.resend_order.clone(), shutdown_msg)),
 				}
 			} else {
-				return Ok((resend_funding_locked, required_revoke, None, None, self.resend_order.clone(), shutdown_msg));
+				return Ok((resend_funding_locked, required_revoke, None, None, None, self.resend_order.clone(), shutdown_msg));
 			}
 		} else if msg.next_local_commitment_number == our_next_remote_commitment_number - 1 {
 			if required_revoke.is_some() {
@@ -2847,10 +2859,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
 			if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) != 0 {
 				self.monitor_pending_commitment_signed = true;
-				return Ok((resend_funding_locked, None, None, None, self.resend_order.clone(), shutdown_msg));
+				return Ok((resend_funding_locked, None, None, None, None, self.resend_order.clone(), shutdown_msg));
 			}
 
-			return Ok((resend_funding_locked, required_revoke, Some(self.get_last_commitment_update(logger)), None, self.resend_order.clone(), shutdown_msg));
+			return Ok((resend_funding_locked, required_revoke, Some(self.get_last_commitment_update(logger)), None, None, self.resend_order.clone(), shutdown_msg));
 		} else {
 			return Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction"));
 		}
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -1819,6 +1819,49 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 		} else { false }
 	}
 
+	// Fail a list of HTLCs. Only called when 1 or more HTLCs fails to be freed
+	// from the holding cell. In this case, the HTLCs need to be failed
+	// backwards or, if they were one of our outgoing HTLCs, then their failure
+	// needs to be surfaced to the user.
+	fn fail_htlcs(&self, mut htlcs_to_fail: Vec<(HTLCSource, PaymentHash)>, channel_id: [u8; 32]) -> Result<(), MsgHandleErrInternal> {
+		for (htlc_src, payment_hash) in htlcs_to_fail.drain(..) {
+			match htlc_src {
+				HTLCSource::PreviousHopData(HTLCPreviousHopData { incoming_packet_shared_secret, .. }) => {
+					match self.channel_state.lock().unwrap().by_id.entry(channel_id) {
+						hash_map::Entry::Occupied(chan_entry) => {
+							// We can unwrap here because the only possible error is
+							// failure to fetch the short channel ID of the channel, and
+							// the short channel ID was already verified to be existent
+							// earlier.
+							let upd = self.get_channel_update(&chan_entry.get()).unwrap();
+							let reason = onion_utils::build_first_hop_failure_packet(&incoming_packet_shared_secret, 0x1000|7, &{
+											let mut res = Vec::with_capacity(8 + 128);
+											res.extend_from_slice(&byte_utils::be16_to_array(upd.contents.flags));
+											res.extend_from_slice(&upd.encode_with_len()[..]);
+											res
+										}[..]);
+							self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_src.clone(), &payment_hash, HTLCFailReason::LightningError{err: reason});
+						},
+						hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel", channel_id))
+					}
+				},
+				HTLCSource::OutboundRoute { .. } => {
+					self.pending_events.lock().unwrap().push(
+						events::Event::PaymentFailed {
+							payment_hash,
+							rejected_by_dest: false,
+							#[cfg(test)]
+							error_code: None,
+							#[cfg(test)]
+							error_data: None
+						}
+					);
+				}
+			}
+		}
+		Ok(())
+	}
+
 	/// Fails an HTLC backwards to the sender of it to us.
 	/// Note that while we take a channel_state lock as input, we do *not* assume consistency here.
 	/// There are several callsites that do stupid things like loop over a list of payment_hashes
@@ -2666,7 +2709,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 	}
 
 	fn internal_revoke_and_ack(&self, their_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result<(), MsgHandleErrInternal> {
-		let (pending_forwards, mut pending_failures, short_channel_id) = {
+		let (pending_forwards, mut pending_failures, short_channel_id, htlcs_to_fail) = {
 			let mut channel_state_lock = self.channel_state.lock().unwrap();
 			let channel_state = &mut *channel_state_lock;
 			match channel_state.by_id.entry(msg.channel_id) {
@@ -2675,7 +2718,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 						return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!", msg.channel_id));
 					}
 					let was_frozen_for_monitor = chan.get().is_awaiting_monitor_update();
-					let (commitment_update, pending_forwards, pending_failures, closing_signed, monitor_update) =
+					let (commitment_update, pending_forwards, pending_failures, closing_signed, monitor_update, htlcs_to_fail) =
 						try_chan_entry!(self, chan.get_mut().revoke_and_ack(&msg, &self.fee_estimator, &self.logger), channel_state, chan);
 					if let Err(e) = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update) {
 						if was_frozen_for_monitor {
@@ -2697,14 +2740,15 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 							msg,
 						});
 					}
-					(pending_forwards, pending_failures, chan.get().get_short_channel_id().expect("RAA should only work on a short-id-available channel"))
+					(pending_forwards, pending_failures, chan.get().get_short_channel_id().expect("RAA should only work on a short-id-available channel"), htlcs_to_fail)
 				},
 				hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel", msg.channel_id))
 			}
 		};
 		for failure in pending_failures.drain(..) {
 			self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), failure.0, &failure.1, failure.2);
 		}
+		if let Err(e) = self.fail_htlcs(htlcs_to_fail, msg.channel_id) { return Err(e) };
 		self.forward_htlcs(&mut [(short_channel_id, pending_forwards)]);
 
 		Ok(())
@@ -2777,7 +2821,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 				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 (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, mut order, shutdown) =
+				let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, htlcs_to_fail_opt, mut order, shutdown) =
 					try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan);
 				if let Some(monitor_update) = monitor_update_opt {
 					if let Err(e) = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update) {
@@ -2794,6 +2838,9 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 						//TODO: Resend the funding_locked if needed once we get the monitor running again
 					}
 				}
+				if let Some(htlcs_to_fail) = htlcs_to_fail_opt {
+					if let Err(e) = self.fail_htlcs(htlcs_to_fail, msg.channel_id) { return Err(e) };
+				}
 				if let Some(msg) = funding_locked {
 					channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked {
 						node_id: their_node_id.clone(),
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
