From b7a50a6b028f8badb71f1866829b7202fb047522 Mon Sep 17 00:00:00 2001 From: okekefrancis112 Date: Thu, 5 Feb 2026 10:15:29 +0100 Subject: [PATCH 1/2] test: use route SCID instead of channel announcement SCID In onion failure tests, use the SCID from the actual route (route.paths[0].hops[N].short_channel_id) instead of from the channel announcement (channels[N].0.contents.short_channel_id). This is more correct as tests should verify what actually happened in the route, especially now that routes may use SCID aliases for first hops while channel announcements contain real SCIDs. Co-Authored-By: Claude Sonnet 4.5 --- lightning/src/ln/onion_route_tests.rs | 38 +++++++++++++-------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 27e0cfafade..c8d0b989c72 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -419,7 +419,7 @@ fn test_fee_failures() { // malleated the payment before forwarding, taking funds when they shouldn't have. However, // because we ignore channel update contents, we will still blame the 2nd channel. let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); - let short_channel_id = channels[1].0.contents.short_channel_id; + let short_channel_id = route.paths[0].hops[1].short_channel_id; run_onion_failure_test( "fee_insufficient", 100, @@ -510,7 +510,7 @@ fn test_onion_failure() { }; // intermediate node failure - let short_channel_id = channels[1].0.contents.short_channel_id; + let short_channel_id = route.paths[0].hops[1].short_channel_id; run_onion_failure_test( "invalid_realm", 0, @@ -552,7 +552,7 @@ fn test_onion_failure() { ); // final node failure - let short_channel_id = channels[1].0.contents.short_channel_id; + let short_channel_id = route.paths[0].hops[1].short_channel_id; run_onion_failure_test( "invalid_realm", 3, @@ -815,7 +815,7 @@ fn test_onion_failure() { // Our immediate peer sent UpdateFailMalformedHTLC because it couldn't understand the onion in // the UpdateAddHTLC that we sent. - let short_channel_id = channels[0].0.contents.short_channel_id; + let short_channel_id = route.paths[0].hops[0].short_channel_id; run_onion_failure_test( "invalid_onion_version", 0, @@ -870,7 +870,7 @@ fn test_onion_failure() { Some(HTLCHandlingFailureType::InvalidOnion), ); - let short_channel_id = channels[1].0.contents.short_channel_id; + let short_channel_id = route.paths[0].hops[1].short_channel_id; let chan_update = ChannelUpdate::dummy(short_channel_id); let mut err_data = Vec::new(); @@ -941,7 +941,7 @@ fn test_onion_failure() { Some(next_hop_failure.clone()), ); - let short_channel_id = channels[1].0.contents.short_channel_id; + let short_channel_id = route.paths[0].hops[1].short_channel_id; run_onion_failure_test_with_fail_intercept( "permanent_channel_failure", 100, @@ -974,7 +974,7 @@ fn test_onion_failure() { Some(next_hop_failure.clone()), ); - let short_channel_id = channels[1].0.contents.short_channel_id; + let short_channel_id = route.paths[0].hops[1].short_channel_id; run_onion_failure_test_with_fail_intercept( "required_channel_feature_missing", 100, @@ -1026,7 +1026,7 @@ fn test_onion_failure() { Some(HTLCHandlingFailureType::InvalidForward { requested_forward_scid: short_channel_id }), ); - let short_channel_id = channels[1].0.contents.short_channel_id; + let short_channel_id = route.paths[0].hops[1].short_channel_id; let amt_to_forward = { let (per_peer_state, mut peer_state); let chan = get_channel_ref!(nodes[1], nodes[2], per_peer_state, peer_state, channels[1].2); @@ -1064,7 +1064,7 @@ fn test_onion_failure() { // We ignore channel update contents in onion errors, so will blame the 2nd channel even though // the first node is the one that messed up. - let short_channel_id = channels[1].0.contents.short_channel_id; + let short_channel_id = route.paths[0].hops[1].short_channel_id; run_onion_failure_test( "fee_insufficient", 100, @@ -1083,7 +1083,7 @@ fn test_onion_failure() { Some(next_hop_failure.clone()), ); - let short_channel_id = channels[1].0.contents.short_channel_id; + let short_channel_id = route.paths[0].hops[1].short_channel_id; run_onion_failure_test( "incorrect_cltv_expiry", 100, @@ -1103,7 +1103,7 @@ fn test_onion_failure() { Some(next_hop_failure.clone()), ); - let short_channel_id = channels[1].0.contents.short_channel_id; + let short_channel_id = route.paths[0].hops[1].short_channel_id; run_onion_failure_test( "expiry_too_soon", 100, @@ -1190,7 +1190,7 @@ fn test_onion_failure() { true, Some(LocalHTLCFailureReason::FinalIncorrectCLTVExpiry), None, - Some(channels[1].0.contents.short_channel_id), + Some(route.paths[0].hops[1].short_channel_id), Some(HTLCHandlingFailureType::Receive { payment_hash }), ); @@ -1220,11 +1220,11 @@ fn test_onion_failure() { true, Some(LocalHTLCFailureReason::FinalIncorrectHTLCAmount), None, - Some(channels[1].0.contents.short_channel_id), + Some(route.paths[0].hops[1].short_channel_id), Some(HTLCHandlingFailureType::Receive { payment_hash }), ); - let short_channel_id = channels[1].0.contents.short_channel_id; + let short_channel_id = route.paths[0].hops[1].short_channel_id; run_onion_failure_test( "channel_disabled", 100, @@ -1386,7 +1386,7 @@ fn test_onion_failure() { node_id: route.paths[0].hops[1].pubkey, is_permanent: true, }), - Some(channels[1].0.contents.short_channel_id), + Some(route.paths[0].hops[1].short_channel_id), None, ); @@ -1455,10 +1455,10 @@ fn test_onion_failure() { true, Some(LocalHTLCFailureReason::TemporaryChannelFailure), Some(NetworkUpdate::ChannelFailure { - short_channel_id: channels[1].0.contents.short_channel_id, + short_channel_id: route.paths[0].hops[1].short_channel_id, is_permanent: false, }), - Some(channels[1].0.contents.short_channel_id), + Some(route.paths[0].hops[1].short_channel_id), Some(next_hop_failure.clone()), ); run_onion_failure_test_with_fail_intercept( @@ -1505,10 +1505,10 @@ fn test_onion_failure() { true, Some(LocalHTLCFailureReason::TemporaryChannelFailure), Some(NetworkUpdate::ChannelFailure { - short_channel_id: channels[1].0.contents.short_channel_id, + short_channel_id: route.paths[0].hops[1].short_channel_id, is_permanent: false, }), - Some(channels[1].0.contents.short_channel_id), + Some(route.paths[0].hops[1].short_channel_id), None, ); run_onion_failure_test( From a153dbe7f66f30a60521b4bd18c23ac160a17c55 Mon Sep 17 00:00:00 2001 From: okekefrancis112 Date: Thu, 5 Feb 2026 10:16:14 +0100 Subject: [PATCH 2/2] fix: prefer outbound SCID alias over real SCID for spliced channels With channel splicing, channels may spontaneously change their on-chain short channel IDs (SCIDs) as new funding transactions are created. However, SCID aliases remain stable across splices. Change get_outbound_payment_scid() to prefer outbound_scid_alias over short_channel_id (previously was the opposite). This ensures first-hop routing uses stable identifiers that won't change when channels are spliced. Updated tests to reflect that: - First hop uses the alias (from get_outbound_payment_scid) - Subsequent hops use real SCIDs from network gossip Closes #3268 Co-Authored-By: Claude Sonnet 4.5 --- lightning/src/ln/channel_state.rs | 6 +- lightning/src/ln/payment_tests.rs | 122 +++++++++++++--------- lightning/src/ln/priv_short_conf_tests.rs | 16 ++- lightning/src/routing/router.rs | 2 +- 4 files changed, 87 insertions(+), 59 deletions(-) diff --git a/lightning/src/ln/channel_state.rs b/lightning/src/ln/channel_state.rs index 86e53ba3262..18ccd9ffb58 100644 --- a/lightning/src/ln/channel_state.rs +++ b/lightning/src/ln/channel_state.rs @@ -499,12 +499,12 @@ 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. + /// This is either the [`ChannelDetails::outbound_scid_alias`], if set, or the + /// [`ChannelDetails::short_channel_id`]. See those for more information. /// /// [`Route`]: crate::routing::router::Route pub fn get_outbound_payment_scid(&self) -> Option { - 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. diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index f73c55fd6c6..2677d8d2547 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -247,12 +247,18 @@ fn mpp_retry_overpay() { 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. + // Find which path goes through each node (paths may be in any order). + let (path_to_b_idx, path_to_c_idx) = + if route.paths[0].hops[0].pubkey == node_b_id { (0, 1) } else { (1, 0) }; + 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 overpaid_amount_1 = + route.paths[path_to_b_idx].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 overpaid_amount_2 = + route.paths[path_to_c_idx].fee_msat() as u32 - chan_2_update.contents.fee_base_msat; let total_overpaid_amount = overpaid_amount_1 + overpaid_amount_2; @@ -301,11 +307,12 @@ fn mpp_retry_overpay() { 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(); - assert_eq!(first_path_value, 36_000_000); + // The path to node B (path_to_b_idx) succeeded with 36M; remove it so we retry only the C path. + let succeeded_path_value = route.paths[path_to_b_idx].final_value_msat(); + assert_eq!(succeeded_path_value, 36_000_000); - route.paths.remove(0); - route_params.final_value_msat -= first_path_value; + route.paths.remove(path_to_b_idx); + route_params.final_value_msat -= succeeded_path_value; let chan_4_scid = chan_4_update.contents.short_channel_id; route_params.payment_params.previously_failed_channels.push(chan_4_scid); // Check the remaining max total routing fee for the second attempt accounts only for 1_000 msat @@ -1781,13 +1788,32 @@ fn preflight_probes_yield_event() { .unwrap(); let recv_value = 50_000_000; - 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(); + let route_params = + RouteParameters::from_payment_params_and_value(payment_params.clone(), recv_value); + + // Compute the route first to determine path ordering (probe results follow route path order). + let node_0_id = nodes[0].node.get_our_node_id(); + let node_1_id = nodes[1].node.get_our_node_id(); + let usable_channels = nodes[0].node.list_usable_channels(); + let first_hops = usable_channels.iter().collect::>(); + let inflight_htlcs = nodes[0].node.compute_inflight_htlcs(); + let route = nodes[0] + .router + .router + .find_route(&node_0_id, &route_params, Some(&first_hops), inflight_htlcs) + .unwrap(); - let expected_route: &[(&[&Node], PaymentHash)] = - &[(&[&nodes[1], &nodes[3]], res[0].0), (&[&nodes[2], &nodes[3]], res[1].0)]; + // Determine which path index goes to node 1 vs node 2 + let (path_to_1_idx, path_to_2_idx) = + if route.paths[0].hops[0].pubkey == node_1_id { (0, 1) } else { (1, 0) }; - assert_eq!(res.len(), expected_route.len()); + let res = nodes[0].node.send_preflight_probes(route_params, None).unwrap(); + assert_eq!(res.len(), 2); + + let expected_route: &[(&[&Node], PaymentHash)] = &[ + (&[&nodes[1], &nodes[3]], res[path_to_1_idx].0), + (&[&nodes[2], &nodes[3]], res[path_to_2_idx].0), + ]; send_probe_along_route(&nodes[0], expected_route); @@ -2029,29 +2055,40 @@ fn test_trivial_inflight_htlc_tracking() { // Send and claim the payment. Inflight HTLCs should be empty. let (_, payment_hash, _, payment_id) = send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 500000); let inflight_htlcs = node_chanmgrs[0].compute_inflight_htlcs(); - { - let mut per_peer_lock; - let mut peer_state_lock; - let channel_1 = - get_channel_ref!(&nodes[0], nodes[1], per_peer_lock, peer_state_lock, chan_1_id); + // Inflight HTLCs are tracked by the SCID used in routes: + // - First hop uses alias via get_outbound_payment_scid() + // - Subsequent hops use the real SCID from gossip/network graph + let chan_1_scid = nodes[0] + .node + .list_channels() + .iter() + .find(|c| c.channel_id == chan_1_id) + .unwrap() + .get_outbound_payment_scid() + .unwrap(); + let chan_2_scid = nodes[1] + .node + .list_channels() + .iter() + .find(|c| c.channel_id == chan_2_id) + .unwrap() + .short_channel_id + .unwrap(); + + { 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(), + chan_1_scid, ); assert_eq!(chan_1_used_liquidity, None); } { - let mut per_peer_lock; - let mut peer_state_lock; - let channel_2 = - get_channel_ref!(&nodes[1], nodes[2], per_peer_lock, peer_state_lock, chan_2_id); - let chan_2_used_liquidity = inflight_htlcs.used_liquidity_msat( &NodeId::from_pubkey(&node_b_id), &NodeId::from_pubkey(&node_c_id), - channel_2.funding().get_short_channel_id().unwrap(), + chan_2_scid, ); assert_eq!(chan_2_used_liquidity, None); @@ -2071,29 +2108,19 @@ fn test_trivial_inflight_htlc_tracking() { route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 500000); let inflight_htlcs = node_chanmgrs[0].compute_inflight_htlcs(); { - let mut per_peer_lock; - let mut peer_state_lock; - let channel_1 = - get_channel_ref!(&nodes[0], nodes[1], per_peer_lock, peer_state_lock, chan_1_id); - 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(), + chan_1_scid, ); // First hop accounts for expected 1000 msat fee assert_eq!(chan_1_used_liquidity, Some(501000)); } { - let mut per_peer_lock; - let mut peer_state_lock; - let channel_2 = - get_channel_ref!(&nodes[1], nodes[2], per_peer_lock, peer_state_lock, chan_2_id); - let chan_2_used_liquidity = inflight_htlcs.used_liquidity_msat( &NodeId::from_pubkey(&node_b_id), &NodeId::from_pubkey(&node_c_id), - channel_2.funding().get_short_channel_id().unwrap(), + chan_2_scid, ); assert_eq!(chan_2_used_liquidity, Some(500000)); @@ -2113,28 +2140,18 @@ fn test_trivial_inflight_htlc_tracking() { let inflight_htlcs = node_chanmgrs[0].compute_inflight_htlcs(); { - let mut per_peer_lock; - let mut peer_state_lock; - let channel_1 = - get_channel_ref!(&nodes[0], nodes[1], per_peer_lock, peer_state_lock, chan_1_id); - 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(), + chan_1_scid, ); assert_eq!(chan_1_used_liquidity, None); } { - let mut per_peer_lock; - let mut peer_state_lock; - let channel_2 = - get_channel_ref!(&nodes[1], nodes[2], per_peer_lock, peer_state_lock, chan_2_id); - let chan_2_used_liquidity = inflight_htlcs.used_liquidity_msat( &NodeId::from_pubkey(&node_b_id), &NodeId::from_pubkey(&node_c_id), - channel_2.funding().get_short_channel_id().unwrap(), + chan_2_scid, ); assert_eq!(chan_2_used_liquidity, None); } @@ -2176,15 +2193,16 @@ fn test_holding_cell_inflight_htlcs() { let inflight_htlcs = node_chanmgrs[0].compute_inflight_htlcs(); { - let mut per_peer_lock; - let mut peer_state_lock; - let channel = - get_channel_ref!(&nodes[0], nodes[1], per_peer_lock, peer_state_lock, channel_id); + // Inflight HTLCs are tracked by the SCID used in routes, which is now the alias. + // Use the route's SCID (from get_outbound_payment_scid) to query inflight liquidity. + let chan_details = + nodes[0].node.list_channels().into_iter().find(|c| c.channel_id == channel_id).unwrap(); + let route_scid = chan_details.get_outbound_payment_scid().unwrap(); 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(), + route_scid, ); assert_eq!(used_liquidity, Some(2000000)); diff --git a/lightning/src/ln/priv_short_conf_tests.rs b/lightning/src/ln/priv_short_conf_tests.rs index 2035af15046..dac42bd1cb2 100644 --- a/lightning/src/ln/priv_short_conf_tests.rs +++ b/lightning/src/ln/priv_short_conf_tests.rs @@ -1117,11 +1117,12 @@ fn test_0conf_channel_reorg() { 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(); + let outbound_scid = bs_chan.get_outbound_payment_scid().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, outbound_scid); send_along_route_with_secret( &nodes[1], route.clone(), @@ -1290,12 +1291,21 @@ fn test_0conf_channel_reorg() { let onion = RecipientOnionFields::secret_only(payment_secret); let id = PaymentId([0; 32]); - nodes[1].node.send_payment_with_route(route, payment_hash, onion.clone(), id).unwrap(); + // Use the original (real) SCID in the route to test that it no longer works after the + // channel announcement propagation delay. + 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); diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index b27dee1a450..6cc3a4be8dd 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -9054,7 +9054,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); }