Avoid saturating channels before we split payments #1605
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1605 +/- ##
==========================================
+ Coverage 90.86% 91.23% +0.37%
==========================================
Files 80 80
Lines 44437 47610 +3173
Branches 44437 47610 +3173
==========================================
+ Hits 40377 43439 +3062
- Misses 4060 4171 +111
Continue to review full report at Codecov.
|
| @@ -233,6 +243,7 @@ impl_writeable_tlv_based!(PaymentParameters, { | |||
| (2, features, option), | |||
| (3, max_path_count, (default_value, DEFAULT_MAX_PATH_COUNT)), | |||
| (4, route_hints, vec_type), | |||
| (5, max_channel_saturation_power_of_half, (default_value, 1)), | |||
There was a problem hiding this comment.
Curious why 5 was skipped before :)
There was a problem hiding this comment.
Odd and even numbers have different semantics (odds are ignored if an old version doesnt understand it, evens cause a read failure) so they aren't interchangeable.
There was a problem hiding this comment.
Ah of course, part of the "okay to be odd" rule.
TheBlueMatt
force-pushed
the
2022-07-smaller-mpp-parts
branch
from
de3e19e to
dc3447a
Compare
|
Concept ACK |
| /// capacity, a value of one will only use up to half its capacity, two 1/4, etc. | ||
| /// | ||
| /// Default value: 1 | ||
| pub max_channel_saturation_power_of_half: u8, |
There was a problem hiding this comment.
I think
There was a problem hiding this comment.
Cause I really hate the idea of doing a divide in the scorer. Divides are slow, man, shifts are cheap. Quite likely it doesn't actually matter, but still.
There was a problem hiding this comment.
Fair enough, I assumed this to be the main reason for it. While there might be ways to work around the floating point division, your solution is probably cleaner, even though it has the drawback that no saturation between 0.5 and 1.0 times the capacity can be specified.
There was a problem hiding this comment.
Yea, if anything I'm more worried about nothing between 1/8 and 1/4 than 1/2 and 1 :)
|
I believe this preliminarily addresses #1276? Linking for now. |
|
Indeed. Dunno if we should close #1276 after this or not, but its at least a huge step towards it. |
TheBlueMatt
force-pushed
the
2022-07-smaller-mpp-parts
branch
from
dc3447a to
6a60020
Compare
TheBlueMatt
force-pushed
the
2022-07-smaller-mpp-parts
branch
from
6a60020 to
e8950ac
Compare
TheBlueMatt
force-pushed
the
2022-07-smaller-mpp-parts
branch
from
e8950ac to
d2a8906
Compare
Currently we only opt to split a payment into an MPP if we have completely and totally used a channel's available capacity (up to the announced htlc_max or on-chain capacity, whichever is lower). This is obviously destined to fail as channels are unlikely to have their full capacity available. Here we do the minimum viable fix by simply limiting channels to only using up to a configurable power-of-1/2. We default this new configuration knob to 1 (1/2 of the channel) so as to avoid a substantial change but in the future we may consider changing this to 2 (1/4) or even 3 (1/8).
In order to avoid failing to find paths due to the new channel saturation limit, if we fail to find enough paths, we simply disable the saturation limit for further path finding iterations. Because we can now increase the maximum sent over a given channel during routefinding, we may now generate redundant paths for the same payment. Because this is wasteful in the network, we add an additional pass during routefinding to merge redundant paths. Note that two tests which previously attempted to send exactly the available liquidity over a channel which charged an absolute fee need updating - in those cases the router will first collect a path that is saturation-limited, then attempt to collect a second path without a saturation limit while stil honoring the existing utilized capacity on the channel, causing failure as the absolute fee must be included.
TheBlueMatt
force-pushed
the
2022-07-smaller-mpp-parts
branch
from
d2a8906 to
e6d40a7
Compare
|
Squashed without further changes. |
Currently we only opt to split a payment into an MPP if we have
completely and totally used a channel's available capacity (up to
the announced htlc_max or on-chain capacity, whichever is lower).
This is obviously destined to fail as channels are unlikely to have
their full capacity available.
Here we do the minimum viable fix by simply limiting channels to
only using up to a configurable power-of-1/2. We default this new
configuration knob to 1 (1/2 of the channel) so as to avoid a
substantial change but in the future we may consider changing this
to 2 (1/4) or even 3 (1/8).
In a (somewhat optional, though I'd prefer it) second commit, we ensure we don't spuriously fail to find a route just because of the saturation limit, opting to remove the saturation limit during pathfinding if we stop finding new paths. This at least notably means we don't have to have a ton of test changes, as it makes the behavior almost entirely backwards compatible even across various edge cases.