Prevent duplicate PaymentSent events

by removing all pending outbound payments associated with the same
MPP payment after the preimage is received

Author: valentinewallace

lightning/src/ln/channelmanager.rs

@@ -3140,17 +3140,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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,20 +5687,15 @@ 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 } => {
 				assert_eq!(payment_preimage, *preimage);
 			},
 			_ => panic!("Unexpected event"),
 		}
-		match events[1] {
-			Event::PaymentSent { payment_preimage: ref preimage } => {
-				assert_eq!(payment_preimage, *preimage);
-			},
-			_ => panic!("Unexpected event"),
-		}
 	}
 
 	#[test]

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) {

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