Merge pull request #2148 from TheBlueMatt/2023-04-claim-from-closed

Allow claiming a payment if a channel with an HTLC has closed

Author: TheBlueMatt

lightning/src/events/mod.rs

@@ -321,7 +321,9 @@ pub enum Event {
 	///
 	/// # Note
 	/// LDK will not stop an inbound payment from being paid multiple times, so multiple
-	/// `PaymentClaimable` events may be generated for the same payment.
+	/// `PaymentClaimable` events may be generated for the same payment. In such a case it is
+	/// polite (and required in the lightning specification) to fail the payment the second time
+	/// and give the sender their money back rather than accepting double payment.
 	///
 	/// # Note
 	/// This event used to be called `PaymentReceived` in LDK versions 0.0.112 and earlier.
@@ -349,6 +351,14 @@ pub enum Event {
 		via_channel_id: Option<[u8; 32]>,
 		/// The `user_channel_id` indicating over which channel we received the payment.
 		via_user_channel_id: Option<u128>,
+		/// The block height at which this payment will be failed back and will no longer be
+		/// eligible for claiming.
+		///
+		/// Prior to this height, a call to [`ChannelManager::claim_funds`] is guaranteed to
+		/// succeed, however you should wait for [`Event::PaymentClaimed`] to be sure.
+		///
+		/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds
+		claim_deadline: Option<u32>,
 	},
 	/// Indicates a payment has been claimed and we've received money!
 	///
@@ -770,7 +780,7 @@ impl Writeable for Event {
 				// We never write out FundingGenerationReady events as, upon disconnection, peers
 				// drop any channels which have not yet exchanged funding_signed.
 			},
-			&Event::PaymentClaimable { ref payment_hash, ref amount_msat, ref purpose, ref receiver_node_id, ref via_channel_id, ref via_user_channel_id } => {
+			&Event::PaymentClaimable { ref payment_hash, ref amount_msat, ref purpose, ref receiver_node_id, ref via_channel_id, ref via_user_channel_id, ref claim_deadline } => {
 				1u8.write(writer)?;
 				let mut payment_secret = None;
 				let payment_preimage;
@@ -791,6 +801,7 @@ impl Writeable for Event {
 					(4, amount_msat, required),
 					(5, via_user_channel_id, option),
 					(6, 0u64, required), // user_payment_id required for compatibility with 0.0.103 and earlier
+					(7, claim_deadline, option),
 					(8, payment_preimage, option),
 				});
 			},
@@ -989,6 +1000,7 @@ impl MaybeReadable for Event {
 					let mut receiver_node_id = None;
 					let mut _user_payment_id = None::<u64>; // For compatibility with 0.0.103 and earlier
 					let mut via_channel_id = None;
+					let mut claim_deadline = None;
 					let mut via_user_channel_id = None;
 					read_tlv_fields!(reader, {
 						(0, payment_hash, required),
@@ -998,6 +1010,7 @@ impl MaybeReadable for Event {
 						(4, amount_msat, required),
 						(5, via_user_channel_id, option),
 						(6, _user_payment_id, option),
+						(7, claim_deadline, option),
 						(8, payment_preimage, option),
 					});
 					let purpose = match payment_secret {
@@ -1015,6 +1028,7 @@ impl MaybeReadable for Event {
 						purpose,
 						via_channel_id,
 						via_user_channel_id,
+						claim_deadline,
 					}))
 				};
 				f()

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -208,7 +208,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
 	let events_3 = nodes[1].node.get_and_clear_pending_events();
 	assert_eq!(events_3.len(), 1);
 	match events_3[0] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, via_user_channel_id: _ } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
 			assert_eq!(payment_hash_1, *payment_hash);
 			assert_eq!(amount_msat, 1_000_000);
 			assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
@@ -581,7 +581,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 	let events_5 = nodes[1].node.get_and_clear_pending_events();
 	assert_eq!(events_5.len(), 1);
 	match events_5[0] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, via_user_channel_id: _ } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
 			assert_eq!(payment_hash_2, *payment_hash);
 			assert_eq!(amount_msat, 1_000_000);
 			assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
@@ -699,7 +699,7 @@ fn test_monitor_update_fail_cs() {
 	let events = nodes[1].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 1);
 	match events[0] {
-		Event::PaymentClaimable { payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, via_user_channel_id: _ } => {
+		Event::PaymentClaimable { payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
 			assert_eq!(payment_hash, our_payment_hash);
 			assert_eq!(amount_msat, 1_000_000);
 			assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());
@@ -1692,7 +1692,7 @@ fn test_monitor_update_fail_claim() {
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
 	match events[0] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, via_user_channel_id } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, via_user_channel_id, .. } => {
 			assert_eq!(payment_hash_2, *payment_hash);
 			assert_eq!(1_000_000, amount_msat);
 			assert_eq!(receiver_node_id.unwrap(), nodes[0].node.get_our_node_id());
@@ -1709,7 +1709,7 @@ fn test_monitor_update_fail_claim() {
 		_ => panic!("Unexpected event"),
 	}
 	match events[1] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, via_user_channel_id: _ } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
 			assert_eq!(payment_hash_3, *payment_hash);
 			assert_eq!(1_000_000, amount_msat);
 			assert_eq!(receiver_node_id.unwrap(), nodes[0].node.get_our_node_id());

lightning/src/ln/channelmanager.rs

@@ -3359,8 +3359,10 @@ where
 											}
 										}
 										let mut total_value = claimable_htlc.sender_intended_value;
+										let mut earliest_expiry = claimable_htlc.cltv_expiry;
 										for htlc in htlcs.iter() {
 											total_value += htlc.sender_intended_value;
+											earliest_expiry = cmp::min(earliest_expiry, htlc.cltv_expiry);
 											match &htlc.onion_payload {
 												OnionPayload::Invoice { .. } => {
 													if htlc.total_msat != $payment_data.total_msat {
@@ -3393,6 +3395,7 @@ where
 												amount_msat,
 												via_channel_id: Some(prev_channel_id),
 												via_user_channel_id: Some(prev_user_channel_id),
+												claim_deadline: Some(earliest_expiry - HTLC_FAIL_BACK_BUFFER),
 											});
 											payment_claimable_generated = true;
 										} else {
@@ -3446,6 +3449,7 @@ where
 													hash_map::Entry::Vacant(e) => {
 														let amount_msat = claimable_htlc.value;
 														claimable_htlc.total_value_received = Some(amount_msat);
+														let claim_deadline = Some(claimable_htlc.cltv_expiry - HTLC_FAIL_BACK_BUFFER);
 														let purpose = events::PaymentPurpose::SpontaneousPayment(preimage);
 														e.insert((purpose.clone(), vec![claimable_htlc]));
 														let prev_channel_id = prev_funding_outpoint.to_channel_id();
@@ -3456,6 +3460,7 @@ where
 															purpose,
 															via_channel_id: Some(prev_channel_id),
 															via_user_channel_id: Some(prev_user_channel_id),
+															claim_deadline,
 														});
 													},
 													hash_map::Entry::Occupied(_) => {
@@ -3931,16 +3936,18 @@ where
 	/// Provides a payment preimage in response to [`Event::PaymentClaimable`], generating any
 	/// [`MessageSendEvent`]s needed to claim the payment.
 	///
-	/// Note that calling this method does *not* guarantee that the payment has been claimed. You
-	/// *must* wait for an [`Event::PaymentClaimed`] event which upon a successful claim will be
-	/// provided to your [`EventHandler`] when [`process_pending_events`] is next called.
+	/// This method is guaranteed to ensure the payment has been claimed but only if the current
+	/// height is strictly below [`Event::PaymentClaimable::claim_deadline`]. To avoid race
+	/// conditions, you should wait for an [`Event::PaymentClaimed`] before considering the payment
+	/// successful. It will generally be available in the next [`process_pending_events`] call.
 	///
 	/// Note that if you did not set an `amount_msat` when calling [`create_inbound_payment`] or
 	/// [`create_inbound_payment_for_hash`] you must check that the amount in the `PaymentClaimable`
 	/// 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.
 	///
 	/// [`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
@@ -3977,50 +3984,17 @@ where
 		};
 		debug_assert!(!sources.is_empty());
 
-		// If we are claiming an MPP payment, we check that all channels which contain a claimable
-		// HTLC still exist. While this isn't guaranteed to remain true if a channel closes while
-		// we're claiming (or even after we claim, before the commitment update dance completes),
-		// it should be a relatively rare race, and we'd rather not claim HTLCs that require us to
-		// go on-chain (and lose the on-chain fee to do so) than just reject the payment.
-		//
-		// Note that we'll still always get our funds - as long as the generated
-		// `ChannelMonitorUpdate` makes it out to the relevant monitor we can claim on-chain.
-		//
-		// If we find an HTLC which we would need to claim but for which we do not have a
-		// channel, we will fail all parts of the MPP payment. While we could wait and see if
-		// the sender retries the already-failed path(s), it should be a pretty rare case where
-		// we got all the HTLCs and then a channel closed while we were waiting for the user to
-		// provide the preimage, so worrying too much about the optimal handling isn't worth
-		// it.
+		// Just in case one HTLC has been failed between when we generated the `PaymentClaimable`
+		// and when we got here we need to check that the amount we're about to claim matches the
+		// amount we told the user in the last `PaymentClaimable`. We also do a sanity-check that
+		// the MPP parts all have the same `total_msat`.
 		let mut claimable_amt_msat = 0;
 		let mut prev_total_msat = None;
 		let mut expected_amt_msat = None;
 		let mut valid_mpp = true;
 		let mut errs = Vec::new();
 		let per_peer_state = self.per_peer_state.read().unwrap();
 		for htlc in sources.iter() {
-			let (counterparty_node_id, chan_id) = match self.short_to_chan_info.read().unwrap().get(&htlc.prev_hop.short_channel_id) {
-				Some((cp_id, chan_id)) => (cp_id.clone(), chan_id.clone()),
-				None => {
-					valid_mpp = false;
-					break;
-				}
-			};
-
-			let peer_state_mutex_opt = per_peer_state.get(&counterparty_node_id);
-			if peer_state_mutex_opt.is_none() {
-				valid_mpp = false;
-				break;
-			}
-
-			let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
-			let peer_state = &mut *peer_state_lock;
-
-			if peer_state.channel_by_id.get(&chan_id).is_none() {
-				valid_mpp = false;
-				break;
-			}
-
 			if prev_total_msat.is_some() && prev_total_msat != Some(htlc.total_msat) {
 				log_error!(self.logger, "Somehow ended up with an MPP payment with different expected total amounts - this should not be reachable!");
 				debug_assert!(false);

lightning/src/ln/functional_test_utils.rs

@@ -1680,7 +1680,7 @@ macro_rules! expect_payment_claimable {
 		let events = $node.node.get_and_clear_pending_events();
 		assert_eq!(events.len(), 1);
 		match events[0] {
-			$crate::events::Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id: _, via_user_channel_id: _ } => {
+			$crate::events::Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, .. } => {
 				assert_eq!($expected_payment_hash, *payment_hash);
 				assert_eq!($expected_recv_value, amount_msat);
 				assert_eq!($expected_receiver_node_id, receiver_node_id.unwrap());
@@ -1963,9 +1963,10 @@ pub fn send_along_route_with_secret<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>,
 	payment_id
 }
 
-pub fn do_pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &[&Node<'a, 'b, 'c>], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: Option<PaymentSecret>, ev: MessageSendEvent, payment_claimable_expected: bool, clear_recipient_events: bool, expected_preimage: Option<PaymentPreimage>) {
+pub fn do_pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &[&Node<'a, 'b, 'c>], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: Option<PaymentSecret>, ev: MessageSendEvent, payment_claimable_expected: bool, clear_recipient_events: bool, expected_preimage: Option<PaymentPreimage>) -> Option<Event> {
 	let mut payment_event = SendEvent::from_event(ev);
 	let mut prev_node = origin_node;
+	let mut event = None;
 
 	for (idx, &node) in expected_path.iter().enumerate() {
 		assert_eq!(node.node.get_our_node_id(), payment_event.node_id);
@@ -1981,7 +1982,7 @@ pub fn do_pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_p
 			if payment_claimable_expected {
 				assert_eq!(events_2.len(), 1);
 				match events_2[0] {
-					Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref via_channel_id, ref via_user_channel_id } => {
+					Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, ref via_channel_id, ref via_user_channel_id, claim_deadline } => {
 						assert_eq!(our_payment_hash, *payment_hash);
 						assert_eq!(node.node.get_our_node_id(), receiver_node_id.unwrap());
 						match &purpose {
@@ -1997,9 +1998,11 @@ pub fn do_pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_p
 						assert_eq!(amount_msat, recv_value);
 						assert!(node.node.list_channels().iter().any(|details| details.channel_id == via_channel_id.unwrap()));
 						assert!(node.node.list_channels().iter().any(|details| details.user_channel_id == via_user_channel_id.unwrap()));
+						assert!(claim_deadline.unwrap() > node.best_block_info().1);
 					},
 					_ => panic!("Unexpected event"),
 				}
+				event = Some(events_2[0].clone());
 			} else {
 				assert!(events_2.is_empty());
 			}
@@ -2013,10 +2016,11 @@ pub fn do_pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_p
 
 		prev_node = node;
 	}
+	event
 }
 
-pub fn pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &[&Node<'a, 'b, 'c>], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: Option<PaymentSecret>, ev: MessageSendEvent, payment_claimable_expected: bool, expected_preimage: Option<PaymentPreimage>) {
-	do_pass_along_path(origin_node, expected_path, recv_value, our_payment_hash, our_payment_secret, ev, payment_claimable_expected, true, expected_preimage);
+pub fn pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &[&Node<'a, 'b, 'c>], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: Option<PaymentSecret>, ev: MessageSendEvent, payment_claimable_expected: bool, expected_preimage: Option<PaymentPreimage>) -> Option<Event> {
+	do_pass_along_path(origin_node, expected_path, recv_value, our_payment_hash, our_payment_secret, ev, payment_claimable_expected, true, expected_preimage)
 }
 
 pub fn pass_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&[&Node<'a, 'b, 'c>]], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: PaymentSecret) {

lightning/src/ln/functional_tests.rs

@@ -1997,7 +1997,7 @@ fn test_channel_reserve_holding_cell_htlcs() {
 	let events = nodes[2].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
 	match events[0] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, via_user_channel_id: _ } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
 			assert_eq!(our_payment_hash_21, *payment_hash);
 			assert_eq!(recv_value_21, amount_msat);
 			assert_eq!(nodes[2].node.get_our_node_id(), receiver_node_id.unwrap());
@@ -2013,7 +2013,7 @@ fn test_channel_reserve_holding_cell_htlcs() {
 		_ => panic!("Unexpected event"),
 	}
 	match events[1] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, via_user_channel_id: _ } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
 			assert_eq!(our_payment_hash_22, *payment_hash);
 			assert_eq!(recv_value_22, amount_msat);
 			assert_eq!(nodes[2].node.get_our_node_id(), receiver_node_id.unwrap());
@@ -3803,7 +3803,7 @@ fn do_test_drop_messages_peer_disconnect(messages_delivered: u8, simulate_broken
 	let events_2 = nodes[1].node.get_and_clear_pending_events();
 	assert_eq!(events_2.len(), 1);
 	match events_2[0] {
-		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, via_user_channel_id: _ } => {
+		Event::PaymentClaimable { ref payment_hash, ref purpose, amount_msat, receiver_node_id, via_channel_id, .. } => {
 			assert_eq!(payment_hash_1, *payment_hash);
 			assert_eq!(amount_msat, 1_000_000);
 			assert_eq!(receiver_node_id.unwrap(), nodes[1].node.get_our_node_id());