From 82d40eefb2e6887c6667d1672bfe1e078936dad8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 19 Mar 2020 18:16:07 -0400 Subject: [PATCH] Deduplicate HTLC preimage events from channelmonitor. This avoids calling get_update_fulfill_htlc_and_commit twice for the same HTLC if we have to rescan a block. --- lightning/src/ln/channel.rs | 20 ++++++++++++++++---- lightning/src/ln/channelmonitor.rs | 28 ++++++++++++++++------------ 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index b4c0a90cb..57ccab1ad 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1138,8 +1138,11 @@ impl Channel { } /// Per HTLC, only one get_update_fail_htlc or get_update_fulfill_htlc call may be made. - /// In such cases we debug_assert!(false) and return an IgnoreError. Thus, will always return - /// Ok(_) if debug assertions are turned on and preconditions are met. + /// In such cases we debug_assert!(false) and return a ChannelError::Ignore. Thus, will always + /// return Ok(_) if debug assertions are turned on or preconditions are met. + /// + /// Note that it is still possible to hit these assertions in case we find a preimage on-chain + /// but then have a reorg which settles on an HTLC-failure on chain. fn get_update_fulfill_htlc(&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage) -> Result<(Option, Option), ChannelError> { // Either ChannelFunded got set (which means it won't be unset) or there is no way any // caller thought we could have something claimed (cause we wouldn't have accepted in an @@ -1167,6 +1170,7 @@ impl Channel { } else { log_warn!(self, "Have preimage and want to fulfill HTLC with payment hash {} we already failed against channel {}", log_bytes!(htlc.payment_hash.0), log_bytes!(self.channel_id())); } + debug_assert!(false, "Tried to fulfill an HTLC that was already fail/fulfilled"); return Ok((None, None)); }, _ => { @@ -1202,6 +1206,7 @@ impl Channel { if htlc_id_arg == htlc_id { // Make sure we don't leave latest_monitor_update_id incremented here: self.latest_monitor_update_id -= 1; + debug_assert!(false, "Tried to fulfill an HTLC that was already fulfilled"); return Ok((None, None)); } }, @@ -1210,6 +1215,7 @@ impl Channel { log_warn!(self, "Have preimage and want to fulfill HTLC with pending failure against channel {}", log_bytes!(self.channel_id())); // TODO: We may actually be able to switch to a fulfill here, though its // rare enough it may not be worth the complexity burden. + debug_assert!(false, "Tried to fulfill an HTLC that was already failed"); return Ok((None, Some(monitor_update))); } }, @@ -1261,8 +1267,11 @@ impl Channel { } /// Per HTLC, only one get_update_fail_htlc or get_update_fulfill_htlc call may be made. - /// In such cases we debug_assert!(false) and return an IgnoreError. Thus, will always return - /// Ok(_) if debug assertions are turned on and preconditions are met. + /// In such cases we debug_assert!(false) and return a ChannelError::Ignore. Thus, will always + /// return Ok(_) if debug assertions are turned on or preconditions are met. + /// + /// Note that it is still possible to hit these assertions in case we find a preimage on-chain + /// but then have a reorg which settles on an HTLC-failure on chain. pub fn get_update_fail_htlc(&mut self, htlc_id_arg: u64, err_packet: msgs::OnionErrorPacket) -> Result, ChannelError> { if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) { panic!("Was asked to fail an HTLC when channel was not in an operational state"); @@ -1279,6 +1288,7 @@ impl Channel { match htlc.state { InboundHTLCState::Committed => {}, InboundHTLCState::LocalRemoved(_) => { + debug_assert!(false, "Tried to fail an HTLC that was already fail/fulfilled"); return Ok(None); }, _ => { @@ -1299,11 +1309,13 @@ impl Channel { match pending_update { &HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => { if htlc_id_arg == htlc_id { + debug_assert!(false, "Tried to fail an HTLC that was already fulfilled"); return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID")); } }, &HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } => { if htlc_id_arg == htlc_id { + debug_assert!(false, "Tried to fail an HTLC that was already failed"); return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID")); } }, diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index 514a95d27..e3cea9771 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -2278,19 +2278,23 @@ impl ChannelMonitor { if let Some((source, payment_hash)) = payment_data { let mut payment_preimage = PaymentPreimage([0; 32]); if accepted_preimage_claim { - payment_preimage.0.copy_from_slice(&input.witness[3]); - self.pending_htlcs_updated.push(HTLCUpdate { - source, - payment_preimage: Some(payment_preimage), - payment_hash - }); + if !self.pending_htlcs_updated.iter().any(|update| update.source == source) { + payment_preimage.0.copy_from_slice(&input.witness[3]); + self.pending_htlcs_updated.push(HTLCUpdate { + source, + payment_preimage: Some(payment_preimage), + payment_hash + }); + } } else if offered_preimage_claim { - payment_preimage.0.copy_from_slice(&input.witness[1]); - self.pending_htlcs_updated.push(HTLCUpdate { - source, - payment_preimage: Some(payment_preimage), - payment_hash - }); + if !self.pending_htlcs_updated.iter().any(|update| update.source == source) { + payment_preimage.0.copy_from_slice(&input.witness[1]); + self.pending_htlcs_updated.push(HTLCUpdate { + source, + payment_preimage: Some(payment_preimage), + payment_hash + }); + } } else { log_info!(self, "Failing HTLC with payment_hash {} timeout by a spend tx, waiting for confirmation (at height{})", log_bytes!(payment_hash.0), height + ANTI_REORG_DELAY - 1); match self.onchain_events_waiting_threshold_conf.entry(height + ANTI_REORG_DELAY - 1) { -- 2.39.5