Uh oh!
There was an error while loading. Please reload this page.
Try to overpay the recipient if we fail to find a path at all and limit overpay - #2604
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
valentinewallace
commented
Sep 27, 2023
Needs a rebase on #2575 to fix CI. |
Uh oh!
There was an error while loading. Please reload this page.
| channel_saturation_pow_half = 0; | ||
| } else if already_collected_value_msat < final_value_msat && path_value_msat != recommended_value_msat && !found_new_path { | ||
| log_trace!(logger, "Failed to collect enough value, but running again to collect extra paths with a potentially higher limit."); | ||
| path_value_msat = recommended_value_msat; |
There was a problem hiding this comment.
Should we only do this if we hit_minimum_limit? It doesn't hurt to go around again, but isn't hit_min_limit the only way for there to be uncollected paths remaining?
There was a problem hiding this comment.
Mmm, fair, yea, I can add that restriction.
50633ed to
ae9df4aCompareUh oh!
There was an error while loading. Please reload this page.
tnull
left a comment
There was a problem hiding this comment.
Feel free to squash fixups (when rebasing).
ae9df4a to
bbdf4b9CompareSquashed without change - it shouldn't need rebase as its based directly on #2575, as long as that gets merged without any change in the commits git[hub] is smart enough to ignore it. |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@## main #2604 +/- ##
==========================================
+ Coverage 89.00% 89.04% +0.03%
==========================================
Files 112 112 Lines 86273 86337 +64 Branches 86273 86337 +64 ==========================================
+ Hits 76791 76876 +85 + Misses 7248 7230 -18 + Partials 2234 2231 -3
☔ View full report in Codecov by Sentry. |
The merge-base changed after approval.
TheBlueMatt
commented
Sep 28, 2023
Lollll that's new. Sorry, have to re-ack. |
The merge-base changed after approval.
TheBlueMatt
commented
Sep 28, 2023
Ugh wtf. Okay I'm gonna have to change ack policy wtf. |
While this doesn't matter much in practice, if we go around again when route-finding to try to meet an htlc_minimum_msat, we use the `recommended_value_msat` which can work if we meet the `htlc_minimum_msat` on a channel exactly, so using >= rather than > can capture cases with 1msat more.
Previously we'd only try to overpay if we managed to find a path to the recipient which was sufficient. However, if we fail to find any path to the recipient at all we should still retry overpaying the recipient. Ultimately we should be silling to pay whatever reasonable performance penalty if the alternative is not finding a path at all, which we do here.
If the user told us to limit their total fee exposure, we should do so including any potential overpayment to the recipient, which is ultimately a part of the "fee" as far as the user is concerned.
This may be useful in debugging routing failures in the future.
bbdf4b9 to
fa48df6CompareTheBlueMatt
commented
Sep 28, 2023
Rebased without change, I think now maybe it wont drop reviews any time there's a merge. |
TheBlueMatt
commented
Sep 28, 2023
As usual, there is no setting anywhere that seems to line up with what just happened, so I guess we just have to require it be based on an upstream merge commit 🤷 |
Previously we'd only try to overpay if we managed to find a path
to the recipient which was sufficient. However, if we fail to find
any path to the recipient at all we should still retry overpaying
the recipient. Ultimately we should be silling to pay whatever
reasonable performance penalty if the alternative is not finding a
path at all, which we do here.
Depends on #2575.