From: Matt Corallo <git@bluematt.me>
Date: Sat, 3 Dec 2022 05:38:24 +0000 (+0000)
Subject: Move TODO from `handle_monitor_update_res` into `Channel`
X-Git-Tag: v0.0.114-beta~9^2~4
X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=46c6fb7f91b180fcae8358c82228008fa4eab2c4;p=rust-lightning

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
---

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 13d51624f..896c47504 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -558,6 +558,11 @@ pub(super) struct Channel<Signer: ChannelSigner> {
 	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>,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 7e9dc77ad..ee3b50936 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -1374,15 +1374,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)