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,18 +112,13 @@ 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,
@@ -518,7 +512,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 +526,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 +585,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 +610,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 +632,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

@@ -5092,7 +5092,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};
@@ -5102,10 +5102,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;
@@ -5166,9 +5165,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::{RAACommitmentOrder, PaymentSendFailure, PaymentId, RecipientOnionFields};
+use crate::ln::channelmanager::{RAACommitmentOrder, PaymentId, 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, CachingRouter, 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;
@@ -2390,10 +2388,7 @@ where
 	fee_estimator: LowerBoundedFeeEstimator<F>,
 	chain_monitor: M,
 	tx_broadcaster: T,
-	#[cfg(fuzzing)]
-	pub router: R,
-	#[cfg(not(fuzzing))]
-	router: R,
+	router: CachingRouter<R>,
 	message_router: MR,
 
 	/// See `ChannelManager` struct-level documentation for lock order requirements.
@@ -3503,7 +3498,7 @@ where
 			fee_estimator: LowerBoundedFeeEstimator::new(fee_est),
 			chain_monitor,
 			tx_broadcaster,
-			router,
+			router: CachingRouter::new(router),
 			message_router,
 
 			best_block: RwLock::new(params.best_block),
@@ -4606,21 +4601,21 @@ 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(
+	/// Sends a payment along a given route. See [`Self::send_payment`] for more info.
+	/// [`Route::route_params`] is required to be `Some`.
+	pub fn send_payment_with_route(
 		&self, 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().ok_or(RetryableSendFailure::RouteNotFound)?;
+		self.router.route_cache.lock().unwrap().insert(payment_id, 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, &self.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
@@ -4649,7 +4644,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
@@ -4664,7 +4660,7 @@ where
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
 		self.pending_outbound_payments
 			.send_payment(payment_hash, recipient_onion, payment_id, retry_strategy, route_params,
-				&*self.router, self.list_usable_channels(), || self.compute_inflight_htlcs(),
+				&self.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))
 	}
@@ -4741,7 +4737,7 @@ where
 		let features = self.bolt12_invoice_features();
 		self.pending_outbound_payments
 			.send_payment_for_bolt12_invoice(
-				invoice, payment_id, &*self.router, self.list_usable_channels(), features,
+				invoice, payment_id, &self.router, self.list_usable_channels(), features,
 				|| self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, &self,
 				&self.secp_ctx, best_block_height, &self.logger, &self.pending_events,
 				|args| self.send_payment_along_path(args)
@@ -4816,7 +4812,7 @@ where
 		let mut res = Ok(());
 		PersistenceNotifierGuard::optionally_notify(self, || {
 			let outbound_pmts_res = self.pending_outbound_payments.send_payment_for_static_invoice(
-				payment_id, &*self.router, self.list_usable_channels(), || self.compute_inflight_htlcs(),
+				payment_id, &self.router, self.list_usable_channels(), || self.compute_inflight_htlcs(),
 				&self.entropy_source, &self.node_signer, &self, &self.secp_ctx, best_block_height,
 				&self.logger, &self.pending_events, |args| self.send_payment_along_path(args)
 			);
@@ -4892,7 +4888,7 @@ where
 		let best_block_height = self.best_block.read().unwrap().height;
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
 		self.pending_outbound_payments.send_spontaneous_payment(payment_preimage, recipient_onion,
-			payment_id, retry_strategy, route_params, &*self.router, self.list_usable_channels(),
+			payment_id, retry_strategy, route_params, &self.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))
 	}
@@ -6266,7 +6262,7 @@ where
 		}
 
 		let best_block_height = self.best_block.read().unwrap().height;
-		self.pending_outbound_payments.check_retry_payments(&*self.router, || self.list_usable_channels(),
+		self.pending_outbound_payments.check_retry_payments(&self.router, || self.list_usable_channels(),
 			|| self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height,
 			&self.pending_events, &self.logger, |args| self.send_payment_along_path(args));
 
@@ -14084,7 +14080,7 @@ where
 			fee_estimator: bounded_fee_estimator,
 			chain_monitor: args.chain_monitor,
 			tx_broadcaster: args.tx_broadcaster,
-			router: args.router,
+			router: CachingRouter::new(args.router),
 			message_router: args.message_router,
 
 			best_block: RwLock::new(BestBlock::new(best_block_hash, best_block_height)),
@@ -14331,7 +14327,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;
@@ -14757,14 +14753,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.pending_outbound_payments.pending_outbound_payments.lock().unwrap().is_empty());
 	}