Include ChannelUpdates for failed channels in PaymentParameters

If we receive channel updates as part of failure onions, we know
include them in `PaymentParameters::previously_failed_channels`.

In the next step, we can then verify the updates against the network
graph and account for them in routing.

Author: tnull

lightning/src/events/mod.rs

@@ -125,12 +125,13 @@ pub enum PathFailure {
 	},
 	/// A hop on the path failed to forward our payment.
 	OnPath {
-		/// If present, this [`NetworkUpdate`] can be applied to a [`NetworkGraph`] so that
-		/// routing decisions can permanently take the update into account.
+		/// If present, this [`NetworkUpdate`] will be taken into account when retrying the
+		/// payment.
 		///
-		/// However, note that if applying the update is obversable to the rest of the network
-		/// (e.g., through public gossip), it can introduce a privacy leak as malicious
-		/// intermediaries may fail HTLCs and try to infer the payment's origin.
+		/// Note that while this update can also be applied permanently to the [`NetworkGraph`],
+		/// doing so in a manner observable to the rest of the network (e.g., through public
+		/// gossip) can introduce a privacy leak as malicious intermediaries may fail HTLCs and
+		/// try to infer the payment's origin.
 		///
 		/// [`NetworkUpdate`]: crate::routing::gossip::NetworkUpdate
 		/// [`NetworkGraph`]: crate::routing::gossip::NetworkGraph
@@ -628,10 +629,9 @@ pub enum Event {
 		/// the payment has failed, not just the route in question. If this is not set, the payment may
 		/// be retried via a different route.
 		payment_failed_permanently: bool,
-		/// Extra error details based on the failure type. May contain an update that can be
-		/// applied to a [`NetworkGraph`].
+		/// Extra error details based on the failure type.
 		///
-		/// [`NetworkGraph`]: crate::routing::gossip::NetworkGraph
+		/// May contain an update that will be taken into account on retry.
 		failure: PathFailure,
 		/// The payment path that failed.
 		path: Path,

lightning/src/ln/functional_test_utils.rs

@@ -2156,7 +2156,8 @@ macro_rules! expect_payment_failed {
 pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 	payment_failed_events: Vec<Event>, expected_payment_hash: PaymentHash,
 	expected_payment_failed_permanently: bool, conditions: PaymentFailedConditions<'e>
-) {
+) -> Option<msgs::ChannelUpdate> {
+	let mut channel_update = None;
 	if conditions.expected_mpp_parts_remain { assert_eq!(payment_failed_events.len(), 1); } else { assert_eq!(payment_failed_events.len(), 2); }
 	let expected_payment_id = match &payment_failed_events[0] {
 		Event::PaymentPathFailed { payment_hash, payment_failed_permanently, payment_id, failure,
@@ -2185,6 +2186,7 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 							}
 							const CHAN_DISABLED_FLAG: u8 = 2;
 							assert_eq!(msg.contents.flags & CHAN_DISABLED_FLAG, 0);
+							channel_update = Some(msg.clone());
 						},
 						NetworkUpdate::ChannelFailure { short_channel_id, is_permanent } if chan_closed => {
 							if let Some(scid) = conditions.expected_blamed_scid {
@@ -2215,6 +2217,7 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 			_ => panic!("Unexpected second event"),
 		}
 	}
+	channel_update
 }
 
 pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>(

lightning/src/ln/msgs.rs

@@ -1109,7 +1109,7 @@ pub struct ChannelAnnouncement {
 /// The unsigned part of a [`channel_update`] message.
 ///
 /// [`channel_update`]: https://github.com/lightning/bolts/blob/master/07-routing-gossip.md#the-channel_update-message
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, Hash, PartialEq, Eq)]
 pub struct UnsignedChannelUpdate {
 	/// The genesis hash of the blockchain where the channel is to be opened
 	pub chain_hash: BlockHash,
@@ -1147,7 +1147,7 @@ pub struct UnsignedChannelUpdate {
 /// A [`channel_update`] message to be sent to or received from a peer.
 ///
 /// [`channel_update`]: https://github.com/lightning/bolts/blob/master/07-routing-gossip.md#the-channel_update-message
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, Hash, PartialEq, Eq)]
 pub struct ChannelUpdate {
 	/// A signature of the channel update
 	pub signature: Signature,

lightning/src/ln/outbound_payment.rs

@@ -13,11 +13,13 @@ use bitcoin::hashes::Hash;
 use bitcoin::hashes::sha256::Hash as Sha256;
 use bitcoin::secp256k1::{self, Secp256k1, SecretKey};
 
+use crate::routing::gossip::NetworkUpdate;
 use crate::sign::{EntropySource, NodeSigner, Recipient};
 use crate::events::{self, PaymentFailureReason};
 use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
 use crate::ln::channelmanager::{ChannelDetails, EventCompletionAction, HTLCSource, PaymentId};
 use crate::ln::onion_utils::{DecodedOnionFailure, HTLCFailReason};
+use crate::ln::msgs::ChannelUpdate;
 use crate::offers::invoice::Bolt12Invoice;
 use crate::routing::router::{InFlightHtlcs, Path, PaymentParameters, Route, RouteParameters, Router};
 use crate::util::errors::APIError;
@@ -129,9 +131,9 @@ impl PendingOutboundPayment {
 			_ => false,
 		}
 	}
-	pub fn insert_previously_failed_scid(&mut self, scid: u64) {
+	pub fn insert_previously_failed_scid(&mut self, scid: u64, channel_update: Option<ChannelUpdate>) {
 		if let PendingOutboundPayment::Retryable { payment_params: Some(params), .. } = self {
-			params.previously_failed_channels.push(scid);
+			params.previously_failed_channels.push((scid, channel_update));
 		}
 	}
 	fn is_awaiting_invoice(&self) -> bool {
@@ -248,7 +250,7 @@ impl PendingOutboundPayment {
 		if insert_res {
 			if let PendingOutboundPayment::Retryable {
 				ref mut pending_amt_msat, ref mut pending_fee_msat,
-				ref mut remaining_max_total_routing_fee_msat, .. 
+				ref mut remaining_max_total_routing_fee_msat, ..
 			} = self {
 					*pending_amt_msat += path.final_value_msat();
 					let path_fee_msat = path.fee_msat();
@@ -1142,7 +1144,7 @@ impl OutboundPayments {
 				if let APIError::ChannelUnavailable { .. } = e {
 					let scid = path.hops[0].short_channel_id;
 					failed_scid = Some(scid);
-					route_params.payment_params.previously_failed_channels.push(scid);
+					route_params.payment_params.previously_failed_channels.push((scid, None));
 				}
 				events.push_back((events::Event::PaymentPathFailed {
 					payment_id: Some(payment_id),
@@ -1616,7 +1618,13 @@ impl OutboundPayments {
 				// TODO: If we decided to blame ourselves (or one of our channels) in
 				// process_onion_failure we should close that channel as it implies our
 				// next-hop is needlessly blaming us!
-				payment.get_mut().insert_previously_failed_scid(scid);
+
+				let channel_update = network_update.as_ref().and_then(|upd| {
+					if let NetworkUpdate::ChannelUpdateMessage { msg } = upd {
+						Some(msg)
+					} else { None }
+				});
+				payment.get_mut().insert_previously_failed_scid(scid, channel_update.cloned());
 			}
 
 			if payment_is_probe || !is_retryable_now || payment_failed_permanently {
@@ -1953,7 +1961,7 @@ mod tests {
 		};
 		router.expect_find_route(route_params.clone(), Ok(route.clone()));
 		let mut route_params_w_failed_scid = route_params.clone();
-		route_params_w_failed_scid.payment_params.previously_failed_channels.push(failed_scid);
+		route_params_w_failed_scid.payment_params.previously_failed_channels.push((failed_scid, None));
 		let mut route_w_failed_scid = route.clone();
 		route_w_failed_scid.route_params = Some(route_params_w_failed_scid.clone());
 		router.expect_find_route(route_params_w_failed_scid, Ok(route_w_failed_scid));

lightning/src/ln/payment_tests.rs

@@ -135,15 +135,18 @@ fn mpp_retry() {
 		_ => panic!("Unexpected event")
 	}
 	events.remove(1);
-	expect_payment_failed_conditions_event(events, payment_hash, false, PaymentFailedConditions::new().mpp_parts_remain());
+
+	let channel_update = expect_payment_failed_conditions_event(events, payment_hash, false,
+		PaymentFailedConditions::new().mpp_parts_remain().blamed_chan_closed(false));
 
 	// Rebalance the channel so the second half of the payment can succeed.
 	send_payment(&nodes[3], &vec!(&nodes[2])[..], 1_500_000);
 
 	// Retry the second half of the payment and make sure it succeeds.
 	route.paths.remove(0);
 	route_params.final_value_msat = 1_000_000;
-	route_params.payment_params.previously_failed_channels.push(chan_4_update.contents.short_channel_id);
+	route_params.payment_params.previously_failed_channels.push(
+		(chan_4_update.contents.short_channel_id, channel_update));
 	// Check the remaining max total routing fee for the second attempt is 50_000 - 1_000 msat fee
 	// used by the first path
 	route_params.max_total_routing_fee_msat = Some(max_total_routing_fee_msat - 1_000);
@@ -242,8 +245,8 @@ fn mpp_retry_overpay() {
 		_ => panic!("Unexpected event")
 	}
 	events.remove(1);
-	expect_payment_failed_conditions_event(events, payment_hash, false,
-		PaymentFailedConditions::new().mpp_parts_remain());
+	let channel_update = expect_payment_failed_conditions_event(events, payment_hash, false,
+		PaymentFailedConditions::new().mpp_parts_remain().blamed_chan_closed(false));
 
 	// Rebalance the channel so the second half of the payment can succeed.
 	send_payment(&nodes[3], &vec!(&nodes[2])[..], 38_000_000);
@@ -254,7 +257,8 @@ fn mpp_retry_overpay() {
 
 	route.paths.remove(0);
 	route_params.final_value_msat -= first_path_value;
-	route_params.payment_params.previously_failed_channels.push(chan_4_update.contents.short_channel_id);
+	route_params.payment_params.previously_failed_channels.push(
+		(chan_4_update.contents.short_channel_id, channel_update));
 	// Check the remaining max total routing fee for the second attempt accounts only for 1_000 msat
 	// base fee, but not for overpaid value of the first try.
 	route_params.max_total_routing_fee_msat.as_mut().map(|m| *m -= 1000);
@@ -2429,7 +2433,7 @@ fn auto_retry_partial_failure() {
 
 	// Configure the retry1 paths
 	let mut payment_params = route_params.payment_params.clone();
-	payment_params.previously_failed_channels.push(chan_2_id);
+	payment_params.previously_failed_channels.push((chan_2_id, None));
 	let mut retry_1_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 2);
 	retry_1_params.max_total_routing_fee_msat = None;
 
@@ -2460,7 +2464,7 @@ fn auto_retry_partial_failure() {
 
 	// Configure the retry2 path
 	let mut payment_params = retry_1_params.payment_params.clone();
-	payment_params.previously_failed_channels.push(chan_3_id);
+	payment_params.previously_failed_channels.push((chan_3_id, None));
 	let mut retry_2_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat / 4);
 	retry_2_params.max_total_routing_fee_msat = None;
 
@@ -2735,7 +2739,7 @@ fn retry_multi_path_single_failed_payment() {
 	route.paths[0].hops[0].fee_msat = 50_000_001;
 	route.paths[1].hops[0].fee_msat = 50_000_000;
 	let mut pay_params = route.route_params.clone().unwrap().payment_params;
-	pay_params.previously_failed_channels.push(chans[1].short_channel_id.unwrap());
+	pay_params.previously_failed_channels.push((chans[1].short_channel_id.unwrap(), None));
 
 	let mut retry_params = RouteParameters::from_payment_params_and_value(pay_params, 100_000_000);
 	retry_params.max_total_routing_fee_msat = None;
@@ -2819,7 +2823,7 @@ fn immediate_retry_on_failure() {
 	route.paths[0].hops[0].fee_msat = 50_000_000;
 	route.paths[1].hops[0].fee_msat = 50_000_001;
 	let mut pay_params = route_params.payment_params.clone();
-	pay_params.previously_failed_channels.push(chans[0].short_channel_id.unwrap());
+	pay_params.previously_failed_channels.push((chans[0].short_channel_id.unwrap(), None));
 	let retry_params = RouteParameters::from_payment_params_and_value(pay_params, amt_msat);
 	route.route_params = Some(retry_params.clone());
 	nodes[0].router.expect_find_route(retry_params, Ok(route.clone()));
@@ -2865,8 +2869,11 @@ fn no_extra_retries_on_back_to_back_fail() {
 	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
 	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 
-	let chan_1_scid = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 0).0.contents.short_channel_id;
-	let chan_2_scid = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 0).0.contents.short_channel_id;
+	let chan_1_update = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 0).0;
+	let chan_2_update = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 0).0;
+
+	let chan_1_scid = chan_1_update.contents.short_channel_id;
+	let chan_2_scid = chan_2_update.contents.short_channel_id;
 
 	let amt_msat = 200_000_000;
 	let (_, payment_hash, _, payment_secret) = get_route_and_payment_hash!(&nodes[0], nodes[1], amt_msat);
@@ -2926,7 +2933,8 @@ fn no_extra_retries_on_back_to_back_fail() {
 	route.route_params.as_mut().unwrap().max_total_routing_fee_msat = None;
 	nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
 	let mut second_payment_params = route_params.payment_params.clone();
-	second_payment_params.previously_failed_channels = vec![chan_2_scid, chan_2_scid];
+	second_payment_params.previously_failed_channels = vec![
+		(chan_2_scid, Some(chan_2_update.clone())), (chan_2_scid, Some(chan_2_update))];
 	// On retry, we'll only return one path
 	route.paths.remove(1);
 	route.paths[0].hops[1].fee_msat = amt_msat;
@@ -3070,8 +3078,11 @@ fn test_simple_partial_retry() {
 	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
 	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 
-	let chan_1_scid = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 0).0.contents.short_channel_id;
-	let chan_2_scid = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 0).0.contents.short_channel_id;
+	let chan_1_update = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 0).0;
+	let chan_2_update = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 0).0;
+
+	let chan_1_scid = chan_1_update.contents.short_channel_id;
+	let chan_2_scid = chan_2_update.contents.short_channel_id;
 
 	let amt_msat = 200_000_000;
 	let (_, payment_hash, _, payment_secret) = get_route_and_payment_hash!(&nodes[0], nodes[2], amt_msat);
@@ -3132,7 +3143,7 @@ fn test_simple_partial_retry() {
 	nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
 
 	let mut second_payment_params = route_params.payment_params.clone();
-	second_payment_params.previously_failed_channels = vec![chan_2_scid];
+	second_payment_params.previously_failed_channels = vec![(chan_2_scid, Some(chan_2_update))];
 	// On retry, we'll only be asked for one path (or 100k sats)
 	route.paths.remove(0);
 	let mut retry_params = RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat / 2);
@@ -3354,7 +3365,7 @@ fn test_threaded_payment_retries() {
 		// we should still ultimately fail for the same reason - because we're trying to send too
 		// many HTLCs at once.
 		let mut new_route_params = route_params.clone();
-		previously_failed_channels.push(route.paths[0].hops[1].short_channel_id);
+		previously_failed_channels.push((route.paths[0].hops[1].short_channel_id, None));
 		new_route_params.payment_params.previously_failed_channels = previously_failed_channels.clone();
 		new_route_params.max_total_routing_fee_msat.as_mut().map(|m| *m -= 100_000);
 		route.paths[0].hops[1].short_channel_id += 1;
@@ -3765,14 +3776,15 @@ fn test_retry_custom_tlvs() {
 		_ => panic!("Unexpected event")
 	}
 	events.remove(1);
-	expect_payment_failed_conditions_event(events, payment_hash, false,
-		PaymentFailedConditions::new().mpp_parts_remain());
+	let channel_update = expect_payment_failed_conditions_event(events, payment_hash, false,
+		PaymentFailedConditions::new().mpp_parts_remain().blamed_chan_closed(false));
 
 	// Rebalance the channel so the retry of the payment can succeed.
 	send_payment(&nodes[2], &vec!(&nodes[1])[..], 1_500_000);
 
 	// Retry the payment and make sure it succeeds
-	route_params.payment_params.previously_failed_channels.push(chan_2_update.contents.short_channel_id);
+	route_params.payment_params.previously_failed_channels.push(
+		(chan_2_update.contents.short_channel_id, channel_update));
 	route.route_params = Some(route_params.clone());
 	nodes[0].router.expect_find_route(route_params, Ok(route));
 	nodes[0].node.process_pending_htlc_forwards();

lightning/src/routing/router.rs

@@ -17,7 +17,7 @@ use crate::blinded_path::{BlindedHop, BlindedPath};
 use crate::ln::PaymentHash;
 use crate::ln::channelmanager::{ChannelDetails, PaymentId};
 use crate::ln::features::{Bolt11InvoiceFeatures, Bolt12InvoiceFeatures, ChannelFeatures, NodeFeatures};
-use crate::ln::msgs::{DecodeError, ErrorAction, LightningError, MAX_VALUE_MSAT};
+use crate::ln::msgs::{ChannelUpdate, DecodeError, ErrorAction, LightningError, MAX_VALUE_MSAT};
 use crate::offers::invoice::{BlindedPayInfo, Bolt12Invoice};
 use crate::routing::gossip::{DirectedChannelInfo, EffectiveCapacity, ReadOnlyNetworkGraph, NetworkGraph, NodeId, RoutingFees};
 use crate::routing::scoring::{ChannelUsage, LockableScore, ScoreLookUp};
@@ -583,10 +583,14 @@ pub struct PaymentParameters {
 	/// Default value: 2
 	pub max_channel_saturation_power_of_half: u8,
 
-	/// A list of SCIDs which this payment was previously attempted over and which caused the
+	/// A list of failed channels which this payment was previously attempted over and which caused the
 	/// payment to fail. Future attempts for the same payment shouldn't be relayed through any of
-	/// these SCIDs.
-	pub previously_failed_channels: Vec<u64>,
+	/// the given SCIDs.
+	///
+	/// Will include a channel update for the failed channel, if we received one in the
+	/// the failure onion. This update should be verified against the network graph and may then be
+	/// considered when retrying the payment.
+	pub previously_failed_channels: Vec<(u64, Option<ChannelUpdate>)>,
 }
 
 impl Writeable for PaymentParameters {
@@ -1792,7 +1796,8 @@ where L::Target: Logger {
 						  recommended_value_msat >= $next_hops_path_htlc_minimum_msat));
 
 					let payment_failed_on_this_channel = scid_opt.map_or(false,
-						|scid| payment_params.previously_failed_channels.contains(&scid));
+						|scid| payment_params.previously_failed_channels
+							.iter().any(|(failed_scid, _)| scid == *failed_scid));
 
 					let should_log_candidate = match $candidate {
 						CandidateRouteHop::FirstHop { .. } => true,
@@ -6297,11 +6302,12 @@ mod tests {
 				Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes)
 			{
 				for chan in route.paths[0].hops.iter() {
-					assert!(!payment_params.previously_failed_channels.contains(&chan.short_channel_id));
+					assert!(!payment_params.previously_failed_channels
+						.iter().any(|(failed_scid, _)| &chan.short_channel_id == failed_scid));
 				}
 				let victim = (u64::from_ne_bytes(random_seed_bytes[0..8].try_into().unwrap()) as usize)
 					% route.paths[0].hops.len();
-				payment_params.previously_failed_channels.push(route.paths[0].hops[victim].short_channel_id);
+				payment_params.previously_failed_channels.push((route.paths[0].hops[victim].short_channel_id, None));
 			} else { break; }
 		}
 	}

pending_changelog/avoid_observable_updates.txt

@@ -0,0 +1,3 @@
+## Backwards Compatibility
+
+* Outbound payments pending retry that have been serialized with LDK pior to 0.0.118 will lose information about previously failed channels upon upgrade.