Reinstate ChannelManager::send_payment_with_route API

Support more ergonomically sending payments to specific routes.

We removed the original version of this API because it was hard to work with,
but the concept of sending a payment to a specific route is still useful.
Previously, users were able to do this via manually matching the payment id in
their router, but that's cumbersome when we could just handle it internally.

Author: valentinewallace

fuzz/src/chanmon_consistency.rs

@@ -48,7 +48,7 @@ use lightning::ln::channel::FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
 use lightning::ln::channel_state::ChannelDetails;
 use lightning::ln::channelmanager::{
 	ChainParameters, ChannelManager, ChannelManagerReadArgs, PaymentId, RecentPaymentDetails,
-	RecipientOnionFields, Retry,
+	RecipientOnionFields,
 };
 use lightning::ln::functional_test_utils::*;
 use lightning::ln::inbound_payment::ExpandedKey;
@@ -82,7 +82,6 @@ use bitcoin::secp256k1::{self, Message, PublicKey, Scalar, Secp256k1, SecretKey}
 
 use lightning::io::Cursor;
 use std::cmp::{self, Ordering};
-use std::collections::VecDeque;
 use std::mem;
 use std::sync::atomic;
 use std::sync::{Arc, Mutex};
@@ -113,22 +112,14 @@ impl FeeEstimator for FuzzEstimator {
 	}
 }
 
-struct FuzzRouter {
-	pub next_routes: Mutex<VecDeque<Route>>,
-}
+struct FuzzRouter {}
 
 impl Router for FuzzRouter {
 	fn find_route(
 		&self, _payer: &PublicKey, _params: &RouteParameters,
 		_first_hops: Option<&[&ChannelDetails]>, _inflight_htlcs: InFlightHtlcs,
 	) -> Result<Route, msgs::LightningError> {
-		if let Some(route) = self.next_routes.lock().unwrap().pop_front() {
-			return Ok(route);
-		}
-		Err(msgs::LightningError {
-			err: String::from("Not implemented"),
-			action: msgs::ErrorAction::IgnoreError,
-		})
+		unreachable!()
 	}
 
 	fn create_blinded_payment_paths<T: secp256k1::Signing + secp256k1::Verification>(
@@ -518,7 +509,7 @@ fn send_payment(
 		PaymentParameters::from_node_id(source.get_our_node_id(), TEST_FINAL_CLTV),
 		amt,
 	);
-	source.router.next_routes.lock().unwrap().push_back(Route {
+	let route = Route {
 		paths: vec![Path {
 			hops: vec![RouteHop {
 				pubkey: dest.get_our_node_id(),
@@ -532,11 +523,10 @@ fn send_payment(
 			blinded_tail: None,
 		}],
 		route_params: Some(route_params.clone()),
-	});
+	};
 	let onion = RecipientOnionFields::secret_only(payment_secret);
 	let payment_id = PaymentId(payment_id);
-	let res =
-		source.send_payment(payment_hash, onion, payment_id, route_params, Retry::Attempts(0));
+	let res = source.send_payment_with_route(route, payment_hash, onion, payment_id);
 	match res {
 		Err(err) => {
 			panic!("Errored with {:?} on initial payment send", err);
@@ -592,7 +582,7 @@ fn send_hop_payment(
 		PaymentParameters::from_node_id(source.get_our_node_id(), TEST_FINAL_CLTV),
 		amt,
 	);
-	source.router.next_routes.lock().unwrap().push_back(Route {
+	let route = Route {
 		paths: vec![Path {
 			hops: vec![
 				RouteHop {
@@ -617,11 +607,10 @@ fn send_hop_payment(
 			blinded_tail: None,
 		}],
 		route_params: Some(route_params.clone()),
-	});
+	};
 	let onion = RecipientOnionFields::secret_only(payment_secret);
 	let payment_id = PaymentId(payment_id);
-	let res =
-		source.send_payment(payment_hash, onion, payment_id, route_params, Retry::Attempts(0));
+	let res = source.send_payment_with_route(route, payment_hash, onion, payment_id);
 	match res {
 		Err(err) => {
 			panic!("Errored with {:?} on initial payment send", err);
@@ -640,7 +629,7 @@ fn send_hop_payment(
 pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
 	let out = SearchingOutput::new(underlying_out);
 	let broadcast = Arc::new(TestBroadcaster {});
-	let router = FuzzRouter { next_routes: Mutex::new(VecDeque::new()) };
+	let router = FuzzRouter {};
 
 	macro_rules! make_node {
 		($node_id: expr, $fee_estimator: expr) => {{

lightning/src/chain/channelmonitor.rs

@@ -5096,7 +5096,7 @@ mod tests {
 	use crate::chain::chaininterface::LowerBoundedFeeEstimator;
 
 	use super::ChannelMonitorUpdateStep;
-	use crate::{check_added_monitors, check_spends, get_local_commitment_txn, get_monitor, get_route_and_payment_hash, unwrap_send_err};
+	use crate::{check_added_monitors, check_spends, get_local_commitment_txn, get_monitor, get_route_and_payment_hash};
 	use crate::chain::{BestBlock, Confirm};
 	use crate::chain::channelmonitor::{ChannelMonitor, WithChannelMonitor};
 	use crate::chain::package::{weight_offered_htlc, weight_received_htlc, weight_revoked_offered_htlc, weight_revoked_received_htlc, WEIGHT_REVOKED_OUTPUT};
@@ -5106,10 +5106,9 @@ mod tests {
 	use crate::types::payment::{PaymentPreimage, PaymentHash};
 	use crate::ln::channel_keys::{DelayedPaymentBasepoint, DelayedPaymentKey, HtlcBasepoint, RevocationBasepoint, RevocationKey};
 	use crate::ln::chan_utils::{self,HTLCOutputInCommitment, ChannelPublicKeys, ChannelTransactionParameters, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters};
-	use crate::ln::channelmanager::{PaymentSendFailure, PaymentId, RecipientOnionFields};
+	use crate::ln::channelmanager::{PaymentId, RecipientOnionFields};
 	use crate::ln::functional_test_utils::*;
 	use crate::ln::script::ShutdownScript;
-	use crate::util::errors::APIError;
 	use crate::util::test_utils::{TestLogger, TestBroadcaster, TestFeeEstimator};
 	use crate::util::ser::{ReadableArgs, Writeable};
 	use crate::util::logger::Logger;
@@ -5170,9 +5169,9 @@ mod tests {
 		// If the ChannelManager tries to update the channel, however, the ChainMonitor will pass
 		// the update through to the ChannelMonitor which will refuse it (as the channel is closed).
 		let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 100_000);
-		unwrap_send_err!(nodes[1].node.send_payment_with_route(route, payment_hash,
-				RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)
-			), false, APIError::MonitorUpdateInProgress, {});
+		nodes[1].node.send_payment_with_route(route, payment_hash,
+			RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)
+		).unwrap();
 		check_added_monitors!(nodes[1], 1);
 
 		// Build a new ChannelMonitorUpdate which contains both the failing commitment tx update

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -19,13 +19,12 @@ use crate::chain::channelmonitor::{ANTI_REORG_DELAY, ChannelMonitor};
 use crate::chain::transaction::OutPoint;
 use crate::chain::{ChannelMonitorUpdateStatus, Listen, Watch};
 use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose, ClosureReason, HTLCDestination};
-use crate::ln::channelmanager::{PaymentId, PaymentSendFailure, RAACommitmentOrder, RecipientOnionFields};
+use crate::ln::channelmanager::{PaymentId, RAACommitmentOrder, RecipientOnionFields};
 use crate::ln::channel::AnnouncementSigsState;
 use crate::ln::msgs;
 use crate::ln::types::ChannelId;
 use crate::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler};
 use crate::util::test_channel_signer::TestChannelSigner;
-use crate::util::errors::APIError;
 use crate::util::ser::{ReadableArgs, Writeable};
 use crate::util::test_utils::TestBroadcaster;
 
@@ -133,9 +132,9 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
 	chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
 
 	{
-		unwrap_send_err!(nodes[0].node.send_payment_with_route(route, payment_hash_1,
-				RecipientOnionFields::secret_only(payment_secret_1), PaymentId(payment_hash_1.0)
-			), false, APIError::MonitorUpdateInProgress, {});
+		nodes[0].node.send_payment_with_route(route, payment_hash_1,
+			RecipientOnionFields::secret_only(payment_secret_1), PaymentId(payment_hash_1.0)
+		).unwrap();
 		check_added_monitors!(nodes[0], 1);
 	}
 
@@ -190,9 +189,9 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
 	let (route, payment_hash_2, _, payment_secret_2) = get_route_and_payment_hash!(&nodes[0], nodes[1], 1000000);
 	{
 		chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
-		unwrap_send_err!(nodes[0].node.send_payment_with_route(route, payment_hash_2,
-				RecipientOnionFields::secret_only(payment_secret_2), PaymentId(payment_hash_2.0)
-			), false, APIError::MonitorUpdateInProgress, {});
+		nodes[0].node.send_payment_with_route(route, payment_hash_2,
+			RecipientOnionFields::secret_only(payment_secret_2), PaymentId(payment_hash_2.0)
+		).unwrap();
 		check_added_monitors!(nodes[0], 1);
 	}
 
@@ -257,9 +256,9 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 	let (route, payment_hash_2, payment_preimage_2, payment_secret_2) = get_route_and_payment_hash!(nodes[0], nodes[1], 1000000);
 	{
 		chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
-		unwrap_send_err!(nodes[0].node.send_payment_with_route(route, payment_hash_2,
-				RecipientOnionFields::secret_only(payment_secret_2), PaymentId(payment_hash_2.0)
-			), false, APIError::MonitorUpdateInProgress, {});
+		nodes[0].node.send_payment_with_route(route, payment_hash_2,
+			RecipientOnionFields::secret_only(payment_secret_2), PaymentId(payment_hash_2.0)
+		).unwrap();
 		check_added_monitors!(nodes[0], 1);
 	}
 
@@ -2004,16 +2003,10 @@ fn test_path_paused_mpp() {
 	chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
 	chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
 
-	// Now check that we get the right return value, indicating that the first path succeeded but
-	// the second got a MonitorUpdateInProgress err. This implies
-	// PaymentSendFailure::PartialFailure as some paths succeeded, preventing retry.
-	if let Err(PaymentSendFailure::PartialFailure { results, ..}) = nodes[0].node.send_payment_with_route(
+	// The first path should have succeeded with the second getting a MonitorUpdateInProgress err.
+	nodes[0].node.send_payment_with_route(
 		route, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)
-	) {
-		assert_eq!(results.len(), 2);
-		if let Ok(()) = results[0] {} else { panic!(); }
-		if let Err(APIError::MonitorUpdateInProgress) = results[1] {} else { panic!(); }
-	} else { panic!(); }
+	).unwrap();
 	check_added_monitors!(nodes[0], 2);
 	chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
 

lightning/src/ln/channelmanager.rs

@@ -55,9 +55,7 @@ use crate::ln::channel_state::ChannelDetails;
 use crate::types::features::{Bolt12InvoiceFeatures, ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures};
 #[cfg(any(feature = "_test_utils", test))]
 use crate::types::features::Bolt11InvoiceFeatures;
-use crate::routing::router::{BlindedTail, InFlightHtlcs, Path, Payee, PaymentParameters, RouteParameters, Router};
-#[cfg(test)]
-use crate::routing::router::Route;
+use crate::routing::router::{BlindedTail, FixedRouter, InFlightHtlcs, Path, Payee, PaymentParameters, Route, RouteParameters, Router};
 use crate::ln::onion_payment::{check_incoming_htlc_cltv, create_recv_pending_htlc_info, create_fwd_pending_htlc_info, decode_incoming_update_add_htlc_onion, InboundHTLCErr, NextPacketDetails};
 use crate::ln::msgs;
 use crate::ln::onion_utils;
@@ -2398,9 +2396,6 @@ where
 	fee_estimator: LowerBoundedFeeEstimator<F>,
 	chain_monitor: M,
 	tx_broadcaster: T,
-	#[cfg(fuzzing)]
-	pub router: R,
-	#[cfg(not(fuzzing))]
 	router: R,
 	message_router: MR,
 
@@ -4583,21 +4578,31 @@ where
 		}
 	}
 
-	// Deprecated send method, for testing use [`Self::send_payment`] and
-	// [`TestRouter::expect_find_route`] instead.
-	//
-	// [`TestRouter::expect_find_route`]: crate::util::test_utils::TestRouter::expect_find_route
-	#[cfg(test)]
-	pub(crate) fn send_payment_with_route(
-		&self, route: Route, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields,
+	/// Sends a payment along a given route. See [`Self::send_payment`] for more info.
+	///
+	/// LDK will not automatically retry this payment, though it may be manually re-sent after an
+	/// [`Event::PaymentFailed`] is generated.
+	pub fn send_payment_with_route(
+		&self, mut route: Route, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields,
 		payment_id: PaymentId
-	) -> Result<(), PaymentSendFailure> {
+	) -> Result<(), RetryableSendFailure> {
 		let best_block_height = self.best_block.read().unwrap().height;
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
+		let route_params = route.route_params.clone().unwrap_or_else(|| {
+			// Create a dummy route params since they're a required parameter but unused in this case
+			let (payee_node_id, cltv_delta) = route.paths.first()
+				.and_then(|path| path.hops.last().map(|hop| (hop.pubkey, hop.cltv_expiry_delta as u32)))
+				.unwrap_or_else(|| (PublicKey::from_slice(&[2; 32]).unwrap(), MIN_FINAL_CLTV_EXPIRY_DELTA as u32));
+			let dummy_payment_params = PaymentParameters::from_node_id(payee_node_id, cltv_delta);
+			RouteParameters::from_payment_params_and_value(dummy_payment_params, route.get_total_amount())
+		});
+		if route.route_params.is_none() { route.route_params = Some(route_params.clone()); }
+		let router = FixedRouter::new(route);
 		self.pending_outbound_payments
-			.send_payment_with_route(&route, payment_hash, recipient_onion, payment_id,
-				&self.entropy_source, &self.node_signer, best_block_height,
-				|args| self.send_payment_along_path(args))
+			.send_payment(payment_hash, recipient_onion, payment_id, Retry::Attempts(0),
+				route_params, &&router, self.list_usable_channels(), || self.compute_inflight_htlcs(),
+				&self.entropy_source, &self.node_signer, best_block_height, &self.logger,
+				&self.pending_events, |args| self.send_payment_along_path(args))
 	}
 
 	/// Sends a payment to the route found using the provided [`RouteParameters`], retrying failed
@@ -4626,7 +4631,8 @@ where
 	/// [`ChannelManager::list_recent_payments`] for more information.
 	///
 	/// Routes are automatically found using the [`Router] provided on startup. To fix a route for a
-	/// particular payment, match the [`PaymentId`] passed to [`Router::find_route_with_id`].
+	/// particular payment, use [`Self::send_payment_with_route`] or match the [`PaymentId`] passed to
+	/// [`Router::find_route_with_id`].
 	///
 	/// [`Event::PaymentSent`]: events::Event::PaymentSent
 	/// [`Event::PaymentFailed`]: events::Event::PaymentFailed
@@ -14391,7 +14397,7 @@ mod tests {
 	use crate::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
 	use crate::ln::types::ChannelId;
 	use crate::types::payment::{PaymentPreimage, PaymentHash, PaymentSecret};
-	use crate::ln::channelmanager::{create_recv_pending_htlc_info, HTLCForwardInfo, inbound_payment, PaymentId, PaymentSendFailure, RecipientOnionFields, InterceptId};
+	use crate::ln::channelmanager::{create_recv_pending_htlc_info, HTLCForwardInfo, inbound_payment, PaymentId, RecipientOnionFields, InterceptId};
 	use crate::ln::functional_test_utils::*;
 	use crate::ln::msgs::{self, ErrorAction};
 	use crate::ln::msgs::ChannelMessageHandler;
@@ -14822,14 +14828,17 @@ mod tests {
 		route.paths[1].hops[0].short_channel_id = chan_2_id;
 		route.paths[1].hops[1].short_channel_id = chan_4_id;
 
-		match nodes[0].node.send_payment_with_route(route, payment_hash,
-			RecipientOnionFields::spontaneous_empty(), PaymentId(payment_hash.0))
-		.unwrap_err() {
-			PaymentSendFailure::ParameterError(APIError::APIMisuseError { ref err }) => {
-				assert!(regex::Regex::new(r"Payment secret is required for multi-path payments").unwrap().is_match(err))
-			},
-			_ => panic!("unexpected error")
+		nodes[0].node.send_payment_with_route(route, payment_hash,
+			RecipientOnionFields::spontaneous_empty(), PaymentId(payment_hash.0)).unwrap();
+		let events = nodes[0].node.get_and_clear_pending_events();
+		assert_eq!(events.len(), 1);
+		match events[0] {
+			Event::PaymentFailed { reason, .. } => {
+				assert_eq!(reason.unwrap(), crate::events::PaymentFailureReason::UnexpectedError);
+			}
+			_ => panic!()
 		}
+		nodes[0].logger.assert_log_contains("lightning::ln::outbound_payment", "Payment secret is required for multi-path payments", 2);
 		assert!(nodes[0].node.list_recent_payments().is_empty());
 	}
 

lightning/src/ln/functional_test_utils.rs

@@ -1069,30 +1069,27 @@ macro_rules! get_local_commitment_txn {
 /// Check the error from attempting a payment.
 #[macro_export]
 macro_rules! unwrap_send_err {
-	($res: expr, $all_failed: expr, $type: pat, $check: expr) => {
-		match &$res {
-			&Err(PaymentSendFailure::AllFailedResendSafe(ref fails)) if $all_failed => {
-				assert_eq!(fails.len(), 1);
-				match fails[0] {
-					$type => { $check },
-					_ => panic!(),
-				}
-			},
-			&Err(PaymentSendFailure::PartialFailure { ref results, .. }) if !$all_failed => {
-				assert_eq!(results.len(), 1);
-				match results[0] {
-					Err($type) => { $check },
-					_ => panic!(),
-				}
-			},
-			&Err(PaymentSendFailure::PathParameterError(ref result)) if !$all_failed => {
-				assert_eq!(result.len(), 1);
-				match result[0] {
-					Err($type) => { $check },
-					_ => panic!(),
+	($node: expr, $res: expr, $all_failed: expr, $type: pat, $check: expr) => {
+		assert!($res.is_ok());
+		let events = $node.node.get_and_clear_pending_events();
+		assert!(events.len() == 2);
+		match &events[0] {
+			crate::events::Event::PaymentPathFailed { failure, .. } => {
+				match failure {
+					crate::events::PathFailure::InitialSend { err } => {
+						match err {
+							$type => { $check },
+							_ => panic!()
+						}
+					},
+					_ => panic!()
 				}
 			},
-			_ => {panic!()},
+			_ => panic!()
+		}
+		match &events[1] {
+			crate::events::Event::PaymentFailed { .. } => {},
+			_ => panic!()
 		}
 	}
 }