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 13d51624f740608427715ae20ba2b534ff30d72a..896c475049b4df56673dfa5e2c3ca7d45760e448 100644 (file)
--- 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 7e9dc77ad182df1b6fde66b18fa52ebd95bebf96..ee3b50936a5e74d3b6fe84f7e89c3174e10f4f6e 100644 (file)
--- 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)