Stop failing back on monitor udpate fails, moving towards addressing #661

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -2517,13 +2517,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	/// implicitly dropping) and the payment_hashes of HTLCs we tried to add but are dropping.
 	/// No further message handling calls may be made until a channel_reestablish dance has
 	/// completed.
-	pub fn remove_uncommitted_htlcs_and_mark_paused<L: Deref>(&mut self, logger: &L) -> Vec<(HTLCSource, PaymentHash)> where L::Target: Logger {
-		let mut outbound_drops = Vec::new();
-
+	pub fn remove_uncommitted_htlcs_and_mark_paused<L: Deref>(&mut self, logger: &L)  where L::Target: Logger {
 		assert_eq!(self.channel_state & ChannelState::ShutdownComplete as u32, 0);
 		if self.channel_state < ChannelState::FundingSent as u32 {
 			self.channel_state = ChannelState::ShutdownComplete as u32;
-			return outbound_drops;
+			return;
 		}
 		// Upon reconnect we have to start the closing_signed dance over, but shutdown messages
 		// will be retransmitted.
@@ -2566,23 +2564,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			}
 		}
 
-		self.holding_cell_htlc_updates.retain(|htlc_update| {
-			match htlc_update {
-				// Note that currently on channel reestablish we assert that there are
-				// no holding cell HTLC update_adds, so if in the future we stop
-				// dropping added HTLCs here and failing them backwards, then there will
-				// need to be corresponding changes made in the Channel's re-establish
-				// logic.
-				&HTLCUpdateAwaitingACK::AddHTLC { ref payment_hash, ref source, .. } => {
-					outbound_drops.push((source.clone(), payment_hash.clone()));
-					false
-				},
-				&HTLCUpdateAwaitingACK::ClaimHTLC {..} | &HTLCUpdateAwaitingACK::FailHTLC {..} => true,
-			}
-		});
 		self.channel_state |= ChannelState::PeerDisconnected as u32;
-		log_debug!(logger, "Peer disconnection resulted in {} remote-announced HTLC drops and {} waiting-to-locally-announced HTLC drops on channel {}", outbound_drops.len(), inbound_drop_count, log_bytes!(self.channel_id()));
-		outbound_drops
+		log_debug!(logger, "Peer disconnection resulted in {} waiting-to-locally-announced HTLC drops on channel {}", inbound_drop_count, log_bytes!(self.channel_id()));
 	}
 
 	/// Indicates that a ChannelMonitor update failed to be stored by the client and further
@@ -2757,7 +2740,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>, RAACommitmentOrder, Vec<(HTLCSource, PaymentHash)>, 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
@@ -2808,15 +2791,15 @@ 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".to_owned()));
 				}
 				// 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, RAACommitmentOrder::CommitmentFirst, Vec::new(), shutdown_msg));
 			}
 
 			// We have OurFundingLocked set!
 			let next_per_commitment_point = self.holder_keys.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
 			return Ok((Some(msgs::FundingLocked {
 				channel_id: self.channel_id(),
 				next_per_commitment_point,
-			}), None, None, None, RAACommitmentOrder::CommitmentFirst, shutdown_msg));
+			}), None, None, None, RAACommitmentOrder::CommitmentFirst, Vec::new(), shutdown_msg));
 		}
 
 		let required_revoke = if msg.next_remote_commitment_number + 1 == INITIAL_COMMITMENT_NUMBER - self.cur_holder_commitment_transaction_number {
@@ -2857,14 +2840,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			}
 
 			if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) == 0 {
-				// Note that if in the future we no longer drop holding cell update_adds on peer
-				// disconnect, this logic will need to be updated.
-				for htlc_update in self.holding_cell_htlc_updates.iter() {
-					if let &HTLCUpdateAwaitingACK::AddHTLC { .. } = htlc_update {
-						debug_assert!(false, "There shouldn't be any add-HTLCs in the holding cell now because they should have been dropped on peer disconnect. Panic here because said HTLCs won't be handled correctly.");
-					}
-				}
-
 				// We're up-to-date and not waiting on a remote revoke (if we are our
 				// channel_reestablish should result in them sending a revoke_and_ack), but we may
 				// have received some updates while we were disconnected. Free the holding cell
@@ -2873,20 +2848,14 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 					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)), htlcs_to_fail)) => {
-						// If in the future we no longer drop holding cell update_adds on peer
-						// disconnect, we may be handed some HTLCs to fail backwards here.
-						assert!(htlcs_to_fail.is_empty());
-						return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(monitor_update), self.resend_order.clone(), shutdown_msg));
+						return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(monitor_update), self.resend_order.clone(), htlcs_to_fail, shutdown_msg));
 					},
 					Ok((None, htlcs_to_fail)) => {
-						// If in the future we no longer drop holding cell update_adds on peer
-						// disconnect, we may be handed some HTLCs to fail backwards here.
-						assert!(htlcs_to_fail.is_empty());
-						return Ok((resend_funding_locked, required_revoke, None, None, self.resend_order.clone(), shutdown_msg));
+						return Ok((resend_funding_locked, required_revoke, None, None, self.resend_order.clone(), htlcs_to_fail, 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, self.resend_order.clone(), Vec::new(), shutdown_msg));
 			}
 		} else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1 {
 			if required_revoke.is_some() {
@@ -2897,10 +2866,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, self.resend_order.clone(), Vec::new(), 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, self.resend_order.clone(), Vec::new(), shutdown_msg));
 		} else {
 			return Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction".to_owned()));
 		}
@@ -4094,7 +4063,7 @@ impl Readable for InboundHTLCRemovalReason {
 impl<ChanSigner: ChannelKeys + Writeable> Writeable for Channel<ChanSigner> {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
 		// Note that we write out as if remove_uncommitted_htlcs_and_mark_paused had just been
-		// called but include holding cell updates (and obviously we don't modify self).
+		// called.
 
 		writer.write_all(&[SERIALIZATION_VERSION; 1])?;
 		writer.write_all(&[MIN_SERIALIZATION_VERSION; 1])?;

lightning/src/ln/channelmanager.rs

@@ -2924,15 +2924,15 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 				// disconnect, so Channel's reestablish will never hand us any holding cell
 				// freed HTLCs to fail backwards. If in the future we no longer drop pending
 				// add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here.
-				let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, order, shutdown) =
+				let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, order, htlcs_failed_forward, shutdown) =
 					try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan);
 				if let Some(msg) = shutdown {
 					channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
 						node_id: counterparty_node_id.clone(),
 						msg,
 					});
 				}
-				handle_chan_restoration!(self, channel_state_lock, channel_state, chan, revoke_and_ack, commitment_update, order, monitor_update_opt, Vec::new(), Vec::new(), Vec::new(), false, funding_locked);
+				handle_chan_restoration!(self, channel_state_lock, channel_state, chan, revoke_and_ack, commitment_update, order, monitor_update_opt, Vec::new(), Vec::new(), htlcs_failed_forward, false, funding_locked);
 				Ok(())
 			},
 			hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
@@ -3322,7 +3322,6 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send, T: Deref + Sync + Send, K:
 	fn peer_disconnected(&self, counterparty_node_id: &PublicKey, no_connection_possible: bool) {
 		let _consistency_lock = self.total_consistency_lock.read().unwrap();
 		let mut failed_channels = Vec::new();
-		let mut failed_payments = Vec::new();
 		let mut no_channels_remain = true;
 		{
 			let mut channel_state_lock = self.channel_state.lock().unwrap();
@@ -3351,16 +3350,8 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send, T: Deref + Sync + Send, K:
 				log_debug!(self.logger, "Marking channels with {} disconnected and generating channel_updates", log_pubkey!(counterparty_node_id));
 				channel_state.by_id.retain(|_, chan| {
 					if chan.get_counterparty_node_id() == *counterparty_node_id {
-						// Note that currently on channel reestablish we assert that there are no
-						// holding cell add-HTLCs, so if in the future we stop removing uncommitted HTLCs
-						// on peer disconnect here, there will need to be corresponding changes in
-						// reestablish logic.
-						let failed_adds = chan.remove_uncommitted_htlcs_and_mark_paused(&self.logger);
+						chan.remove_uncommitted_htlcs_and_mark_paused(&self.logger);
 						chan.to_disabled_marked();
-						if !failed_adds.is_empty() {
-							let chan_update = self.get_channel_update(&chan).map(|u| u.encode_with_len()).unwrap(); // Cannot add/recv HTLCs before we have a short_id so unwrap is safe
-							failed_payments.push((chan_update, failed_adds));
-						}
 						if chan.is_shutdown() {
 							if let Some(short_id) = chan.get_short_channel_id() {
 								short_to_id.remove(&short_id);
@@ -3401,11 +3392,6 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send, T: Deref + Sync + Send, K:
 		for failure in failed_channels.drain(..) {
 			self.finish_force_close_channel(failure);
 		}
-		for (chan_update, mut htlc_sources) in failed_payments {
-			for (htlc_source, payment_hash) in htlc_sources.drain(..) {
-				self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_source, &payment_hash, HTLCFailReason::Reason { failure_code: 0x1000 | 7, data: chan_update.clone() });
-			}
-		}
 	}
 
 	fn peer_connected(&self, counterparty_node_id: &PublicKey, init_msg: &msgs::Init) {