From 14153edeed100bc311bd1eca95ac4717c4d6583c Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Tue, 6 Aug 2024 17:17:52 -0500 Subject: [PATCH] Make payment_hash optional in Event::PaymentFailed When abandoning a BOLT12 payment before a Bolt12Invoice is received, an Event::InvoiceRequestFailed is generated and the abandonment reason is lost. Make payment_hash optional in Event::PaymentFailed so that Event::InvoiceRequestFailed can be removed in favor of it. --- lightning/src/events/mod.rs | 17 +++++++++++++++-- lightning/src/ln/blinded_payment_tests.rs | 2 +- lightning/src/ln/chanmon_update_fail_tests.rs | 2 +- lightning/src/ln/channelmanager.rs | 3 ++- lightning/src/ln/functional_test_utils.rs | 4 ++-- lightning/src/ln/functional_tests.rs | 18 +++++++++--------- lightning/src/ln/offers_tests.rs | 2 +- lightning/src/ln/onion_route_tests.rs | 2 +- lightning/src/ln/outbound_payment.rs | 12 ++++++------ lightning/src/ln/payment_tests.rs | 10 +++++----- 10 files changed, 43 insertions(+), 29 deletions(-) diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index 393acbc02..a94d6c118 100644 --- a/lightning/src/events/mod.rs +++ b/lightning/src/events/mod.rs @@ -879,10 +879,12 @@ pub enum Event { /// /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment payment_id: PaymentId, - /// The hash that was given to [`ChannelManager::send_payment`]. + /// The hash that was given to [`ChannelManager::send_payment`]. `None` if the payment failed + /// before receiving an invoice when paying a BOLT12 [`Offer`]. /// /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment - payment_hash: PaymentHash, + /// [`Offer`]: crate::offers::offer::Offer + payment_hash: Option, /// The reason the payment failed. This is only `None` for events generated or serialized /// by versions prior to 0.0.115. reason: Option, @@ -1555,10 +1557,15 @@ impl Writeable for Event { }, &Event::PaymentFailed { ref payment_id, ref payment_hash, ref reason } => { 15u8.write(writer)?; + let (payment_hash, invoice_received) = match payment_hash { + Some(payment_hash) => (payment_hash, true), + None => (&PaymentHash([0; 32]), false), + }; write_tlv_fields!(writer, { (0, payment_id, required), (1, reason, option), (2, payment_hash, required), + (3, invoice_received, required), }) }, &Event::OpenChannelRequest { .. } => { @@ -1932,11 +1939,17 @@ impl MaybeReadable for Event { let mut payment_hash = PaymentHash([0; 32]); let mut payment_id = PaymentId([0; 32]); let mut reason = None; + let mut invoice_received: Option = None; read_tlv_fields!(reader, { (0, payment_id, required), (1, reason, upgradable_option), (2, payment_hash, required), + (3, invoice_received, option), }); + let payment_hash = match invoice_received { + Some(invoice_received) => invoice_received.then(|| payment_hash), + None => (payment_hash != PaymentHash([0; 32])).then(|| payment_hash), + }; Ok(Some(Event::PaymentFailed { payment_id, payment_hash, diff --git a/lightning/src/ln/blinded_payment_tests.rs b/lightning/src/ln/blinded_payment_tests.rs index c1d65fd1a..9d246f1a7 100644 --- a/lightning/src/ln/blinded_payment_tests.rs +++ b/lightning/src/ln/blinded_payment_tests.rs @@ -1068,7 +1068,7 @@ fn blinded_path_retries() { assert_eq!(evs.len(), 1); match evs[0] { Event::PaymentFailed { payment_hash: ev_payment_hash, reason, .. } => { - assert_eq!(ev_payment_hash, payment_hash); + assert_eq!(ev_payment_hash, Some(payment_hash)); // We have 1 retry attempt remaining, but we're out of blinded paths to try. assert_eq!(reason, Some(PaymentFailureReason::RouteNotFound)); }, diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 9bc48f12f..52669637d 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -1776,7 +1776,7 @@ fn test_monitor_update_on_pending_forwards() { } else { panic!("Unexpected event!"); } match events[2] { Event::PaymentFailed { payment_hash, .. } => { - assert_eq!(payment_hash, payment_hash_1); + assert_eq!(payment_hash, Some(payment_hash_1)); }, _ => panic!("Unexpected event"), } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 617c58c2e..7b903cd02 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1681,7 +1681,8 @@ where /// channel_manager.process_pending_events(&|event| { /// match event { /// Event::PaymentSent { payment_hash, .. } => println!("Paid {}", payment_hash), -/// Event::PaymentFailed { payment_hash, .. } => println!("Failed paying {}", payment_hash), +/// Event::PaymentFailed { payment_hash: Some(payment_hash), .. } => +/// println!("Failed paying {}", payment_hash), /// // ... /// # _ => {}, /// } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 5f1b7123d..2845ddd9a 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -2520,7 +2520,7 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>( if !conditions.expected_mpp_parts_remain { match &payment_failed_events[1] { Event::PaymentFailed { ref payment_hash, ref payment_id, ref reason } => { - assert_eq!(*payment_hash, expected_payment_hash, "unexpected second payment_hash"); + assert_eq!(*payment_hash, Some(expected_payment_hash), "unexpected second payment_hash"); assert_eq!(*payment_id, expected_payment_id); assert_eq!(reason.unwrap(), if expected_payment_failed_permanently { PaymentFailureReason::RecipientRejected @@ -3162,7 +3162,7 @@ pub fn pass_failed_payment_back<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe if i == expected_paths.len() - 1 { match events[1] { Event::PaymentFailed { ref payment_hash, ref payment_id, ref reason } => { - assert_eq!(*payment_hash, our_payment_hash, "unexpected second payment_hash"); + assert_eq!(*payment_hash, Some(our_payment_hash), "unexpected second payment_hash"); assert_eq!(*payment_id, expected_payment_id); assert_eq!(reason.unwrap(), expected_fail_reason); } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 99063334a..617df3ce7 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -3380,7 +3380,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use ))); assert!(events.iter().any(|ev| matches!( ev, - Event::PaymentFailed { ref payment_hash, .. } if *payment_hash == fourth_payment_hash + Event::PaymentFailed { ref payment_hash, .. } if *payment_hash == Some(fourth_payment_hash) ))); nodes[1].node.process_pending_htlc_forwards(); @@ -3442,7 +3442,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use } match events[1] { Event::PaymentFailed { ref payment_hash, .. } => { - assert_eq!(*payment_hash, first_payment_hash); + assert_eq!(*payment_hash, Some(first_payment_hash)); }, _ => panic!("Unexpected event"), } @@ -3454,7 +3454,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use } match events[3] { Event::PaymentFailed { ref payment_hash, .. } => { - assert_eq!(*payment_hash, second_payment_hash); + assert_eq!(*payment_hash, Some(second_payment_hash)); }, _ => panic!("Unexpected event"), } @@ -3466,7 +3466,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use } match events[5] { Event::PaymentFailed { ref payment_hash, .. } => { - assert_eq!(*payment_hash, third_payment_hash); + assert_eq!(*payment_hash, Some(third_payment_hash)); }, _ => panic!("Unexpected event"), } @@ -3572,7 +3572,7 @@ fn fail_backward_pending_htlc_upon_channel_failure() { } match events[1] { Event::PaymentFailed { payment_hash, .. } => { - assert_eq!(payment_hash, failed_payment_hash); + assert_eq!(payment_hash, Some(failed_payment_hash)); }, _ => panic!("Unexpected event"), } @@ -3851,7 +3851,7 @@ fn test_simple_peer_disconnect() { } match events[3] { Event::PaymentFailed { payment_hash, .. } => { - assert_eq!(payment_hash, payment_hash_5); + assert_eq!(payment_hash, Some(payment_hash_5)); }, _ => panic!("Unexpected event"), } @@ -6007,7 +6007,7 @@ fn test_fail_holding_cell_htlc_upon_free() { } match &events[1] { &Event::PaymentFailed { ref payment_hash, .. } => { - assert_eq!(our_payment_hash.clone(), *payment_hash); + assert_eq!(Some(our_payment_hash), *payment_hash); }, _ => panic!("Unexpected event"), } @@ -6095,7 +6095,7 @@ fn test_free_and_fail_holding_cell_htlcs() { } match &events[1] { &Event::PaymentFailed { ref payment_hash, .. } => { - assert_eq!(payment_hash_2.clone(), *payment_hash); + assert_eq!(Some(payment_hash_2), *payment_hash); }, _ => panic!("Unexpected event"), } @@ -7048,7 +7048,7 @@ fn test_channel_failed_after_message_with_badonion_node_perm_bits_set() { } match events_5[1] { Event::PaymentFailed { payment_hash, .. } => { - assert_eq!(payment_hash, our_payment_hash); + assert_eq!(payment_hash, Some(our_payment_hash)); }, _ => panic!("Unexpected event"), } diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs index ee0f731b2..5b81251f6 100644 --- a/lightning/src/ln/offers_tests.rs +++ b/lightning/src/ln/offers_tests.rs @@ -2168,7 +2168,7 @@ fn fails_paying_invoice_with_unknown_required_features() { match get_event!(david, Event::PaymentFailed) { Event::PaymentFailed { payment_id: event_payment_id, - payment_hash: event_payment_hash, + payment_hash: Some(event_payment_hash), reason: Some(event_reason), } => { assert_eq!(event_payment_id, payment_id); diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index bd28f9698..94c2cfd2e 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -215,7 +215,7 @@ fn run_onion_failure_test_with_fail_intercept( } match events[1] { Event::PaymentFailed { payment_hash: ev_payment_hash, payment_id: ev_payment_id, reason: ref ev_reason } => { - assert_eq!(*payment_hash, ev_payment_hash); + assert_eq!(Some(*payment_hash), ev_payment_hash); assert_eq!(payment_id, ev_payment_id); assert_eq!(if expected_retryable { PaymentFailureReason::RetriesExhausted diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 5cc31f605..c04e186f1 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -961,7 +961,7 @@ impl OutboundPayments { if let PendingOutboundPayment::Abandoned { payment_hash, reason, .. } = pmt { pending_events.lock().unwrap().push_back((events::Event::PaymentFailed { payment_id: *pmt_id, - payment_hash: *payment_hash, + payment_hash: Some(*payment_hash), reason: *reason, }, None)); retain = false; @@ -1127,7 +1127,7 @@ impl OutboundPayments { if $payment.get().remaining_parts() == 0 { pending_events.lock().unwrap().push_back((events::Event::PaymentFailed { payment_id, - payment_hash, + payment_hash: Some(payment_hash), reason: *reason, }, None)); $payment.remove(); @@ -1795,7 +1795,7 @@ impl OutboundPayments { if !payment_is_probe { full_failure_ev = Some(events::Event::PaymentFailed { payment_id: *payment_id, - payment_hash: *payment_hash, + payment_hash: Some(*payment_hash), reason: *reason, }); } @@ -1864,7 +1864,7 @@ impl OutboundPayments { if payment.get().remaining_parts() == 0 { pending_events.lock().unwrap().push_back((events::Event::PaymentFailed { payment_id, - payment_hash: *payment_hash, + payment_hash: Some(*payment_hash), reason: *reason, }, None)); payment.remove(); @@ -2337,7 +2337,7 @@ mod tests { ); assert!(!outbound_payments.has_pending_payments()); - let payment_hash = invoice.payment_hash(); + let payment_hash = Some(invoice.payment_hash()); let reason = Some(PaymentFailureReason::PaymentExpired); assert!(!pending_events.lock().unwrap().is_empty()); @@ -2398,7 +2398,7 @@ mod tests { ); assert!(!outbound_payments.has_pending_payments()); - let payment_hash = invoice.payment_hash(); + let payment_hash = Some(invoice.payment_hash()); let reason = Some(PaymentFailureReason::RouteNotFound); assert!(!pending_events.lock().unwrap().is_empty()); diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 3ab055d2a..083c32e53 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -2268,7 +2268,7 @@ fn do_automatic_retries(test: AutoRetry) { } else { match events[1] { Event::PaymentFailed { payment_hash: ev_payment_hash, .. } => { - assert_eq!(payment_hash, ev_payment_hash); + assert_eq!(Some(payment_hash), ev_payment_hash); }, _ => panic!("Unexpected event"), } @@ -2351,7 +2351,7 @@ fn do_automatic_retries(test: AutoRetry) { assert_eq!(events.len(), 1); match events[0] { Event::PaymentFailed { payment_hash: ref ev_payment_hash, payment_id: ref ev_payment_id, reason: ref ev_reason } => { - assert_eq!(payment_hash, *ev_payment_hash); + assert_eq!(Some(payment_hash), *ev_payment_hash); assert_eq!(PaymentId(payment_hash.0), *ev_payment_id); assert_eq!(PaymentFailureReason::RetriesExhausted, ev_reason.unwrap()); }, @@ -2388,7 +2388,7 @@ fn do_automatic_retries(test: AutoRetry) { assert_eq!(events.len(), 1); match events[0] { Event::PaymentFailed { payment_hash: ref ev_payment_hash, payment_id: ref ev_payment_id, reason: ref ev_reason } => { - assert_eq!(payment_hash, *ev_payment_hash); + assert_eq!(Some(payment_hash), *ev_payment_hash); assert_eq!(PaymentId(payment_hash.0), *ev_payment_id); assert_eq!(PaymentFailureReason::RetriesExhausted, ev_reason.unwrap()); }, @@ -2409,7 +2409,7 @@ fn do_automatic_retries(test: AutoRetry) { assert_eq!(events.len(), 1); match events[0] { Event::PaymentFailed { payment_hash: ref ev_payment_hash, payment_id: ref ev_payment_id, reason: ref ev_reason } => { - assert_eq!(payment_hash, *ev_payment_hash); + assert_eq!(Some(payment_hash), *ev_payment_hash); assert_eq!(PaymentId(payment_hash.0), *ev_payment_id); assert_eq!(PaymentFailureReason::RouteNotFound, ev_reason.unwrap()); }, @@ -3099,7 +3099,7 @@ fn no_extra_retries_on_back_to_back_fail() { } match events[1] { Event::PaymentFailed { payment_hash: ref ev_payment_hash, payment_id: ref ev_payment_id, reason: ref ev_reason } => { - assert_eq!(payment_hash, *ev_payment_hash); + assert_eq!(Some(payment_hash), *ev_payment_hash); assert_eq!(PaymentId(payment_hash.0), *ev_payment_id); assert_eq!(PaymentFailureReason::RetriesExhausted, ev_reason.unwrap()); }, -- 2.39.5