Fix PaymentPathFailed::payment_failed_permanently on blinded path fail
#2576
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TheBlueMatt
reviewed
Sep 14, 2023
60 tasks
|
Since you're touching this code, can you also fix #2532? |
valentinewallace
force-pushed
the
2023-09-fix-outbound-bp-fail-ev
branch
from
9bde6f2 to
db052de
Compare
|
Added a commit addressing #2532 and rebased in hopes of fixing CI. |
valentinewallace
force-pushed
the
2023-09-fix-outbound-bp-fail-ev
branch
from
db052de to
697150c
Compare
Codecov ReportPatch coverage:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2576 +/- ##
==========================================
- Coverage 90.63% 88.81% -1.82%
==========================================
Files 113 113
Lines 59054 84461 +25407
Branches 59054 84461 +25407
==========================================
+ Hits 53522 75013 +21491
- Misses 5532 7243 +1711
- Partials 0 2205 +2205
☔ View full report in Codecov by Sentry. |
TheBlueMatt
reviewed
Sep 17, 2023
| network_update = Some(NetworkUpdate::ChannelFailure { | ||
| short_channel_id: route_hop.short_channel_id, | ||
| short_channel_id: failing_route_hop.short_channel_id, |
There was a problem hiding this comment.
We should remove the network update entirely. Even with perm set to false this will still mark the channel disabled even though it isn't.
There was a problem hiding this comment.
We only mark the channel as disabled if is_permanent is set. It's nice to set it because then we can more easily keep the behavior of defaulting to NetworkUpdate::NodeFailure on a totally bogus channel update.
valentinewallace
force-pushed
the
2023-09-fix-outbound-bp-fail-ev
branch
from
697150c to
9e2f95e
Compare
jkczyz
reviewed
Sep 20, 2023
valentinewallace
force-pushed
the
2023-09-fix-outbound-bp-fail-ev
branch
2 times, most recently
from
1bdc2af to
0c62e8c
Compare
This variable is ultimately for setting PaymentPathFailed::payment_failed_permanently, so use this name rather than flipping a bool back and forth.
Improves readability.
Our ultimate goal with this field is to set PaymentPathFailed::payment_failed_permanently, so use this name rather than flipping a bool back and forth across methods.
We don't support sending to paths where we are the intro node yet, but may as well set the failure correctly now.
Previously this value would be incorrectly set to true because we wouldn't account for blinded hops when determining if we were processing the last hop's failure packet.
We've run into this several times in the wild, likely due to ElementsProject/lightning#6200 wherein a node on the path will error with 0x1000 but not provide a channel update (a spec violation). Previously, we would blame the inbound edge even though the buggy peer wanted us to blame the outbound edge. Since this issue seems to be recurring and our blaming the inbound edge is causing us to punish innocent channels, trust the peer that the outbound edge is the one to blame.
valentinewallace
force-pushed
the
2023-09-fix-outbound-bp-fail-ev
branch
from
0c62e8c to
6299f7d
Compare
jkczyz
approved these changes
Sep 25, 2023
TheBlueMatt
approved these changes
Sep 25, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously this value would be incorrectly set to true because we wouldn't
account for blinded hops when determining if we were processing the last hop's
failure packet.
This is tested in the follow-up. I'm not ready to update that PR yet because I'm still reworking the commit history, but I have a WIP branch if anyone wants to verify the test coverage.
Includes a few minor cleanups of
onion_utils::process_onion_failureand friends.Also closes #2532.