Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pay: fix re-adding payment amount back to estimated capacity #7188

Merged

Conversation

JssDWt
Copy link
Contributor

@JssDWt JssDWt commented Mar 31, 2024

Fixes #7177

Below lines of code failed to actually re-add the amount back to the channel hint's estimated capacity.

if (remove && !amount_msat_add(
&curhint->estimated_capacity,
curhint->estimated_capacity,
curhop->amount)) {

I'm not sure why tbh, it seems like a reasonable piece of code. However, it made the corresponding test (added in this PR) fail, because the channel hint's estimated capacity of the sender's own channel was below the amount he was trying to send. I tested this by logging the channel_hints before and after the operation in those lines. When substracting the amount (remove == false) the estimated capacity of the sender's channel after the operation was smaller than before the operation. When adding the amount back (remove == true) the estimated capacity of the sender's channel after the operation was equal to what it was before the operation.

The consequence of this was that the estimated channel capacity in the sender's own channel was ever decreasing during the pay lifecycle.

@JssDWt
Copy link
Contributor Author

JssDWt commented Mar 31, 2024

Might fix #6793
Might fix #5259

@cdecker
Copy link
Member

cdecker commented Mar 31, 2024

Very interesting, I don't have an explanation either, because afaik the evaluation order for boolean expressions is left-to-right and the function should only be called if remove == true, but then again I'm no compiler expert, and I might be overlooking something.

@rustyrussell
Copy link
Contributor

This is brilliant!

The old code:

if (remove && add_overflows(a, &b)) { FAIL }
else if (sub_overflows(a, &b) { FAIL }

So, unless the add overflowed, it immediately subtracted again!

This whole function is problematic, but your fix is minimal. I'll build a cleanup PR on top...

@rustyrussell
Copy link
Contributor

Oh, can you add a Changelog-Fixed line to the first commit message please? e.g.

Changelog-Fixed: Plugins: pay now correctly estimates channel capacity after payment failure, fixing some retry cases.

@rustyrussell rustyrussell added this to the v24.05 milestone Apr 2, 2024
@JssDWt
Copy link
Contributor Author

JssDWt commented Apr 2, 2024

So, unless the add overflowed, it immediately subtracted again!

Oh wow I can't believe I didn't see that!

@JssDWt JssDWt force-pushed the jssdwt-fix-removing-channel-hints branch from 5f5696c to 524ef81 Compare April 2, 2024 14:46
Changelog-Fixed: Plugins: pay now correctly estimates channel capacity
after payment failure, fixing some retry cases.

The `estimated_capacity` was properly substracted from the channel
hint, but adding the amount back did not work. The amount was added back
and then immediately substracted again. This caused the
`estimated_capacity` to ever decrease. This commit makes sure re-adding
the amount to the `estimated_capacity` works as expected.
Tests whether we're able to route around a low-fee channel in the graph. We
should be able to find the more expensive route if the cheaper route is
depleted.
@JssDWt JssDWt force-pushed the jssdwt-fix-removing-channel-hints branch from 524ef81 to 8433c27 Compare April 2, 2024 14:49
@JssDWt
Copy link
Contributor Author

JssDWt commented Apr 2, 2024

@rustyrussell added that text to the commit message and rebased

@rustyrussell rustyrussell merged commit 09d6bb9 into ElementsProject:master Apr 4, 2024
35 checks passed
@JssDWt JssDWt deleted the jssdwt-fix-removing-channel-hints branch April 5, 2024 08:27
@cdecker cdecker mentioned this pull request Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pay plugin fails to route around depleted low fee channels
3 participants