From 46c6fb7f91b180fcae8358c82228008fa4eab2c4 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 3 Dec 2022 05:38:24 +0000 Subject: [PATCH] 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 --- lightning/src/ln/channel.rs | 5 +++++ lightning/src/ln/channelmanager.rs | 9 --------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 13d51624..896c4750 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -558,6 +558,11 @@ pub(super) struct Channel { 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, diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 7e9dc77a..ee3b5093 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) -- 2.30.2