From 8f1763159e8b2902f2952388967f8c67d11219ad Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 19 Aug 2021 20:44:45 -0400 Subject: [PATCH] Prevent duplicate PaymentSent events by removing all pending outbound payments associated with the same MPP payment after the preimage is received --- lightning/src/ln/channelmanager.rs | 27 ++++++++--------------- lightning/src/ln/functional_test_utils.rs | 4 +++- lightning/src/util/events.rs | 5 ++++- 3 files changed, 16 insertions(+), 20 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 9b0f78b22..0389bd5cd 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3140,17 +3140,13 @@ impl ChannelMana let mut session_priv_bytes = [0; 32]; session_priv_bytes.copy_from_slice(&session_priv[..]); let mut outbounds = self.pending_outbound_payments.lock().unwrap(); - if let Some(sessions) = outbounds.get_mut(&mpp_id) { - if sessions.remove(&session_priv_bytes) { - self.pending_events.lock().unwrap().push( - events::Event::PaymentSent { payment_preimage } - ); - if sessions.len() == 0 { - outbounds.remove(&mpp_id); - } - } else { - log_trace!(self.logger, "Received duplicative fulfill for HTLC with payment_preimage {}", log_bytes!(payment_preimage.0)); - } + let found_payment = if let Some(mut sessions) = outbounds.remove(&mpp_id) { + sessions.remove(&session_priv_bytes) + } else { false }; + if found_payment { + self.pending_events.lock().unwrap().push( + events::Event::PaymentSent { payment_preimage } + ); } else { log_trace!(self.logger, "Received duplicative fulfill for HTLC with payment_preimage {}", log_bytes!(payment_preimage.0)); } @@ -5691,7 +5687,8 @@ mod tests { nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_third_raa); check_added_monitors!(nodes[0], 1); - // There's an existing bug that generates a PaymentSent event for each MPP path, so handle that here. + // Note that successful MPP payments will generate 1 event upon the first path's success. No + // further events will be generated for subsequence path successes. let events = nodes[0].node.get_and_clear_pending_events(); match events[0] { Event::PaymentSent { payment_preimage: ref preimage } => { @@ -5699,12 +5696,6 @@ mod tests { }, _ => panic!("Unexpected event"), } - match events[1] { - Event::PaymentSent { payment_preimage: ref preimage } => { - assert_eq!(payment_preimage, *preimage); - }, - _ => panic!("Unexpected event"), - } } #[test] diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 0389da497..ffca2ecd5 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1242,9 +1242,11 @@ pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, exp if !skip_last { last_update_fulfill_dance!(origin_node, expected_route.first().unwrap()); - expect_payment_sent!(origin_node, our_payment_preimage); } } + if !skip_last { + expect_payment_sent!(origin_node, our_payment_preimage); + } } pub fn claim_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], our_payment_preimage: PaymentPreimage) { diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index d63dd88b7..bf1bd0743 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -112,8 +112,11 @@ pub enum Event { /// payment is to pay an invoice or to send a spontaneous payment. purpose: PaymentPurpose, }, - /// Indicates an outbound payment we made succeeded (ie it made it all the way to its target + /// Indicates an outbound payment we made succeeded (i.e. it made it all the way to its target /// and we got back the payment preimage for it). + /// + /// Note for MPP payments: in rare cases, this event may be preceded by a `PaymentFailed` event. + /// In this situation, you SHOULD treat this payment as having succeeded. PaymentSent { /// The preimage to the hash given to ChannelManager::send_payment. /// Note that this serves as a payment receipt, if you wish to have such a thing, you must -- 2.39.5