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

@@ -3144,16 +3144,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 let Some(mut sessions) = outbounds.remove(&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);
-						}
 						return
-					}
+					} else { unreachable!(); }
 				}
 				log_trace!(self.logger, "Received duplicative fulfill for HTLC with payment_preimage {}", log_bytes!(payment_preimage.0));
 			},
@@ -5687,20 +5684,13 @@ 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.
 		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

@@ -1237,9 +1237,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

@@ -111,8 +111,12 @@ 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: we generate this event as soon as one of the payment paths succeeds. In
+	/// the case of a buggy payee, this may be followed by a PaymentFailed or MPPFragmentFailed event
+	/// if subsequent path(s) fail. However, this case should be extremely rare.
 	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