Merge pull request #4567 from Alkamal01/prefer-outbound-scid-alias-for-routing

Prefer outbound SCID alias over real SCID when routing

Author: TheBlueMatt

lightning/src/ln/channel_state.rs

@@ -312,8 +312,10 @@ pub struct ChannelDetails {
 	/// Note that if [`inbound_scid_alias`] is set, it must be used for invoices and inbound
 	/// payments instead of this. See [`get_inbound_payment_scid`].
 	///
-	/// For channels with [`confirmations_required`] set to `Some(0)`, [`outbound_scid_alias`] may
-	/// be used in place of this in outbound routes. See [`get_outbound_payment_scid`].
+	/// For routing outbound payments, this value should not be used if [`outbound_scid_alias`] is
+	/// set. [`outbound_scid_alias`] provides a stable routing identifier across splices, whereas
+	/// this value will change when a splice confirms.
+	/// Use [`get_outbound_payment_scid`] to pick the appropriate value.
 	///
 	/// When a channel is spliced, this continues to refer to the original pre-splice channel
 	/// state until the splice transaction reaches sufficient confirmations to be locked (and we
@@ -323,21 +325,17 @@ pub struct ChannelDetails {
 	/// [`outbound_scid_alias`]: Self::outbound_scid_alias
 	/// [`get_inbound_payment_scid`]: Self::get_inbound_payment_scid
 	/// [`get_outbound_payment_scid`]: Self::get_outbound_payment_scid
-	/// [`confirmations_required`]: Self::confirmations_required
 	pub short_channel_id: Option<u64>,
 	/// An optional [`short_channel_id`] alias for this channel, randomly generated by us and
-	/// usable in place of [`short_channel_id`] to reference the channel in outbound routes when
-	/// the channel has not yet been confirmed (as long as [`confirmations_required`] is
-	/// `Some(0)`).
+	/// usable in place of [`short_channel_id`] to route outbound payments. Because this alias is
+	/// assigned at channel open and remains stable across splices, it should be used for routing
+	/// instead of the real [`short_channel_id`] (which changes each time a splice confirms).
+	/// See [`get_outbound_payment_scid`].
 	///
 	/// This will be `None` as long as the channel is not available for routing outbound payments.
 	///
-	/// When a channel is spliced, this continues to refer to the original pre-splice channel
-	/// state until the splice transaction reaches sufficient confirmations to be locked (and we
-	/// exchange `splice_locked` messages with our peer).
-	///
 	/// [`short_channel_id`]: Self::short_channel_id
-	/// [`confirmations_required`]: Self::confirmations_required
+	/// [`get_outbound_payment_scid`]: Self::get_outbound_payment_scid
 	pub outbound_scid_alias: Option<u64>,
 	/// An optional [`short_channel_id`] alias for this channel, randomly generated by our
 	/// counterparty and usable in place of [`short_channel_id`] in invoice route hints. Our
@@ -513,12 +511,16 @@ impl ChannelDetails {
 	/// This should be used in [`Route`]s to describe the first hop or in other contexts where
 	/// we're sending or forwarding a payment outbound over this channel.
 	///
-	/// This is either the [`ChannelDetails::short_channel_id`], if set, or the
-	/// [`ChannelDetails::outbound_scid_alias`]. See those for more information.
+	/// Returns [`outbound_scid_alias`] if set, otherwise [`short_channel_id`]. The alias is
+	/// preferred because when a splice confirms the real SCID changes, whereas the alias assigned
+	/// at channel open remains stable.
+	///
+	/// [`outbound_scid_alias`]: ChannelDetails::outbound_scid_alias
+	/// [`short_channel_id`]: ChannelDetails::short_channel_id
 	///
 	/// [`Route`]: crate::routing::router::Route
 	pub fn get_outbound_payment_scid(&self) -> Option<u64> {
-		self.short_channel_id.or(self.outbound_scid_alias)
+		self.outbound_scid_alias.or(self.short_channel_id)
 	}
 
 	/// Gets the funding output for this channel, if available.

lightning/src/ln/onion_route_tests.rs

@@ -815,13 +815,16 @@ fn test_onion_failure() {
 	let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], None, None);
 
 	// Our immediate peer sent UpdateFailMalformedHTLC because it couldn't understand the onion in
-	// the UpdateAddHTLC that we sent.
+	// the UpdateAddHTLC that we sent. These tests explicitly route via the real SCID (not the
+	// alias) so the expected_short_channel_id assertions below match.
 	let short_channel_id = channels[0].0.contents.short_channel_id;
+	let mut route_via_real_scid = route.clone();
+	route_via_real_scid.paths[0].hops[0].short_channel_id = short_channel_id;
 	run_onion_failure_test(
 		"invalid_onion_version",
 		0,
 		&nodes,
-		&route,
+		&route_via_real_scid,
 		&payment_hash,
 		&payment_secret,
 		|msg| {
@@ -839,7 +842,7 @@ fn test_onion_failure() {
 		"invalid_onion_hmac",
 		0,
 		&nodes,
-		&route,
+		&route_via_real_scid,
 		&payment_hash,
 		&payment_secret,
 		|msg| {
@@ -857,7 +860,7 @@ fn test_onion_failure() {
 		"invalid_onion_key",
 		0,
 		&nodes,
-		&route,
+		&route_via_real_scid,
 		&payment_hash,
 		&payment_secret,
 		|msg| {

lightning/src/ln/payment_tests.rs

@@ -250,13 +250,16 @@ fn mpp_retry_overpay() {
 	let (mut route, hash, payment_preimage, pay_secret) =
 		get_route_and_payment_hash!(nodes[0], nodes[3], payment_params, amt_msat, max_fee);
 
-	// Check we overpay on the second path which we're about to fail.
+	// Check we overpay on the second path which we're about to fail. Path ordering is not fixed,
+	// so we identify paths by first-hop pubkey.
 	assert_eq!(chan_1_update.contents.fee_proportional_millionths, 0);
-	let overpaid_amount_1 = route.paths[0].fee_msat() as u32 - chan_1_update.contents.fee_base_msat;
+	let path_via_b = route.paths.iter().find(|p| p.hops[0].pubkey == node_b_id).unwrap();
+	let overpaid_amount_1 = path_via_b.fee_msat() as u32 - chan_1_update.contents.fee_base_msat;
 	assert_eq!(overpaid_amount_1, 0);
 
 	assert_eq!(chan_2_update.contents.fee_proportional_millionths, 0);
-	let overpaid_amount_2 = route.paths[1].fee_msat() as u32 - chan_2_update.contents.fee_base_msat;
+	let path_via_c = route.paths.iter().find(|p| p.hops[0].pubkey == node_c_id).unwrap();
+	let overpaid_amount_2 = path_via_c.fee_msat() as u32 - chan_2_update.contents.fee_base_msat;
 
 	let total_overpaid_amount = overpaid_amount_1 + overpaid_amount_2;
 
@@ -304,11 +307,13 @@ fn mpp_retry_overpay() {
 	// Rebalance the channel so the second half of the payment can succeed.
 	send_payment(&nodes[3], &[&nodes[2]], 38_000_000);
 
-	// Retry the second half of the payment and make sure it succeeds.
-	let first_path_value = route.paths[0].final_value_msat();
+	// Retry the second half of the payment and make sure it succeeds. Identify the successful
+	// path (through nodes[1]) by first-hop pubkey, since path ordering is not stable.
+	let path_via_b_idx = route.paths.iter().position(|p| p.hops[0].pubkey == node_b_id).unwrap();
+	let first_path_value = route.paths[path_via_b_idx].final_value_msat();
 	assert_eq!(first_path_value, 36_000_000);
 
-	route.paths.remove(0);
+	route.paths.remove(path_via_b_idx);
 	route_params.final_value_msat -= first_path_value;
 	let chan_4_scid = chan_4_update.contents.short_channel_id;
 	route_params.payment_params.previously_failed_channels.push(chan_4_scid);
@@ -2023,8 +2028,18 @@ fn preflight_probes_yield_event() {
 	let route_params = RouteParameters::from_payment_params_and_value(payment_params, recv_value);
 	let res = nodes[0].node.send_preflight_probes(route_params, None).unwrap();
 
+	// Path ordering depends on outbound SCID selection. Determine which res entry corresponds
+	// to which path by comparing the alias SCIDs of the two channels.
+	let node_b_id = nodes[1].node.get_our_node_id();
+	let node_c_id = nodes[2].node.get_our_node_id();
+	let chans = nodes[0].node.list_usable_channels();
+	let chan_to_b = chans.iter().find(|c| c.counterparty.node_id == node_b_id).unwrap();
+	let chan_to_c = chans.iter().find(|c| c.counterparty.node_id == node_c_id).unwrap();
+	let b_first = chan_to_b.get_outbound_payment_scid() < chan_to_c.get_outbound_payment_scid();
+	let (hash_b, hash_c) = if b_first { (res[0].0, res[1].0) } else { (res[1].0, res[0].0) };
+
 	let expected_route: &[(&[&Node], PaymentHash)] =
-		&[(&[&nodes[1], &nodes[3]], res[0].0), (&[&nodes[2], &nodes[3]], res[1].0)];
+		&[(&[&nodes[1], &nodes[3]], hash_b), (&[&nodes[2], &nodes[3]], hash_c)];
 
 	assert_eq!(res.len(), expected_route.len());
 
@@ -2319,7 +2334,7 @@ fn test_trivial_inflight_htlc_tracking() {
 		let chan_1_used_liquidity = inflight_htlcs.used_liquidity_msat(
 			&NodeId::from_pubkey(&node_a_id),
 			&NodeId::from_pubkey(&node_b_id),
-			channel_1.funding().get_short_channel_id().unwrap(),
+			channel_1.context().outbound_scid_alias(),
 		);
 		// First hop accounts for expected 1000 msat fee
 		assert_eq!(chan_1_used_liquidity, Some(501000));
@@ -2429,7 +2444,7 @@ fn test_holding_cell_inflight_htlcs() {
 		let used_liquidity = inflight_htlcs.used_liquidity_msat(
 			&NodeId::from_pubkey(&node_a_id),
 			&NodeId::from_pubkey(&node_b_id),
-			channel.funding().get_short_channel_id().unwrap(),
+			channel.context().outbound_scid_alias(),
 		);
 
 		assert_eq!(used_liquidity, Some(2000000));

lightning/src/ln/priv_short_conf_tests.rs

@@ -1108,16 +1108,16 @@ fn test_0conf_channel_reorg() {
 	mine_transaction(&nodes[1], &tx);
 	mine_transaction(&nodes[2], &tx);
 
-	// Send a payment using the channel's real SCID, which will be public in a few blocks once we
-	// can generate a channel_announcement.
+	// Send a payment using the channel's alias SCID. The channel itself will be public in a few
+	// blocks once we can generate a channel_announcement.
 	let bs_chans = nodes[1].node.list_usable_channels();
 	let bs_chan = bs_chans.iter().find(|chan| chan.counterparty.node_id == node_c_id).unwrap();
 	let original_scid = bs_chan.short_channel_id.unwrap();
 	assert_eq!(nodes[2].node.list_usable_channels()[0].short_channel_id.unwrap(), original_scid);
 
 	let (mut route, payment_hash, payment_preimage, payment_secret) =
 		get_route_and_payment_hash!(nodes[1], nodes[2], 10_000);
-	assert_eq!(route.paths[0].hops[0].short_channel_id, original_scid);
+	assert_eq!(route.paths[0].hops[0].short_channel_id, bs_chan.outbound_scid_alias.unwrap());
 	send_along_route_with_secret(
 		&nodes[1],
 		route.clone(),
@@ -1188,7 +1188,7 @@ fn test_0conf_channel_reorg() {
 	assert_ne!(original_scid, new_scid);
 	assert_eq!(nodes[2].node.list_usable_channels()[0].short_channel_id.unwrap(), new_scid);
 
-	// At this point, the channel should happily forward or send payments with either the old SCID
+	// At this point, the channel should happily forward or send payments with either the alias SCID
 	// or the new SCID...
 	send_along_route_with_secret(
 		&nodes[1],
@@ -1286,12 +1286,22 @@ fn test_0conf_channel_reorg() {
 
 	let onion = RecipientOnionFields::secret_only(payment_secret, 10_000);
 	let id = PaymentId([0; 32]);
-	nodes[1].node.send_payment_with_route(route, payment_hash, onion.clone(), id).unwrap();
+
+	// The route uses the alias SCID, which is stable across reorgs. To verify the old real SCID
+	// is invalidated after propagation delay, we explicitly build a route using original_scid.
+	let mut old_scid_route = route.clone();
+	old_scid_route.paths[0].hops[0].short_channel_id = original_scid;
+	nodes[1].node.send_payment_with_route(old_scid_route, payment_hash, onion.clone(), id).unwrap();
 	let mut conditions = PaymentFailedConditions::new();
 	conditions.reason = Some(PaymentFailureReason::RouteNotFound);
 	expect_payment_failed_conditions(&nodes[1], payment_hash, false, conditions);
 
-	nodes[0].node.send_payment_with_route(forwarded_route, payment_hash, onion, id).unwrap();
+	let mut old_scid_forwarded_route = forwarded_route.clone();
+	old_scid_forwarded_route.paths[0].hops[1].short_channel_id = original_scid;
+	nodes[0]
+		.node
+		.send_payment_with_route(old_scid_forwarded_route, payment_hash, onion, id)
+		.unwrap();
 	check_added_monitors(&nodes[0], 1);
 	let mut ev = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(ev.len(), 1);

lightning/src/ln/reload_tests.rs

@@ -956,6 +956,12 @@ fn test_mpp_claim_htlc_fulfills_unblocked_on_reload() {
 	let chan_id_b = chan_b.2;
 	let scid_a = chan_a.0.contents.short_channel_id;
 	let scid_b = chan_b.0.contents.short_channel_id;
+	// Routes to a directly-connected peer use the outbound SCID alias, so payment path success
+	// events report the alias rather than the real SCID announced in gossip.
+	let payment_scid_a = nodes[0].node.list_channels().iter()
+		.find(|chan| chan.channel_id == chan_id_a).unwrap().get_outbound_payment_scid().unwrap();
+	let payment_scid_b = nodes[0].node.list_channels().iter()
+		.find(|chan| chan.channel_id == chan_id_b).unwrap().get_outbound_payment_scid().unwrap();
 
 	// Send an MPP payment to nodes[1]. `send_along_route_with_secret` leaves the payment
 	// claimable but unclaimed, so nodes[1] still has both inbound HTLCs live when we start
@@ -1214,7 +1220,7 @@ fn test_mpp_claim_htlc_fulfills_unblocked_on_reload() {
 		}
 	}
 	assert!(saw_startup_payment_sent);
-	assert_eq!(startup_success_scids, vec![scid_a]);
+	assert_eq!(startup_success_scids, vec![payment_scid_a]);
 
 	// Handling the claim event runs the event-completion action that releases the remaining
 	// RAA-blocked monitor update. The startup unblock path already released channel A, so channel B
@@ -1270,7 +1276,7 @@ fn test_mpp_claim_htlc_fulfills_unblocked_on_reload() {
 		Event::PaymentPathSuccessful { payment_hash: Some(path_hash), path, .. } => {
 			assert_eq!(*path_hash, payment_hash);
 			assert_eq!(path.hops.len(), 1);
-			assert_eq!(path.hops[0].short_channel_id, scid_b);
+			assert_eq!(path.hops[0].short_channel_id, payment_scid_b);
 		},
 		_ => panic!("Unexpected final payment event: {:?}", final_payment_events[0]),
 	}

lightning/src/ln/splicing_tests.rs

@@ -3880,7 +3880,7 @@ fn fail_splice_on_tx_complete_error() {
 
 	// Queue an outgoing HTLC to the holding cell. It should be freed once we exit quiescence.
 	let (route, payment_hash, _payment_preimage, payment_secret) =
-		get_route_and_payment_hash!(initiator, acceptor, 1_000_000);
+		get_route_and_payment_hash!(acceptor, initiator, 1_000_000);
 	let onion = RecipientOnionFields::secret_only(payment_secret, 1_000_000);
 	let payment_id = PaymentId(payment_hash.0);
 	acceptor.node.send_payment_with_route(route, payment_hash, onion, payment_id).unwrap();

lightning/src/routing/router.rs

@@ -9187,7 +9187,7 @@ mod tests {
 		assert_eq!(route.paths.len(), 1);
 		assert_eq!(route.get_total_amount(), amt_msat);
 		assert_eq!(route.paths[0].hops.len(), 2);
-		assert_eq!(route.paths[0].hops[0].short_channel_id, 1);
+		assert_eq!(route.paths[0].hops[0].short_channel_id, 44);
 		assert_eq!(route.paths[0].hops[1].short_channel_id, 45);
 		assert_eq!(route.get_total_fees(), 123);
 	}