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

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!
 	///
@@ -773,7 +783,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;
@@ -794,6 +804,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),
  });
  },
@@ -992,6 +1003,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),
@@ -1001,6 +1013,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 {
@@ -1018,6 +1031,7 @@ impl MaybeReadable for Event {
  purpose,
  via_channel_id,
  via_user_channel_id,
+ claim_deadline,
  }))
  };
  f()

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -204,7 +204,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());
@@ -573,7 +573,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());
@@ -690,7 +690,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());
@@ -1668,7 +1668,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());
@@ -1685,7 +1685,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

@@ -3363,8 +3363,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 {
@@ -3397,6 +3399,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 {
@@ -3450,6 +3453,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();
@@ -3460,6 +3464,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(_) => {
@@ -3935,16 +3940,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
@@ -3981,50 +3988,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());
@@ -1962,9 +1962,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);
@@ -1980,7 +1981,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 {
@@ -1996,9 +1997,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());
  }
@@ -2012,10 +2015,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

@@ -1966,7 +1966,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());
@@ -1982,7 +1982,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());
@@ -3764,7 +3764,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());