Routing: reduce impact of second chance #10278
Conversation
Previously our mission control would not punish a route if a policy failure was provided to us, in the form of a "second chance". For various reasons outlined in lightningnetwork#6883 we may not want to blindly retry a route if that case is true. With this commit we change the effect of the second chance to instead apply penalties with half the value. This way we do not exclude this route from future attempts and also encourage alternative route selection.
We add a test to demonstrate that the second chance penalty can be effective. In this test an initially cheap route is attempted but errors with a new returned channel policy. After applying that policy we should pick the cheaper route again, but the second-chance penalty will prevent us from doing so. Instead an alternative route succeeds.
GeorgeTsagk
added
privacy
Summary of ChangesHello @GeorgeTsagk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This PR modifies the routing logic to apply a 'half penalty' instead of an immediate retry when a payment fails with a fee policy update and is granted a second chance. This change aims to improve route selection by encouraging the use of alternative paths first, while still allowing the original route to be retried later. It also enhances privacy. The changes involve adjusting how failures are recorded in Mission Control and are validated with updated and new test cases. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement to the routing logic by applying a half-penalty for payment failures that include a fee policy update. This change discourages immediate retries on the same route, promoting the exploration of alternative paths and enhancing user privacy. The implementation is clever, using a timestamp shift to achieve the half-penalty effect.
I've identified one high-severity issue where the half-penalty is incorrectly applied to successfully forwarded hops, not just the failing one. This could negatively impact future pathfinding. A fix is suggested below.
The new tests are well-designed and effectively demonstrate the intended behavior of the new logic. Overall, this is a great enhancement to the routing mechanism.
| // full penalty in order to not totally exclude this route from | ||
| // future attempts, but also encourage alternative route | ||
| // selection. | ||
| if secondChance { |
There was a problem hiding this comment.
The half-penalty should only be applied to the failing channel pair, not to the pairs that successfully forwarded the HTLC. Currently, if secondChance is true, the timestamp is adjusted for all pairs in the route, including successful ones. This incorrectly records successful forwards with a timestamp in the past, which could negatively impact future pathfinding.
The logic should be adjusted to only apply the half-penalty to the failing pair.
| if secondChance { | |
| if secondChance && !pairResult.success { |
There was a problem hiding this comment.
I believe this check is unnecessary as a second chance is only granted upon failure.
1 participant
Description
This PR changes the impact of granting second chances to payment failures that return a fee policy update. Previously we'd immediately (within a min interval) retry a route if that was the case, but that might not be ideal (see #6883 for more info).
Instead we apply a half penalty, which discourages immediate future attempts over the same route. Other routes with greater success probability will be attempted as the penalty is not absolute but still severe. If alternative routes prove ineffective then the same route may be attempted again. It's important to be able to eventually retry the penalized route as it may be the only route available to reach the destination.
Apart from the reasons included in the issue linked above, avoiding immediate route retries is good for privacy, as it is a mitigation against an on-path adversary trying analyze time deltas of HTLCs for the purpose of locating the sender / receiver in the graph.
Testing
The weight of the penalty is not too harsh in order to allow for previously captured test cases to still pass. A new test was also added which demonstrates how alternative routes can be picked.
Refs
Closes #6883 (since it addresses all the concerns outlined in that issue)
Replaces #6891