From 4a8d01dd195cf1b0c4a53ab609700e24f3a9f2a3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 4 Apr 2023 04:21:45 +0000 Subject: [PATCH] Add a `claim_deadline` field to `PaymentClaimable` with guarantees Now that we guarantee `claim_payment` will always succeed we have to let the user know what the deadline is. We still fail payments if they haven't been claimed in time, which we now expose in `PaymentClaimable`. --- lightning/src/events/mod.rs | 18 ++++++++++++++++-- lightning/src/ln/chanmon_update_fail_tests.rs | 10 +++++----- lightning/src/ln/channelmanager.rs | 13 ++++++++++--- lightning/src/ln/functional_test_utils.rs | 14 +++++++++----- lightning/src/ln/functional_tests.rs | 6 +++--- lightning/src/ln/payment_tests.rs | 9 ++++++++- 6 files changed, 51 insertions(+), 19 deletions(-) diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index 2d576d33..318cc2a6 100644 --- a/lightning/src/events/mod.rs +++ b/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, + /// 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, }, /// 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::; // 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() diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 94df7371..d12fe5c3 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/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()); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 8b536c5d..241235fd 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/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,9 +3940,10 @@ 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` @@ -3945,6 +3951,7 @@ where /// 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 diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index d7fd9c8e..2d52135d 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/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, ev: MessageSendEvent, payment_claimable_expected: bool, clear_recipient_events: bool, expected_preimage: Option) { +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, ev: MessageSendEvent, payment_claimable_expected: bool, clear_recipient_events: bool, expected_preimage: Option) -> Option { 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, ev: MessageSendEvent, payment_claimable_expected: bool, expected_preimage: Option) { - 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, ev: MessageSendEvent, payment_claimable_expected: bool, expected_preimage: Option) -> Option { + 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) { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 4f73627f..9c0a4b4b 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/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()); diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index fb50f5a4..028af20e 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -2898,9 +2898,16 @@ fn do_claim_from_closed_chan(fail_payment: bool) { assert_eq!(send_msgs.len(), 2); pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 10_000_000, payment_hash, Some(payment_secret), send_msgs.remove(0), false, None); - pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 10_000_000, + let receive_event = pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 10_000_000, payment_hash, Some(payment_secret), send_msgs.remove(0), true, None); + match receive_event.unwrap() { + Event::PaymentClaimable { claim_deadline, .. } => { + assert_eq!(claim_deadline.unwrap(), final_cltv - HTLC_FAIL_BACK_BUFFER); + }, + _ => panic!(), + } + // Ensure that the claim_deadline is correct, with the payment failing at exactly the given // height. connect_blocks(&nodes[3], final_cltv - HTLC_FAIL_BACK_BUFFER - nodes[3].best_block_info().1 -- 2.30.2