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/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( 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); }