Merge pull request #1893 from valentinewallace/2022-12-jit-forwards-followup

HTLC JIT channel interception followup + minor cleanups

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -253,7 +253,7 @@ fn check_api_err(api_err: APIError) {
 	match api_err {
 		APIError::APIMisuseError { .. } => panic!("We can't misuse the API"),
 		APIError::FeeRateTooHigh { .. } => panic!("We can't send too much fee?"),
-		APIError::RouteError { .. } => panic!("Our routes should work"),
+		APIError::InvalidRoute { .. } => panic!("Our routes should work"),
 		APIError::ChannelUnavailable { err } => {
 			// Test the error against a list of errors we can hit, and reject
 			// all others. If you hit this panic, the list of acceptable errors

lightning/src/ln/channelmanager.rs

@@ -2419,10 +2419,10 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		let session_priv = SecretKey::from_slice(&session_priv_bytes[..]).expect("RNG is busted");
 
 		let onion_keys = onion_utils::construct_onion_keys(&self.secp_ctx, &path, &session_priv)
-			.map_err(|_| APIError::RouteError{err: "Pubkey along hop was maliciously selected"})?;
+			.map_err(|_| APIError::InvalidRoute{err: "Pubkey along hop was maliciously selected"})?;
 		let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(path, total_value, payment_secret, cur_height, keysend_preimage)?;
 		if onion_utils::route_size_insane(&onion_payloads) {
-			return Err(APIError::RouteError{err: "Route size too large considering onion data"});
+			return Err(APIError::InvalidRoute{err: "Route size too large considering onion data"});
 		}
 		let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, payment_hash);
 
@@ -2439,7 +2439,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 			if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(id) {
 				match {
 					if chan.get().get_counterparty_node_id() != path.first().unwrap().pubkey {
-						return Err(APIError::RouteError{err: "Node ID mismatch on first hop!"});
+						return Err(APIError::InvalidRoute{err: "Node ID mismatch on first hop!"});
 					}
 					if !chan.get().is_live() {
 						return Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected/pending monitor update!".to_owned()});
@@ -2514,7 +2514,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	/// fields for more info.
 	///
 	/// If a pending payment is currently in-flight with the same [`PaymentId`] provided, this
-	/// method will error with an [`APIError::RouteError`]. Note, however, that once a payment
+	/// method will error with an [`APIError::InvalidRoute`]. Note, however, that once a payment
 	/// is no longer pending (either via [`ChannelManager::abandon_payment`], or handling of an
 	/// [`Event::PaymentSent`]) LDK will not stop you from sending a second payment with the same
 	/// [`PaymentId`].
@@ -2533,7 +2533,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	/// PaymentSendFailure for more info.
 	///
 	/// In general, a path may raise:
-	///  * [`APIError::RouteError`] when an invalid route or forwarding parameter (cltv_delta, fee,
+	///  * [`APIError::InvalidRoute`] when an invalid route or forwarding parameter (cltv_delta, fee,
 	///    node public key) is specified.
 	///  * [`APIError::ChannelUnavailable`] if the next-hop channel is not available for updates
 	///    (including due to previous monitor update failure or new permanent monitor update
@@ -2598,7 +2598,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
 	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>, onion_session_privs: Vec<[u8; 32]>) -> Result<(), PaymentSendFailure> {
 		if route.paths.len() < 1 {
-			return Err(PaymentSendFailure::ParameterError(APIError::RouteError{err: "There must be at least one path to send over"}));
+			return Err(PaymentSendFailure::ParameterError(APIError::InvalidRoute{err: "There must be at least one path to send over"}));
 		}
 		if payment_secret.is_none() && route.paths.len() > 1 {
 			return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError{err: "Payment secret is required for multi-path payments".to_string()}));
@@ -2608,12 +2608,12 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		let mut path_errs = Vec::with_capacity(route.paths.len());
 		'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"}));
+				path_errs.push(Err(APIError::InvalidRoute{err: "Path didn't go anywhere/had bogus size"}));
 				continue 'path_check;
 			}
 			for (idx, hop) in path.iter().enumerate() {
 				if idx != path.len() - 1 && hop.pubkey == our_node_id {
-					path_errs.push(Err(APIError::RouteError{err: "Path went through us but wasn't a simple rebalance loop to us"}));
+					path_errs.push(Err(APIError::InvalidRoute{err: "Path went through us but wasn't a simple rebalance loop to us"}));
 					continue 'path_check;
 				}
 			}
@@ -3092,20 +3092,20 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		let next_hop_scid = match self.channel_state.lock().unwrap().by_id.get(next_hop_channel_id) {
 			Some(chan) => {
 				if !chan.is_usable() {
-					return Err(APIError::APIMisuseError {
-						err: format!("Channel with id {:?} not fully established", next_hop_channel_id)
+					return Err(APIError::ChannelUnavailable {
+						err: format!("Channel with id {} not fully established", log_bytes!(*next_hop_channel_id))
 					})
 				}
 				chan.get_short_channel_id().unwrap_or(chan.outbound_scid_alias())
 			},
-			None => return Err(APIError::APIMisuseError {
-				err: format!("Channel with id {:?} not found", next_hop_channel_id)
+			None => return Err(APIError::ChannelUnavailable {
+				err: format!("Channel with id {} not found", log_bytes!(*next_hop_channel_id))
 			})
 		};
 
 		let payment = self.pending_intercepted_htlcs.lock().unwrap().remove(&intercept_id)
 			.ok_or_else(|| APIError::APIMisuseError {
-				err: format!("Payment with intercept id {:?} not found", intercept_id.0)
+				err: format!("Payment with intercept id {} not found", log_bytes!(intercept_id.0))
 			})?;
 
 		let routing = match payment.forward_info.routing {
@@ -3140,7 +3140,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
 		let payment = self.pending_intercepted_htlcs.lock().unwrap().remove(&intercept_id)
 			.ok_or_else(|| APIError::APIMisuseError {
-				err: format!("Payment with InterceptId {:?} not found", intercept_id)
+				err: format!("Payment with intercept id {} not found", log_bytes!(intercept_id.0))
 			})?;
 
 		if let PendingHTLCRouting::Forward { short_channel_id, .. } = payment.forward_info.routing {

lightning/src/ln/functional_tests.rs

@@ -6023,7 +6023,7 @@ fn test_update_add_htlc_bolt2_sender_cltv_expiry_too_high() {
 		.with_features(channelmanager::provided_invoice_features());
 	let (mut route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], payment_params, 100000000, 0);
 	route.paths[0].last_mut().unwrap().cltv_expiry_delta = 500000001;
-	unwrap_send_err!(nodes[0].node.send_payment(&route, our_payment_hash, &Some(our_payment_secret), PaymentId(our_payment_hash.0)), true, APIError::RouteError { ref err },
+	unwrap_send_err!(nodes[0].node.send_payment(&route, our_payment_hash, &Some(our_payment_secret), PaymentId(our_payment_hash.0)), true, APIError::InvalidRoute { ref err },
 		assert_eq!(err, &"Channel CLTV overflowed?"));
 }
 

lightning/src/ln/onion_utils.rs

@@ -182,11 +182,11 @@ pub(super) fn build_onion_payloads(path: &Vec<RouteHop>, total_msat: u64, paymen
 		});
 		cur_value_msat += hop.fee_msat;
 		if cur_value_msat >= 21000000 * 100000000 * 1000 {
-			return Err(APIError::RouteError{err: "Channel fees overflowed?"});
+			return Err(APIError::InvalidRoute{err: "Channel fees overflowed?"});
 		}
 		cur_cltv += hop.cltv_expiry_delta as u32;
 		if cur_cltv >= 500000000 {
-			return Err(APIError::RouteError{err: "Channel CLTV overflowed?"});
+			return Err(APIError::InvalidRoute{err: "Channel CLTV overflowed?"});
 		}
 		last_short_channel_id = hop.short_channel_id;
 	}

lightning/src/ln/payment_tests.rs

@@ -20,7 +20,7 @@ use crate::ln::channelmanager::{self, BREAKDOWN_TIMEOUT, ChannelManager, MPP_TIM
 use crate::ln::msgs;
 use crate::ln::msgs::ChannelMessageHandler;
 use crate::routing::gossip::RoutingFees;
-use crate::routing::router::{find_route, get_route, PaymentParameters, RouteHint, RouteHintHop, RouteParameters};
+use crate::routing::router::{get_route, PaymentParameters, RouteHint, RouteHintHop, RouteParameters};
 use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
 use crate::util::test_utils;
 use crate::util::errors::APIError;
@@ -1429,9 +1429,10 @@ fn do_test_intercepted_payment(test: InterceptTest) {
 		final_value_msat: amt_msat,
 		final_cltv_expiry_delta: TEST_FINAL_CLTV,
 	};
-	let route = find_route(
-		&nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph, None, nodes[0].logger,
-		&scorer, &random_seed_bytes
+	let route = get_route(
+		&nodes[0].node.get_our_node_id(), &route_params.payment_params,
+		&nodes[0].network_graph.read_only(), None, route_params.final_value_msat,
+		route_params.final_cltv_expiry_delta, nodes[0].logger, &scorer, &random_seed_bytes
 	).unwrap();
 
 	let (payment_hash, payment_secret) = nodes[2].node.create_inbound_payment(Some(amt_msat), 60 * 60).unwrap();
@@ -1466,7 +1467,7 @@ fn do_test_intercepted_payment(test: InterceptTest) {
 
 	// Check for unknown channel id error.
 	let unknown_chan_id_err = nodes[1].node.forward_intercepted_htlc(intercept_id, &[42; 32], nodes[2].node.get_our_node_id(), expected_outbound_amount_msat).unwrap_err();
-	assert_eq!(unknown_chan_id_err , APIError::APIMisuseError { err: format!("Channel with id {:?} not found", [42; 32]) });
+	assert_eq!(unknown_chan_id_err , APIError::ChannelUnavailable  { err: format!("Channel with id {} not found", log_bytes!([42; 32])) });
 
 	if test == InterceptTest::Fail {
 		// Ensure we can fail the intercepted payment back.
@@ -1490,7 +1491,7 @@ fn do_test_intercepted_payment(test: InterceptTest) {
 		// Check that we'll fail as expected when sending to a channel that isn't in `ChannelReady` yet.
 		let temp_chan_id = nodes[1].node.create_channel(nodes[2].node.get_our_node_id(), 100_000, 0, 42, None).unwrap();
 		let unusable_chan_err = nodes[1].node.forward_intercepted_htlc(intercept_id, &temp_chan_id, nodes[2].node.get_our_node_id(), expected_outbound_amount_msat).unwrap_err();
-		assert_eq!(unusable_chan_err , APIError::APIMisuseError { err: format!("Channel with id {:?} not fully established", temp_chan_id) });
+		assert_eq!(unusable_chan_err , APIError::ChannelUnavailable { err: format!("Channel with id {} not fully established", log_bytes!(temp_chan_id)) });
 		assert_eq!(nodes[1].node.get_and_clear_pending_msg_events().len(), 1);
 
 		// Open the just-in-time channel so the payment can then be forwarded.
@@ -1540,8 +1541,7 @@ fn do_test_intercepted_payment(test: InterceptTest) {
 		};
 		connect_block(&nodes[0], &block);
 		connect_block(&nodes[1], &block);
-		let block_count = 183; // find_route adds a random CLTV offset, so hardcode rather than summing consts
-		for _ in 0..block_count {
+		for _ in 0..TEST_FINAL_CLTV {
 			block.header.prev_blockhash = block.block_hash();
 			connect_block(&nodes[0], &block);
 			connect_block(&nodes[1], &block);
@@ -1561,6 +1561,8 @@ fn do_test_intercepted_payment(test: InterceptTest) {
 		// Check for unknown intercept id error.
 		let (_, channel_id) = open_zero_conf_channel(&nodes[1], &nodes[2], None);
 		let unknown_intercept_id_err = nodes[1].node.forward_intercepted_htlc(intercept_id, &channel_id, nodes[2].node.get_our_node_id(), expected_outbound_amount_msat).unwrap_err();
-		assert_eq!(unknown_intercept_id_err , APIError::APIMisuseError { err: format!("Payment with intercept id {:?} not found", intercept_id.0) });
+		assert_eq!(unknown_intercept_id_err , APIError::APIMisuseError { err: format!("Payment with intercept id {} not found", log_bytes!(intercept_id.0)) });
+		let unknown_intercept_id_err = nodes[1].node.fail_intercepted_htlc(intercept_id).unwrap_err();
+		assert_eq!(unknown_intercept_id_err , APIError::APIMisuseError { err: format!("Payment with intercept id {} not found", log_bytes!(intercept_id.0)) });
 	}
 }

lightning/src/util/errors.rs

@@ -35,7 +35,7 @@ pub enum APIError {
 	},
 	/// A malformed Route was provided (eg overflowed value, node id mismatch, overly-looped route,
 	/// too-many-hops, etc).
-	RouteError {
+	InvalidRoute {
 		/// A human-readable error message
 		err: &'static str
 	},
@@ -74,7 +74,7 @@ impl fmt::Debug for APIError {
 		match *self {
 			APIError::APIMisuseError {ref err} => write!(f, "Misuse error: {}", err),
 			APIError::FeeRateTooHigh {ref err, ref feerate} => write!(f, "{} feerate: {}", err, feerate),
-			APIError::RouteError {ref err} => write!(f, "Route error: {}", err),
+			APIError::InvalidRoute {ref err} => write!(f, "Invalid route provided: {}", err),
 			APIError::ChannelUnavailable {ref err} => write!(f, "Channel unavailable: {}", err),
 			APIError::MonitorUpdateInProgress => f.write_str("Client indicated a channel monitor update is in progress but not yet complete"),
 			APIError::IncompatibleShutdownScript { ref script } => {

lightning/src/util/persist.rs

@@ -10,10 +10,17 @@
 
 use core::ops::Deref;
 use bitcoin::hashes::hex::ToHex;
-use crate::io::{self};
+use crate::io;
 use crate::routing::scoring::WriteableScore;
 
-use crate::{chain::{keysinterface::{Sign, KeysInterface}, self, transaction::{OutPoint}, chaininterface::{BroadcasterInterface, FeeEstimator}, chainmonitor::{Persist, MonitorUpdateId}, channelmonitor::{ChannelMonitor, ChannelMonitorUpdate}}, ln::channelmanager::ChannelManager, routing::gossip::NetworkGraph};
+use crate::chain;
+use crate::chain::chaininterface::{BroadcasterInterface, FeeEstimator};
+use crate::chain::chainmonitor::{Persist, MonitorUpdateId};
+use crate::chain::keysinterface::{Sign, KeysInterface};
+use crate::chain::transaction::OutPoint;
+use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate};
+use crate::ln::channelmanager::ChannelManager;
+use crate::routing::gossip::NetworkGraph;
 use super::{logger::Logger, ser::Writeable};
 
 /// Trait for a key-value store for persisting some writeable object at some key