Change PaymentPathFailed's optional network update to a Failure enum

This let us capture the errors when we fail without committing to an HTLC vs
failing via update_fail.

Author: valentinewallace

lightning-background-processor/src/lib.rs

@@ -33,7 +33,7 @@ use lightning::routing::gossip::{NetworkGraph, P2PGossipSync};
 use lightning::routing::utxo::UtxoLookup;
 use lightning::routing::router::Router;
 use lightning::routing::scoring::{Score, WriteableScore};
-use lightning::util::events::{Event, EventHandler, EventsProvider};
+use lightning::util::events::{Event, EventHandler, EventsProvider, Failure};
 use lightning::util::logger::Logger;
 use lightning::util::persist::Persister;
 use lightning_rapid_gossip_sync::RapidGossipSync;
@@ -215,10 +215,10 @@ where
 fn handle_network_graph_update<L: Deref>(
 	network_graph: &NetworkGraph<L>, event: &Event
 ) where L::Target: Logger {
-	if let Event::PaymentPathFailed { ref network_update, .. } = event {
-		if let Some(network_update) = network_update {
-			network_graph.handle_network_update(&network_update);
-		}
+	if let Event::PaymentPathFailed {
+		failure: Failure::OnPath { network_update: Some(ref upd) }, .. } = event
+	{
+		network_graph.handle_network_update(upd);
 	}
 }
 
@@ -673,7 +673,7 @@ mod tests {
 	use lightning::routing::router::{DefaultRouter, RouteHop};
 	use lightning::routing::scoring::{ChannelUsage, Score};
 	use lightning::util::config::UserConfig;
-	use lightning::util::events::{Event, MessageSendEventsProvider, MessageSendEvent};
+	use lightning::util::events::{Event, Failure, MessageSendEventsProvider, MessageSendEvent};
 	use lightning::util::ser::Writeable;
 	use lightning::util::test_utils;
 	use lightning::util::persist::KVStorePersister;
@@ -1365,7 +1365,7 @@ mod tests {
 			payment_id: None,
 			payment_hash: PaymentHash([42; 32]),
 			payment_failed_permanently: false,
-			network_update: None,
+			failure: Failure::OnPath { network_update: None },
 			path: path.clone(),
 			short_channel_id: Some(scored_scid),
 			retry: None,
@@ -1385,7 +1385,7 @@ mod tests {
 			payment_id: None,
 			payment_hash: PaymentHash([42; 32]),
 			payment_failed_permanently: true,
-			network_update: None,
+			failure: Failure::OnPath { network_update: None },
 			path: path.clone(),
 			short_channel_id: None,
 			retry: None,

lightning/src/ln/functional_test_utils.rs

@@ -24,7 +24,7 @@ use crate::util::enforcing_trait_impls::EnforcingSigner;
 use crate::util::scid_utils;
 use crate::util::test_utils;
 use crate::util::test_utils::{panicking, TestChainMonitor};
-use crate::util::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose};
+use crate::util::events::{Event, Failure, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose};
 use crate::util::errors::APIError;
 use crate::util::config::UserConfig;
 use crate::util::ser::{ReadableArgs, Writeable};
@@ -1818,7 +1818,7 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 ) {
 	if conditions.expected_mpp_parts_remain { assert_eq!(payment_failed_events.len(), 1); } else { assert_eq!(payment_failed_events.len(), 2); }
 	let expected_payment_id = match &payment_failed_events[0] {
-		Event::PaymentPathFailed { payment_hash, payment_failed_permanently, path, retry, payment_id, network_update, short_channel_id,
+		Event::PaymentPathFailed { payment_hash, payment_failed_permanently, path, retry, payment_id, failure, short_channel_id,
 			#[cfg(test)]
 			error_code,
 			#[cfg(test)]
@@ -1843,23 +1843,24 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 			}
 
 			if let Some(chan_closed) = conditions.expected_blamed_chan_closed {
-				match network_update {
-					Some(NetworkUpdate::ChannelUpdateMessage { ref msg }) if !chan_closed => {
-						if let Some(scid) = conditions.expected_blamed_scid {
-							assert_eq!(msg.contents.short_channel_id, scid);
-						}
-						const CHAN_DISABLED_FLAG: u8 = 2;
-						assert_eq!(msg.contents.flags & CHAN_DISABLED_FLAG, 0);
-					},
-					Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }) if chan_closed => {
-						if let Some(scid) = conditions.expected_blamed_scid {
-							assert_eq!(*short_channel_id, scid);
-						}
-						assert!(is_permanent);
-					},
-					Some(_) => panic!("Unexpected update type"),
-					None => panic!("Expected update"),
-				}
+				if let Failure::OnPath { network_update: Some(upd) } = failure {
+					match upd {
+						NetworkUpdate::ChannelUpdateMessage { ref msg } if !chan_closed => {
+							if let Some(scid) = conditions.expected_blamed_scid {
+								assert_eq!(msg.contents.short_channel_id, scid);
+							}
+							const CHAN_DISABLED_FLAG: u8 = 2;
+							assert_eq!(msg.contents.flags & CHAN_DISABLED_FLAG, 0);
+						},
+						NetworkUpdate::ChannelFailure { short_channel_id, is_permanent } if chan_closed => {
+							if let Some(scid) = conditions.expected_blamed_scid {
+								assert_eq!(*short_channel_id, scid);
+							}
+							assert!(is_permanent);
+						},
+						_ => panic!("Unexpected update type"),
+					}
+				} else { panic!("Expected network update"); }
 			}
 
 			payment_id.unwrap()

lightning/src/ln/functional_tests.rs

@@ -31,7 +31,7 @@ use crate::ln::msgs;
 use crate::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ErrorAction};
 use crate::util::enforcing_trait_impls::EnforcingSigner;
 use crate::util::test_utils;
-use crate::util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose, ClosureReason, HTLCDestination};
+use crate::util::events::{Event, Failure, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose, ClosureReason, HTLCDestination};
 use crate::util::errors::APIError;
 use crate::util::ser::{Writeable, ReadableArgs};
 use crate::util::config::UserConfig;
@@ -3235,12 +3235,12 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
 			let events = nodes[0].node.get_and_clear_pending_events();
 			assert_eq!(events.len(), 6);
 			match events[0] {
-				Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => {
+				Event::PaymentPathFailed { ref payment_hash, ref failure, .. } => {
 					assert!(failed_htlcs.insert(payment_hash.0));
 					// If we delivered B's RAA we got an unknown preimage error, not something
 					// that we should update our routing table for.
 					if !deliver_bs_raa {
-						assert!(network_update.is_some());
+						if let Failure::OnPath { network_update: Some(_) } = failure { } else { panic!("Unexpected path failure") }
 					}
 				},
 				_ => panic!("Unexpected event"),
@@ -3252,9 +3252,8 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
 				_ => panic!("Unexpected event"),
 			}
 			match events[2] {
-				Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => {
+				Event::PaymentPathFailed { ref payment_hash, failure: Failure::OnPath { network_update: Some(_) }, .. } => {
 					assert!(failed_htlcs.insert(payment_hash.0));
-					assert!(network_update.is_some());
 				},
 				_ => panic!("Unexpected event"),
 			}
@@ -3265,9 +3264,8 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
 				_ => panic!("Unexpected event"),
 			}
 			match events[4] {
-				Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => {
+				Event::PaymentPathFailed { ref payment_hash, failure: Failure::OnPath { network_update: Some(_) }, .. } => {
 					assert!(failed_htlcs.insert(payment_hash.0));
-					assert!(network_update.is_some());
 				},
 				_ => panic!("Unexpected event"),
 			}
@@ -5148,14 +5146,14 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
 	let mut as_failds = HashSet::new();
 	let mut as_updates = 0;
 	for event in as_events.iter() {
-		if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref network_update, .. } = event {
+		if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref failure, .. } = event {
 			assert!(as_failds.insert(*payment_hash));
 			if *payment_hash != payment_hash_2 {
 				assert_eq!(*payment_failed_permanently, deliver_last_raa);
 			} else {
 				assert!(!payment_failed_permanently);
 			}
-			if network_update.is_some() {
+			if let Failure::OnPath { network_update: Some(_) } = failure {
 				as_updates += 1;
 			}
 		} else if let &Event::PaymentFailed { .. } = event {
@@ -5174,14 +5172,14 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
 	let mut bs_failds = HashSet::new();
 	let mut bs_updates = 0;
 	for event in bs_events.iter() {
-		if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref network_update, .. } = event {
+		if let &Event::PaymentPathFailed { ref payment_hash, ref payment_failed_permanently, ref failure, .. } = event {
 			assert!(bs_failds.insert(*payment_hash));
 			if *payment_hash != payment_hash_1 && *payment_hash != payment_hash_5 {
 				assert_eq!(*payment_failed_permanently, deliver_last_raa);
 			} else {
 				assert!(!payment_failed_permanently);
 			}
-			if network_update.is_some() {
+			if let Failure::OnPath { network_update: Some(_) } = failure {
 				bs_updates += 1;
 			}
 		} else if let &Event::PaymentFailed { .. } = event {
@@ -5695,11 +5693,10 @@ fn test_fail_holding_cell_htlc_upon_free() {
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
 	match &events[0] {
-		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref short_channel_id, .. } => {
+		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, failure: Failure::OnPath { network_update: None }, ref short_channel_id, .. } => {
 			assert_eq!(PaymentId(our_payment_hash.0), *payment_id.as_ref().unwrap());
 			assert_eq!(our_payment_hash.clone(), *payment_hash);
 			assert_eq!(*payment_failed_permanently, false);
-			assert_eq!(*network_update, None);
 			assert_eq!(*short_channel_id, Some(route.paths[0][0].short_channel_id));
 		},
 		_ => panic!("Unexpected event"),
@@ -5785,11 +5782,10 @@ fn test_free_and_fail_holding_cell_htlcs() {
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
 	match &events[0] {
-		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref short_channel_id, .. } => {
+		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, failure: Failure::OnPath { network_update: None }, 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!(*network_update, None);
 			assert_eq!(*short_channel_id, Some(route_2.paths[0][0].short_channel_id));
 		},
 		_ => panic!("Unexpected event"),
@@ -6687,8 +6683,7 @@ fn test_channel_failed_after_message_with_badonion_node_perm_bits_set() {
 	// Expect a PaymentPathFailed event with a ChannelFailure network update for the channel between
 	// the node originating the error to its next hop.
 	match events_5[0] {
-		Event::PaymentPathFailed { network_update:
-			Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }), error_code, ..
+		Event::PaymentPathFailed { error_code, failure: Failure::OnPath { network_update: Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }) }, ..
 		} => {
 			assert_eq!(short_channel_id, chan_2.0.contents.short_channel_id);
 			assert!(is_permanent);

lightning/src/ln/onion_route_tests.rs

@@ -23,7 +23,7 @@ use crate::ln::features::{InitFeatures, InvoiceFeatures};
 use crate::ln::msgs;
 use crate::ln::msgs::{ChannelMessageHandler, ChannelUpdate};
 use crate::ln::wire::Encode;
-use crate::util::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
+use crate::util::events::{Event, Failure, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
 use crate::util::ser::{Writeable, Writer};
 use crate::util::test_utils;
 use crate::util::config::{UserConfig, ChannelConfig};
@@ -167,7 +167,7 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
-	if let &Event::PaymentPathFailed { ref payment_failed_permanently, ref network_update, ref short_channel_id, ref error_code, .. } = &events[0] {
+	if let &Event::PaymentPathFailed { ref payment_failed_permanently, ref short_channel_id, ref error_code, failure: Failure::OnPath { ref network_update }, .. } = &events[0] {
 		assert_eq!(*payment_failed_permanently, !expected_retryable);
 		assert_eq!(*error_code, expected_error_code);
 		if expected_channel_update.is_some() {

lightning/src/ln/outbound_payment.rs

@@ -1205,7 +1205,7 @@ impl OutboundPayments {
 					payment_id: Some(*payment_id),
 					payment_hash: payment_hash.clone(),
 					payment_failed_permanently: !payment_retryable,
-					network_update,
+					failure: events::Failure::OnPath { network_update },
 					path: path.clone(),
 					short_channel_id,
 					retry,
@@ -1248,13 +1248,13 @@ fn push_payment_path_failed_evs<I: Iterator<Item = Result<(), APIError>>>(
 ) {
 	let mut events = pending_events.lock().unwrap();
 	for (path, path_res) in paths.into_iter().zip(path_results) {
-		if path_res.is_err() {
+		if let Err(e) = path_res {
 			let scid = path[0].short_channel_id;
 			events.push(events::Event::PaymentPathFailed {
 				payment_id: Some(payment_id),
 				payment_hash,
 				payment_failed_permanently: false,
-				network_update: None,
+				failure: events::Failure::InitialSend { err: e },
 				path,
 				short_channel_id: Some(scid),
 				retry: None,

lightning/src/ln/payment_tests.rs

@@ -24,7 +24,7 @@ use crate::ln::outbound_payment::Retry;
 use crate::routing::gossip::{EffectiveCapacity, RoutingFees};
 use crate::routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RouteParameters};
 use crate::routing::scoring::ChannelUsage;
-use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
+use crate::util::events::{ClosureReason, Event, Failure, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
 use crate::util::test_utils;
 use crate::util::errors::APIError;
 use crate::util::ser::Writeable;
@@ -2135,9 +2135,12 @@ fn retry_multi_path_single_failed_payment() {
 	assert_eq!(events.len(), 1);
 	match events[0] {
 		Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently: false,
-			network_update: None, short_channel_id: Some(expected_scid), .. } => {
+			failure: Failure::InitialSend { err: APIError::ChannelUnavailable { err: ref err_msg }},
+			short_channel_id: Some(expected_scid), .. } =>
+		{
 			assert_eq!(payment_hash, ev_payment_hash);
 			assert_eq!(expected_scid, route.paths[1][0].short_channel_id);
+			assert!(err_msg.contains("max HTLC"));
 		},
 		_ => panic!("Unexpected event"),
 	}
@@ -2206,9 +2209,12 @@ fn immediate_retry_on_failure() {
 	assert_eq!(events.len(), 1);
 	match events[0] {
 		Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently: false,
-		network_update: None, short_channel_id: Some(expected_scid), .. } => {
+			failure: Failure::InitialSend { err: APIError::ChannelUnavailable { err: ref err_msg }},
+			short_channel_id: Some(expected_scid), .. } =>
+		{
 			assert_eq!(payment_hash, ev_payment_hash);
 			assert_eq!(expected_scid, route.paths[1][0].short_channel_id);
+			assert!(err_msg.contains("max HTLC"));
 		},
 		_ => panic!("Unexpected event"),
 	}