Enforce explicit claims on payments with even custom TLVs

Because we don't know which custom TLV type numbers the user is
expecting (and it would be cumbersome for them to tell us), instead of
failing unknown even custom TLVs on deserialization, we accept all
custom TLVs, and pass them to the user to check whether they recognize
them and choose to fail back if they don't. However, a user may not
check for custom TLVs, in which case we should reject any even custom
TLVs as unknown.

This commit makes sure a user must explicitly accept a payment with
even custom TLVs, by (1) making the default
`ChannelManager::claim_funds` fail if the payment had even custom TLVs
and (2) adding a new function
`ChannelManager::claim_funds_with_known_custom_tlvs` that accepts them.

This commit also refactors our custom TLVs test and updates various
documentation to account for this.

Author: alecchendev

lightning/src/events/mod.rs

@@ -355,9 +355,19 @@ pub enum Event {
 	/// Note that if the preimage is not known, you should call
 	/// [`ChannelManager::fail_htlc_backwards`] or [`ChannelManager::fail_htlc_backwards_with_reason`]
 	/// to free up resources for this HTLC and avoid network congestion.
-	/// If you fail to call either [`ChannelManager::claim_funds`], [`ChannelManager::fail_htlc_backwards`],
-	/// or [`ChannelManager::fail_htlc_backwards_with_reason`] within the HTLC's timeout, the HTLC will be
-	/// automatically failed.
+	///
+	/// If [`Event::PaymentClaimable::onion_fields`] is `Some`, and includes custom TLVs with even type
+	/// numbers, you should use [`ChannelManager::fail_htlc_backwards_with_reason`] with
+	/// [`FailureCode::InvalidOnionPayload`] if you fail to decode these TLVs, or
+	/// [`ChannelManager::claim_funds_with_known_custom_tlvs`] if you successfully decode them.
+	/// If you don't intend to check for custom TLVs, you can simply use
+	/// [`ChannelManager::claim_funds`], which will automatically fail back even custom TLVs.
+	///
+	/// If you fail to call [`ChannelManager::claim_funds`],
+	/// [`ChannelManager::claim_funds_with_known_custom_tlvs`],
+	/// [`ChannelManager::fail_htlc_backwards`], or
+	/// [`ChannelManager::fail_htlc_backwards_with_reason`] within the HTLC's timeout, the HTLC will
+	/// be automatically failed.
 	///
 	/// # Note
 	/// LDK will not stop an inbound payment from being paid multiple times, so multiple
@@ -369,6 +379,8 @@ pub enum Event {
 	/// This event used to be called `PaymentReceived` in LDK versions 0.0.112 and earlier.
 	///
 	/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds
+	/// [`ChannelManager::claim_funds_with_known_custom_tlvs`]: crate::ln::channelmanager::ChannelManager::claim_funds_with_known_custom_tlvs
+	/// [`FailureCode::InvalidOnionPayload`]: crate::ln::channelmanager::FailureCode::InvalidOnionPayload
 	/// [`ChannelManager::fail_htlc_backwards`]: crate::ln::channelmanager::ChannelManager::fail_htlc_backwards
 	/// [`ChannelManager::fail_htlc_backwards_with_reason`]: crate::ln::channelmanager::ChannelManager::fail_htlc_backwards_with_reason
 	PaymentClaimable {

lightning/src/ln/channelmanager.rs

@@ -4388,13 +4388,35 @@ where
 	/// event matches your expectation. If you fail to do so and call this method, you may provide
 	/// the sender "proof-of-payment" when they did not fulfill the full expected payment.
 	///
+	/// This function will fail the payment if it has custom TLVs with even type numbers, as we
+	/// will assume they are unknown. If you intend to accept even custom TLVs, you should use
+	/// [`claim_funds_with_known_custom_tlvs`].
+	///
 	/// [`Event::PaymentClaimable`]: crate::events::Event::PaymentClaimable
 	/// [`Event::PaymentClaimable::claim_deadline`]: crate::events::Event::PaymentClaimable::claim_deadline
 	/// [`Event::PaymentClaimed`]: crate::events::Event::PaymentClaimed
 	/// [`process_pending_events`]: EventsProvider::process_pending_events
 	/// [`create_inbound_payment`]: Self::create_inbound_payment
 	/// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash
+	/// [`claim_funds_with_known_custom_tlvs`]: Self::claim_funds_with_known_custom_tlvs
 	pub fn claim_funds(&self, payment_preimage: PaymentPreimage) {
+		self.claim_payment_internal(payment_preimage, false);
+	}
+
+	/// This is a variant of [`claim_funds`] that allows accepting a payment with custom TLVs with
+	/// even type numbers.
+	///
+	/// # Note
+	///
+	/// You MUST check you've understood all even TLVs before using this to
+	/// claim, otherwise you may unintentionally agree to some protocol you do not understand.
+	///
+	/// [`claim_funds`]: Self::claim_funds
+	pub fn claim_funds_with_known_custom_tlvs(&self, payment_preimage: PaymentPreimage) {
+		self.claim_payment_internal(payment_preimage, true);
+	}
+
+	fn claim_payment_internal(&self, payment_preimage: PaymentPreimage, custom_tlvs_known: bool) {
 		let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
 
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
@@ -4421,6 +4443,23 @@ where
 					log_error!(self.logger, "Got a duplicate pending claimable event on payment hash {}! Please report this bug",
 						log_bytes!(payment_hash.0));
 				}
+
+				if let Some(RecipientOnionFields { custom_tlvs: Some(ref tlvs), .. }) = payment.onion_fields {
+					if !custom_tlvs_known && tlvs.iter().any(|(typ, _)| typ % 2 == 0) {
+						log_info!(self.logger, "Cannot accept payment with unknown even TLVs. Rejecting payment with payment hash {}",
+							log_bytes!(payment_hash.0));
+						claimable_payments.pending_claiming_payments.remove(&payment_hash);
+						mem::drop(claimable_payments);
+						for htlc in payment.htlcs {
+							let reason = self.get_htlc_fail_reason_from_failure_code(FailureCode::InvalidOnionPayload(None), &htlc);
+							let source = HTLCSource::PreviousHopData(htlc.prev_hop);
+							let receiver = HTLCDestination::FailedPayment { payment_hash };
+							self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
+						}
+						return;
+					}
+				}
+
 				payment.htlcs
 			} else { return; }
 		};

lightning/src/ln/functional_test_utils.rs

@@ -2169,7 +2169,10 @@ pub fn do_claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>,
 		assert_eq!(path.last().unwrap().node.get_our_node_id(), expected_paths[0].last().unwrap().node.get_our_node_id());
 	}
 	expected_paths[0].last().unwrap().node.claim_funds(our_payment_preimage);
+	pass_claimed_payment_along_route(origin_node, expected_paths, skip_last, our_payment_preimage)
+}
 
+pub fn pass_claimed_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_preimage: PaymentPreimage) -> u64 {
 	let claim_event = expected_paths[0].last().unwrap().node.get_and_clear_pending_events();
 	assert_eq!(claim_event.len(), 1);
 	match claim_event[0] {

lightning/src/ln/payment_tests.rs

@@ -3197,12 +3197,20 @@ fn claim_from_closed_chan() {
 }
 
 #[test]
-fn test_custom_tlvs() {
-	do_test_custom_tlvs(true);
-	do_test_custom_tlvs(false);
+fn test_custom_tlvs_basic() {
+	do_test_custom_tlvs(false, false, false);
+	do_test_custom_tlvs(true, false, false);
 }
 
-fn do_test_custom_tlvs(spontaneous: bool) {
+#[test]
+fn test_custom_tlvs_explicit_claim() {
+	// Test that when receiving even custom TLVs the user must explicitly accept in case they
+	// are unknown.
+	do_test_custom_tlvs(false, true, false);
+	do_test_custom_tlvs(false, true, true);
+}
+
+fn do_test_custom_tlvs(spontaneous: bool, even_tlvs: bool, known_tlvs: bool) {
 	let chanmon_cfgs = create_chanmon_cfgs(2);
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None; 2]);
@@ -3214,7 +3222,7 @@ fn do_test_custom_tlvs(spontaneous: bool) {
 	let (mut route, our_payment_hash, our_payment_preimage, our_payment_secret) = get_route_and_payment_hash!(&nodes[0], &nodes[1], amt_msat);
 	let payment_id = PaymentId(our_payment_hash.0);
 	let custom_tlvs = vec![
-		(5482373483, vec![1, 2, 3, 4]),
+		(if even_tlvs { 5482373482 } else { 5482373483 }, vec![1, 2, 3, 4]),
 		(5482373487, vec![0x42u8; 16]),
 	];
 	let onion_fields = RecipientOnionFields {
@@ -3257,7 +3265,22 @@ fn do_test_custom_tlvs(spontaneous: bool) {
 		_ => panic!("Unexpected event"),
 	}
 
-	claim_payment(&nodes[0], &[&nodes[1]], our_payment_preimage);
+	match (known_tlvs, even_tlvs) {
+		(true, _) => {
+			nodes[1].node.claim_funds_with_known_custom_tlvs(our_payment_preimage);
+			let expected_total_fee_msat = pass_claimed_payment_along_route(&nodes[0], &[&[&nodes[1]]], false, our_payment_preimage);
+			expect_payment_sent!(&nodes[0], our_payment_preimage, Some(expected_total_fee_msat));
+		},
+		(false, false) => {
+			claim_payment(&nodes[0], &[&nodes[1]], our_payment_preimage);
+		},
+		(false, true) => {
+			nodes[1].node.claim_funds(our_payment_preimage);
+			let expected_destinations = vec![HTLCDestination::FailedPayment { payment_hash: our_payment_hash }];
+			expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], expected_destinations);
+			pass_failed_payment_back(&nodes[0], &[&[&nodes[1]]], false, our_payment_hash, PaymentFailureReason::RecipientRejected);
+		}
+	}
 }
 
 #[test]