fix: SCID preference for outbound payments on spliced channel#4376
fix: SCID preference for outbound payments on spliced channel#4376okekefrancis112 wants to merge 1 commit intolightningdevkit:mainfrom
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.07% 86.06% -0.01%
==========================================
Files 156 156
Lines 103442 103442
Branches 103442 103442
==========================================
- Hits 89033 89024 -9
- Misses 11894 11901 +7
- Partials 2515 2517 +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?
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?
There was a problem hiding this comment.
Ok, let me work on this now. Thank you for your reviews.
okekefrancis112
force-pushed
the
prefer_outbound_scid
branch
4 times, most recently
from
5fdb327 to
05f4282
Compare
okekefrancis112
force-pushed
the
prefer_outbound_scid
branch
from
73a36ff to
5307078
Compare
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