Avoid saturating channels before we split payments by TheBlueMatt · Pull Request #1605 · lightningdevkit/rust-lightning

TheBlueMatt · 2022-07-09T04:29:59Z

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.

codecov-commenter · 2022-07-09T04:43:47Z

Codecov Report

Merging #1605 (d2a8906) into main (4e5f74a) will increase coverage by 0.37%.
The diff coverage is 95.87%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/routing/gossip.rs	`91.72% <ø> (-0.23%)`	⬇️
lightning/src/routing/router.rs	`92.52% <95.45%> (+0.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.80% <100.00%> (-0.31%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.88% <100.00%> (ø)`
lightning/src/util/events.rs	`39.25% <0.00%> (-0.29%)`	⬇️
lightning/src/ln/channel.rs	`89.41% <0.00%> (+0.66%)`	⬆️
lightning/src/routing/scoring.rs	`97.56% <0.00%> (+1.49%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e5f74a...d2a8906. Read the comment docs.

dunxen · 2022-07-09T07:13:08Z

 	(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)),


Curious why 5 was skipped before :)

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.

Ah of course, part of the "okay to be odd" rule.

moneyball · 2022-07-09T23:14:29Z

Concept ACK

tnull · 2022-07-11T10:50:01Z

+	/// 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,


I think $\frac{1}{2}^x$ should generally work fine, but why don't we allow to just define a simple percentage or share $x \in [0,1]$ of the channel capacity here? Seems more straight forward and probably also more readable when not familiar with how this knob works?

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.

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.

Yea, if anything I'm more worried about nothing between 1/8 and 1/4 than 1/2 and 1 :)

tnull · 2022-07-11T13:31:01Z

I believe this preliminarily addresses #1276? Linking for now.

TheBlueMatt · 2022-07-11T15:53:01Z

Indeed. Dunno if we should close #1276 after this or not, but its at least a huge step towards it.

wpaulino

First time reviewing this part of the codebase so you may want a more reassuring ACK from someone else. LGTM though, feel free to squash.

tnull

LGTM

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 · 2022-07-14T15:30:00Z

Squashed without further changes.

TheBlueMatt · 2022-07-09T04:29:59Z

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.

codecov-commenter · 2022-07-09T04:43:47Z

Codecov Report

Merging #1605 (d2a8906) into main (4e5f74a) will increase coverage by 0.37%.
The diff coverage is 95.87%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/routing/gossip.rs	`91.72% <ø> (-0.23%)`	⬇️
lightning/src/routing/router.rs	`92.52% <95.45%> (+0.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.80% <100.00%> (-0.31%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.88% <100.00%> (ø)`
lightning/src/util/events.rs	`39.25% <0.00%> (-0.29%)`	⬇️
lightning/src/ln/channel.rs	`89.41% <0.00%> (+0.66%)`	⬆️
lightning/src/routing/scoring.rs	`97.56% <0.00%> (+1.49%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e5f74a...d2a8906. Read the comment docs.

dunxen · 2022-07-09T07:13:08Z

 	(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)),


Curious why 5 was skipped before :)

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.

Ah of course, part of the "okay to be odd" rule.

moneyball · 2022-07-09T23:14:29Z

Concept ACK

tnull · 2022-07-11T10:50:01Z

+	/// 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,


I think $\frac{1}{2}^x$ should generally work fine, but why don't we allow to just define a simple percentage or share $x \in [0,1]$ of the channel capacity here? Seems more straight forward and probably also more readable when not familiar with how this knob works?

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.

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.

Yea, if anything I'm more worried about nothing between 1/8 and 1/4 than 1/2 and 1 :)

tnull · 2022-07-11T13:31:01Z

I believe this preliminarily addresses #1276? Linking for now.

TheBlueMatt · 2022-07-11T15:53:01Z

Indeed. Dunno if we should close #1276 after this or not, but its at least a huge step towards it.

wpaulino

First time reviewing this part of the codebase so you may want a more reassuring ACK from someone else. LGTM though, feel free to squash.

tnull

LGTM

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 · 2022-07-14T15:30:00Z

Squashed without further changes.

TheBlueMatt · 2022-07-09T04:29:59Z

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.

codecov-commenter · 2022-07-09T04:43:47Z

Codecov Report

Merging #1605 (d2a8906) into main (4e5f74a) will increase coverage by 0.37%.
The diff coverage is 95.87%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/routing/gossip.rs	`91.72% <ø> (-0.23%)`	⬇️
lightning/src/routing/router.rs	`92.52% <95.45%> (+0.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.80% <100.00%> (-0.31%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.88% <100.00%> (ø)`
lightning/src/util/events.rs	`39.25% <0.00%> (-0.29%)`	⬇️
lightning/src/ln/channel.rs	`89.41% <0.00%> (+0.66%)`	⬆️
lightning/src/routing/scoring.rs	`97.56% <0.00%> (+1.49%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e5f74a...d2a8906. Read the comment docs.

dunxen · 2022-07-09T07:13:08Z

 	(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)),


Curious why 5 was skipped before :)

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.

Ah of course, part of the "okay to be odd" rule.

moneyball · 2022-07-09T23:14:29Z

Concept ACK

tnull · 2022-07-11T10:50:01Z

+	/// 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,


I think $\frac{1}{2}^x$ should generally work fine, but why don't we allow to just define a simple percentage or share $x \in [0,1]$ of the channel capacity here? Seems more straight forward and probably also more readable when not familiar with how this knob works?

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.

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.

Yea, if anything I'm more worried about nothing between 1/8 and 1/4 than 1/2 and 1 :)

tnull · 2022-07-11T13:31:01Z

I believe this preliminarily addresses #1276? Linking for now.

TheBlueMatt · 2022-07-11T15:53:01Z

Indeed. Dunno if we should close #1276 after this or not, but its at least a huge step towards it.

wpaulino

First time reviewing this part of the codebase so you may want a more reassuring ACK from someone else. LGTM though, feel free to squash.

tnull

LGTM

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 · 2022-07-14T15:30:00Z

Squashed without further changes.

TheBlueMatt · 2022-07-09T04:29:59Z

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.

codecov-commenter · 2022-07-09T04:43:47Z

Codecov Report

Merging #1605 (d2a8906) into main (4e5f74a) will increase coverage by 0.37%.
The diff coverage is 95.87%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/routing/gossip.rs	`91.72% <ø> (-0.23%)`	⬇️
lightning/src/routing/router.rs	`92.52% <95.45%> (+0.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.80% <100.00%> (-0.31%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.88% <100.00%> (ø)`
lightning/src/util/events.rs	`39.25% <0.00%> (-0.29%)`	⬇️
lightning/src/ln/channel.rs	`89.41% <0.00%> (+0.66%)`	⬆️
lightning/src/routing/scoring.rs	`97.56% <0.00%> (+1.49%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e5f74a...d2a8906. Read the comment docs.

dunxen · 2022-07-09T07:13:08Z

 	(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)),


Curious why 5 was skipped before :)

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.

Ah of course, part of the "okay to be odd" rule.

moneyball · 2022-07-09T23:14:29Z

Concept ACK

tnull · 2022-07-11T10:50:01Z

+	/// 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,


I think $\frac{1}{2}^x$ should generally work fine, but why don't we allow to just define a simple percentage or share $x \in [0,1]$ of the channel capacity here? Seems more straight forward and probably also more readable when not familiar with how this knob works?

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.

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.

Yea, if anything I'm more worried about nothing between 1/8 and 1/4 than 1/2 and 1 :)

tnull · 2022-07-11T13:31:01Z

I believe this preliminarily addresses #1276? Linking for now.

TheBlueMatt · 2022-07-11T15:53:01Z

Indeed. Dunno if we should close #1276 after this or not, but its at least a huge step towards it.

wpaulino

First time reviewing this part of the codebase so you may want a more reassuring ACK from someone else. LGTM though, feel free to squash.

tnull

LGTM

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 · 2022-07-14T15:30:00Z

Squashed without further changes.

TheBlueMatt · 2022-07-09T04:29:59Z

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.

codecov-commenter · 2022-07-09T04:43:47Z

Codecov Report

Merging #1605 (d2a8906) into main (4e5f74a) will increase coverage by 0.37%.
The diff coverage is 95.87%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/routing/gossip.rs	`91.72% <ø> (-0.23%)`	⬇️
lightning/src/routing/router.rs	`92.52% <95.45%> (+0.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.80% <100.00%> (-0.31%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.88% <100.00%> (ø)`
lightning/src/util/events.rs	`39.25% <0.00%> (-0.29%)`	⬇️
lightning/src/ln/channel.rs	`89.41% <0.00%> (+0.66%)`	⬆️
lightning/src/routing/scoring.rs	`97.56% <0.00%> (+1.49%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e5f74a...d2a8906. Read the comment docs.

dunxen · 2022-07-09T07:13:08Z

 	(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)),


Curious why 5 was skipped before :)

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.

Ah of course, part of the "okay to be odd" rule.

moneyball · 2022-07-09T23:14:29Z

Concept ACK

tnull · 2022-07-11T10:50:01Z

+	/// 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,


I think $\frac{1}{2}^x$ should generally work fine, but why don't we allow to just define a simple percentage or share $x \in [0,1]$ of the channel capacity here? Seems more straight forward and probably also more readable when not familiar with how this knob works?

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.

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.

Yea, if anything I'm more worried about nothing between 1/8 and 1/4 than 1/2 and 1 :)

tnull · 2022-07-11T13:31:01Z

I believe this preliminarily addresses #1276? Linking for now.

TheBlueMatt · 2022-07-11T15:53:01Z

Indeed. Dunno if we should close #1276 after this or not, but its at least a huge step towards it.

wpaulino

First time reviewing this part of the codebase so you may want a more reassuring ACK from someone else. LGTM though, feel free to squash.

tnull

LGTM

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 · 2022-07-14T15:30:00Z

Squashed without further changes.

TheBlueMatt · 2022-07-09T04:29:59Z

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.

codecov-commenter · 2022-07-09T04:43:47Z

Codecov Report

Merging #1605 (d2a8906) into main (4e5f74a) will increase coverage by 0.37%.
The diff coverage is 95.87%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/routing/gossip.rs	`91.72% <ø> (-0.23%)`	⬇️
lightning/src/routing/router.rs	`92.52% <95.45%> (+0.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.80% <100.00%> (-0.31%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.88% <100.00%> (ø)`
lightning/src/util/events.rs	`39.25% <0.00%> (-0.29%)`	⬇️
lightning/src/ln/channel.rs	`89.41% <0.00%> (+0.66%)`	⬆️
lightning/src/routing/scoring.rs	`97.56% <0.00%> (+1.49%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e5f74a...d2a8906. Read the comment docs.

dunxen · 2022-07-09T07:13:08Z

 	(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)),


Curious why 5 was skipped before :)

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.

Ah of course, part of the "okay to be odd" rule.

moneyball · 2022-07-09T23:14:29Z

Concept ACK

tnull · 2022-07-11T10:50:01Z

+	/// 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,


I think $\frac{1}{2}^x$ should generally work fine, but why don't we allow to just define a simple percentage or share $x \in [0,1]$ of the channel capacity here? Seems more straight forward and probably also more readable when not familiar with how this knob works?

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.

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.

Yea, if anything I'm more worried about nothing between 1/8 and 1/4 than 1/2 and 1 :)

tnull · 2022-07-11T13:31:01Z

I believe this preliminarily addresses #1276? Linking for now.

TheBlueMatt · 2022-07-11T15:53:01Z

Indeed. Dunno if we should close #1276 after this or not, but its at least a huge step towards it.

wpaulino

First time reviewing this part of the codebase so you may want a more reassuring ACK from someone else. LGTM though, feel free to squash.

tnull

LGTM

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 · 2022-07-14T15:30:00Z

Squashed without further changes.

TheBlueMatt · 2022-07-09T04:29:59Z

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.

codecov-commenter · 2022-07-09T04:43:47Z

Codecov Report

Merging #1605 (d2a8906) into main (4e5f74a) will increase coverage by 0.37%.
The diff coverage is 95.87%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/routing/gossip.rs	`91.72% <ø> (-0.23%)`	⬇️
lightning/src/routing/router.rs	`92.52% <95.45%> (+0.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.80% <100.00%> (-0.31%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.88% <100.00%> (ø)`
lightning/src/util/events.rs	`39.25% <0.00%> (-0.29%)`	⬇️
lightning/src/ln/channel.rs	`89.41% <0.00%> (+0.66%)`	⬆️
lightning/src/routing/scoring.rs	`97.56% <0.00%> (+1.49%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e5f74a...d2a8906. Read the comment docs.

dunxen · 2022-07-09T07:13:08Z

 	(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)),


Curious why 5 was skipped before :)

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.

Ah of course, part of the "okay to be odd" rule.

moneyball · 2022-07-09T23:14:29Z

Concept ACK

tnull · 2022-07-11T10:50:01Z

+	/// 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,


I think $\frac{1}{2}^x$ should generally work fine, but why don't we allow to just define a simple percentage or share $x \in [0,1]$ of the channel capacity here? Seems more straight forward and probably also more readable when not familiar with how this knob works?

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.

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.

Yea, if anything I'm more worried about nothing between 1/8 and 1/4 than 1/2 and 1 :)

tnull · 2022-07-11T13:31:01Z

I believe this preliminarily addresses #1276? Linking for now.

TheBlueMatt · 2022-07-11T15:53:01Z

Indeed. Dunno if we should close #1276 after this or not, but its at least a huge step towards it.

wpaulino

First time reviewing this part of the codebase so you may want a more reassuring ACK from someone else. LGTM though, feel free to squash.

tnull

LGTM

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 · 2022-07-14T15:30:00Z

Squashed without further changes.

TheBlueMatt · 2022-07-09T04:29:59Z

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.

codecov-commenter · 2022-07-09T04:43:47Z

Codecov Report

Merging #1605 (d2a8906) into main (4e5f74a) will increase coverage by 0.37%.
The diff coverage is 95.87%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/routing/gossip.rs	`91.72% <ø> (-0.23%)`	⬇️
lightning/src/routing/router.rs	`92.52% <95.45%> (+0.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.80% <100.00%> (-0.31%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.88% <100.00%> (ø)`
lightning/src/util/events.rs	`39.25% <0.00%> (-0.29%)`	⬇️
lightning/src/ln/channel.rs	`89.41% <0.00%> (+0.66%)`	⬆️
lightning/src/routing/scoring.rs	`97.56% <0.00%> (+1.49%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e5f74a...d2a8906. Read the comment docs.

dunxen · 2022-07-09T07:13:08Z

 	(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)),


Curious why 5 was skipped before :)

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.

Ah of course, part of the "okay to be odd" rule.

moneyball · 2022-07-09T23:14:29Z

Concept ACK

tnull · 2022-07-11T10:50:01Z

+	/// 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,


I think $\frac{1}{2}^x$ should generally work fine, but why don't we allow to just define a simple percentage or share $x \in [0,1]$ of the channel capacity here? Seems more straight forward and probably also more readable when not familiar with how this knob works?

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.

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.

Yea, if anything I'm more worried about nothing between 1/8 and 1/4 than 1/2 and 1 :)

tnull · 2022-07-11T13:31:01Z

I believe this preliminarily addresses #1276? Linking for now.

TheBlueMatt · 2022-07-11T15:53:01Z

Indeed. Dunno if we should close #1276 after this or not, but its at least a huge step towards it.

wpaulino

First time reviewing this part of the codebase so you may want a more reassuring ACK from someone else. LGTM though, feel free to squash.

tnull

LGTM

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 · 2022-07-14T15:30:00Z

Squashed without further changes.

dunxen reviewed Jul 9, 2022
View reviewed changes

tnull self-requested a review July 9, 2022 13:51

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from de3e19e to dc3447aCompare July 9, 2022 15:26

tnull reviewed Jul 11, 2022
View reviewed changes

tnull linked an issue Jul 11, 2022 that may be closed by this pull request
Revisit MPP splitting heuristic #1276
Closed

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from dc3447a to 6a60020Compare July 11, 2022 16:20

TheBlueMatt added the Seeking Code Review label Jul 11, 2022

TheBlueMatt added this to the 0.0.110 milestone Jul 12, 2022

wpaulino reviewed Jul 12, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from 6a60020 to e8950acCompare July 12, 2022 21:35

TheBlueMatt mentioned this pull request Jul 12, 2022
Always pick the best paths, rather than (poorly) attempting to randomize #1610
Merged

TheBlueMatt removed the Seeking Code Review label Jul 13, 2022

TheBlueMatt assigned tnull and wpaulino Jul 13, 2022

tnull previously approved these changes Jul 13, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt dismissed tnull’s stale review via d2a8906July 13, 2022 16:30

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from e8950ac to d2a8906Compare July 13, 2022 16:30

wpaulino previously approved these changes Jul 13, 2022
View reviewed changes

TheBlueMatt added 3 commits July 13, 2022 18:36

Add a test of the new channel saturation limit
e6d40a7

tnull previously approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt dismissed stale reviews from tnull and wpaulino via e6d40a7July 14, 2022 15:29

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from d2a8906 to e6d40a7Compare July 14, 2022 15:29

tnull approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt unassigned tnull Jul 14, 2022

wpaulino approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt merged commit 5cca9a0 into lightningdevkit:mainJul 14, 2022

dunxen reviewed Jul 9, 2022
View reviewed changes

tnull self-requested a review July 9, 2022 13:51

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from de3e19e to dc3447aCompare July 9, 2022 15:26

tnull reviewed Jul 11, 2022
View reviewed changes

tnull linked an issue Jul 11, 2022 that may be closed by this pull request
Revisit MPP splitting heuristic #1276
Closed

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from dc3447a to 6a60020Compare July 11, 2022 16:20

TheBlueMatt added the Seeking Code Review label Jul 11, 2022

TheBlueMatt added this to the 0.0.110 milestone Jul 12, 2022

wpaulino reviewed Jul 12, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from 6a60020 to e8950acCompare July 12, 2022 21:35

TheBlueMatt mentioned this pull request Jul 12, 2022
Always pick the best paths, rather than (poorly) attempting to randomize #1610
Merged

TheBlueMatt removed the Seeking Code Review label Jul 13, 2022

TheBlueMatt assigned tnull and wpaulino Jul 13, 2022

tnull previously approved these changes Jul 13, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt dismissed tnull’s stale review via d2a8906July 13, 2022 16:30

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from e8950ac to d2a8906Compare July 13, 2022 16:30

wpaulino previously approved these changes Jul 13, 2022
View reviewed changes

TheBlueMatt added 3 commits July 13, 2022 18:36

Add a test of the new channel saturation limit
e6d40a7

tnull previously approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt dismissed stale reviews from tnull and wpaulino via e6d40a7July 14, 2022 15:29

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from d2a8906 to e6d40a7Compare July 14, 2022 15:29

tnull approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt unassigned tnull Jul 14, 2022

wpaulino approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt merged commit 5cca9a0 into lightningdevkit:mainJul 14, 2022

dunxen reviewed Jul 9, 2022
View reviewed changes

tnull self-requested a review July 9, 2022 13:51

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from de3e19e to dc3447aCompare July 9, 2022 15:26

tnull reviewed Jul 11, 2022
View reviewed changes

tnull linked an issue Jul 11, 2022 that may be closed by this pull request
Revisit MPP splitting heuristic #1276
Closed

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from dc3447a to 6a60020Compare July 11, 2022 16:20

TheBlueMatt added the Seeking Code Review label Jul 11, 2022

TheBlueMatt added this to the 0.0.110 milestone Jul 12, 2022

wpaulino reviewed Jul 12, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from 6a60020 to e8950acCompare July 12, 2022 21:35

TheBlueMatt mentioned this pull request Jul 12, 2022
Always pick the best paths, rather than (poorly) attempting to randomize #1610
Merged

TheBlueMatt removed the Seeking Code Review label Jul 13, 2022

TheBlueMatt assigned tnull and wpaulino Jul 13, 2022

tnull previously approved these changes Jul 13, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt dismissed tnull’s stale review via d2a8906July 13, 2022 16:30

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from e8950ac to d2a8906Compare July 13, 2022 16:30

wpaulino previously approved these changes Jul 13, 2022
View reviewed changes

TheBlueMatt added 3 commits July 13, 2022 18:36

Add a test of the new channel saturation limit
e6d40a7

tnull previously approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt dismissed stale reviews from tnull and wpaulino via e6d40a7July 14, 2022 15:29

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from d2a8906 to e6d40a7Compare July 14, 2022 15:29

tnull approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt unassigned tnull Jul 14, 2022

wpaulino approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt merged commit 5cca9a0 into lightningdevkit:mainJul 14, 2022

dunxen reviewed Jul 9, 2022
View reviewed changes

tnull self-requested a review July 9, 2022 13:51

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from de3e19e to dc3447aCompare July 9, 2022 15:26

tnull reviewed Jul 11, 2022
View reviewed changes

tnull linked an issue Jul 11, 2022 that may be closed by this pull request
Revisit MPP splitting heuristic #1276
Closed

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from dc3447a to 6a60020Compare July 11, 2022 16:20

TheBlueMatt added the Seeking Code Review label Jul 11, 2022

TheBlueMatt added this to the 0.0.110 milestone Jul 12, 2022

wpaulino reviewed Jul 12, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from 6a60020 to e8950acCompare July 12, 2022 21:35

TheBlueMatt mentioned this pull request Jul 12, 2022
Always pick the best paths, rather than (poorly) attempting to randomize #1610
Merged

TheBlueMatt removed the Seeking Code Review label Jul 13, 2022

TheBlueMatt assigned tnull and wpaulino Jul 13, 2022

tnull previously approved these changes Jul 13, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt dismissed tnull’s stale review via d2a8906July 13, 2022 16:30

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from e8950ac to d2a8906Compare July 13, 2022 16:30

wpaulino previously approved these changes Jul 13, 2022
View reviewed changes

TheBlueMatt added 3 commits July 13, 2022 18:36

Add a test of the new channel saturation limit
e6d40a7

tnull previously approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt dismissed stale reviews from tnull and wpaulino via e6d40a7July 14, 2022 15:29

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from d2a8906 to e6d40a7Compare July 14, 2022 15:29

tnull approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt unassigned tnull Jul 14, 2022

wpaulino approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt merged commit 5cca9a0 into lightningdevkit:mainJul 14, 2022

dunxen reviewed Jul 9, 2022
View reviewed changes

tnull self-requested a review July 9, 2022 13:51

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from de3e19e to dc3447aCompare July 9, 2022 15:26

tnull reviewed Jul 11, 2022
View reviewed changes

tnull linked an issue Jul 11, 2022 that may be closed by this pull request
Revisit MPP splitting heuristic #1276
Closed

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from dc3447a to 6a60020Compare July 11, 2022 16:20

TheBlueMatt added the Seeking Code Review label Jul 11, 2022

TheBlueMatt added this to the 0.0.110 milestone Jul 12, 2022

wpaulino reviewed Jul 12, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from 6a60020 to e8950acCompare July 12, 2022 21:35

TheBlueMatt mentioned this pull request Jul 12, 2022
Always pick the best paths, rather than (poorly) attempting to randomize #1610
Merged

TheBlueMatt removed the Seeking Code Review label Jul 13, 2022

TheBlueMatt assigned tnull and wpaulino Jul 13, 2022

tnull previously approved these changes Jul 13, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt dismissed tnull’s stale review via d2a8906July 13, 2022 16:30

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from e8950ac to d2a8906Compare July 13, 2022 16:30

wpaulino previously approved these changes Jul 13, 2022
View reviewed changes

TheBlueMatt added 3 commits July 13, 2022 18:36

Add a test of the new channel saturation limit
e6d40a7

tnull previously approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt dismissed stale reviews from tnull and wpaulino via e6d40a7July 14, 2022 15:29

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from d2a8906 to e6d40a7Compare July 14, 2022 15:29

tnull approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt unassigned tnull Jul 14, 2022

wpaulino approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt merged commit 5cca9a0 into lightningdevkit:mainJul 14, 2022

dunxen reviewed Jul 9, 2022
View reviewed changes

tnull self-requested a review July 9, 2022 13:51

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from de3e19e to dc3447aCompare July 9, 2022 15:26

tnull reviewed Jul 11, 2022
View reviewed changes

tnull linked an issue Jul 11, 2022 that may be closed by this pull request
Revisit MPP splitting heuristic #1276
Closed

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from dc3447a to 6a60020Compare July 11, 2022 16:20

TheBlueMatt added the Seeking Code Review label Jul 11, 2022

TheBlueMatt added this to the 0.0.110 milestone Jul 12, 2022

wpaulino reviewed Jul 12, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from 6a60020 to e8950acCompare July 12, 2022 21:35

TheBlueMatt mentioned this pull request Jul 12, 2022
Always pick the best paths, rather than (poorly) attempting to randomize #1610
Merged

TheBlueMatt removed the Seeking Code Review label Jul 13, 2022

TheBlueMatt assigned tnull and wpaulino Jul 13, 2022

tnull previously approved these changes Jul 13, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt dismissed tnull’s stale review via d2a8906July 13, 2022 16:30

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from e8950ac to d2a8906Compare July 13, 2022 16:30

wpaulino previously approved these changes Jul 13, 2022
View reviewed changes

TheBlueMatt added 3 commits July 13, 2022 18:36

Add a test of the new channel saturation limit
e6d40a7

tnull previously approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt dismissed stale reviews from tnull and wpaulino via e6d40a7July 14, 2022 15:29

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from d2a8906 to e6d40a7Compare July 14, 2022 15:29

tnull approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt unassigned tnull Jul 14, 2022

wpaulino approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt merged commit 5cca9a0 into lightningdevkit:mainJul 14, 2022

dunxen reviewed Jul 9, 2022
View reviewed changes

tnull self-requested a review July 9, 2022 13:51

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from de3e19e to dc3447aCompare July 9, 2022 15:26

tnull reviewed Jul 11, 2022
View reviewed changes

tnull linked an issue Jul 11, 2022 that may be closed by this pull request
Revisit MPP splitting heuristic #1276
Closed

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from dc3447a to 6a60020Compare July 11, 2022 16:20

TheBlueMatt added the Seeking Code Review label Jul 11, 2022

TheBlueMatt added this to the 0.0.110 milestone Jul 12, 2022

wpaulino reviewed Jul 12, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from 6a60020 to e8950acCompare July 12, 2022 21:35

TheBlueMatt mentioned this pull request Jul 12, 2022
Always pick the best paths, rather than (poorly) attempting to randomize #1610
Merged

TheBlueMatt removed the Seeking Code Review label Jul 13, 2022

TheBlueMatt assigned tnull and wpaulino Jul 13, 2022

tnull previously approved these changes Jul 13, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt dismissed tnull’s stale review via d2a8906July 13, 2022 16:30

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from e8950ac to d2a8906Compare July 13, 2022 16:30

wpaulino previously approved these changes Jul 13, 2022
View reviewed changes

TheBlueMatt added 3 commits July 13, 2022 18:36

Add a test of the new channel saturation limit
e6d40a7

tnull previously approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt dismissed stale reviews from tnull and wpaulino via e6d40a7July 14, 2022 15:29

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from d2a8906 to e6d40a7Compare July 14, 2022 15:29

tnull approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt unassigned tnull Jul 14, 2022

wpaulino approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt merged commit 5cca9a0 into lightningdevkit:mainJul 14, 2022

dunxen reviewed Jul 9, 2022
View reviewed changes

tnull self-requested a review July 9, 2022 13:51

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from de3e19e to dc3447aCompare July 9, 2022 15:26

tnull reviewed Jul 11, 2022
View reviewed changes

tnull linked an issue Jul 11, 2022 that may be closed by this pull request
Revisit MPP splitting heuristic #1276
Closed

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from dc3447a to 6a60020Compare July 11, 2022 16:20

TheBlueMatt added the Seeking Code Review label Jul 11, 2022

TheBlueMatt added this to the 0.0.110 milestone Jul 12, 2022

wpaulino reviewed Jul 12, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from 6a60020 to e8950acCompare July 12, 2022 21:35

TheBlueMatt mentioned this pull request Jul 12, 2022
Always pick the best paths, rather than (poorly) attempting to randomize #1610
Merged

TheBlueMatt removed the Seeking Code Review label Jul 13, 2022

TheBlueMatt assigned tnull and wpaulino Jul 13, 2022

tnull previously approved these changes Jul 13, 2022
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt dismissed tnull’s stale review via d2a8906July 13, 2022 16:30

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from e8950ac to d2a8906Compare July 13, 2022 16:30

wpaulino previously approved these changes Jul 13, 2022
View reviewed changes

TheBlueMatt added 3 commits July 13, 2022 18:36

Add a test of the new channel saturation limit
e6d40a7

tnull previously approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt dismissed stale reviews from tnull and wpaulino via e6d40a7July 14, 2022 15:29

TheBlueMatt force-pushed the 2022-07-smaller-mpp-parts branch from d2a8906 to e6d40a7Compare July 14, 2022 15:29

tnull approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt unassigned tnull Jul 14, 2022

wpaulino approved these changes Jul 14, 2022
View reviewed changes

TheBlueMatt merged commit 5cca9a0 into lightningdevkit:mainJul 14, 2022

Uh oh!

Conversation

TheBlueMatt commented Jul 9, 2022

Uh oh!

codecov-commenter commented Jul 9, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moneyball commented Jul 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnullJul 12, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull commented Jul 11, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Jul 11, 2022

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jul 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

TheBlueMatt commented Jul 9, 2022

Uh oh!

codecov-commenter commented Jul 9, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moneyball commented Jul 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnullJul 12, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 9, 2022•
edited
Loading

tnullJul 12, 2022•
edited
Loading

tnull commented Jul 11, 2022•
edited
Loading

codecov-commenter commented Jul 9, 2022•
edited
Loading

tnullJul 12, 2022•
edited
Loading

tnull commented Jul 11, 2022•
edited
Loading

codecov-commenter commented Jul 9, 2022•
edited
Loading