Move BOLT12 invoice features check

When handling a BOLT12 invoice, and invoice error is sent if the invoice
contains unknown required features. However, since the payment is still
in state AwaitingInvoice, abandoning it results in losing the reason
since an InvoiceRequestFailed event would be generated. Move the check
to PendingOutboundPayments such that the payment is first moved to state
InvoiceReceived so that a PaymentFailed event is generated instead.

Author: jkczyz

lightning/src/ln/channelmanager.rs

@@ -4233,9 +4233,10 @@ where
 	fn send_payment_for_verified_bolt12_invoice(&self, invoice: &Bolt12Invoice, payment_id: PaymentId) -> Result<(), Bolt12PaymentError> {
 		let best_block_height = self.best_block.read().unwrap().height;
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
+		let features = self.bolt12_invoice_features();
 		self.pending_outbound_payments
 			.send_payment_for_bolt12_invoice(
-				invoice, payment_id, &self.router, self.list_usable_channels(),
+				invoice, payment_id, &self.router, self.list_usable_channels(), features,
 				|| self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, &self,
 				&self.secp_ctx, best_block_height, &self.logger, &self.pending_events,
 				|args| self.send_payment_along_path(args)
@@ -10838,27 +10839,6 @@ where
 					&self.logger, None, None, Some(invoice.payment_hash()),
 				);
 
-				let features = self.bolt12_invoice_features();
-				if invoice.invoice_features().requires_unknown_bits_from(&features) {
-					log_trace!(
-						logger, "Invoice requires unknown features: {:?}",
-						invoice.invoice_features(),
-					);
-					self.abandon_payment_with_reason(
-						payment_id, PaymentFailureReason::UnknownRequiredFeatures,
-					);
-
-					let error = InvoiceError::from(Bolt12SemanticError::UnknownRequiredFeatures);
-					let response = match responder {
-						Some(responder) => responder.respond(OffersMessage::InvoiceError(error)),
-						None => {
-							log_trace!(logger, "No reply path to send error: {:?}", error);
-							ResponseInstruction::NoResponse
-						},
-					};
-					return response;
-				}
-
 				if self.default_configuration.manually_handle_bolt12_invoices {
 					let event = Event::InvoiceReceived {
 						payment_id, invoice, context, responder,
@@ -10867,24 +10847,34 @@ where
 					return ResponseInstruction::NoResponse;
 				}
 
-				match self.send_payment_for_verified_bolt12_invoice(&invoice, payment_id) {
+				let error = match self.send_payment_for_verified_bolt12_invoice(
+					&invoice, payment_id,
+				) {
+					// Payments with UnknownRequiredFeatures error will already have been abandoned.
+					Err(Bolt12PaymentError::UnknownRequiredFeatures) => {
+						log_trace!(
+							logger, "Invoice requires unknown features: {:?}",
+							invoice.invoice_features()
+						);
+						InvoiceError::from(Bolt12SemanticError::UnknownRequiredFeatures)
+					},
 					// Payments with SendingFailed error will already have been abandoned.
 					Err(Bolt12PaymentError::SendingFailed(e)) => {
 						log_trace!(logger, "Failed paying invoice: {:?}", e);
-
-						let err = InvoiceError::from_string(format!("{:?}", e));
-						match responder {
-							Some(responder) => responder.respond(OffersMessage::InvoiceError(err)),
-							None => {
-								log_trace!(logger, "No reply path to send error: {:?}", err);
-								ResponseInstruction::NoResponse
-							},
-						}
+						InvoiceError::from_string(format!("{:?}", e))
 					},
 					// Otherwise, don't abandon unknown, pending, or successful payments.
 					Err(Bolt12PaymentError::UnexpectedInvoice)
 						| Err(Bolt12PaymentError::DuplicateInvoice)
-						| Ok(()) => ResponseInstruction::NoResponse,
+						| Ok(()) => return ResponseInstruction::NoResponse,
+				};
+
+				match responder {
+					Some(responder) => responder.respond(OffersMessage::InvoiceError(error)),
+					None => {
+						log_trace!(logger, "No reply path to send error: {:?}", error);
+						ResponseInstruction::NoResponse
+					},
 				}
 			},
 			#[cfg(async_payments)]

lightning/src/ln/outbound_payment.rs

@@ -19,6 +19,7 @@ use crate::events::{self, PaymentFailureReason};
 use crate::ln::types::{PaymentHash, PaymentPreimage, PaymentSecret};
 use crate::ln::channel_state::ChannelDetails;
 use crate::ln::channelmanager::{EventCompletionAction, HTLCSource, PaymentId};
+use crate::ln::features::Bolt12InvoiceFeatures;
 use crate::ln::onion_utils;
 use crate::ln::onion_utils::{DecodedOnionFailure, HTLCFailReason};
 use crate::offers::invoice::Bolt12Invoice;
@@ -510,6 +511,8 @@ pub enum Bolt12PaymentError {
 	UnexpectedInvoice,
 	/// Payment for an invoice with the corresponding [`PaymentId`] was already initiated.
 	DuplicateInvoice,
+	/// The invoice was valid for the corresponding [`PaymentId`], but required unknown features.
+	UnknownRequiredFeatures,
 	/// The invoice was valid for the corresponding [`PaymentId`], but sending the payment failed.
 	SendingFailed(RetryableSendFailure),
 }
@@ -783,9 +786,9 @@ impl OutboundPayments {
 		R: Deref, ES: Deref, NS: Deref, NL: Deref, IH, SP, L: Deref
 	>(
 		&self, invoice: &Bolt12Invoice, payment_id: PaymentId, router: &R,
-		first_hops: Vec<ChannelDetails>, inflight_htlcs: IH, entropy_source: &ES, node_signer: &NS,
-		node_id_lookup: &NL, secp_ctx: &Secp256k1<secp256k1::All>, best_block_height: u32,
-		logger: &L,
+		first_hops: Vec<ChannelDetails>, features: Bolt12InvoiceFeatures, inflight_htlcs: IH,
+		entropy_source: &ES, node_signer: &NS, node_id_lookup: &NL,
+		secp_ctx: &Secp256k1<secp256k1::All>, best_block_height: u32, logger: &L,
 		pending_events: &Mutex<VecDeque<(events::Event, Option<EventCompletionAction>)>>,
 		send_payment_along_path: SP,
 	) -> Result<(), Bolt12PaymentError>
@@ -819,6 +822,13 @@ impl OutboundPayments {
 			hash_map::Entry::Vacant(_) => return Err(Bolt12PaymentError::UnexpectedInvoice),
 		}
 
+		if invoice.invoice_features().requires_unknown_bits_from(&features) {
+			self.abandon_payment(
+				payment_id, PaymentFailureReason::UnknownRequiredFeatures, pending_events,
+			);
+			return Err(Bolt12PaymentError::UnknownRequiredFeatures);
+		}
+
 		let mut payment_params = PaymentParameters::from_bolt12_invoice(&invoice);
 
 		// Advance any blinded path where the introduction node is our node.
@@ -1951,7 +1961,7 @@ mod tests {
 	use crate::events::{Event, PathFailure, PaymentFailureReason};
 	use crate::ln::types::PaymentHash;
 	use crate::ln::channelmanager::{PaymentId, RecipientOnionFields};
-	use crate::ln::features::{ChannelFeatures, NodeFeatures};
+	use crate::ln::features::{Bolt12InvoiceFeatures, ChannelFeatures, NodeFeatures};
 	use crate::ln::msgs::{ErrorAction, LightningError};
 	use crate::ln::outbound_payment::{Bolt12PaymentError, OutboundPayments, Retry, RetryableSendFailure, StaleExpiration};
 	#[cfg(feature = "std")]
@@ -2319,9 +2329,9 @@ mod tests {
 
 		assert_eq!(
 			outbound_payments.send_payment_for_bolt12_invoice(
-				&invoice, payment_id, &&router, vec![], || InFlightHtlcs::new(), &&keys_manager,
-				&&keys_manager, &EmptyNodeIdLookUp {}, &secp_ctx, 0, &&logger, &pending_events,
-				|_| panic!()
+				&invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(),
+				|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {},
+				&secp_ctx, 0, &&logger, &pending_events, |_| panic!()
 			),
 			Err(Bolt12PaymentError::SendingFailed(RetryableSendFailure::PaymentExpired)),
 		);
@@ -2380,9 +2390,9 @@ mod tests {
 
 		assert_eq!(
 			outbound_payments.send_payment_for_bolt12_invoice(
-				&invoice, payment_id, &&router, vec![], || InFlightHtlcs::new(), &&keys_manager,
-				&&keys_manager, &EmptyNodeIdLookUp {}, &secp_ctx, 0, &&logger, &pending_events,
-				|_| panic!()
+				&invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(),
+				|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {},
+				&secp_ctx, 0, &&logger, &pending_events, |_| panic!()
 			),
 			Err(Bolt12PaymentError::SendingFailed(RetryableSendFailure::RouteNotFound)),
 		);
@@ -2454,9 +2464,9 @@ mod tests {
 		assert!(!outbound_payments.has_pending_payments());
 		assert_eq!(
 			outbound_payments.send_payment_for_bolt12_invoice(
-				&invoice, payment_id, &&router, vec![], || InFlightHtlcs::new(), &&keys_manager,
-				&&keys_manager, &EmptyNodeIdLookUp {}, &secp_ctx, 0, &&logger, &pending_events,
-				|_| panic!()
+				&invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(),
+				|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {},
+				&secp_ctx, 0, &&logger, &pending_events, |_| panic!()
 			),
 			Err(Bolt12PaymentError::UnexpectedInvoice),
 		);
@@ -2472,9 +2482,9 @@ mod tests {
 
 		assert_eq!(
 			outbound_payments.send_payment_for_bolt12_invoice(
-				&invoice, payment_id, &&router, vec![], || InFlightHtlcs::new(), &&keys_manager,
-				&&keys_manager, &EmptyNodeIdLookUp {}, &secp_ctx, 0, &&logger, &pending_events,
-				|_| Ok(())
+				&invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(),
+				|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {},
+				&secp_ctx, 0, &&logger, &pending_events, |_| Ok(())
 			),
 			Ok(()),
 		);
@@ -2483,9 +2493,9 @@ mod tests {
 
 		assert_eq!(
 			outbound_payments.send_payment_for_bolt12_invoice(
-				&invoice, payment_id, &&router, vec![], || InFlightHtlcs::new(), &&keys_manager,
-				&&keys_manager, &EmptyNodeIdLookUp {}, &secp_ctx, 0, &&logger, &pending_events,
-				|_| panic!()
+				&invoice, payment_id, &&router, vec![], Bolt12InvoiceFeatures::empty(),
+				|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, &EmptyNodeIdLookUp {},
+				&secp_ctx, 0, &&logger, &pending_events, |_| panic!()
 			),
 			Err(Bolt12PaymentError::DuplicateInvoice),
 		);