fix: SCID preference for outbound payments on spliced channel #4376
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
TheBlueMatt
requested review from
TheBlueMatt
and removed request for
wpaulino
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Please fix rustfmt.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4376 +/- ##
=======================================
Coverage 86.01% 86.02%
=======================================
Files 156 156
Lines 102857 102857
Branches 102857 102857
=======================================
+ Hits 88474 88480 +6
+ Misses 11876 11868 -8
- Partials 2507 2509 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@TheBlueMatt, please, are all the checks needed to pass before my work gets reviewed?. Just asking, because while the checks are happening, maybe a new commit is added to main that causes a conflict, and before you know it, my check here is failing. |
okekefrancis112
force-pushed
the
prefer_outbound_scid
branch
from
30bf388 to
e00baa1
Compare
|
✅ Added second reviewer: @joostjager |
okekefrancis112
force-pushed
the
prefer_outbound_scid
branch
2 times, most recently
from
f6dd57d to
e00baa1
Compare
|
All checks pass now. @TheBlueMatt and @joostjager |
|
|
||
| // 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; |
There was a problem hiding this comment.
I think there is some independent refactoring/correctness improvement here that could go in a separate commit?
The commit that makes the actual change can then also contain the test changes/additions that it requires.
There was a problem hiding this comment.
Alright, I will work on it and get back to you. @joostjager
There was a problem hiding this comment.
Hi @joostjager, I have made the fix and pushed. Can you help me review?
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 <noreply@anthropic.com>
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 lightningdevkit#3268 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
okekefrancis112
force-pushed
the
prefer_outbound_scid
branch
from
e00baa1 to
a153dbe
Compare
|
You don't need to tag people to ask for review so urgently. There's a lot going on on the project, you can hit the review request button and people will get to it when they have a chance. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
basically lgtm, one nit otherwise great, thanks!
| // 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; |
There was a problem hiding this comment.
Rather than pulling the expected scid value out of the route, can you instead pull the value out of the channel list?
4 participants
Summary
Spliced channels can cause real SCIDs to change unexpectedly, which breaks the
assumption that they are more stable than SCID aliases. This PR corrects the
outbound payment SCID selection logic to reflect this reality.
Details
ChannelDetails::get_outbound_payment_scidnow prefers SCID aliases.This aligns outbound routing with current splicing behavior.
Tests that depended on the old assumption were updated accordingly.
Closes: #4249