Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 7, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab25589
Allow claiming a payment if a channel with an HTLC has closed
TheBlueMatt Apr 4, 2023
4a8d01d
Add a `claim_deadline` field to `PaymentClaimable` with guarantees
TheBlueMatt Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions lightning/src/events/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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!
///
Expand Down Expand Up @@ -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;
Expand All @@ -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),
});
},
Expand Down Expand Up @@ -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),
Expand All @@ -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 {
Expand All @@ -1018,6 +1031,7 @@ impl MaybeReadable for Event {
purpose,
via_channel_id,
via_user_channel_id,
claim_deadline,
}))
};
f()
Expand Down
10 changes: 5 additions & 5 deletions lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand All @@ -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());
Expand Down
54 changes: 14 additions & 40 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
Expand All @@ -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(_) => {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
14 changes: 9 additions & 5 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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);
Expand All @@ -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 {
Expand All @@ -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());
}
Expand All @@ -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) {
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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());
Expand Down Expand Up @@ -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());
Expand Down
Loading