Reuse the PaymentHash as PaymentId for new outbound payments

In c986e52, an `MppId` was added
to `HTLCSource` objects as a way of correlating HTLCs which belong
to the same payment when the `ChannelManager` sees an HTLC
succeed/fail. This allows it to have awareness of the state of all
HTLCs in a payment when it generates the ultimate user-facing
payment success/failure events. This was used in the same PR to
avoid generating duplicative success/failure events for a single
payment.

Because the field was only used as an internal token to correlate
HTLCs, and retries were not supported, it was generated randomly by
calling the `KeysInterface`'s 32-byte random-fetching function.
This also provided a backwards-compatibility story as the existing
HTLC randomization key was re-used for older clients.

In 28eea12 `MppId` was renamed to
the current `PaymentId` which was then used expose the
`retry_payment` interface, allowing users to send new HTLCs which
are considered a part of an existing payment.

At no point has the payment-sending API seriously considered
idempotency, a major drawback which leaves the API unsafe in most
deployments. Luckily, there is a simple solution - because the
`PaymentId` must be unique, and because payment information for a
given payment is held for several blocks after a payment
completes/fails, it represents an obvious idempotency token.

Here we simply reuse the `PaymentHash` as the `PaymentId` in
`send_payment`, ensuring idempotency of payments with a given hash.

Author: TheBlueMatt

lightning-invoice/src/utils.rs

@@ -526,7 +526,7 @@ where
 		&self, route: &Route, payment_preimage: PaymentPreimage,
 	) -> Result<PaymentId, PaymentSendFailure> {
 		self.send_spontaneous_payment(route, Some(payment_preimage))
-			.map(|(_, payment_id)| payment_id)
+			.map(|payment_hash| PaymentId(payment_hash.0))
 	}
 
 	fn retry_payment(

lightning/src/ln/channelmanager.rs

@@ -2454,35 +2454,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			let mut channel_lock = self.channel_state.lock().unwrap();
 
 			let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
-			let payment_entry = pending_outbounds.entry(payment_id);
-			if let hash_map::Entry::Occupied(payment) = &payment_entry {
-				if !payment.get().is_retryable() {
+			let payment = if let Some(payment) = pending_outbounds.get_mut(&payment_id) {
+				if !payment.is_retryable() {
 					return Err(APIError::RouteError {
 						err: "Payment already completed"
 					});
 				}
-			}
+				payment
+			} else {
+				debug_assert!(false, "This shouldn't happen generally as callsites check this, but technically its race-y and could");
+				return Err(APIError::RouteError { err: "Payment was completed or failed while we were processing it" });
+			};
 
 			let id = match channel_lock.short_to_chan_info.get(&path.first().unwrap().short_channel_id) {
 				None => return Err(APIError::ChannelUnavailable{err: "No channel available with first hop!".to_owned()}),
 				Some((_cp_id, chan_id)) => chan_id.clone(),
 			};
 
-			macro_rules! insert_outbound_payment {
-				() => {
-					let payment = payment_entry.or_insert_with(|| PendingOutboundPayment::Retryable {
-						session_privs: HashSet::new(),
-						pending_amt_msat: 0,
-						pending_fee_msat: Some(0),
-						payment_hash: *payment_hash,
-						payment_secret: *payment_secret,
-						starting_block_height: self.best_block.read().unwrap().height(),
-						total_msat: total_value,
-					});
-					assert!(payment.insert(session_priv_bytes, path));
-				}
-			}
-
 			let channel_state = &mut *channel_lock;
 			if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(id) {
 				match {
@@ -2512,7 +2500,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						{
 							(ChannelMonitorUpdateStatus::PermanentFailure, Err(e)) => break Err(e),
 							(ChannelMonitorUpdateStatus::Completed, Ok(())) => {
-								insert_outbound_payment!();
+								assert!(payment.insert(session_priv_bytes, path));
 							},
 							(ChannelMonitorUpdateStatus::InProgress, Err(_)) => {
 								// Note that MonitorUpdateInProgress here indicates (per function
@@ -2521,7 +2509,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 								// indicating that it is unsafe to retry the payment wholesale,
 								// which we do in the send_payment check for
 								// MonitorUpdateInProgress, below.
-								insert_outbound_payment!(); // Only do this after possibly break'ing on Perm failure above.
+								assert!(payment.insert(session_priv_bytes, path)); // Only do this after possibly break'ing on Perm failure above.
 								return Err(APIError::MonitorUpdateInProgress);
 							},
 							_ => unreachable!(),
@@ -2540,7 +2528,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 							},
 						});
 					},
-					None => { insert_outbound_payment!(); },
+					None => { assert!(payment.insert(session_priv_bytes, path)); },
 				}
 			} else { unreachable!(); }
 			return Ok(());
@@ -2559,12 +2547,18 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// Value parameters are provided via the last hop in route, see documentation for RouteHop
 	/// fields for more info.
 	///
-	/// Note that if the payment_hash already exists elsewhere (eg you're sending a duplicative
-	/// payment), we don't do anything to stop you! We always try to ensure that if the provided
-	/// next hop knows the preimage to payment_hash they can claim an additional amount as
-	/// specified in the last hop in the route! Thus, you should probably do your own
-	/// payment_preimage tracking (which you should already be doing as they represent "proof of
-	/// payment") and prevent double-sends yourself.
+	/// If a pending payment is currently in-flight with the same [`PaymentHash`] provided, this
+	/// method will error with an [`APIError::RouteError`]. Note, however, that once a payment
+	/// is no longer pending (either via [`ChannelManager::abandon_payment`], or handling of an
+	/// [`Event::PaymentSent`]) a second payment with the same [`PaymentHash`] can be sent.
+	///
+	/// Thus, in order to ensure that duplicate payments are not sent, you should likely implement
+	/// your own [`PaymentHash`]-based tracking of payments, including storing received
+	/// [`PaymentPreimage`]s as proof of payment.
+	///
+	/// For backwards-compatibility, this method returns a [`PaymentId`] which uniquely represents
+	/// this payment and is used in [`ChannelManager::abandon_payment`] and
+	/// [`ChannelManager::retry_payment`], however the vaule is always the [`PaymentHash`].
 	///
 	/// May generate SendHTLCs message(s) event on success, which should be relayed.
 	///
@@ -2590,14 +2584,50 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// newer nodes, it will be provided to you in the invoice. If you do not have one, the Route
 	/// must not contain multiple paths as multi-path payments require a recipient-provided
 	/// payment_secret.
+	///
 	/// If a payment_secret *is* provided, we assume that the invoice had the payment_secret feature
 	/// bit set (either as required or as available). If multiple paths are present in the Route,
 	/// we assume the invoice had the basic_mpp feature set.
+	///
+	/// [`Event::PaymentSent`]: events::Event::PaymentSent
 	pub fn send_payment(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>) -> Result<PaymentId, PaymentSendFailure> {
-		self.send_payment_internal(route, payment_hash, payment_secret, None, None, None)
+		let total_value_msat = route.paths.iter().map(|path| path.last().map(|h| h.fee_msat).unwrap_or(0)).sum();
+		let payment_id = PaymentId(payment_hash.0);
+		self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, total_value_msat)?;
+		self.send_payment_internal(route, payment_hash, payment_secret, None, payment_id, None)?;
+		Ok(payment_id)
+	}
+
+	#[cfg(any(test, feature = "_test_utils"))]
+	pub fn send_payment_with_id(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, payment_id: PaymentId) -> Result<(), PaymentSendFailure> {
+		let total_value_msat = route.paths.iter().map(|path| path.last().map(|h| h.fee_msat).unwrap_or(0)).sum();
+		self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, total_value_msat)?;
+		self.send_payment_internal(route, payment_hash, payment_secret, None, payment_id, None)
 	}
 
-	fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>, payment_id: Option<PaymentId>, recv_value_msat: Option<u64>) -> Result<PaymentId, PaymentSendFailure> {
+	// Only public for testing, this should otherwise never be called direcly
+	pub(crate) fn add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, total_msat: u64) -> Result<(), PaymentSendFailure> {
+		let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
+		match pending_outbounds.entry(payment_id) {
+			hash_map::Entry::Occupied(_) => Err(PaymentSendFailure::ParameterError(APIError::RouteError {
+				err: "Payment already in progress"
+			})),
+			hash_map::Entry::Vacant(entry) => {
+				entry.insert(PendingOutboundPayment::Retryable {
+					session_privs: HashSet::new(),
+					pending_amt_msat: 0,
+					pending_fee_msat: Some(0),
+					payment_hash,
+					payment_secret,
+					starting_block_height: self.best_block.read().unwrap().height(),
+					total_msat,
+				});
+				Ok(())
+			},
+		}
+	}
+
+	fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>, payment_id: PaymentId, recv_value_msat: Option<u64>) -> Result<(), PaymentSendFailure> {
 		if route.paths.len() < 1 {
 			return Err(PaymentSendFailure::ParameterError(APIError::RouteError{err: "There must be at least one path to send over"}));
 		}
@@ -2607,7 +2637,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		let mut total_value = 0;
 		let our_node_id = self.get_our_node_id();
 		let mut path_errs = Vec::with_capacity(route.paths.len());
-		let payment_id = if let Some(id) = payment_id { id } else { PaymentId(self.keys_manager.get_secure_random_bytes()) };
 		'path_check: for path in route.paths.iter() {
 			if path.len() < 1 || path.len() > 20 {
 				path_errs.push(Err(APIError::RouteError{err: "Path didn't go anywhere/had bogus size"}));
@@ -2667,12 +2696,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				} else { None },
 			})
 		} else if has_err {
-			// If we failed to send any paths, we shouldn't have inserted the new PaymentId into
-			// our `pending_outbound_payments` map at all.
-			debug_assert!(self.pending_outbound_payments.lock().unwrap().get(&payment_id).is_none());
+			// If we failed to send any paths, we should remove the new PaymentId from the
+			// `pending_outbound_payments` map, as the user isn't expected to `abandon_payment`.
+			let removed = self.pending_outbound_payments.lock().unwrap().remove(&payment_id).is_some();
+			debug_assert!(removed, "We should always have a pending payment to remove here");
 			Err(PaymentSendFailure::AllFailedRetrySafe(results.drain(..).map(|r| r.unwrap_err()).collect()))
 		} else {
-			Ok(payment_id)
+			Ok(())
 		}
 	}
 
@@ -2733,7 +2763,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				}))
 			}
 		};
-		return self.send_payment_internal(route, payment_hash, &payment_secret, None, Some(payment_id), Some(total_msat)).map(|_| ())
+		self.send_payment_internal(route, payment_hash, &payment_secret, None, payment_id, Some(total_msat))
 	}
 
 	/// Signals that no further retries for the given payment will occur.
@@ -2773,22 +2803,30 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// would be able to guess -- otherwise, an intermediate node may claim the payment and it will
 	/// never reach the recipient.
 	///
-	/// See [`send_payment`] documentation for more details on the return value of this function.
+	/// See [`send_payment`] documentation for more details on the return value of this function
+	/// and idempotency guarantees provided.
+	///
+	/// Note that the [`PaymentId`] for the generated payment will be
+	/// `PaymentId(PaymentHashReturned.0)`.
 	///
 	/// Similar to regular payments, you MUST NOT reuse a `payment_preimage` value. See
 	/// [`send_payment`] for more information about the risks of duplicate preimage usage.
 	///
 	/// Note that `route` must have exactly one path.
 	///
 	/// [`send_payment`]: Self::send_payment
-	pub fn send_spontaneous_payment(&self, route: &Route, payment_preimage: Option<PaymentPreimage>) -> Result<(PaymentHash, PaymentId), PaymentSendFailure> {
+	pub fn send_spontaneous_payment(&self, route: &Route, payment_preimage: Option<PaymentPreimage>) -> Result<PaymentHash, PaymentSendFailure> {
 		let preimage = match payment_preimage {
 			Some(p) => p,
 			None => PaymentPreimage(self.keys_manager.get_secure_random_bytes()),
 		};
 		let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner());
-		match self.send_payment_internal(route, payment_hash, &None, Some(preimage), None, None) {
-			Ok(payment_id) => Ok((payment_hash, payment_id)),
+		let total_value_msat = route.paths.iter().map(|path| path.last().map(|h| h.fee_msat).unwrap_or(0)).sum();
+		let payment_id = PaymentId(payment_hash.0);
+		self.add_new_pending_payment(payment_hash, None, payment_id, total_value_msat)?;
+
+		match self.send_payment_internal(route, payment_hash, &None, Some(preimage), payment_id, None) {
+			Ok(()) => Ok(payment_hash),
 			Err(e) => Err(e)
 		}
 	}
@@ -2798,7 +2836,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// us to easily discern them from real payments.
 	pub fn send_probe(&self, hops: Vec<RouteHop>) -> Result<(PaymentHash, PaymentId), PaymentSendFailure> {
 		let payment_id = PaymentId(self.keys_manager.get_secure_random_bytes());
-
 		let payment_hash = self.probing_cookie_from_id(&payment_id);
 
 		if hops.len() < 2 {
@@ -2809,8 +2846,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 
 		let route = Route { paths: vec![hops], payment_params: None };
 
-		match self.send_payment_internal(&route, payment_hash, &None, None, Some(payment_id), None) {
-			Ok(payment_id) => Ok((payment_hash, payment_id)),
+		let total_value_msat = route.paths.iter().map(|path| path.last().map(|h| h.fee_msat).unwrap_or(0)).sum();
+		self.add_new_pending_payment(payment_hash, None, payment_id, total_value_msat)?;
+
+		match self.send_payment_internal(&route, payment_hash, &None, None, payment_id, None) {
+			Ok(()) => Ok((payment_hash, payment_id)),
 			Err(e) => Err(e)
 		}
 	}
@@ -7400,6 +7440,7 @@ mod tests {
 		// Use the utility function send_payment_along_path to send the payment with MPP data which
 		// indicates there are more HTLCs coming.
 		let cur_height = CHAN_CONFIRM_DEPTH + 1; // route_payment calls send_payment, which adds 1 to the current height. So we do the same here to match.
+		nodes[0].node.add_new_pending_payment(our_payment_hash, Some(payment_secret), payment_id, 200_000).unwrap();
 		nodes[0].node.send_payment_along_path(&route.paths[0], &route.payment_params, &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
 		check_added_monitors!(nodes[0], 1);
 		let mut events = nodes[0].node.get_and_clear_pending_msg_events();
@@ -7561,7 +7602,7 @@ mod tests {
 			&nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph,
 			None, nodes[0].logger, &scorer, &random_seed_bytes
 		).unwrap();
-		let (payment_hash, _) = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage)).unwrap();
+		let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage)).unwrap();
 		check_added_monitors!(nodes[0], 1);
 		let mut events = nodes[0].node.get_and_clear_pending_msg_events();
 		assert_eq!(events.len(), 1);
@@ -7571,7 +7612,7 @@ mod tests {
 
 		// Next, attempt a regular payment and make sure it fails.
 		let payment_secret = PaymentSecret([43; 32]);
-		nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
+		nodes[0].node.send_payment_with_id(&route, payment_hash, &Some(payment_secret), PaymentId(payment_secret.0)).unwrap();
 		check_added_monitors!(nodes[0], 1);
 		let mut events = nodes[0].node.get_and_clear_pending_msg_events();
 		assert_eq!(events.len(), 1);
@@ -7614,7 +7655,7 @@ mod tests {
 		let _chan = create_chan_between_nodes(&nodes[0], &nodes[1], channelmanager::provided_init_features(), channelmanager::provided_init_features());
 		let route_params = RouteParameters {
 			payment_params: PaymentParameters::for_keysend(payee_pubkey),
-			final_value_msat: 10000,
+			final_value_msat: 10_000,
 			final_cltv_expiry_delta: 40,
 		};
 		let network_graph = nodes[0].network_graph;
@@ -7628,7 +7669,8 @@ mod tests {
 
 		let test_preimage = PaymentPreimage([42; 32]);
 		let mismatch_payment_hash = PaymentHash([43; 32]);
-		let _ = nodes[0].node.send_payment_internal(&route, mismatch_payment_hash, &None, Some(test_preimage), None, None).unwrap();
+		nodes[0].node.add_new_pending_payment(mismatch_payment_hash, None, PaymentId(mismatch_payment_hash.0), 10_000).unwrap();
+		nodes[0].node.send_payment_internal(&route, mismatch_payment_hash, &None, Some(test_preimage), PaymentId(mismatch_payment_hash.0), None).unwrap();
 		check_added_monitors!(nodes[0], 1);
 
 		let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
@@ -7658,7 +7700,7 @@ mod tests {
 		let _chan = create_chan_between_nodes(&nodes[0], &nodes[1], channelmanager::provided_init_features(), channelmanager::provided_init_features());
 		let route_params = RouteParameters {
 			payment_params: PaymentParameters::for_keysend(payee_pubkey),
-			final_value_msat: 10000,
+			final_value_msat: 10_000,
 			final_cltv_expiry_delta: 40,
 		};
 		let network_graph = nodes[0].network_graph;
@@ -7673,7 +7715,8 @@ mod tests {
 		let test_preimage = PaymentPreimage([42; 32]);
 		let test_secret = PaymentSecret([43; 32]);
 		let payment_hash = PaymentHash(Sha256::hash(&test_preimage.0).into_inner());
-		let _ = nodes[0].node.send_payment_internal(&route, payment_hash, &Some(test_secret), Some(test_preimage), None, None).unwrap();
+		nodes[0].node.add_new_pending_payment(payment_hash, Some(test_secret), PaymentId(payment_hash.0), 10_000).unwrap();
+		nodes[0].node.send_payment_internal(&route, payment_hash, &Some(test_secret), Some(test_preimage), PaymentId(payment_hash.0), None).unwrap();
 		check_added_monitors!(nodes[0], 1);
 
 		let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
@@ -7865,7 +7908,7 @@ pub mod bench {
 	use chain::Listen;
 	use chain::chainmonitor::{ChainMonitor, Persist};
 	use chain::keysinterface::{KeysManager, KeysInterface, InMemorySigner};
-	use ln::channelmanager::{self, BestBlock, ChainParameters, ChannelManager, PaymentHash, PaymentPreimage};
+	use ln::channelmanager::{self, BestBlock, ChainParameters, ChannelManager, PaymentHash, PaymentPreimage, PaymentId};
 	use ln::features::{InitFeatures, InvoiceFeatures};
 	use ln::functional_test_utils::*;
 	use ln::msgs::{ChannelMessageHandler, Init};

lightning/src/ln/functional_test_utils.rs

@@ -1636,7 +1636,8 @@ pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>(
 }
 
 pub fn send_along_route_with_secret<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route, expected_paths: &[&[&Node<'a, 'b, 'c>]], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: PaymentSecret) -> PaymentId {
-	let payment_id = origin_node.node.send_payment(&route, our_payment_hash, &Some(our_payment_secret)).unwrap();
+	let payment_id = PaymentId(origin_node.keys_manager.backing.get_secure_random_bytes());
+	origin_node.node.send_payment_with_id(&route, our_payment_hash, &Some(our_payment_secret), payment_id).unwrap();
 	check_added_monitors!(origin_node, expected_paths.len());
 	pass_along_route(origin_node, expected_paths, recv_value, our_payment_hash, our_payment_secret);
 	payment_id