Merge pull request #3534 from valentinewallace/2025-01-send-with-route

Reinstate `ChannelManager::send_payment_with_route` API

Author: TheBlueMatt

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,
 
@@ -4586,21 +4581,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
@@ -4629,7 +4634,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
@@ -14358,7 +14364,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;
@@ -14789,14 +14795,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!()
 		}
 	}
 }