Real getroutes fixes #7600
Merged
Real getroutes fixes #7600
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
rustyrussell
force-pushed
the
guilt/getroutes-fixes
branch
from
d995105 to
6f0d8b7
Compare
ShahanaFarooqui
force-pushed
the
guilt/getroutes-fixes
branch
from
6f0d8b7 to
cb342e6
Compare
…name. Based on great test case from https://github.com/daywalker90 ``` E AssertionError: assert {'107x2x0/1': 'Path total 285720859 > spendable 285718000', '108x1x0/1': 'Path total 384721849 > spendable 384718000'} == {} E Left contains 2 more items: E {'107x2x0/1': 'Path total 285720859 > spendable 285718000', E '108x1x0/1': 'Path total 384721849 > spendable 384718000'} E Full diff: E { E - , E + '107x2x0/1': 'Path total 285720859 > spendable 285718000', E + '108x1x0/1': 'Path total 384721849 > spendable 384718000', E } ``` Signed-off-by: Rusty Russell <[email protected]>
Conversion is lossy, and we don't want to spend more than the channel, so it's conservative to round down here. This doesn't actually help our test though! Signed-off-by: Rusty Russell <[email protected]>
|
Yes! We need to consider the cap on the first segment as well. |
Lagrang3
reviewed
Aug 22, 2024
|
CI is breaking at |
Lagrang3
approved these changes
Aug 22, 2024
rustyrussell
force-pushed
the
guilt/getroutes-fixes
branch
from
69084ef to
6359bbb
Compare
It seems we didn't handle it correctly: we need to cap the first segment as well as the others, as far as I can tell. Also, it can be less than the maximum capacity. Signed-off-by: Rusty Russell <[email protected]>
Of course, we still will, since spendable is for a single HTLC, but this also shows why we should treat *minimum* as the incorrect answer if they cross, too. Signed-off-by: Rusty Russell <[email protected]> Fixes: ElementsProject#7563
Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
guilt/getroutes-fixes
branch
from
6359bbb to
e893b74
Compare
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.
This actually fixed the issue pointed out by @daywalker90
It's a bit of a hack: I'm assuming that we have some cumulative overflow in our calculations somewhere, as we do use sats not millisats for efficiency.