Try to overpay the recipient if we fail to find a path at all and limit overpay #2604
Conversation
|
Needs a rebase on #2575 to fix CI. |
| @@ -2398,6 +2398,9 @@ where L::Target: Logger { | |||
| // because we deterministically terminated the search due to low liquidity. | |||
| if !found_new_path && channel_saturation_pow_half != 0 { | |||
| 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.
TheBlueMatt
force-pushed
the
2023-09-route-overpay-limit
branch
3 times, most recently
from
50633ed to
ae9df4a
Compare
TheBlueMatt
force-pushed
the
2023-09-route-overpay-limit
branch
from
ae9df4a to
bbdf4b9
Compare
|
Squashed 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. |
TheBlueMatt
dismissed
tnull’s stale review
The merge-base changed after approval.
Lollll that's new. Sorry, have to re-ack. |
TheBlueMatt
dismissed
tnull’s stale review
The merge-base changed after approval.
|
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.
TheBlueMatt
force-pushed
the
2023-09-route-overpay-limit
branch
from
bbdf4b9 to
fa48df6
Compare
|
Rebased without change, I think now maybe it wont drop reviews any time there's a merge. |
|
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.