Prefer outbound SCID alias over real SCID when routing by Alkamal01 · Pull Request #4567 · lightningdevkit/rust-lightning

Alkamal01 · 2026-04-14T10:25:28Z

With splices, channels may spontaneously change SCIDs, making our assumption that SCIDs were more stable than SCID aliases in ChannelDetails::get_outbound_payment_scid wrong. This swaps which we prefer, and fixes up the tests that break as a result.

ldk-reviews-bot · 2026-04-14T10:25:31Z

👋 I see @wpaulino was un-assigned.
If you'd like another reviewer assignment, please click here.

ldk-claude-review-bot · 2026-04-14T10:34:55Z

 no-std-check/target
 msrv-no-dev-deps-check/target
 lightning-tests/target
+.claude


Nit: This .claude gitignore entry is unrelated to the SCID alias change. Consider dropping it from this PR and submitting it separately (or as part of a repo-housekeeping commit).

ldk-claude-review-bot · 2026-04-14T10:34:56Z

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

+	assert_eq!(res.len(), 2);


assert_eq!(res.len(), 2) is asserted here and again on line 1814 (assert_eq!(res.len(), expected_route.len())). The first one is redundant — consider removing one of them.

ldk-claude-review-bot · 2026-04-14T10:37:02Z

I've completed a thorough review of every file and hunk in this PR. All prior review comments have been addressed or are no longer applicable (the .gitignore change is gone, and the doc comments were updated correctly).

No new issues found.

The core logic change (outbound_scid_alias.or(short_channel_id) instead of short_channel_id.or(outbound_scid_alias)) is correct and well-motivated by splice stability. All test adjustments are consistent with the new behavior:

router.rs:9181 — correctly updated from 1 to 44 because the third sub-test's first hop now resolves to alias 44 instead of real SCID 1
payment_tests.rs — MPP and inflight tests correctly identify paths by pubkey or alias instead of assuming index order
priv_short_conf_tests.rs — explicitly constructs old-SCID routes for invalidation testing, since routes now naturally use the alias
splicing_tests.rs — fixes a latent bug where get_route_and_payment_hash! was called from the wrong node's perspective (masked before because both sides shared the same real SCID, but exposed now that each side uses its own alias)
onion_route_tests.rs — uses the route's SCID (now the alias) for failure matching

TheBlueMatt

Thanks! One nit about comments but otherwise LGTM.

TheBlueMatt · 2026-04-14T13:24:30Z

 	// 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;
+	// get_outbound_payment_scid now prefers the alias SCID for the first hop, so the


claude loves to leave comments that describe what it did, describing things in terms of what it is changing and how things used to be, but that isn't actually a good comment. I'm not sure this needs a comment, but if it does it shouldn't be phrased as "X now does" but rather simply "The SCID in the failure should use the SCID alias as we specified in the route". This class of issues is repeated in a few places.

ldk-claude-review-bot · 2026-04-14T14:55:53Z

 	///
-	/// 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`]. The alias is preferred because with splices,
+	/// the real SCID can change when a splice confirms, whereas the alias remains stable.


Nit: The docstrings on short_channel_id (line 315-316) and outbound_scid_alias (lines 328-331) are now stale. They still say the alias is only used "in place of" the real SCID for 0-conf channels, but with this change the alias is now the preferred identifier for all outbound routes, not just a 0-conf fallback. Consider updating them to match the new semantics described here.

ldk-claude-review-bot · 2026-04-14T15:23:03Z

+	// Send a payment using the channel's alias SCID, which will be public in a few blocks once
+	// we can generate a channel_announcement.


Nit: The comment now reads "Send a payment using the channel's alias SCID, which will be public in a few blocks…" — but the alias SCID itself never becomes public. It's the channel that becomes public (announced via the real SCID in channel_announcement). The original wording ("real SCID, which will be public") was accurate for the real SCID; consider rewording to avoid implying the alias is what gets announced:

Suggested change

// Send a payment using the channel's alias 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.

codecov · 2026-04-14T17:06:57Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.00%. Comparing base (2ebc372) to head (841d912).
⚠️ Report is 36 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4567      +/-   ##
==========================================
- Coverage   87.01%   87.00%   -0.02%     
==========================================
  Files         163      163              
  Lines      108682   108742      +60     
  Branches   108682   108742      +60     
==========================================
+ Hits        94572    94613      +41     
- Misses      11631    11648      +17     
- Partials     2479     2481       +2

Flag	Coverage Δ
fuzzing	`40.19% <100.00%> (-0.04%)`	⬇️
tests	`86.09% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

TheBlueMatt · 2026-04-23T14:11:34Z

 	///
-	/// 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`].
+	/// [`outbound_scid_alias`] is preferred over this for outbound routes when set, as it remains


Just saying "X is preferred over this" is a bit of a weird doc for a POD struct field. THe comment before wasn't great either but let's be more explicit about why/when/how the other field is "preferred". Same below.

TheBlueMatt · 2026-04-23T14:48:38Z

+	// outbound alias, which is what acceptor's channel lookup uses.
 	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);


This doesn't make any sense? Finding a route from the payee to the payer is going to result in the payment immediately failing cause we're trying to pay ourselves. That isn't solving the isue its just breaking the test.

Good catch, that was a mistake on my side.
During the rebase, some unrelated changes slipped in (the test_splice_rbf_insufficient_feerate_high removal and div_ceil changes). I’ve reverted those.

On the routing issue: the problem wasn’t that we should route from payee to payer. The bug was that I was building the route from the initiator’s perspective, even though in this case the acceptor is the sender. That mismatch is what caused the confusion.

I’ve fixed it so the route is now constructed from the acceptor’s perspective, which keeps the direction correct and makes the test behave as intended.

I also updated the comment in onion_route_tests.rs to clarify the behavior.

…ent_scid With splicing, the real SCID changes when a splice confirms while the outbound_scid_alias remains stable. Prefer alias-first in get_outbound_payment_scid so routes built before a splice confirmation stay valid after. Also fix route direction in fail_splice_on_tx_complete_error and update onion_route_tests comment to state intent rather than describe change.

ldk-claude-review-bot reviewed Apr 14, 2026

View reviewed changes

ldk-reviews-bot requested a review from wpaulino April 14, 2026 10:35

Alkamal01 force-pushed the prefer-outbound-scid-alias-for-routing branch from b5156c6 to 181df48 Compare April 14, 2026 10:40

TheBlueMatt reviewed Apr 14, 2026

View reviewed changes

TheBlueMatt removed the request for review from wpaulino April 14, 2026 13:38

Alkamal01 force-pushed the prefer-outbound-scid-alias-for-routing branch from 181df48 to 1ee9685 Compare April 14, 2026 14:52

ldk-claude-review-bot reviewed Apr 14, 2026

View reviewed changes

Alkamal01 force-pushed the prefer-outbound-scid-alias-for-routing branch from 1ee9685 to c49f1b4 Compare April 14, 2026 15:18

ldk-claude-review-bot reviewed Apr 14, 2026

View reviewed changes

Alkamal01 force-pushed the prefer-outbound-scid-alias-for-routing branch from c49f1b4 to 841d912 Compare April 14, 2026 15:36

TheBlueMatt reviewed Apr 23, 2026

View reviewed changes

Alkamal01 force-pushed the prefer-outbound-scid-alias-for-routing branch from e14566e to f8f4404 Compare April 23, 2026 18:02

Alkamal01 force-pushed the prefer-outbound-scid-alias-for-routing branch from f8f4404 to b91dd2d Compare April 23, 2026 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer outbound SCID alias over real SCID when routing#4567

Prefer outbound SCID alias over real SCID when routing#4567
Alkamal01 wants to merge 1 commit intolightningdevkit:mainfrom
Alkamal01:prefer-outbound-scid-alias-for-routing

Alkamal01 commented Apr 14, 2026

Uh oh!

ldk-reviews-bot commented Apr 14, 2026 •

edited

Loading

Uh oh!

ldk-claude-review-bot Apr 14, 2026

Uh oh!

ldk-claude-review-bot Apr 14, 2026

Uh oh!

ldk-claude-review-bot commented Apr 14, 2026 •

edited

Loading

Uh oh!

TheBlueMatt left a comment

Uh oh!

TheBlueMatt Apr 14, 2026

Uh oh!

ldk-claude-review-bot Apr 14, 2026

Uh oh!

ldk-claude-review-bot Apr 14, 2026

Uh oh!

codecov Bot commented Apr 14, 2026 •

edited

Loading

Uh oh!

TheBlueMatt Apr 23, 2026

Uh oh!

TheBlueMatt Apr 23, 2026

Uh oh!

Alkamal01 Apr 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		// Send a payment using the channel's alias SCID, which will be public in a few blocks once
		// we can generate a channel_announcement.

Conversation

Alkamal01 commented Apr 14, 2026

Uh oh!

ldk-reviews-bot commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-claude-review-bot commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ldk-reviews-bot commented Apr 14, 2026 •

edited

Loading

ldk-claude-review-bot commented Apr 14, 2026 •

edited

Loading

codecov Bot commented Apr 14, 2026 •

edited

Loading