From: Matt Corallo Date: Wed, 7 Sep 2022 21:09:50 +0000 (+0000) Subject: Drop redundant code in `fail_holding_cell_htlcs` X-Git-Tag: v0.0.111~11^2~2 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=fd328e71dac9bcd64895bea5a1805191387f8241;p=rust-lightning Drop redundant code in `fail_holding_cell_htlcs` `fail_holding_cell_htlcs` calls through to `fail_htlc_backwards_internal` for HTLCs that need to be failed-backwards but opts to generate its own payment failure events for `HTLCSource:;OutboundRoute` HTLCs. There is no reason for that as `fail_htlc_backwards_internal` will also happily generate (now-)equivalent events for `HTLCSource::OutboundRoute` HTLCs. Thus, we can drop the redundant code and always call `fail_htlc_backwards_internal` for each HTLC in `fail_holding_cell_htlcs`. --- diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 80d0fea5b..cb8d26764 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3812,62 +3812,17 @@ impl ChannelMana counterparty_node_id: &PublicKey ) { for (htlc_src, payment_hash) in htlcs_to_fail.drain(..) { - match htlc_src { - HTLCSource::PreviousHopData(HTLCPreviousHopData { .. }) => { - let (failure_code, onion_failure_data) = - match self.channel_state.lock().unwrap().by_id.entry(channel_id) { - hash_map::Entry::Occupied(chan_entry) => { - self.get_htlc_inbound_temp_fail_err_and_data(0x1000|7, &chan_entry.get()) - }, - hash_map::Entry::Vacant(_) => (0x4000|10, Vec::new()) - }; - let channel_state = self.channel_state.lock().unwrap(); + let mut channel_state = self.channel_state.lock().unwrap(); + let (failure_code, onion_failure_data) = + match channel_state.by_id.entry(channel_id) { + hash_map::Entry::Occupied(chan_entry) => { + self.get_htlc_inbound_temp_fail_err_and_data(0x1000|7, &chan_entry.get()) + }, + hash_map::Entry::Vacant(_) => (0x4000|10, Vec::new()) + }; - let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id.clone()), channel_id }; - self.fail_htlc_backwards_internal(channel_state, htlc_src, &payment_hash, HTLCFailReason::Reason { failure_code, data: onion_failure_data }, receiver) - }, - HTLCSource::OutboundRoute { session_priv, payment_id, path, payment_params, .. } => { - let mut session_priv_bytes = [0; 32]; - session_priv_bytes.copy_from_slice(&session_priv[..]); - let mut outbounds = self.pending_outbound_payments.lock().unwrap(); - if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) { - if payment.get_mut().remove(&session_priv_bytes, Some(&path)) && !payment.get().is_fulfilled() { - let retry = if let Some(payment_params_data) = payment_params { - let path_last_hop = path.last().expect("Outbound payments must have had a valid path"); - Some(RouteParameters { - payment_params: payment_params_data, - final_value_msat: path_last_hop.fee_msat, - final_cltv_expiry_delta: path_last_hop.cltv_expiry_delta, - }) - } else { None }; - let mut pending_events = self.pending_events.lock().unwrap(); - pending_events.push(events::Event::PaymentPathFailed { - payment_id: Some(payment_id), - payment_hash, - payment_failed_permanently: false, - network_update: None, - all_paths_failed: payment.get().remaining_parts() == 0, - path: path.clone(), - short_channel_id: None, - retry, - #[cfg(test)] - error_code: None, - #[cfg(test)] - error_data: None, - }); - if payment.get().abandoned() && payment.get().remaining_parts() == 0 { - pending_events.push(events::Event::PaymentFailed { - payment_id, - payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"), - }); - payment.remove(); - } - } - } else { - log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0)); - } - }, - }; + let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id.clone()), channel_id }; + self.fail_htlc_backwards_internal(channel_state, htlc_src, &payment_hash, HTLCFailReason::Reason { failure_code, data: onion_failure_data }, receiver); } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 214e7cdad..2efeb8852 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -6264,15 +6264,13 @@ fn test_fail_holding_cell_htlc_upon_free() { let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match &events[0] { - &Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, ref error_code, ref error_data, .. } => { + &Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, .. } => { assert_eq!(our_payment_id, *payment_id.as_ref().unwrap()); assert_eq!(our_payment_hash.clone(), *payment_hash); assert_eq!(*payment_failed_permanently, false); assert_eq!(*all_paths_failed, true); assert_eq!(*network_update, None); - assert_eq!(*short_channel_id, None); - assert_eq!(*error_code, None); - assert_eq!(*error_data, None); + assert_eq!(*short_channel_id, Some(route.paths[0][0].short_channel_id)); }, _ => panic!("Unexpected event"), } @@ -6350,15 +6348,13 @@ fn test_free_and_fail_holding_cell_htlcs() { let events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match &events[0] { - &Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, ref error_code, ref error_data, .. } => { + &Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, .. } => { assert_eq!(payment_id_2, *payment_id.as_ref().unwrap()); assert_eq!(payment_hash_2.clone(), *payment_hash); assert_eq!(*payment_failed_permanently, false); assert_eq!(*all_paths_failed, true); assert_eq!(*network_update, None); - assert_eq!(*short_channel_id, None); - assert_eq!(*error_code, None); - assert_eq!(*error_data, None); + assert_eq!(*short_channel_id, Some(route_2.paths[0][0].short_channel_id)); }, _ => panic!("Unexpected event"), }