Add reason to Event::PaymentFailed

alecchendev · alecchendev · commit 207a3d4e48a0 · 2023-04-07T13:05:20.000-05:00
This includes adding a reason to `PendingOutboundPayment::Abandoned` and
using that reason when pushing an `Event::PaymentFailed`.
diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs
@@ -465,6 +465,9 @@ pub enum Event {
 		///
 		/// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment
 		payment_hash: PaymentHash,
+		/// The reason the payment failed. This is only `None` for events generated or serialized
+		/// by versions prior to 0.0.115.
+		reason: Option<PaymentFailureReason>,
 	},
 	/// Indicates that a path for an outbound payment was successful.
 	///
@@ -903,10 +906,11 @@ impl Writeable for Event {
 					(4, *path, vec_type)
 				})
 			},
-			&Event::PaymentFailed { ref payment_id, ref payment_hash } => {
+			&Event::PaymentFailed { ref payment_id, ref payment_hash, ref reason } => {
 				15u8.write(writer)?;
 				write_tlv_fields!(writer, {
 					(0, payment_id, required),
+					(1, reason, option),
 					(2, payment_hash, required),
 				})
 			},
@@ -1195,13 +1199,16 @@ impl MaybeReadable for Event {
 				let f = || {
 					let mut payment_hash = PaymentHash([0; 32]);
 					let mut payment_id = PaymentId([0; 32]);
+					let mut reason = None;
 					read_tlv_fields!(reader, {
 						(0, payment_id, required),
+						(1, reason, upgradable_option),
 						(2, payment_hash, required),
 					});
 					Ok(Some(Event::PaymentFailed {
 						payment_id,
 						payment_hash,
+						reason,
 					}))
 				};
 				f()
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -36,7 +36,7 @@ use crate::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, Fee
 use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, HTLC_FAIL_BACK_BUFFER, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY, MonitorEvent, CLOSED_CHANNEL_UPDATE_ID};
 use crate::chain::transaction::{OutPoint, TransactionData};
 use crate::events;
-use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason, HTLCDestination};
+use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason, HTLCDestination, PaymentFailureReason};
 // Since this struct is returned in `list_channels` methods, expose it here in case users want to
 // construct one themselves.
 use crate::ln::{inbound_payment, PaymentHash, PaymentPreimage, PaymentSecret};
@@ -2702,7 +2702,7 @@ where
 	/// [`Event::PaymentSent`]: events::Event::PaymentSent
 	pub fn abandon_payment(&self, payment_id: PaymentId) {
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
-		self.pending_outbound_payments.abandon_payment(payment_id, &self.pending_events);
+		self.pending_outbound_payments.abandon_payment(payment_id, PaymentFailureReason::UserAbandoned, &self.pending_events);
 	}
 
 	/// Send a spontaneous payment, which is a payment that does not require the recipient to have
diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
@@ -13,7 +13,7 @@
 use crate::chain::{BestBlock, ChannelMonitorUpdateStatus, Confirm, Listen, Watch, keysinterface::EntropySource};
 use crate::chain::channelmonitor::ChannelMonitor;
 use crate::chain::transaction::OutPoint;
-use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose};
+use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, PaymentFailureReason};
 use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret};
 use crate::ln::channelmanager::{ChainParameters, ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure, PaymentId, MIN_CLTV_EXPIRY_DELTA};
 use crate::routing::gossip::{P2PGossipSync, NetworkGraph, NetworkUpdate};
@@ -1920,9 +1920,14 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 	};
 	if !conditions.expected_mpp_parts_remain {
 		match &payment_failed_events[1] {
-			Event::PaymentFailed { ref payment_hash, ref payment_id } => {
+			Event::PaymentFailed { ref payment_hash, ref payment_id, ref reason } => {
 				assert_eq!(*payment_hash, expected_payment_hash, "unexpected second payment_hash");
 				assert_eq!(*payment_id, expected_payment_id);
+				assert_eq!(reason.unwrap(), if expected_payment_failed_permanently {
+					PaymentFailureReason::RecipientRejected
+				} else {
+					PaymentFailureReason::RetriesExhausted
+				});
 			}
 			_ => panic!("Unexpected second event"),
 		}
@@ -2216,10 +2221,10 @@ pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe
 	let expected_destinations: Vec<HTLCDestination> = repeat(HTLCDestination::FailedPayment { payment_hash: our_payment_hash }).take(expected_paths.len()).collect();
 	expect_pending_htlcs_forwardable_and_htlc_handling_failed!(expected_paths[0].last().unwrap(), expected_destinations);
 
-	pass_failed_payment_back(origin_node, expected_paths, skip_last, our_payment_hash);
+	pass_failed_payment_back(origin_node, expected_paths, skip_last, our_payment_hash, PaymentFailureReason::RecipientRejected);
 }
 
-pub fn pass_failed_payment_back<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths_slice: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash) {
+pub fn pass_failed_payment_back<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths_slice: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash, expected_fail_reason: PaymentFailureReason) {
 	let mut expected_paths: Vec<_> = expected_paths_slice.iter().collect();
 	check_added_monitors!(expected_paths[0].last().unwrap(), expected_paths.len());
 
@@ -2305,9 +2310,10 @@ pub fn pass_failed_payment_back<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe
 			};
 			if i == expected_paths.len() - 1 {
 				match events[1] {
-					Event::PaymentFailed { ref payment_hash, ref payment_id } => {
+					Event::PaymentFailed { ref payment_hash, ref payment_id, ref reason } => {
 						assert_eq!(*payment_hash, our_payment_hash, "unexpected second payment_hash");
 						assert_eq!(*payment_id, expected_payment_id);
+						assert_eq!(reason.unwrap(), expected_fail_reason);
 					}
 					_ => panic!("Unexpected second event"),
 				}
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
@@ -18,7 +18,7 @@ use crate::chain::channelmonitor;
 use crate::chain::channelmonitor::{CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
 use crate::chain::transaction::OutPoint;
 use crate::chain::keysinterface::{ChannelSigner, EcdsaChannelSigner, EntropySource};
-use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination};
+use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination, PaymentFailureReason};
 use crate::ln::{PaymentPreimage, PaymentSecret, PaymentHash};
 use crate::ln::channel::{commitment_tx_base_weight, COMMITMENT_TX_WEIGHT_PER_HTLC, CONCURRENT_INBOUND_HTLC_FEE_BUFFER, FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE, MIN_AFFORDABLE_HTLC_COUNT};
 use crate::ln::channelmanager::{self, PaymentId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
@@ -9501,7 +9501,7 @@ fn test_double_partial_claim() {
 	];
 	expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[3], failed_destinations);
 
-	pass_failed_payment_back(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_hash);
+	pass_failed_payment_back(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_hash, PaymentFailureReason::RecipientRejected);
 
 	// nodes[1] now retries one of the two paths...
 	nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret), PaymentId(payment_hash.0)).unwrap();
diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
@@ -13,7 +13,7 @@
 
 use crate::chain::channelmonitor::{CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS};
 use crate::chain::keysinterface::{EntropySource, NodeSigner, Recipient};
-use crate::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure};
+use crate::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason};
 use crate::ln::{PaymentHash, PaymentSecret};
 use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
 use crate::ln::channelmanager::{HTLCForwardInfo, FailureCode, CLTV_FAR_FAR_AWAY, MIN_CLTV_EXPIRY_DELTA, PendingAddHTLCInfo, PendingHTLCInfo, PendingHTLCRouting, PaymentId};
@@ -212,9 +212,14 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
 		panic!("Unexpected event");
 	}
 	match events[1] {
-		Event::PaymentFailed { payment_hash: ev_payment_hash, payment_id: ev_payment_id } => {
+		Event::PaymentFailed { payment_hash: ev_payment_hash, payment_id: ev_payment_id, reason: ref ev_reason } => {
 			assert_eq!(*payment_hash, ev_payment_hash);
 			assert_eq!(payment_id, ev_payment_id);
+			assert_eq!(if expected_retryable {
+				PaymentFailureReason::RetriesExhausted
+			} else {
+				PaymentFailureReason::RecipientRejected
+			}, ev_reason.unwrap());
 		}
 		_ => panic!("Unexpected second event"),
 	}
diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
@@ -14,7 +14,7 @@ use bitcoin::hashes::sha256::Hash as Sha256;
 use bitcoin::secp256k1::{self, Secp256k1, SecretKey};
 
 use crate::chain::keysinterface::{EntropySource, NodeSigner, Recipient};
-use crate::events;
+use crate::events::{self, PaymentFailureReason};
 use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
 use crate::ln::channelmanager::{ChannelDetails, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, PaymentId};
 use crate::ln::onion_utils::HTLCFailReason;
@@ -68,6 +68,8 @@ pub(crate) enum PendingOutboundPayment {
 	Abandoned {
 		session_privs: HashSet<[u8; 32]>,
 		payment_hash: PaymentHash,
+		/// Will be `None` if the payment was serialized before 0.0.115.
+		reason: Option<PaymentFailureReason>,
 	},
 }
 
@@ -145,21 +147,22 @@ impl PendingOutboundPayment {
 		*self = PendingOutboundPayment::Fulfilled { session_privs, payment_hash, timer_ticks_without_htlcs: 0 };
 	}
 
-	fn mark_abandoned(&mut self) -> Result<(), ()> {
-		let mut session_privs = HashSet::new();
-		let our_payment_hash;
-		core::mem::swap(&mut session_privs, match self {
+	fn mark_abandoned(&mut self, reason: PaymentFailureReason) -> Result<(), ()> {
+		match self {
 			PendingOutboundPayment::Legacy { .. } |
-			PendingOutboundPayment::Fulfilled { .. } =>
-				return Err(()),
-			PendingOutboundPayment::Retryable { session_privs, payment_hash, .. } |
-			PendingOutboundPayment::Abandoned { session_privs, payment_hash, .. } => {
-				our_payment_hash = *payment_hash;
-				session_privs
+			PendingOutboundPayment::Fulfilled { .. } => Err(()),
+			PendingOutboundPayment::Retryable { session_privs, payment_hash, .. } => {
+				let mut our_session_privs = HashSet::new();
+				core::mem::swap(&mut our_session_privs, session_privs);
+				*self = PendingOutboundPayment::Abandoned {
+					session_privs: our_session_privs,
+					payment_hash: *payment_hash,
+					reason: Some(reason)
+				};
+				Ok(())
 			},
-		});
-		*self = PendingOutboundPayment::Abandoned { session_privs, payment_hash: our_payment_hash };
-		Ok(())
+			PendingOutboundPayment::Abandoned { .. } => Ok(()),
+		}
 	}
 
 	/// panics if path is None and !self.is_fulfilled
@@ -546,10 +549,12 @@ impl OutboundPayments {
 		outbounds.retain(|pmt_id, pmt| {
 			let mut retain = true;
 			if !pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 {
-				if pmt.mark_abandoned().is_ok() {
+				let _ = pmt.mark_abandoned(PaymentFailureReason::RetriesExhausted);
+				if let PendingOutboundPayment::Abandoned { payment_hash, reason, .. } = pmt {
 					pending_events.lock().unwrap().push(events::Event::PaymentFailed {
 						payment_id: *pmt_id,
-						payment_hash: pmt.payment_hash().expect("PendingOutboundPayments::Retryable always has a payment hash set"),
+						payment_hash: *payment_hash,
+						reason: *reason,
 					});
 					retain = false;
 				}
@@ -629,7 +634,7 @@ impl OutboundPayments {
 		#[cfg(feature = "std")] {
 			if has_expired(&route_params) {
 				log_error!(logger, "Payment params expired on retry, abandoning payment {}", log_bytes!(payment_id.0));
-				self.abandon_payment(payment_id, pending_events);
+				self.abandon_payment(payment_id, PaymentFailureReason::PaymentExpired, pending_events);
 				return
 			}
 		}
@@ -642,14 +647,14 @@ impl OutboundPayments {
 			Ok(route) => route,
 			Err(e) => {
 				log_error!(logger, "Failed to find a route on retry, abandoning payment {}: {:#?}", log_bytes!(payment_id.0), e);
-				self.abandon_payment(payment_id, pending_events);
+				self.abandon_payment(payment_id, PaymentFailureReason::RouteNotFound, pending_events);
 				return
 			}
 		};
 		for path in route.paths.iter() {
 			if path.len() == 0 {
 				log_error!(logger, "length-0 path in route");
-				self.abandon_payment(payment_id, pending_events);
+				self.abandon_payment(payment_id, PaymentFailureReason::UnexpectedError, pending_events);
 				return
 			}
 		}
@@ -661,13 +666,17 @@ impl OutboundPayments {
 		}
 
 		macro_rules! abandon_with_entry {
-			($payment: expr) => {
-				if $payment.get_mut().mark_abandoned().is_ok() && $payment.get().remaining_parts() == 0 {
-					pending_events.lock().unwrap().push(events::Event::PaymentFailed {
-						payment_id,
-						payment_hash,
-					});
-					$payment.remove();
+			($payment: expr, $reason: expr) => {
+				let _ = $payment.get_mut().mark_abandoned($reason);
+				if let PendingOutboundPayment::Abandoned { reason, .. } = $payment.get() {
+					if $payment.get().remaining_parts() == 0 {
+						pending_events.lock().unwrap().push(events::Event::PaymentFailed {
+							payment_id,
+							payment_hash,
+							reason: *reason,
+						});
+						$payment.remove();
+					}
 				}
 			}
 		}
@@ -682,7 +691,7 @@ impl OutboundPayments {
 							let retry_amt_msat: u64 = route.paths.iter().map(|path| path.last().unwrap().fee_msat).sum();
 							if retry_amt_msat + *pending_amt_msat > *total_msat * (100 + RETRY_OVERFLOW_PERCENTAGE) / 100 {
 								log_error!(logger, "retry_amt_msat of {} will put pending_amt_msat (currently: {}) more than 10% over total_payment_amt_msat of {}", retry_amt_msat, pending_amt_msat, total_msat);
-								abandon_with_entry!(payment);
+								abandon_with_entry!(payment, PaymentFailureReason::UnexpectedError);
 								return
 							}
 							(*total_msat, *payment_secret, *keysend_preimage)
@@ -702,7 +711,7 @@ impl OutboundPayments {
 					};
 					if !payment.get().is_retryable_now() {
 						log_error!(logger, "Retries exhausted for payment id {}", log_bytes!(payment_id.0));
-						abandon_with_entry!(payment);
+						abandon_with_entry!(payment, PaymentFailureReason::RetriesExhausted);
 						return
 					}
 					payment.get_mut().increment_attempts();
@@ -759,12 +768,13 @@ impl OutboundPayments {
 				// initial HTLC-Add messages yet.
 			},
 			PaymentSendFailure::PathParameterError(results) => {
+				log_error!(logger, "Failed to send to route due to parameter error in a single path. Your router is buggy");
 				Self::push_path_failed_evs_and_scids(payment_id, payment_hash, &mut route_params, route.paths, results.into_iter(), pending_events);
-				self.abandon_payment(payment_id, pending_events);
+				self.abandon_payment(payment_id, PaymentFailureReason::UnexpectedError, pending_events);
 			},
 			PaymentSendFailure::ParameterError(e) => {
 				log_error!(logger, "Failed to send to route due to parameter error: {:?}. Your router is buggy", e);
-				self.abandon_payment(payment_id, pending_events);
+				self.abandon_payment(payment_id, PaymentFailureReason::UnexpectedError, pending_events);
 			},
 			PaymentSendFailure::DuplicatePayment => debug_assert!(false), // unreachable
 		}
@@ -1167,15 +1177,21 @@ impl OutboundPayments {
 			}
 
 			if payment_is_probe || !is_retryable_now || !payment_retryable {
-				let _ = payment.get_mut().mark_abandoned(); // we'll only Err if it's a legacy payment
+				let reason = if !payment_retryable {
+					PaymentFailureReason::RecipientRejected
+				} else {
+					PaymentFailureReason::RetriesExhausted
+				};
+				let _ = payment.get_mut().mark_abandoned(reason); // we'll only Err if it's a legacy payment
 				is_retryable_now = false;
 			}
 			if payment.get().remaining_parts() == 0 {
-				if payment.get().abandoned() {
+				if let PendingOutboundPayment::Abandoned { payment_hash, reason, .. }= payment.get() {
 					if !payment_is_probe {
 						full_failure_ev = Some(events::Event::PaymentFailed {
 							payment_id: *payment_id,
-							payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"),
+							payment_hash: *payment_hash,
+							reason: *reason,
 						});
 					}
 					payment.remove();
@@ -1233,15 +1249,17 @@ impl OutboundPayments {
 	}
 
 	pub(super) fn abandon_payment(
-		&self, payment_id: PaymentId, pending_events: &Mutex<Vec<events::Event>>
+		&self, payment_id: PaymentId, reason: PaymentFailureReason, pending_events: &Mutex<Vec<events::Event>>
 	) {
 		let mut outbounds = self.pending_outbound_payments.lock().unwrap();
 		if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
-			if let Ok(()) = payment.get_mut().mark_abandoned() {
+			let _ = payment.get_mut().mark_abandoned(reason);
+			if let PendingOutboundPayment::Abandoned { payment_hash, reason, .. } = payment.get() {
 				if payment.get().remaining_parts() == 0 {
 					pending_events.lock().unwrap().push(events::Event::PaymentFailed {
 						payment_id,
-						payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"),
+						payment_hash: *payment_hash,
+						reason: *reason,
 					});
 					payment.remove();
 				}
@@ -1303,6 +1321,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
 	},
 	(3, Abandoned) => {
 		(0, session_privs, required),
+		(1, reason, option),
 		(2, payment_hash, required),
 	},
 );
@@ -1312,7 +1331,7 @@ mod tests {
 	use bitcoin::network::constants::Network;
 	use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey};
 
-	use crate::events::{Event, PathFailure};
+	use crate::events::{Event, PathFailure, PaymentFailureReason};
 	use crate::ln::PaymentHash;
 	use crate::ln::channelmanager::PaymentId;
 	use crate::ln::features::{ChannelFeatures, NodeFeatures};
@@ -1360,7 +1379,9 @@ mod tests {
 				&pending_events, &|_, _, _, _, _, _, _, _| Ok(()));
 			let events = pending_events.lock().unwrap();
 			assert_eq!(events.len(), 1);
-			if let Event::PaymentFailed { .. } = events[0] { } else { panic!("Unexpected event"); }
+			if let Event::PaymentFailed { ref reason, .. } = events[0] {
+				assert_eq!(reason.unwrap(), PaymentFailureReason::PaymentExpired);
+			} else { panic!("Unexpected event"); }
 		} else {
 			let err = outbound_payments.send_payment(
 				PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), expired_route_params,
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
