Move TODO from handle_monitor_update_res into Channel

The TODO mentioned in `handle_monitor_update_res` about how we
might forget about HTLCs in case of permanent monitor update
failure still applies in spite of all our changes. If a channel is
drop'd in general, monitor-pending updates may be lost if the
monitor update failed to persist.

This was always the case, and is ultimately the general form of the
the specific TODO, so we simply leave comments there

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -542,6 +542,11 @@ pub(super) struct Channel<Signer: Sign> {
 	monitor_pending_channel_ready: bool,
 	monitor_pending_revoke_and_ack: bool,
 	monitor_pending_commitment_signed: bool,
+
+	// TODO: If a channel is drop'd, we don't know whether the `ChannelMonitor` is ultimately
+	// responsible for some of the HTLCs here or not - we don't know whether the update in question
+	// completed or not. We currently ignore these fields entirely when force-closing a channel,
+	// but need to handle this somehow or we run the risk of losing HTLCs!
 	monitor_pending_forwards: Vec<(PendingHTLCInfo, u64)>,
 	monitor_pending_failures: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
 	monitor_pending_finalized_fulfills: Vec<HTLCSource>,

lightning/src/ln/channelmanager.rs

@@ -1483,15 +1483,6 @@ macro_rules! handle_monitor_update_res {
 			ChannelMonitorUpdateStatus::PermanentFailure => {
 				log_error!($self.logger, "Closing channel {} due to monitor update ChannelMonitorUpdateStatus::PermanentFailure", log_bytes!($chan_id[..]));
 				update_maps_on_chan_removal!($self, $chan);
-				// TODO: $failed_fails is dropped here, which will cause other channels to hit the
-				// chain in a confused state! We need to move them into the ChannelMonitor which
-				// will be responsible for failing backwards once things confirm on-chain.
-				// It's ok that we drop $failed_forwards here - at this point we'd rather they
-				// broadcast HTLC-Timeout and pay the associated fees to get their funds back than
-				// us bother trying to claim it just to forward on to another peer. If we're
-				// splitting hairs we'd prefer to claim payments that were to us, but we haven't
-				// given up the preimage yet, so might as well just wait until the payment is
-				// retried, avoiding the on-chain fees.
 				let res: Result<(), _> = Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure".to_owned(), *$chan_id, $chan.get_user_id(),
 						$chan.force_shutdown(false), $self.get_channel_update_for_broadcast(&$chan).ok() ));
 				(res, true)